NotificationsYou must be signed in to change notification settings
Fork9.7k
Star30.8k

[Router] Fix DumpedUrlMatcher behaviour with trailing slash#33362

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

Jump to bottom

Closed

xavierleune wants to merge1 commit intosymfony:3.4fromxavierleune:fix-32996-trailing-slash

Closed

[Router] Fix DumpedUrlMatcher behaviour with trailing slash#33362

xavierleune wants to merge1 commit intosymfony:3.4fromxavierleune:fix-32996-trailing-slash

Conversation

Copy link

Contributor

xavierleune commentedAug 27, 2019

Q	A
Branch?	3.4
Bug fix?	yes
New feature?	no
BC breaks?	no
Deprecations?	no
Tests pass?	yes
Fixed tickets	#32996
License	MIT
Doc PR	¤

The trailing slash management seems not to be quite consistant across matchers. This pull request fixes the behaviour with the current state of mind: when you have an exact route, you should always use it, not trying to redirect request.

Things are a little bit tricky here, specially when it comes to the DumpedUrlMatcher, but it was adding a lot of complexity than trying to keep things in a single function.

carsonbot added Status: Needs Review Routing Bug labels

Aug 27, 2019

[Router] Fix DumpedUrlMatcher behaviour with trailing slash

8a2b788

xavierleune force-pushed thefix-32996-trailing-slash branch from60997a8 to8a2b788Compare

August 27, 2019 16:26

Copy link

Member

nicolas-grekas commentedAug 27, 2019

I just posted#32996 (comment), which I wrote offline before seeing this PR.

nicolas-grekas added this to the3.4 milestone

Aug 27, 2019

nicolas-grekas reviewed

Aug 27, 2019

View reviewed changes

src/Symfony/Component/Routing/Tests/Matcher/RedirectableUrlMatcherTest.php

		$this->assertSame(['_route' =>'foo'],$matcher->match('/foo'));
		}

		publicfunctiontestFallbackPage()

Copy link

Member

nicolas-grekasAug 27, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

This guarded against the regression found in#29297

Copy link

Member

nicolas-grekas commentedAug 27, 2019

(Trick to update fixtures: add afile_put_contents() at the beginning ofassertStringEqualsFile in.phpunit/phpunit-8.3-0/src/Framework/Assert.php or similar.)

Copy link

Member

nicolas-grekas commentedAug 29, 2019•
edited
Loading

Thank you for#32996 (comment) it helps understand your use case a lot.

I think there is no bug on 3.4 so that this PR shouldn't be merged.

Can you confirm routes like this allow working around the behavior that annoys you?

@Route("/hello{_</(?!/)>}", name="hello1")
@Route("/hello{_<(?!/)>}", name="hello2")

Would your use case be fixed by reconfiguring the router to useUrlMatcher instead ofRedirectableUrlMatcher? That would remove the other redirections, so maybe not - but maybe yes if you don't use scheme redirects?

As a follow up, I'd suggest opening a new feature request. Global or local flag/option could be discussed there (we could have a per-route option, and be able to enable it by default like theutf8 one)

Copy link

ContributorAuthor

xavierleune commentedAug 30, 2019

Thanks, following this comment on#32996 I double checked things here and I think there is no bug either in 3.4 or in any 4.X version. Sorry you've lost some time here. So instead of trying to fix things according to documentation, I've opened a PR onsymfony-docs#12247.
I also can confirm that the workaround you suggested is working perfectly fine.

So I'll reopen and rework#33342 to discuss about the global flag.