From d5a8237b1cc73a7a75505fd3c8d2be6789c24a44 Mon Sep 17 00:00:00 2001
From: Nicolas Grekas <nicolas.grekas@gmail.com>
Date: Fri, 25 May 2018 18:30:16 +0200
Subject: [PATCH] [Routing] Account for greediness when merging route patterns

---
 .../Matcher/Dumper/StaticPrefixCollection.php | 12 +++++
 .../Tests/Fixtures/dumper/url_matcher11.php   | 54 +++++++++----------
 .../Routing/Tests/Matcher/UrlMatcherTest.php  | 10 ++++
 3 files changed, 47 insertions(+), 29 deletions(-)

diff --git a/src/Symfony/Component/Routing/Matcher/Dumper/StaticPrefixCollection.php b/src/Symfony/Component/Routing/Matcher/Dumper/StaticPrefixCollection.php
index 05315defcc..cd9e6b4c3e 100644
--- a/src/Symfony/Component/Routing/Matcher/Dumper/StaticPrefixCollection.php
+++ b/src/Symfony/Component/Routing/Matcher/Dumper/StaticPrefixCollection.php
@@ -151,6 +151,7 @@ class StaticPrefixCollection
         $baseLength = \strlen($this->prefix);
         $end = min(\strlen($prefix), \strlen($anotherPrefix));
         $staticLength = null;
+        set_error_handler(array(__CLASS__, 'handleError'));
 
         for ($i = $baseLength; $i < $end && $prefix[$i] === $anotherPrefix[$i]; ++$i) {
             if ('(' === $prefix[$i]) {
@@ -174,13 +175,24 @@ class StaticPrefixCollection
                 if (('?' === ($prefix[$j] ?? '') || '?' === ($anotherPrefix[$j] ?? '')) && ($prefix[$j] ?? '') !== ($anotherPrefix[$j] ?? '')) {
                     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;
             } elseif ('\\' === $prefix[$i] && (++$i === $end || $prefix[$i] !== $anotherPrefix[$i])) {
                 --$i;
                 break;
             }
         }
+        restore_error_handler();
 
         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');
+    }
 }
diff --git a/src/Symfony/Component/Routing/Tests/Fixtures/dumper/url_matcher11.php b/src/Symfony/Component/Routing/Tests/Fixtures/dumper/url_matcher11.php
index a3cbc9e67b..c0bbc582d4 100644
--- a/src/Symfony/Component/Routing/Tests/Fixtures/dumper/url_matcher11.php
+++ b/src/Symfony/Component/Routing/Tests/Fixtures/dumper/url_matcher11.php
@@ -68,30 +68,26 @@ class ProjectUrlMatcher extends Symfony\Component\Routing\Tests\Fixtures\Redirec
                         .'|admin/post/(?'
                             .'|(*:33)'
                             .'|new(*:43)'
-                            .'|(\\d+)(?'
-                                .'|(*:58)'
-                                .'|/(?'
-                                    .'|edit(*:73)'
-                                    .'|delete(*:86)'
-                                .')'
-                            .')'
+                            .'|(\\d+)(*:55)'
+                            .'|(\\d+)/edit(*:72)'
+                            .'|(\\d+)/delete(*:91)'
                         .')'
                         .'|blog/(?'
-                            .'|(*:104)'
-                            .'|rss\\.xml(*:120)'
+                            .'|(*:107)'
+                            .'|rss\\.xml(*:123)'
                             .'|p(?'
-                                .'|age/([^/]++)(*:144)'
-                                .'|osts/([^/]++)(*:165)'
+                                .'|age/([^/]++)(*:147)'
+                                .'|osts/([^/]++)(*:168)'
                             .')'
-                            .'|comments/(\\d+)/new(*:192)'
-                            .'|search(*:206)'
+                            .'|comments/(\\d+)/new(*:195)'
+                            .'|search(*:209)'
                         .')'
                         .'|log(?'
-                            .'|in(*:223)'
-                            .'|out(*:234)'
+                            .'|in(*:226)'
+                            .'|out(*:237)'
                         .')'
                     .')'
-                    .'|/(en|fr)?(*:253)'
+                    .'|/(en|fr)?(*:256)'
                 .')$}sD',
         );
 
@@ -102,18 +98,18 @@ class ProjectUrlMatcher extends Symfony\Component\Routing\Tests\Fixtures\Redirec
                         $routes = array(
                             33 => array(array('_route' => 'a', '_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),
-                            73 => array(array('_route' => 'd', '_locale' => 'en'), array('_locale', 'id'), null, null),
-                            86 => array(array('_route' => 'e', '_locale' => 'en'), array('_locale', 'id'), null, null),
-                            104 => array(array('_route' => 'f', '_locale' => 'en'), array('_locale'), null, null),
-                            120 => array(array('_route' => 'g', '_locale' => 'en'), array('_locale'), null, null),
-                            144 => array(array('_route' => 'h', '_locale' => 'en'), array('_locale', 'page'), null, null),
-                            165 => array(array('_route' => 'i', '_locale' => 'en'), array('_locale', 'page'), null, null),
-                            192 => array(array('_route' => 'j', '_locale' => 'en'), array('_locale', 'id'), null, null),
-                            206 => array(array('_route' => 'k', '_locale' => 'en'), array('_locale'), null, null),
-                            223 => array(array('_route' => 'l', '_locale' => 'en'), array('_locale'), null, null),
-                            234 => array(array('_route' => 'm', '_locale' => 'en'), array('_locale'), null, null),
-                            253 => array(array('_route' => 'n', '_locale' => 'en'), array('_locale'), null, null),
+                            55 => array(array('_route' => 'c', '_locale' => 'en'), array('_locale', 'id'), null, null),
+                            72 => array(array('_route' => 'd', '_locale' => 'en'), array('_locale', 'id'), null, null),
+                            91 => array(array('_route' => 'e', '_locale' => 'en'), array('_locale', 'id'), null, null),
+                            107 => array(array('_route' => 'f', '_locale' => 'en'), array('_locale'), null, null),
+                            123 => array(array('_route' => 'g', '_locale' => 'en'), array('_locale'), null, null),
+                            147 => array(array('_route' => 'h', '_locale' => 'en'), array('_locale', 'page'), null, null),
+                            168 => array(array('_route' => 'i', '_locale' => 'en'), array('_locale', 'page'), null, null),
+                            195 => array(array('_route' => 'j', '_locale' => 'en'), array('_locale', 'id'), null, null),
+                            209 => array(array('_route' => 'k', '_locale' => 'en'), array('_locale'), null, null),
+                            226 => array(array('_route' => 'l', '_locale' => 'en'), array('_locale'), null, null),
+                            237 => array(array('_route' => 'm', '_locale' => 'en'), array('_locale'), null, null),
+                            256 => array(array('_route' => 'n', '_locale' => 'en'), array('_locale'), null, null),
                         );
 
                         list($ret, $vars, $requiredMethods, $requiredSchemes) = $routes[$m];
@@ -139,7 +135,7 @@ class ProjectUrlMatcher extends Symfony\Component\Routing\Tests\Fixtures\Redirec
                         return $ret;
                 }
 
-                if (253 === $m) {
+                if (256 === $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
index 0ba61c9486..b3bfd78271 100644
--- a/src/Symfony/Component/Routing/Tests/Matcher/UrlMatcherTest.php
+++ b/src/Symfony/Component/Routing/Tests/Matcher/UrlMatcherTest.php
@@ -611,6 +611,16 @@ class UrlMatcherTest extends TestCase
         $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)
     {
         return new UrlMatcher($routes, $context ?: new RequestContext());