bug #26253 [Routing] Fix suffix aggregation (nicolas-grekas)

This PR was merged into the 4.1-dev branch.

Discussion
----------

[Routing] Fix suffix aggregation

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

Commits
-------

1466597 [Routing] Fix suffix aggregation

src/Symfony/Component/Routing/Matcher/Dumper/StaticPrefixCollection.php

@@ -65,31 +65,57 @@ public function getRoutes(): array
      */
     public function addRoute(string $prefix, $route, string $staticPrefix = null)
     {
-        $this->guardAgainstAddingNotAcceptedRoutes($prefix);
         if (null === $staticPrefix) {
             list($prefix, $staticPrefix) = $this->getCommonPrefix($prefix, $prefix);
         }
 
         for ($i = \count($this->items) - 1; 0 <= $i; --$i) {
             $item = $this->items[$i];
 
-            if ($item instanceof self && $item->accepts($prefix)) {
-                $item->addRoute($prefix, $route, $staticPrefix);
+            list($commonPrefix, $commonStaticPrefix) = $this->getCommonPrefix($prefix, $this->prefixes[$i]);
 
-                return;
-            }
+            if ($this->prefix === $commonPrefix) {
+                // the new route and a previous one have no common prefix, let's see if they are exclusive to each others
 
-            if ($this->groupWithItem($i, $prefix, $staticPrefix, $route)) {
-                return;
-            }
+                if ($this->prefix !== $staticPrefix && $this->prefix !== $this->staticPrefixes[$i]) {
+                    // the new route and the previous one have exclusive static prefixes
+                    continue;
+                }
 
-            if ($this->staticPrefixes[$i] !== $this->prefixes[$i] && 0 === strpos($staticPrefix, $this->staticPrefixes[$i])) {
-                break;
+                if ($this->prefix === $staticPrefix && $this->prefix === $this->staticPrefixes[$i]) {
+                    // the new route and the previous one have no static prefix
+                    break;
+                }
+
+                if ($this->prefixes[$i] !== $this->staticPrefixes[$i] && $this->prefix === $this->staticPrefixes[$i]) {
+                    // the previous route is non-static and has no static prefix
+                    break;
+                }
+
+                if ($prefix !== $staticPrefix && $this->prefix === $staticPrefix) {
+                    // the new route is non-static and has no static prefix
+                    break;
+                }
+
+                continue;
             }
 
-            if ($staticPrefix !== $prefix && 0 === strpos($this->staticPrefixes[$i], $staticPrefix)) {
-                break;
+            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);
+            } else {
+                // the new route and a previous one have a common prefix, let's merge them
+                $child = new self($commonPrefix);
+                list($child->prefixes[0], $child->staticPrefixes[0]) = $child->getCommonPrefix($this->prefixes[$i], $this->prefixes[$i]);
+                list($child->prefixes[1], $child->staticPrefixes[1]) = $child->getCommonPrefix($prefix, $prefix);
+                $child->items = array($this->items[$i], $route);
+
+                $this->staticPrefixes[$i] = $commonStaticPrefix;
+                $this->prefixes[$i] = $commonPrefix;
+                $this->items[$i] = $child;
             }
+
+            return;
         }
 
         // No optimised case was found, in this case we simple add the route for possible
@@ -115,38 +141,6 @@ public function populateCollection(RouteCollection $routes): RouteCollection
         return $routes;
     }
 
-    /**
-     * Tries to combine a route with another route or group.
-     */
-    private function groupWithItem(int $i, string $prefix, string $staticPrefix, $route): bool
-    {
-        list($commonPrefix, $commonStaticPrefix) = $this->getCommonPrefix($prefix, $this->prefixes[$i]);
-
-        if (\strlen($this->prefix) >= \strlen($commonPrefix)) {
-            return false;
-        }
-
-        $child = new self($commonPrefix);
-
-        $child->staticPrefixes = array($this->staticPrefixes[$i], $staticPrefix);
-        $child->prefixes = array($this->prefixes[$i], $prefix);
-        $child->items = array($this->items[$i], $route);
-
-        $this->staticPrefixes[$i] = $commonStaticPrefix;
-        $this->prefixes[$i] = $commonPrefix;
-        $this->items[$i] = $child;
-
-        return true;
-    }
-
-    /**
-     * Checks whether a prefix can be contained within the group.
-     */
-    private function accepts(string $prefix): bool
-    {
-        return 0 === strpos($prefix, $this->prefix) && '?' !== ($prefix[\strlen($this->prefix)] ?? '');
-    }
-
     /**
      * Gets the full and static common prefixes between two route patterns.
      *
@@ -195,18 +189,4 @@ private function getCommonPrefix(string $prefix, string $anotherPrefix): array
 
         return array(substr($prefix, 0, $i), substr($prefix, 0, $staticLength ?? $i));
     }
-
-    /**
-     * Guards against adding incompatible prefixes in a group.
-     *
-     * @throws \LogicException when a prefix does not belong in a group
-     */
-    private function guardAgainstAddingNotAcceptedRoutes(string $prefix): void
-    {
-        if (!$this->accepts($prefix)) {
-            $message = sprintf('Could not add route with prefix %s to collection with prefix %s', $prefix, $this->prefix);
-
-            throw new \LogicException($message);
-        }
-    }
 }

src/Symfony/Component/Routing/Tests/Fixtures/dumper/url_matcher12.php

@@ -0,0 +1,91 @@
+<?php
+
+use Symfony\Component\Routing\Exception\MethodNotAllowedException;
+use Symfony\Component\Routing\Exception\ResourceNotFoundException;
+use Symfony\Component\Routing\RequestContext;
+
+/**
+ * This class has been auto-generated
+ * by the Symfony Routing Component.
+ */
+class ProjectUrlMatcher extends Symfony\Component\Routing\Matcher\UrlMatcher
+{
+    public function __construct(RequestContext $context)
+    {
+        $this->context = $context;
+    }
+
+    public function match($rawPathinfo)
+    {
+        $allow = array();
+        $pathinfo = rawurldecode($rawPathinfo);
+        $trimmedPathinfo = rtrim($pathinfo, '/');
+        $context = $this->context;
+        $requestMethod = $canonicalMethod = $context->getMethod();
+
+        if ('HEAD' === $requestMethod) {
+            $canonicalMethod = 'GET';
+        }
+
+        $matchedPathinfo = $pathinfo;
+        $regexList = array(
+            0 => '{^(?'
+                    .'|/abc([^/]++)(?'
+                        .'|/1(?'
+                            .'|(*:27)'
+                            .'|0(?'
+                                .'|(*:38)'
+                                .'|0(*:46)'
+                            .')'
+                        .')'
+                        .'|/2(?'
+                            .'|(*:60)'
+                            .'|0(?'
+                                .'|(*:71)'
+                                .'|0(*:79)'
+                            .')'
+                        .')'
+                    .')'
+                .')$}sD',
+        );
+
+        foreach ($regexList as $offset => $regex) {
+            while (preg_match($regex, $matchedPathinfo, $matches)) {
+                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),
+                            60 => array(array('_route' => 'r2'), array('foo'), null, null),
+                            71 => array(array('_route' => 'r20'), array('foo'), null, null),
+                            79 => array(array('_route' => 'r200'), array('foo'), null, null),
+                        );
+
+                        list($ret, $vars, $requiredMethods, $requiredSchemes) = $routes[$m];
+
+                        foreach ($vars as $i => $v) {
+                            if (isset($matches[1 + $i])) {
+                                $ret[$v] = $matches[1 + $i];
+                            }
+                        }
+
+                        if ($requiredMethods && !isset($requiredMethods[$canonicalMethod]) && !isset($requiredMethods[$requestMethod])) {
+                            $allow += $requiredMethods;
+                            break;
+                        }
+
+                        return $ret;
+                }
+
+                if (79 === $m) {
+                    break;
+                }
+                $regex = substr_replace($regex, 'F', $m - $offset, 1 + strlen($m));
+                $offset += strlen($m);
+            }
+        }
+
+        throw $allow ? new MethodNotAllowedException(array_keys($allow)) : new ResourceNotFoundException();
+    }
+}

src/Symfony/Component/Routing/Tests/Matcher/Dumper/PhpMatcherDumperTest.php

@@ -458,6 +458,15 @@ public function getRouteCollections()
         $demoCollection->addRequirements(array('_locale' => 'en|fr'));
         $demoCollection->addDefaults(array('_locale' => 'en'));
 
+        /* test case 12 */
+        $suffixCollection = new RouteCollection();
+        $suffixCollection->add('r1', new Route('abc{foo}/1'));
+        $suffixCollection->add('r2', new Route('abc{foo}/2'));
+        $suffixCollection->add('r10', new Route('abc{foo}/10'));
+        $suffixCollection->add('r20', new Route('abc{foo}/20'));
+        $suffixCollection->add('r100', new Route('abc{foo}/100'));
+        $suffixCollection->add('r200', new Route('abc{foo}/200'));
+
         return array(
            array(new RouteCollection(), 'url_matcher0.php', array()),
            array($collection, 'url_matcher1.php', array()),
@@ -471,6 +480,7 @@ public function getRouteCollections()
            array($hostTreeCollection, 'url_matcher9.php', array()),
            array($chunkedCollection, 'url_matcher10.php', array()),
            array($demoCollection, 'url_matcher11.php', array('base_class' => 'Symfony\Component\Routing\Tests\Fixtures\RedirectableUrlMatcher')),
+           array($suffixCollection, 'url_matcher12.php', array()),
         );
     }