Skip to content

strip trailing slashes from URL before rerouting#14

Open
nickautomatic wants to merge 2 commits intodavist11:masterfrom
niceandserious:master
Open

strip trailing slashes from URL before rerouting#14
nickautomatic wants to merge 2 commits intodavist11:masterfrom
niceandserious:master

Conversation

@nickautomatic
Copy link
Copy Markdown

(to ensure /page/ is routed the same way as /page). Fixes #13

@ltk
Copy link
Copy Markdown

ltk commented Apr 15, 2016

👍 whatchyu think @davist11?

khamer added a commit to imarc/craft-reroute that referenced this pull request Apr 21, 2016
@nicktobolski
Copy link
Copy Markdown

Isn't this one worth merging?

@gjhead
Copy link
Copy Markdown

gjhead commented Sep 21, 2017

Is this one going to get merged?

@davist11
Copy link
Copy Markdown
Owner

Most likely no. Don't want to break existing installs and this add-on likely won't get updated for Craft 3 since https://github.com/nystudio107/retour has since come out and is more full-featured.

@nickautomatic
Copy link
Copy Markdown
Author

nickautomatic commented Oct 10, 2017

@davist11 You're definitely right to be cautious about not breaking things, but in this case it's difficult to imagine what would be likely to be broken by merging this: the only situation I can think of where it would cause unintended consequences is the situation where someone was deliberately routing, eg. /page and /page/ to different places (since all this change does is ignore that trailing slash when rerouting). But that is very much an edge case (it seems unlikely anyone would be deliberately doing that in practice). In practice it's pretty standard to treat the above two paths as identical.

So personally I think merging this pull request should be safe, and is likely to help most users and be unlikely to hinder anyone. Obviously it's your call though, and understandable if you don't have time to spend on a (very handy!) plugin you made quite a while ago.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants