bug #27498 [Routing] Don't reorder past variable-length placeholders …

…(nanocom, nicolas-grekas) This PR was merged into the 4.1 branch. Discussion ---------- [Routing] Don't reorder past variable-length placeholders | Q | A | ------------- | --- | Branch? | 4.1 | Bug fix? | yes | New feature? | no | BC breaks? | no | Deprecations? | no | Tests pass? | no | Fixed tickets | #27491 | License | MIT Commits ------- 44616d9 [Router] regression when matching a route 7a750d4 [Routing] Don't reorder past variable-length placeholders
symfony · Jun 4, 2018 · 2521e7b · 2521e7b
2 parents 7605706 + 44616d9
commit 2521e7b
Show file tree

Hide file tree

Showing 3 changed files with 27 additions and 23 deletions.
diff --git a/src/Symfony/Component/Routing/Matcher/Dumper/StaticPrefixCollection.php b/src/Symfony/Component/Routing/Matcher/Dumper/StaticPrefixCollection.php
@@ -176,7 +176,7 @@ private function getCommonPrefix(string $prefix, string $anotherPrefix): array
                     break;
                 }
                 $subPattern = substr($prefix, $i, $j - $i);
-                if ($prefix !== $anotherPrefix && !preg_match('/^\(\[[^\]]++\]\+\+\)$/', $subPattern) && !preg_match('{(?<!'.$subPattern.')}', '')) {
+                if ($prefix !== $anotherPrefix && !preg_match('{(?<!'.$subPattern.')}', '')) {
                     // sub-patterns of variable length are not considered as common prefixes because their greediness would break in-order matching
                     break;
                 }

diff --git a/src/Symfony/Component/Routing/Tests/Fixtures/dumper/url_matcher12.php b/src/Symfony/Component/Routing/Tests/Fixtures/dumper/url_matcher12.php
@@ -29,21 +29,13 @@ public function match($rawPathinfo)
         $matchedPathinfo = $pathinfo;
         $regexList = array(
             0 => '{^(?'
-                    .'|/abc([^/]++)/(?'
-                        .'|1(?'
-                            .'|(*:27)'
-                            .'|0(?'
-                                .'|(*:38)'
-                                .'|0(*:46)'
-                            .')'
-                        .')'
-                        .'|2(?'
-                            .'|(*:59)'
-                            .'|0(?'
-                                .'|(*:70)'
-                                .'|0(*:78)'
-                            .')'
-                        .')'
+                    .'|/abc(?'
+                        .'|([^/]++)/1(*:24)'
+                        .'|([^/]++)/2(*:41)'
+                        .'|([^/]++)/10(*:59)'
+                        .'|([^/]++)/20(*:77)'
+                        .'|([^/]++)/100(*:96)'
+                        .'|([^/]++)/200(*:115)'
                     .')'
                 .')$}sD',
         );
@@ -53,12 +45,12 @@ public function match($rawPathinfo)
                 switch ($m = (int) $matches['MARK']) {
                     default:
                         $routes = array(
-                            27 => array(array('_route' => 'r1'), array('foo'), null, null),
-                            38 => array(array('_route' => 'r10'), array('foo'), null, null),
-                            46 => array(array('_route' => 'r100'), array('foo'), null, null),
-                            59 => array(array('_route' => 'r2'), array('foo'), null, null),
-                            70 => array(array('_route' => 'r20'), array('foo'), null, null),
-                            78 => array(array('_route' => 'r200'), array('foo'), null, null),
+                            24 => array(array('_route' => 'r1'), array('foo'), null, null),
+                            41 => array(array('_route' => 'r2'), array('foo'), null, null),
+                            59 => array(array('_route' => 'r10'), array('foo'), null, null),
+                            77 => array(array('_route' => 'r20'), array('foo'), null, null),
+                            96 => array(array('_route' => 'r100'), array('foo'), null, null),
+                            115 => array(array('_route' => 'r200'), array('foo'), null, null),
                         );
 
                         list($ret, $vars, $requiredMethods, $requiredSchemes) = $routes[$m];
@@ -84,7 +76,7 @@ public function match($rawPathinfo)
                         return $ret;
                 }
 
-                if (78 === $m) {
+                if (115 === $m) {
                     break;
                 }
                 $regex = substr_replace($regex, 'F', $m - $offset, 1 + strlen($m));

diff --git a/src/Symfony/Component/Routing/Tests/Matcher/UrlMatcherTest.php b/src/Symfony/Component/Routing/Tests/Matcher/UrlMatcherTest.php
@@ -245,6 +245,18 @@ public function testMatchRegression()
         }
     }
 
+    public function testMultipleParams()
+    {
+        $coll = new RouteCollection();
+        $coll->add('foo1', new Route('/foo/{a}/{b}'));
+        $coll->add('foo2', new Route('/foo/{a}/test/test/{b}'));
+        $coll->add('foo3', new Route('/foo/{a}/{b}/{c}/{d}'));
+
+        $route = $this->getUrlMatcher($coll)->match('/foo/test/test/test/bar')['_route'];
+
+        $this->assertEquals('foo2', $route);
+    }
+
     public function testDefaultRequirementForOptionalVariables()
     {
         $coll = new RouteCollection();