diogo/symfony
Archived
1
0
Fork 0
Code Issues Pull Requests Packages Projects Releases Wiki Activity

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

Browse Source 
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
-------

d5a8237b1c [Routing] Account for greediness when merging route patterns
This commit is contained in:
Nicolas Grekas 2018-05-25 23:27:30 +02:00
commit 9e6fbe8fdb
3 changed files with 47 additions and 29 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

12
src/Symfony/Component/Routing/Matcher/Dumper/StaticPrefixCollection.php
View File

@ -151,6 +151,7 @@ class StaticPrefixCollection
$baseLength = \strlen($this->prefix); $baseLength = \strlen($this->prefix);
$end = min(\strlen($prefix), \strlen($anotherPrefix)); $end = min(\strlen($prefix), \strlen($anotherPrefix));
$staticLength = null; $staticLength = null;
set_error_handler(array(__CLASS__, 'handleError'));
for ($i = $baseLength; $i < $end && $prefix[$i] === $anotherPrefix[$i]; ++$i) { for ($i = $baseLength; $i < $end && $prefix[$i] === $anotherPrefix[$i]; ++$i) {
if ('(' === $prefix[$i]) { if ('(' === $prefix[$i]) {
@ -174,13 +175,24 @@ class StaticPrefixCollection
if (('?' === ($prefix[$j] ?? '') || '?' === ($anotherPrefix[$j] ?? '')) && ($prefix[$j] ?? '') !== ($anotherPrefix[$j] ?? '')) { if (('?' === ($prefix[$j] ?? '') || '?' === ($anotherPrefix[$j] ?? '')) && ($prefix[$j] ?? '') !== ($anotherPrefix[$j] ?? '')) {
break; break;
} }
$subPattern = substr($prefix, $i, $j - $i);
if ($prefix !== $anotherPrefix && !preg_match('/^\(\[[^\]]++\]\+\+\)$/', $subPattern) && !preg_match('{(?<!'.$subPattern.')}', '')) {
// sub-patterns of variable length are not considered as common prefixes because their greediness would break in-order matching
break;
}
$i = $j - 1; $i = $j - 1;
} elseif ('\\' === $prefix[$i] && (++$i === $end || $prefix[$i] !== $anotherPrefix[$i])) { } elseif ('\\' === $prefix[$i] && (++$i === $end || $prefix[$i] !== $anotherPrefix[$i])) {
--$i; --$i;
break; break;
} }
} }
restore_error_handler();
return array(substr($prefix, 0, $i), substr($prefix, 0, $staticLength ?? $i)); return array(substr($prefix, 0, $i), substr($prefix, 0, $staticLength ?? $i));
} }
public static function handleError($type, $msg)
{
return 0 === strpos($msg, 'preg_match(): Compilation failed: lookbehind assertion is not fixed length');
}
} }

54
src/Symfony/Component/Routing/Tests/Fixtures/dumper/url_matcher11.php
View File

@ -68,30 +68,26 @@ class ProjectUrlMatcher extends Symfony\Component\Routing\Tests\Fixtures\Redirec
.'|admin/post/(?' .'|admin/post/(?'
.'|(*:33)' .'|(*:33)'
.'|new(*:43)' .'|new(*:43)'
.'|(\\d+)(?' .'|(\\d+)(*:55)'
.'|(*:58)' .'|(\\d+)/edit(*:72)'
.'|/(?' .'|(\\d+)/delete(*:91)'
.'|edit(*:73)'
.'|delete(*:86)'
.')'
.')'
.')' .')'
.'|blog/(?' .'|blog/(?'
.'|(*:104)' .'|(*:107)'
.'|rss\\.xml(*:120)' .'|rss\\.xml(*:123)'
.'|p(?' .'|p(?'
.'|age/([^/]++)(*:144)' .'|age/([^/]++)(*:147)'
.'|osts/([^/]++)(*:165)' .'|osts/([^/]++)(*:168)'
.')' .')'
.'|comments/(\\d+)/new(*:192)' .'|comments/(\\d+)/new(*:195)'
.'|search(*:206)' .'|search(*:209)'
.')' .')'
.'|log(?' .'|log(?'
.'|in(*:223)' .'|in(*:226)'
.'|out(*:234)' .'|out(*:237)'
.')' .')'
.')' .')'
.'|/(en|fr)?(*:253)' .'|/(en|fr)?(*:256)'
.')$}sD', .')$}sD',
); );
@ -102,18 +98,18 @@ class ProjectUrlMatcher extends Symfony\Component\Routing\Tests\Fixtures\Redirec
$routes = array( $routes = array(
33 => array(array('_route' => 'a', '_locale' => 'en'), array('_locale'), null, null), 33 => array(array('_route' => 'a', '_locale' => 'en'), array('_locale'), null, null),
43 => array(array('_route' => 'b', '_locale' => 'en'), array('_locale'), null, null), 43 => array(array('_route' => 'b', '_locale' => 'en'), array('_locale'), null, null),
58 => array(array('_route' => 'c', '_locale' => 'en'), array('_locale', 'id'), null, null), 55 => array(array('_route' => 'c', '_locale' => 'en'), array('_locale', 'id'), null, null),
73 => array(array('_route' => 'd', '_locale' => 'en'), array('_locale', 'id'), null, null), 72 => array(array('_route' => 'd', '_locale' => 'en'), array('_locale', 'id'), null, null),
86 => array(array('_route' => 'e', '_locale' => 'en'), array('_locale', 'id'), null, null), 91 => array(array('_route' => 'e', '_locale' => 'en'), array('_locale', 'id'), null, null),
104 => array(array('_route' => 'f', '_locale' => 'en'), array('_locale'), null, null), 107 => array(array('_route' => 'f', '_locale' => 'en'), array('_locale'), null, null),
120 => array(array('_route' => 'g', '_locale' => 'en'), array('_locale'), null, null), 123 => array(array('_route' => 'g', '_locale' => 'en'), array('_locale'), null, null),
144 => array(array('_route' => 'h', '_locale' => 'en'), array('_locale', 'page'), null, null), 147 => array(array('_route' => 'h', '_locale' => 'en'), array('_locale', 'page'), null, null),
165 => array(array('_route' => 'i', '_locale' => 'en'), array('_locale', 'page'), null, null), 168 => array(array('_route' => 'i', '_locale' => 'en'), array('_locale', 'page'), null, null),
192 => array(array('_route' => 'j', '_locale' => 'en'), array('_locale', 'id'), null, null), 195 => array(array('_route' => 'j', '_locale' => 'en'), array('_locale', 'id'), null, null),
206 => array(array('_route' => 'k', '_locale' => 'en'), array('_locale'), null, null), 209 => array(array('_route' => 'k', '_locale' => 'en'), array('_locale'), null, null),
223 => array(array('_route' => 'l', '_locale' => 'en'), array('_locale'), null, null), 226 => array(array('_route' => 'l', '_locale' => 'en'), array('_locale'), null, null),
234 => array(array('_route' => 'm', '_locale' => 'en'), array('_locale'), null, null), 237 => array(array('_route' => 'm', '_locale' => 'en'), array('_locale'), null, null),
253 => array(array('_route' => 'n', '_locale' => 'en'), array('_locale'), null, null), 256 => array(array('_route' => 'n', '_locale' => 'en'), array('_locale'), null, null),
); );
list($ret, $vars, $requiredMethods, $requiredSchemes) = $routes[$m]; list($ret, $vars, $requiredMethods, $requiredSchemes) = $routes[$m];
@ -139,7 +135,7 @@ class ProjectUrlMatcher extends Symfony\Component\Routing\Tests\Fixtures\Redirec
return $ret; return $ret;
} }
if (253 === $m) { if (256 === $m) {
break; break;
} }
$regex = substr_replace($regex, 'F', $m - $offset, 1 + strlen($m)); $regex = substr_replace($regex, 'F', $m - $offset, 1 + strlen($m));

10
src/Symfony/Component/Routing/Tests/Matcher/UrlMatcherTest.php
View File

@ -611,6 +611,16 @@ class UrlMatcherTest extends TestCase
$this->assertEquals(array('_route' => 'a', 'a' => 'a', 'b' => 'b'), $matcher->match('/a/b')); $this->assertEquals(array('_route' => 'a', 'a' => 'a', 'b' => 'b'), $matcher->match('/a/b'));
} }
public function testDotAllWithCatchAll()
{
$coll = new RouteCollection();
$coll->add('a', new Route('/{id}.html', array(), array('id' => '.+')));
$coll->add('b', new Route('/{all}', array(), array('all' => '.+')));
$matcher = $this->getUrlMatcher($coll);
$this->assertEquals(array('_route' => 'a', 'id' => 'foo/bar'), $matcher->match('/foo/bar.html'));
}
protected function getUrlMatcher(RouteCollection $routes, RequestContext $context = null) protected function getUrlMatcher(RouteCollection $routes, RequestContext $context = null)
{ {
return new UrlMatcher($routes, $context ?: new RequestContext()); return new UrlMatcher($routes, $context ?: new RequestContext());