8000 Correct trailing slash redirection behavior? · Issue #29287 · symfony/symfony · GitHub
[go: up one dir, main page]
Skip to content

Correct trailing slash redirection behavior? #29287

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.

Sign up for GitHub

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

Jump to bottom
Closed
nicolas-grekas opened this issue Nov 22, 2018 · 12 comments
Closed

Correct trailing slash redirection behavior? #29287

nicolas-grekas opened this issue Nov 22, 2018 · 12 comments
Labels
Bug Routing Status: Needs Review

Comments

@nicolas-grekas
Copy link
Member
nicolas-grekas commented Nov 22, 2018 •
edited
Loading

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?
@Nyholm
Copy link
Member
Nyholm commented Nov 22, 2018

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
Copy link
8000 Contributor
Tobion commented Nov 23, 2018

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
Copy link
Member Author

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
Copy link
Contributor
dkarlovi commented Nov 23, 2018 •
edited
Loading

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
Copy link
Member
javiereguiluz commented Nov 23, 2018 •
edited
Loading

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
Copy link
Contributor
Wirone commented Nov 23, 2018

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 nicolas-grekas mentioned this issue Nov 23, 2018
Merged
@nicolas-grekas
Copy link
Member Author

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

@Tobion
Copy link
Contributor
Tobion commented Nov 24, 2018 •
edited
Loading

/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
Copy link
Contributor
Wirone commented Nov 24, 2018

@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.

nicolas-grekas added a commit that referenced this issue Nov 26, 2018
@nicolas-grekas
bug #29297 [Routing] fix trailing slash redirection when using Redire…
0878006 
…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
Copy link
Member
Nyholm commented Nov 26, 2018

Thank you for the discussion and the input.

@murnieza
Copy link

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

@Tobion
Copy link
Contributor
Tobion commented Nov 28, 2018

@murnieza please open a new issue

@nicolas-grekas nicolas-grekas mentioned this issue Nov 29, 2018
Closed
@dkarlovi dkarlovi mentioned this issue Dec 3, 2018
Closed
@Tobion Tobion mentioned this issue Dec 6, 2018
Closed
@nicolas-grekas nicolas-grekas mentioned this issue Aug 27, 2019
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
Bug Routing Status: Needs Review
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
9 participants
@javiereguiluz @dkarlovi @nicolas-grekas @Wirone @Tobion @Nyholm @murnieza @xabbuh @carsonbot
0