Correct trailing slash redirection behavior? #29287

nicolas-grekas · 2018-11-22T19:52:46Z

We spotted the following:

Given two routes (in order):

"a": /foo/
"b": /{name}

for URL /foo, a redirectable router matcher:

matches "a" before 4.1 when being dumped (with internal redirection of course)
matches "b" before 4.1 when being not dumped
matches "b" since 4.1 when being dumped or not

What is the correct behavior?

The text was updated successfully, but these errors were encountered:

Nyholm · 2018-11-22T20:27:47Z

Since “/foo/“ is not an excact match I would assume it matches b.

If route b did not exist I would like it to match a.

Answer: option 3 is the correct behavior.

Tobion · 2018-11-23T01:08:00Z

I agree with @Nyholm. This matches other redirection logic we have. First match exact matches, then fall back to redirect. Otherwise it would be inconsistent with

Given two routes (in order):

"a": /foo
"b": /{name<.+>}

for URL /foo/ should match "b" first, before redirecting to "a"

nicolas-grekas · 2018-11-23T07:05:24Z

Interesting because on Slack I read opposite arguments and they make sense also:

I'd say a), the slash being significant is error prone

If you treat the trailing slash as non-significant, then that should apply here too, this would 100% confuse me if it happened

I'd expect /foo and /foo/ to behave the same if trailing slash redirects and this route is configured first
If you have dynamic fallback routes, they should not match if a redirect is in place, this gives unexpected behaviour, especially if the configuration is defined elsewhere. Imagine a controller with annotations defining /foo/, but the /{name} route being matched despite being defined manually, as last, in the yml

The least surprise principle would hint me at "a" also. Technically, symfony.com would be affected by changing from "a" to "b".

But that not obvious, that's why I asked :)
@Nyholm @Tobion @fabpot does that make better sense to you now or not?

dkarlovi · 2018-11-23T07:43:27Z

The "correct behavior" term is loaded here: does it mean "technically correct" or "correct as expected"?

If you want to be technically correct, I agree with @Nyholm, it's not a full match.

If you want to be working as (IMO) expected, then a) should match. Again, if you're saying the trailing slash is ignored, then it's perceived as non-significant and having /foo not match ^/foo/?$ (examples as perceived by an end user, of course) is unexpected.

javiereguiluz · 2018-11-23T07:58:40Z

I disagree with @Nyholm. In Symfony apps, if your route is /foo/, both /foo/ and /foo work. That's a well-known (and heavily relied upon) behaviour since day one. I don't care about the later catch-all /{name<.+>} route. In my opinion, /foo should keep redirecting to /foo/.

Wirone · 2018-11-23T08:00:19Z

Imagine letter addressed with "street" prefix/suffix and without it - it's still the same address, deliverable (well, probably not always). Or maybe semicolons in Javascript - it's the same instruction whether you put ; at the end or not. For me, trailing / isn't really part of the URL, it's UX detail, personal preference for displaying unique "web address". Even this issue can be accessed with slash and without it.

And IMO there shouldn't be any redirect, just display the same content under both URLs (like Github does).

nicolas-grekas · 2018-11-23T17:29:04Z

The arguments for a) make sense to me, here is a fix: #29297

Tobion · 2018-11-24T04:10:15Z

/foo/ and /foo are not the same URIs. For this reason it does not make sense to match a different route than the one that actually matches. Also if you generate a the uri for "b" with name=foo, you will get /foo BUT it won't actually match route "b" at all. This is totally unexpected.
Also if you give redirection a higher prio than actually matching, you also need to redirect schemes http <-> https before actually trying to find a route with the right scheme to be consistent.

Furthermore, changing the behavior makes the route matching dependent on whether redirectable url matcher is implement or not because you will get different results for the same input.

Wirone · 2018-11-24T10:14:14Z

@Tobion If you define dynamic routes with params that conflict with other routes, it's your fault. Trailing slash doesn't matter. Look at this issue, it's displayed under both URIs because its namespace is unique.

…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

Nyholm · 2018-11-26T11:55:24Z

Thank you for the discussion and the input.

murnieza · 2018-11-28T09:29:31Z

I have route /foo/{bar<.*>} and after this change /foo/ does not match my route anymore.

Tobion · 2018-11-28T12:36:03Z

@murnieza please open a new issue

xabbuh added Bug Routing labels Nov 23, 2018

carsonbot added the Status: Needs Review label Nov 23, 2018

nicolas-grekas mentioned this issue Nov 23, 2018

[Routing] fix trailing slash redirection when using RedirectableUrlMatcher #29297

Merged

nicolas-grekas closed this as completed Nov 26, 2018

nicolas-grekas mentioned this issue Nov 29, 2018

Routes with trailing slash broken after upgrade to 4.1.8 #29363

Closed

dkarlovi mentioned this issue Dec 3, 2018

[Routing] Route with trailing slash redirects to route without slash #29438

Closed

Tobion mentioned this issue Dec 6, 2018

Redirect with trailing slash #29477

Closed

nicolas-grekas mentioned this issue Aug 27, 2019

[Routing] Trailing slash redirection prevents routes to be matched #32996

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Uh oh!

Correct trailing slash redirection behavior? #29287

Correct trailing slash redirection behavior? #29287

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!