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] Account for greediness when merging route patterns #27388

Merged
merged 1 commit into from May 25, 2018

Conversation

Projects
None yet
4 participants
@nicolas-grekas
Member

nicolas-grekas commented May 25, 2018

Q A
Branch? 4.1
Bug fix? yes
New feature? no
BC breaks? no
Deprecations? no
Tests pass? yes
Fixed tickets #27385
License MIT
Doc PR -

Sub-patterns of variable length should not be considered as common prefixes because their greediness would break in-order matching.

@@ -103,6 +103,8 @@ public function addRoute(string $prefix, $route, string $staticPrefix = null)
if ($item instanceof self && $this->prefixes[$i] === $commonPrefix) {
// the new route is a child of a previous one, let's nest it
$item->addRoute($prefix, $route, $staticPrefix);
} elseif ($commonPrefix === $prefix && $commonStaticPrefix !== $prefix) {

This comment has been minimized.

@stof

stof May 25, 2018

Member

we should also have a comment describing this case in the algorithm

@stof

This comment has been minimized.

Member

stof commented May 25, 2018

These diffs on the dumped code are quite unreadable for reviewers...

@nicolas-grekas

This comment has been minimized.

Member

nicolas-grekas commented May 25, 2018

PR updated.
I cannot do much about the diff, but with the correct implementation, it is much shorter.
This is ready.

@nicolas-grekas nicolas-grekas merged commit d5a8237 into symfony:4.1 May 25, 2018

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

nicolas-grekas added a commit that referenced this pull request May 25, 2018

bug #27388 [Routing] Account for greediness when merging route patter…
…ns (nicolas-grekas)

This PR was merged into the 4.1 branch.

Discussion
----------

[Routing] Account for greediness when merging route patterns

| Q             | A
| ------------- | ---
| Branch?       | 4.1
| Bug fix?      | yes
| New feature?  | no
| BC breaks?    | no
| Deprecations? | no
| Tests pass?   | yes
| Fixed tickets | #27385
| License       | MIT
| Doc PR        | -

Sub-patterns of variable length should not be considered as common prefixes because their greediness would break in-order matching.

Commits
-------

d5a8237 [Routing] Account for greediness when merging route patterns

@nicolas-grekas nicolas-grekas deleted the nicolas-grekas:fix-route branch May 25, 2018

@kiler129

This comment has been minimized.

Contributor

kiler129 commented May 25, 2018

Thank you @nicolas-grekas

@fabpot fabpot referenced this pull request May 26, 2018

Merged

Release v4.1.0-BETA3 #27390

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment