8000 UrlMatcher BC break · Issue #31066 · symfony/symfony · GitHub
[go: up one dir, main page]
Skip to content

UrlMatcher BC break #31066

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
umpirsky opened this issue Apr 10, 2019 · 14 comments
Closed

UrlMatcher BC break #31066

umpirsky opened this issue Apr 10, 2019 · 14 comments

Comments

@umpirsky
Copy link
Contributor

Symfony version(s) affected: 4.1.11

Description
With version 4.1.2, urls with trailing slash were not matched, in 4.1.11 they are. I guess it's related to https://symfony.com/doc/4.1/routing.html#routing-trailing-slash-redirection, but it's a BC break.

How to reproduce

I created demo project https://github.com/umpirsky/SUMT which makes it easy to prove this two versions are not behaving the same way while using Symfony public API which should be stable across minor upgrades:

git clone git@github.com:umpirsky/SUMT.git
composer install
git checkout master
./vendor/bin/phpunit src/Umpirsky/SUMT/UrlMatcherTest.php
PHPUnit 8.1.2 by Sebastian Bergmann and contributors.

..                                                                  2 / 2 (100%)

Time: 28 ms, Memory: 4.00 MB

OK (2 tests, 2 assertions)

git checkout 4.1.11
composer install

./vendor/bin/phpunit src/Umpirsky/SUMT/UrlMatcherTest.php
PHPUnit 8.1.2 by Sebastian Bergmann and contributors.

.F                                                                  2 / 2 (100%)

Time: 33 ms, Memory: 4.00 MB

There was 1 failure:

1) Umpirsky\SUMT\UrlMatcherTest::testDontMatch
Failed asserting that exception of type "Symfony\Component\Routing\Exception\ResourceNotFoundException" is thrown.

FAILURES!
Tests: 2, Assertions: 2, Failures: 1.
@nicolas-grekas
Copy link
Member

Please upgrade to 4.2 as 4.1 is not maintained anymore and this might have been fixed already.

@umpirsky
Copy link
Contributor Author

Reproduced with 4.2.5:

git checkout 4.2
composer install

./vendor/bin/phpunit src/Umpirsky/SUMT/UrlMatcherTest.php
PHPUnit 8.1.2 by Sebastian Bergmann and contributors.

.F                                                                  2 / 2 (100%)

Time: 24 ms, Memory: 4.00 MB

There was 1 failure:

1) Umpirsky\SUMT\UrlMatcherTest::testDontMatch
Failed asserting that exception of type "Symfony\Component\Routing\Exception\ResourceNotFoundException" is thrown.

FAILURES!
Tests: 2, Assertions: 2, Failures: 1.

@nicolas-grekas
Copy link
Member

Is this the same as #30863?

@umpirsky
Copy link
Contributor Author

This does not have to do anything with redirection, but route matching. This can be related to #30863, but I'm not sure if it is.

@nicolas-grekas
Copy link
Member

I think we're not going to change the current behavior. There have been many back and forth on the topic and things are pretty stable right now around the current behavior. Sorry to ask you to do so, but please update/reorder your routes to make them work.

@umpirsky
Copy link
Contributor Author

I'm using UrlMatcher directly, I built a tool around it and relied on stable API. I need trailing slash urls to behave differently then regular ones, like they did in the old versions, no route reorder can prevent matching trailing slash urls.

If I define route with path /foo/{bar}, I expect it to match /foo/bar and not /foo/bar/.

@nicolas-grekas
Copy link
Member

With #31107, /foo/{bar} will /foo/bar/ redirect to /foo/bar, unless a preceding route takes over.

@umpirsky
Copy link
Contributor Author

Yes, and that is a BC break.

If it's not going to be fixed I have two options:

  1. Stick with 4.1.2
  2. Add some ugly workaround like this in my code:
+ if (ends_with($url, '/') && $url === rtrim($url, '/')) {
+     throw new ResourceNotFoundException();
+ }

$urlMatcher->match($url);

@nicolas-grekas
Copy link
Member
nicolas-grekas commented Apr 15, 2019 •
edited
Loading

Or updating the routes you define I believe. Without knowing more about your use case and your route collection, I'm not sure I can help you more, but the behavior change is likely not going to be reverted...

@umpirsky
Copy link
Contributor Author
umpirsky commented Apr 15, 2019 •
edited
Loading

OK, let's imagine https://github.com/umpirsky/SUMT/blob/master/src/Umpirsky/SUMT/UrlMatcherTest.php is my use case. How can route update keep the tests pass with the new Symfony version?

@nicolas-grekas
Copy link
Member

With #31107, tests pass on my machine.

@nicolas-grekas nicolas-grekas mentioned this issue Apr 15, 2019
Merged
@umpirsky
Copy link
Contributor Author
umpirsky commented Apr 15, 2019 •
edited
Loading

Great, thanks!

But it will never be merged into 4.1, right?

@nicolas-grekas
Copy link
Member

Nope: https://symfony.com/roadmap/4.1

@umpirsky
Copy link
Contributor Author

:(

fabpot pushed a commit that referenced this issue 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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
2 participants
@umpirsky @nicolas-grekas
0