Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork9.7k
[Routing] fix trailing slash redirection when using RedirectableUrlMatcher#29297
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Uh oh!
There was an error while loading.Please reload this page.
Conversation
HeahDude left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Your arguments convinced me, 👍
Tobion commentedNov 24, 2018
31e4c6e to77b4188Comparenicolas-grekas commentedNov 24, 2018
Behavior a) is what most people use since a very long time. Changing it would break symfony.com. We cannot break URLs without being 200% sure we should do it. The logic in the php dumper that provides behavior a) is non-trivial. I wouldn't call it a mistake but a design choice. Feature-wise, the behavior makes a lot of sense to me: later routes should not impact previous routes. I did ask because that was not obvious at first, but the arguments we gathered are sound. b) is the bug. |
Tobion commentedNov 24, 2018
To me a) is a bug . Why no just fix symfony.com until more people complain about this? Then we can reevaluate re-adding a bogus feature that only existed because of implementation details. |
nicolas-grekas commentedNov 24, 2018
The patch for symfony.com will be totally unexpected, looking a lot like a workaround for a WTF behavior in core. Here is the route in practice:
The fact that the fallback exists is totally unrelated to the fact that People will complainif we change the current behavior. e.g. symfony.com would break. And I'm sure many others will also. |
Tobion commentedNov 24, 2018
You can either make {page} placeholder not accept "blog" (which seems like a logical solution to prevent collision with the blog route) or create a /blog route explicitly that redirects to /blog/. |
nicolas-grekas commentedNov 24, 2018
Sure I can, but there are more routes than just |
fabpot commentedNov 24, 2018
I understand you both@Tobion and@nicolas-grekas. I would slightly prefer b) if we were talking about a new routing system, but in any case, we cannot introduce a BC break on routes now (if symfony.com is affected, we can be sure that we will have tons of people angry of we change the behavior, even of we do so only in 4.2/master). So, to me, a) is the only option at this stage (maybe a note in the docs is in order here). |
Tobion commentedNov 24, 2018 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
Go ahead then. But you probably want to implement the same logic for the opposite redirection which will get pretty uglyhttps://symfony.com/blog/new-in-symfony-4-1-smarter-url-redirections |
nicolas-grekas commentedNov 24, 2018
Tobion commentedNov 24, 2018 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
Actually which this logic in place, it might be claener to have the route compiler generate a regex with an optional trailing slash in the first place. That seems more logical to me than changing the regex later on. This way debug:router makes it also more self-explaining. |
nicolas-grekas commentedNov 24, 2018 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
The fact that the original path contains a trailing slash or not is required to decide which is the canonical path we redirect to. I'd prefer keeping the current way to do so to make the patch as light as possible. |
77b4188 todc4c3f6Comparenicolas-grekas commentedNov 26, 2018 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
Now targeting 3.4, let 2.8 in peace. |
…ctableUrlMatcher (nicolas-grekas)This PR was merged into the 3.4 branch.Discussion----------[Routing] fix trailing slash redirection when using RedirectableUrlMatcher| Q | A| ------------- | ---| Branch? | 3.4| Bug fix? | yes| New feature? | no| BC breaks? | no| Deprecations? | no| Tests pass? | yes| Fixed tickets |#29287| License | MIT| Doc PR | -Thisfixes#29287 by considering that the behavior of the dumped matcher is the correct one: lower priority routes never impact previous ones. I think it's what makes the most sense because that's what requires the most local knowledge to understand what's going on (ie that's the less surprising behavior).Commits-------dc4c3f6 [Routing] fix trailing slash redirection when using RedirectableUrlMatcher
…ctableUrlMatcher (nicolas-grekas)This PR was merged into the 4.1 branch.Discussion----------[Routing] fix trailing slash redirection when using RedirectableUrlMatcher| Q | A| ------------- | ---| Branch? | 4.1| Bug fix? | yes| New feature? | no| BC breaks? | no| Deprecations? | no| Tests pass? | yes| Fixed tickets |#29297| License | MIT| Doc PR | -This is#29297 for 4.1Commits-------6968000 [Routing] fix trailing slash redirection when using RedirectableUrlMatcher
Uh oh!
There was an error while loading.Please reload this page.
Thisfixes#29287 by considering that the behavior of the dumped matcher is the correct one: lower priority routes never impact previous ones. I think it's what makes the most sense because that's what requires the most local knowledge to understand what's going on (ie that's the less surprising behavior).