Skip to content

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

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
Merged
merged 1 commit into from
Apr 17, 2019
Merged

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

merged 1 commit into from
Apr 17, 2019

Conversation

nicolas-grekas
Copy link
Member

@nicolas-grekas nicolas-grekas commented Apr 14, 2019 •
edited
Loading

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 added this to the 4.2 milestone Apr 14, 2019
This was referenced Apr 14, 2019
Closed
Closed
@nicolas-grekas nicolas-grekas force-pushed the routing-fix-trailing-slash branch from e71fec0 to d0c5649 Compare April 14, 2019 16:54
@nicolas-grekas nicolas-grekas force-pushed the routing-fix-trailing-slash branch from d0c5649 to d88833d Compare April 14, 2019 18:05
@fabpot fabpot merged commit d88833d into symfony:4.2 Apr 17, 2019
fabpot pushed a commit that referenced this pull request Apr 17, 2019
@nicolas-grekas
bug #31107 [Routing] fix trailing slash redirection with non-greedy t…
2d2ff38 
…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 mentioned this pull request Apr 17, 2019
Merged
@arjenm
Copy link
Contributor

arjenm commented Apr 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 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
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 routing-fix-trailing-slash branch April 18, 2019 11:12
@nicolas-grekas
Copy link
Member Author

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

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

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
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 that referenced this pull request Apr 19, 2019
@susannemoog @andreaskienast
[BUGFIX] Mark symfony/routing 4.2.7 as conflict
389bb40 
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
@susannemoog @andreaskienast
[BUGFIX] Mark symfony/routing 4.2.7 as conflict
e1845ef 
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 that referenced this pull request Apr 19, 2019
@susannemoog @andreaskienast
[BUGFIX] Mark symfony/routing 4.2.7 as conflict
247cd2a 
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
@susannemoog @andreaskienast
[BUGFIX] Mark symfony/routing 4.2.7 as conflict
c374080 
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 nicolas-grekas mentioned this pull request Apr 19, 2019
Merged
nicolas-grekas added a commit that referenced this pull request Apr 19, 2019
@nicolas-grekas
bug #31182 [Routing] fix trailing slash matching with empty-matching …
bff2db7 
…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
Reviewers

@OskarStark OskarStark OskarStark left review comments

Assignees
No one assigned
Labels
Bug Routing Status: Needs Review
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