Skip to content
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

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

Merged
merged 1 commit into from Apr 17, 2019

Conversation

Projects
None yet
6 participants
@nicolas-grekas
Copy link
Member

nicolas-grekas commented Apr 14, 2019

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+>}

@nicolas-grekas nicolas-grekas force-pushed the nicolas-grekas:routing-fix-trailing-slash branch from d0c5649 to d88833d Apr 14, 2019

@fabpot fabpot merged commit d88833d into symfony:4.2 Apr 17, 2019

3 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
fabbot.io Your code looks good.
Details

fabpot pushed a commit that referenced this pull request Apr 17, 2019

bug #31107 [Routing] fix trailing slash redirection with non-greedy t…
…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

@fabpot fabpot referenced this pull request Apr 17, 2019

Merged

Release v4.2.7 #31148

@arjenm

This comment has been minimized.

Copy link
Contributor

arjenm commented Apr 18, 2019

We have a set of special wild card routes for 'short urls' that eventually are redirected to a 'full url'. I.e. we can open https://example.org/$something or https://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 / and then 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 / and then 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

This comment has been minimized.

Copy link

bmack commented Apr 18, 2019

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-grekas nicolas-grekas deleted the nicolas-grekas:routing-fix-trailing-slash branch Apr 18, 2019

@nicolas-grekas

This comment has been minimized.

Copy link
Member Author

nicolas-grekas commented Apr 18, 2019

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

This comment has been minimized.

Copy link
Member Author

nicolas-grekas commented Apr 18, 2019

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

This comment has been minimized.

Copy link
Contributor

arjenm commented Apr 18, 2019

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:

new Route(
        '/{label}{slash}',
        ['_controller' => [...], 'slash' => ''],
        ['label' => '[^/]+', 'slash' => '/?']
    )
@bmack

This comment has been minimized.

Copy link

bmack commented Apr 18, 2019

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.CMS that referenced this pull request Apr 19, 2019

[BUGFIX] Mark symfony/routing 4.2.7 as conflict
Symfony/routing 4.2.7 changed routing behaviour, breaking backwards
compatibility and our implementation. Reported at symfony:

symfony/symfony#31107 (comment)

Mark that version as conflict until the behaviour is fixed.

Composer commands:
 - composer update --lock

Resolves: #88171
Releases: master, 9.5
Change-Id: I6f2651a605c6339222626d37c307c04b8f0eadf8
Reviewed-on: https://review.typo3.org/c/Packages/TYPO3.CMS/+/60509
Tested-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 request Apr 19, 2019

[BUGFIX] Mark symfony/routing 4.2.7 as conflict
Symfony/routing 4.2.7 changed routing behaviour, breaking backwards
compatibility and our implementation. Reported at symfony:

symfony/symfony#31107 (comment)

Mark that version as conflict until the behaviour is fixed.

Composer commands:
 - composer update --lock

Resolves: #88171
Releases: master, 9.5
Change-Id: I6f2651a605c6339222626d37c307c04b8f0eadf8
Reviewed-on: https://review.typo3.org/c/Packages/TYPO3.CMS/+/60509
Tested-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.CMS that referenced this pull request Apr 19, 2019

[BUGFIX] Mark symfony/routing 4.2.7 as conflict
Symfony/routing 4.2.7 changed routing behaviour, breaking backwards
compatibility and our implementation. Reported at symfony:

symfony/symfony#31107 (comment)

Mark that version as conflict until the behaviour is fixed.

Composer commands:
 - composer update --lock

Resolves: #88171
Releases: master, 9.5
Change-Id: I6f2651a605c6339222626d37c307c04b8f0eadf8
Reviewed-on: https://review.typo3.org/c/Packages/TYPO3.CMS/+/60517
Tested-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 request Apr 19, 2019

[BUGFIX] Mark symfony/routing 4.2.7 as conflict
Symfony/routing 4.2.7 changed routing behaviour, breaking backwards
compatibility and our implementation. Reported at symfony:

symfony/symfony#31107 (comment)

Mark that version as conflict until the behaviour is fixed.

Composer commands:
 - composer update --lock

Resolves: #88171
Releases: master, 9.5
Change-Id: I6f2651a605c6339222626d37c307c04b8f0eadf8
Reviewed-on: https://review.typo3.org/c/Packages/TYPO3.CMS/+/60517
Tested-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 request Apr 19, 2019

bug #31182 [Routing] fix trailing slash matching with empty-matching …
…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 free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.