Movatterモバイル変換

[0]ホーム
URL:

Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

 Sign up
Appearance settings

[Routing] fix trailing slash redirection with non-greedy trailing vars#31107

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

Merged

Conversation

@nicolas-grekas
Copy link
Member

@nicolas-grekasnicolas-grekas commentedApr 14, 2019•
edited
Loading

QA
Branch?4.2
Bug fix?yes
New feature?no
BC breaks?no
Deprecations?no
Tests pass?yes
Fixed tickets#30863,#31066
LicenseMIT
Doc PR-

Fixes redirecting/123/ to/123 when the route is defined as/{foo<\d+>}

marc-romera and umpirsky reacted with hooray emoji
@nicolas-grekasnicolas-grekasforce-pushed therouting-fix-trailing-slash branch fromd0c5649 tod88833dCompareApril 14, 2019 18:05
@fabpotfabpot merged commitd88833d intosymfony:4.2Apr 17, 2019
fabpot pushed a commit that referenced this pull requestApr 17, 2019
…railing vars (nicolas-grekas)This PR was merged into the 4.2 branch.Discussion----------[Routing] fix trailing slash redirection with non-greedy trailing vars| Q             | A| ------------- | ---| Branch?       | 4.2| Bug fix?      | yes| New feature?  | no| BC breaks?    | no| Deprecations? | no| Tests pass?   | yes| Fixed tickets |#30863,#31066| License       | MIT| Doc PR        | -Fixes redirecting `/123/` to `/123` when the route is defined as `/{foo<\d+>}`Commits-------d88833d [Routing] fix trailing slash redirection with non-greedy trailing vars
@fabpotfabpot mentioned this pull requestApr 17, 2019
@arjenm
Copy link
Contributor

arjenm commentedApr 18, 2019•
edited
Loading

We have a set of special wild card routes for 'short urls' that eventually are redirected to a 'full url'. I.e. we can openhttps://example.org/$something orhttps://example.org/$something/ and the controller decides where to redirect depending on the value of '$something'.

But its utterly useless to first redirect to the variant with or without / andthen redirecting to the final url. So we had both scenario's defined as route, one with '/{label}' and one with '/{label}/' which both were setup with the same _controller.

Up untill this fix that worked just fine. Now when requesting the one with 'something/', the browser is first redirect to the variant without / andthen to whatever the controller decides.
I.e. the second route that's a perfect match is ignored due to the other route (imperfect match), doing an early return causing a redirect.

Is there any (clean) way now to define one or more routes to support this scenario, where we just don't care about that trailing slash. But where we do care that a user gets an unnecessary redirect.

@bmack
Copy link

Hey all,

FYI: we're at TYPO3 have hit a similar issue as@arjenm in our nightly builds, I'm currently digging into this change and find out how we at TYPO3 used Symfony Routing the wrong way or if it is a BC issue.

Will keep you updated.
Benni.

@nicolas-grekasnicolas-grekas deleted the routing-fix-trailing-slash branchApril 18, 2019 11:12
@nicolas-grekas
Copy link
MemberAuthor

Is there any (clean) way now to define one or more routes to support this scenario, where we just don't care about that trailing slash. But where we do care that a user gets an unnecessary redirect.

Would changing the order of those two routes work?

@nicolas-grekas
Copy link
MemberAuthor

The other best way is to create a single route that matches with and without the trailing slash in the tail requirement of your route.

@arjenm
Copy link
Contributor

Is there any (clean) way now to define one or more routes to support this scenario, where we just don't care about that trailing slash. But where we do care that a user gets an unnecessary redirect.

Would changing the order of those two routes work?

I'm fairly certain that would cause a redirect for the non-slash version to the slash-version. Which I discovered in this bug:#30721

The other best way is to create a single route that matches with and without the trailing slash in the tail requirement of your route.

Ah, it seems to work with this as the very last route:

newRoute('/{label}{slash}',        ['_controller' => [...],'slash' =>''],        ['label' =>'[^/]+','slash' =>'/?']    )
nicolas-grekas reacted with thumbs up emoji

@bmack
Copy link

Hey,

I added a test to UrlMatcherTest:

    public function testTrailingSlashWorks()    {        $coll = new RouteCollection();        $coll->add('b', new Route('/en-en/{tail}', ['tail' => ''], ['tail' => '.*']));        $matcher = $this->getUrlMatcher($coll);        // this works in 4.2.6 and 4.2.7        $this->assertEquals(['_route' => 'b', 'tail' => ''], $matcher->match('/en-en'));        $this->assertEquals(['_route' => 'b', 'tail' => 'products'], $matcher->match('/en-en/products'));        // works in 4.2.6, but does not work in 4.2.7        $this->assertEquals(['_route' => 'b', 'tail' => ''], $matcher->match('/en-en/'));    }

So you see the first two assertions work. Just tested this with the Git Repo and switching between the tags.

Don't know how to fix though tbh.@nicolas-grekas should I submit a PR with this additional test?

reviewtypo3org pushed a commit to TYPO3/typo3 that referenced this pull requestApr 19, 2019
Symfony/routing 4.2.7 changed routing behaviour, breaking backwardscompatibility and our implementation. Reported at symfony:symfony/symfony#31107 (comment)Mark that version as conflict until the behaviour is fixed.Composer commands: - composer update --lockResolves: #88171Releases: master, 9.5Change-Id: I6f2651a605c6339222626d37c307c04b8f0eadf8Reviewed-on:https://review.typo3.org/c/Packages/TYPO3.CMS/+/60509Tested-by: TYPO3com <noreply@typo3.com>Tested-by: Benni Mack <benni@typo3.org>Tested-by: Andreas Fernandez <a.fernandez@scripting-base.de>Reviewed-by: Benni Mack <benni@typo3.org>Reviewed-by: Oliver Klee <typo3-coding@oliverklee.de>Reviewed-by: Josef Glatz <josefglatz@gmail.com>Reviewed-by: Andreas Fernandez <a.fernandez@scripting-base.de>
TYPO3IncTeam pushed a commit to TYPO3-CMS/core that referenced this pull requestApr 19, 2019
Symfony/routing 4.2.7 changed routing behaviour, breaking backwardscompatibility and our implementation. Reported at symfony:symfony/symfony#31107 (comment)Mark that version as conflict until the behaviour is fixed.Composer commands: - composer update --lockResolves: #88171Releases: master, 9.5Change-Id: I6f2651a605c6339222626d37c307c04b8f0eadf8Reviewed-on:https://review.typo3.org/c/Packages/TYPO3.CMS/+/60509Tested-by: TYPO3com <noreply@typo3.com>Tested-by: Benni Mack <benni@typo3.org>Tested-by: Andreas Fernandez <a.fernandez@scripting-base.de>Reviewed-by: Benni Mack <benni@typo3.org>Reviewed-by: Oliver Klee <typo3-coding@oliverklee.de>Reviewed-by: Josef Glatz <josefglatz@gmail.com>Reviewed-by: Andreas Fernandez <a.fernandez@scripting-base.de>
reviewtypo3org pushed a commit to TYPO3/typo3 that referenced this pull requestApr 19, 2019
Symfony/routing 4.2.7 changed routing behaviour, breaking backwardscompatibility and our implementation. Reported at symfony:symfony/symfony#31107 (comment)Mark that version as conflict until the behaviour is fixed.Composer commands: - composer update --lockResolves: #88171Releases: master, 9.5Change-Id: I6f2651a605c6339222626d37c307c04b8f0eadf8Reviewed-on:https://review.typo3.org/c/Packages/TYPO3.CMS/+/60517Tested-by: TYPO3com <noreply@typo3.com>Tested-by: Andreas Fernandez <a.fernandez@scripting-base.de>Reviewed-by: Andreas Fernandez <a.fernandez@scripting-base.de>
TYPO3IncTeam pushed a commit to TYPO3-CMS/core that referenced this pull requestApr 19, 2019
Symfony/routing 4.2.7 changed routing behaviour, breaking backwardscompatibility and our implementation. Reported at symfony:symfony/symfony#31107 (comment)Mark that version as conflict until the behaviour is fixed.Composer commands: - composer update --lockResolves: #88171Releases: master, 9.5Change-Id: I6f2651a605c6339222626d37c307c04b8f0eadf8Reviewed-on:https://review.typo3.org/c/Packages/TYPO3.CMS/+/60517Tested-by: TYPO3com <noreply@typo3.com>Tested-by: Andreas Fernandez <a.fernandez@scripting-base.de>Reviewed-by: Andreas Fernandez <a.fernandez@scripting-base.de>
nicolas-grekas added a commit that referenced this pull requestApr 19, 2019
…trailing vars (nicolas-grekas)This PR was merged into the 4.2 branch.Discussion----------[Routing] fix trailing slash matching with empty-matching trailing vars| Q             | A| ------------- | ---| Branch?       | 4.2| Bug fix?      | yes| New feature?  | no| BC breaks?    | no| Deprecations? | no| Tests pass?   | yes| Fixed tickets | -| License       | MIT| Doc PR        | -Reported by@bmack in#31107 (comment)This highlights a small inconsistency that exists for a long time (checked on 2.7 at least):`new Route('/en-en/{b}', ['b' => 'bbb'], ['b' => '.*'])` matches `/en-en/``new Route('/en-en/{b}', ['b' => 'bbb'], ['b' => '.+'])` doesn't match it(while both match `/en-en` and `/en-en/foo`)This PR ensures the former behavior is preserved, while#31167 redirects the later to `/en-en`.Commits-------d6da21a [Routing] fix trailing slash matching with empty-matching trailing vars
Sign up for freeto join this conversation on GitHub. Already have an account?Sign in to comment

Reviewers

@OskarStarkOskarStarkOskarStark left review comments

Assignees

No one assigned

Projects

None yet

Milestone

4.2

Development

Successfully merging this pull request may close these issues.

6 participants

@nicolas-grekas@arjenm@bmack@OskarStark@fabpot@carsonbot
[8]ページ先頭
©2009-2025 Movatter.jp