-
-
Notifications
You must be signed in to change notification settings - Fork 9.6k
[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 our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
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, 👍
31e4c6e
to
77b4188
Compare
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. |
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. |
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 complain if we change the current behavior. e.g. symfony.com would break. And I'm sure many others will also. |
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/. |
Sure I can, but there are more routes than just |
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). |
Go ahead then. But you probably want to implement the same logic for the opposite redirection which will get pretty ugly https://symfony.com/blog/new-in-symfony-4-1-smarter-url-redirections |
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. |
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
to
dc4c3f6
Compare
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 | - This fixes #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.1 Commits ------- 6968000 [Routing] fix trailing slash redirection when using RedirectableUrlMatcher
This fixes #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).