Skip to content

Commit 48c3d6e

Browse files
nicolas-grekasnicolas-grekas
committed
[Routing] Account for greediness when merging route patterns
1 parent 54c81d2 commit 48c3d6e

File tree

3 files changed

+47
-29
lines changed

3 files changed

+47
-29
lines changed

‎Matcher/Dumper/StaticPrefixCollection.php‎

Lines changed: 12 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -151,6 +151,7 @@ private function getCommonPrefix(string $prefix, string $anotherPrefix): array
151151
$baseLength = \strlen($this->prefix);
152152
$end = min(\strlen($prefix), \strlen($anotherPrefix));
153153
$staticLength = null;
154+
set_error_handler(array(__CLASS__, 'handleError'));
154155

155156
for ($i = $baseLength; $i < $end && $prefix[$i] === $anotherPrefix[$i]; ++$i) {
156157
if ('(' === $prefix[$i]) {
@@ -174,13 +175,24 @@ private function getCommonPrefix(string $prefix, string $anotherPrefix): array
174175
if (('?' === ($prefix[$j] ?? '') || '?' === ($anotherPrefix[$j] ?? '')) && ($prefix[$j] ?? '') !== ($anotherPrefix[$j] ?? '')) {
175176
break;
176177
}
178+
$subPattern = substr($prefix, $i, $j - $i);
179+
if ($prefix !== $anotherPrefix && !preg_match('/^\(\[[^\]]++\]\+\+\)$/', $subPattern) && !preg_match('{(?<!'.$subPattern.')}', '')) {
180+
// sub-patterns of variable length are not considered as common prefixes because their greediness would break in-order matching
181+
break;
182+
}
177183
$i = $j - 1;
178184
} elseif ('\\' === $prefix[$i] && (++$i === $end || $prefix[$i] !== $anotherPrefix[$i])) {
179185
--$i;
180186
break;
181187
}
182188
}
189+
restore_error_handler();
183190

184191
return array(substr($prefix, 0, $i), substr($prefix, 0, $staticLength ?? $i));
185192
}
193+
194+
public static function handleError($type, $msg)
195+
{
196+
return 0 === strpos($msg, 'preg_match(): Compilation failed: lookbehind assertion is not fixed length');
197+
}
186198
}

‎Tests/Fixtures/dumper/url_matcher11.php‎

Lines changed: 25 additions & 29 deletions
Original file line numberDiff line numberDiff line change
@@ -68,30 +68,26 @@ private function doMatch(string $rawPathinfo, array &$allow = array(), array &$a
6868
.'|admin/post/(?'
6969
.'|(*:33)'
7070
.'|new(*:43)'
71-
.'|(\\d+)(?'
72-
.'|(*:58)'
73-
.'|/(?'
74-
.'|edit(*:73)'
75-
.'|delete(*:86)'
76-
.')'
77-
.')'
71+
.'|(\\d+)(*:55)'
72+
.'|(\\d+)/edit(*:72)'
73+
.'|(\\d+)/delete(*:91)'
7874
.')'
7975
.'|blog/(?'
80-
.'|(*:104)'
81-
.'|rss\\.xml(*:120)'
76+
.'|(*:107)'
77+
.'|rss\\.xml(*:123)'
8278
.'|p(?'
83-
.'|age/([^/]++)(*:144)'
84-
.'|osts/([^/]++)(*:165)'
79+
.'|age/([^/]++)(*:147)'
80+
.'|osts/([^/]++)(*:168)'
8581
.')'
86-
.'|comments/(\\d+)/new(*:192)'
87-
.'|search(*:206)'
82+
.'|comments/(\\d+)/new(*:195)'
83+
.'|search(*:209)'
8884
.')'
8985
.'|log(?'
90-
.'|in(*:223)'
91-
.'|out(*:234)'
86+
.'|in(*:226)'
87+
.'|out(*:237)'
9288
.')'
9389
.')'
94-
.'|/(en|fr)?(*:253)'
90+
.'|/(en|fr)?(*:256)'
9591
.')$}sD',
9692
);
9793

@@ -102,18 +98,18 @@ private function doMatch(string $rawPathinfo, array &$allow = array(), array &$a
10298
$routes = array(
10399
33 => array(array('_route' => 'a', '_locale' => 'en'), array('_locale'), null, null),
104100
43 => array(array('_route' => 'b', '_locale' => 'en'), array('_locale'), null, null),
105-
58 => array(array('_route' => 'c', '_locale' => 'en'), array('_locale', 'id'), null, null),
106-
73 => array(array('_route' => 'd', '_locale' => 'en'), array('_locale', 'id'), null, null),
107-
86 => array(array('_route' => 'e', '_locale' => 'en'), array('_locale', 'id'), null, null),
108-
104 => array(array('_route' => 'f', '_locale' => 'en'), array('_locale'), null, null),
109-
120 => array(array('_route' => 'g', '_locale' => 'en'), array('_locale'), null, null),
110-
144 => array(array('_route' => 'h', '_locale' => 'en'), array('_locale', 'page'), null, null),
111-
165 => array(array('_route' => 'i', '_locale' => 'en'), array('_locale', 'page'), null, null),
112-
192 => array(array('_route' => 'j', '_locale' => 'en'), array('_locale', 'id'), null, null),
113-
206 => array(array('_route' => 'k', '_locale' => 'en'), array('_locale'), null, null),
114-
223 => array(array('_route' => 'l', '_locale' => 'en'), array('_locale'), null, null),
115-
234 => array(array('_route' => 'm', '_locale' => 'en'), array('_locale'), null, null),
116-
253 => array(array('_route' => 'n', '_locale' => 'en'), array('_locale'), null, null),
101+
55 => array(array('_route' => 'c', '_locale' => 'en'), array('_locale', 'id'), null, null),
102+
72 => array(array('_route' => 'd', '_locale' => 'en'), array('_locale', 'id'), null, null),
103+
91 => array(array('_route' => 'e', '_locale' => 'en'), array('_locale', 'id'), null, null),
104+
107 => array(array('_route' => 'f', '_locale' => 'en'), array('_locale'), null, null),
105+
123 => array(array('_route' => 'g', '_locale' => 'en'), array('_locale'), null, null),
106+
147 => array(array('_route' => 'h', '_locale' => 'en'), array('_locale', 'page'), null, null),
107+
168 => array(array('_route' => 'i', '_locale' => 'en'), array('_locale', 'page'), null, null),
108+
195 => array(array('_route' => 'j', '_locale' => 'en'), array('_locale', 'id'), null, null),
109+
209 => array(array('_route' => 'k', '_locale' => 'en'), array('_locale'), null, null),
110+
226 => array(array('_route' => 'l', '_locale' => 'en'), array('_locale'), null, null),
111+
237 => array(array('_route' => 'm', '_locale' => 'en'), array('_locale'), null, null),
112+
256 => array(array('_route' => 'n', '_locale' => 'en'), array('_locale'), null, null),
117113
);
118114

119115
list($ret, $vars, $requiredMethods, $requiredSchemes) = $routes[$m];
@@ -139,7 +135,7 @@ private function doMatch(string $rawPathinfo, array &$allow = array(), array &$a
139135
return $ret;
140136
}
141137

142-
if (253 === $m) {
138+
if (256 === $m) {
143139
break;
144140
}
145141
$regex = substr_replace($regex, 'F', $m - $offset, 1 + strlen($m));

‎Tests/Matcher/UrlMatcherTest.php‎

Lines changed: 10 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -611,6 +611,16 @@ public function testRequirementWithCapturingGroup()
611611
$this->assertEquals(array('_route' => 'a', 'a' => 'a', 'b' => 'b'), $matcher->match('/a/b'));
612612
}
613613

614+
public function testDotAllWithCatchAll()
615+
{
616+
$coll = new RouteCollection();
617+
$coll->add('a', new Route('/{id}.html', array(), array('id' => '.+')));
618+
$coll->add('b', new Route('/{all}', array(), array('all' => '.+')));
619+
620+
$matcher = $this->getUrlMatcher($coll);
621+
$this->assertEquals(array('_route' => 'a', 'id' => 'foo/bar'), $matcher->match('/foo/bar.html'));
622+
}
623+
614624
protected function getUrlMatcher(RouteCollection $routes, RequestContext $context = null)
615625
{
616626
return new UrlMatcher($routes, $context ?: new RequestContext());

0 commit comments

Comments
 (0)