From 1466597bed06a3f89ba0b6029b58903fe5f34afe Mon Sep 17 00:00:00 2001 From: Nicolas Grekas Date: Wed, 21 Feb 2018 09:48:44 +0100 Subject: [PATCH] [Routing] Fix suffix aggregation --- .../Matcher/Dumper/StaticPrefixCollection.php | 96 ++++++++----------- .../Tests/Fixtures/dumper/url_matcher12.php | 91 ++++++++++++++++++ .../Matcher/Dumper/PhpMatcherDumperTest.php | 10 ++ 3 files changed, 139 insertions(+), 58 deletions(-) create mode 100644 src/Symfony/Component/Routing/Tests/Fixtures/dumper/url_matcher12.php diff --git a/src/Symfony/Component/Routing/Matcher/Dumper/StaticPrefixCollection.php b/src/Symfony/Component/Routing/Matcher/Dumper/StaticPrefixCollection.php index a8ee045fe9bc7..014b3c9fdf84a 100644 --- a/src/Symfony/Component/Routing/Matcher/Dumper/StaticPrefixCollection.php +++ b/src/Symfony/Component/Routing/Matcher/Dumper/StaticPrefixCollection.php @@ -65,7 +65,6 @@ 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); } @@ -73,23 +72,50 @@ public function addRoute(string $prefix, $route, string $staticPrefix = null) 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); - } - } } diff --git a/src/Symfony/Component/Routing/Tests/Fixtures/dumper/url_matcher12.php b/src/Symfony/Component/Routing/Tests/Fixtures/dumper/url_matcher12.php new file mode 100644 index 0000000000000..5850cbb07685c --- /dev/null +++ b/src/Symfony/Component/Routing/Tests/Fixtures/dumper/url_matcher12.php @@ -0,0 +1,91 @@ +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(); + } +} diff --git a/src/Symfony/Component/Routing/Tests/Matcher/Dumper/PhpMatcherDumperTest.php b/src/Symfony/Component/Routing/Tests/Matcher/Dumper/PhpMatcherDumperTest.php index 6032fbdd3c03b..daf79c38aede7 100644 --- a/src/Symfony/Component/Routing/Tests/Matcher/Dumper/PhpMatcherDumperTest.php +++ b/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()), ); }