diff --git a/core/lib/Drupal/Core/Routing/PathMatcher.php b/core/lib/Drupal/Core/Routing/PathMatcher.php index a1ef5f71f5060eff50e63226d4a876fea23c2570..b888446c598d641ddf034964ff06dd12458e5ce2 100644 --- a/core/lib/Drupal/Core/Routing/PathMatcher.php +++ b/core/lib/Drupal/Core/Routing/PathMatcher.php @@ -79,20 +79,26 @@ public function matchRequestPartial(Request $request) { * An array of outlines that could match the specified path parts. */ public function getCandidateOutlines(array $parts) { - $number_parts = count($parts); + $ancestors = array(); $length = $number_parts - 1; $end = (1 << $number_parts) - 1; - $candidates = array(); - - $start = pow($number_parts-1, 2); // The highest possible mask is a 1 bit for every part of the path. We will // check every value down from there to generate a possible outline. - $masks = range($end, $start); + $masks = range($end, pow($number_parts - 1, 2)); + // Only examine patterns that actually exist as router items (the masks). foreach ($masks as $i) { - $current = '/'; + if ($i > $end) { + // Only look at masks that are not longer than the path of interest. + continue; + } + elseif ($i < (1 << $length)) { + // We have exhausted the masks of a given length, so decrease the length. + --$length; + } + $current = ''; for ($j = $length; $j >= 0; $j--) { // Check the bit on the $j offset. if ($i & (1 << $j)) { @@ -108,10 +114,9 @@ public function getCandidateOutlines(array $parts) { $current .= '/'; } } - $candidates[] = $current; + $ancestors[] = '/' . $current; } - - return $candidates; + return $ancestors; } } diff --git a/core/lib/Drupal/Core/Routing/RouteCompiler.php b/core/lib/Drupal/Core/Routing/RouteCompiler.php index d3f9b4c573e0a4bfcbb382db97ca0a03c86c77e8..6af19e734e8f6f3d4fec84c5f61da811981b0e4e 100644 --- a/core/lib/Drupal/Core/Routing/RouteCompiler.php +++ b/core/lib/Drupal/Core/Routing/RouteCompiler.php @@ -28,9 +28,11 @@ class RouteCompiler implements RouteCompilerInterface { */ public function compile(Route $route) { - $fit = $this->getFit($route->getPattern()); + $stripped_path = $this->getPathWithoutDefaults($route); - $pattern_outline = $this->getPatternOutline($route->getPattern()); + $fit = $this->getFit($stripped_path); + + $pattern_outline = $this->getPatternOutline($stripped_path); $num_parts = count(explode('/', trim($pattern_outline, '/'))); @@ -159,10 +161,10 @@ private function computeRegexp(array $tokens, $index, $firstOptional) { * Returns the pattern outline. * * The pattern outline is the path pattern but normalized so that all - * placeholders are equal strings. + * placeholders are equal strings and default values are removed. * * @param string $path - * The path pattern to normalize to an outline. + * The path for which we want the normalized outline. * * @return string * The path pattern outline. @@ -181,7 +183,6 @@ public function getPatternOutline($path) { * The fitness of the path, as an integer. */ public function getFit($path) { - $parts = explode('/', trim($path, '/'), static::MAX_PARTS); $number_parts = count($parts); // We store the highest index of parts here to save some work in the fit @@ -197,5 +198,34 @@ public function getFit($path) { return $fit; } + + /** + * Returns the path of the route, without placeholders with a default value. + * + * When computing the path outline and fit, we want to skip default-value + * placeholders. If we didn't, the path would never match. Note that this + * only works for placeholders at the end of the path. Infix placeholders + * with default values don't make sense anyway, so that should not be a + * problem. + * + * @param Route $route + * + * @return string + * The path string, stripped of placeholders that have default values. + */ + protected function getPathWithoutDefaults(Route $route) { + $path = $route->getPattern(); + $defaults = $route->getDefaults(); + + // Remove placeholders with default values from the outline, so that they + // will still match. + $remove = array_map(function($a) { + return '/{' . $a . '}'; + }, array_keys($defaults)); + $path = str_replace($remove, '', $path); + + return $path; + } + } diff --git a/core/modules/system/lib/Drupal/system/Tests/Routing/PathMatcherTest.php b/core/modules/system/lib/Drupal/system/Tests/Routing/PathMatcherTest.php index 302122aa199a6b3607a19bf73cfc709a68d23145..7cd39751b8b05b02d12cb4fc4e3018054ef68f5d 100644 --- a/core/modules/system/lib/Drupal/system/Tests/Routing/PathMatcherTest.php +++ b/core/modules/system/lib/Drupal/system/Tests/Routing/PathMatcherTest.php @@ -155,8 +155,7 @@ function testOutlinePathMatchDefaults() { // All of the matching paths have the correct pattern. foreach ($routes as $route) { $compiled = $route->compile(); - debug($compiled->getPatternOutline()); - $this->assertEqual($route->compile()->getPatternOutline(), '/path/path/%', 'Found path has correct pattern'); + $this->assertEqual($route->compile()->getPatternOutline(), '/some/path', 'Found path has correct pattern'); } $this->assertEqual(count($routes->all()), 1, 'The correct number of routes was found.'); @@ -167,6 +166,85 @@ function testOutlinePathMatchDefaults() { } } + /** + * Confirms that we can find routes whose pattern would match the request. + */ + function testOutlinePathMatchDefaultsCollision() { + $connection = Database::getConnection(); + $matcher = new PathMatcher($connection, 'test_routes'); + + $this->fixtures->createTables($connection); + + $collection = new RouteCollection(); + $collection->add('poink', new Route('/some/path/{value}', array( + 'value' => 'poink', + ))); + $collection->add('narf', new Route('/some/path/here')); + + $dumper = new MatcherDumper($connection, 'test_routes'); + $dumper->addRoutes($collection); + $dumper->dump(); + + $path = '/some/path'; + + $request = Request::create($path, 'GET'); + + try { + $routes = $matcher->matchRequestPartial($request); + + // All of the matching paths have the correct pattern. + foreach ($routes as $route) { + $compiled = $route->compile(); + $this->assertEqual($route->compile()->getPatternOutline(), '/some/path', 'Found path has correct pattern'); + } + + $this->assertEqual(count($routes->all()), 1, 'The correct number of routes was found.'); + $this->assertNotNull($routes->get('poink'), 'The first matching route was found.'); + } + catch (ResourceNotFoundException $e) { + $this->fail('No matching route found with default argument value.'); + } + } + + /** + * Confirms that we can find routes whose pattern would match the request. + */ + function testOutlinePathMatchDefaultsCollision2() { + $connection = Database::getConnection(); + $matcher = new PathMatcher($connection, 'test_routes'); + + $this->fixtures->createTables($connection); + + $collection = new RouteCollection(); + $collection->add('poink', new Route('/some/path/{value}', array( + 'value' => 'poink', + ))); + $collection->add('narf', new Route('/some/path/here')); + + $dumper = new MatcherDumper($connection, 'test_routes'); + $dumper->addRoutes($collection); + $dumper->dump(); + + $path = '/some/path/here'; + + $request = Request::create($path, 'GET'); + + try { + $routes = $matcher->matchRequestPartial($request); + + // All of the matching paths have the correct pattern. + foreach ($routes as $route) { + $this->assertEqual($route->compile()->getPatternOutline(), '/some/path/here', 'Found path has correct pattern'); + } + + $this->assertEqual(count($routes->all()), 1, 'The correct number of routes was found.'); + $this->assertNotNull($routes->get('narf'), 'The first matching route was found.'); + } + catch (ResourceNotFoundException $e) { + $this->fail('No matching route found with default argument value.'); + } + } + /** * Confirm that an exception is thrown when no matching path is found. */ diff --git a/core/modules/system/lib/Drupal/system/Tests/Routing/RouteTest.php b/core/modules/system/lib/Drupal/system/Tests/Routing/RouteTest.php index 7ed212b8ea060818a11f8e26430c39acb935a4ae..9e4ba634733be45b389835bea88c2cd92e84957d 100644 --- a/core/modules/system/lib/Drupal/system/Tests/Routing/RouteTest.php +++ b/core/modules/system/lib/Drupal/system/Tests/Routing/RouteTest.php @@ -28,6 +28,9 @@ function setUp() { parent::setUp(); } + /** + * Confirms that a route compiles properly with the necessary data. + */ public function testCompilation() { $route = new Route('/test/{something}/more'); $route->setOption('compiler_class', 'Drupal\Core\Routing\RouteCompiler'); @@ -38,4 +41,21 @@ public function testCompilation() { $this->assertEqual($compiled->getPatternOutline(), '/test/%/more', 'The pattern outline was correct.'); } + /** + * Confirms that a compiled route with default values has the correct outline. + */ + public function testCompilationDefaultValue() { + // Because "here" has a default value, it should not factor into the + // outline or the fitness. + $route = new Route('/test/{something}/more/{here}', array( + 'here' => 'there', + )); + $route->setOption('compiler_class', 'Drupal\Core\Routing\RouteCompiler'); + $compiled = $route->compile(); + + $this->assertEqual($route, $compiled->getRoute(), 'Compiled route has the correct route object.'); + $this->assertEqual($compiled->getFit(), 5 /* That's 101 binary*/, 'The fit was correct.'); + $this->assertEqual($compiled->getPatternOutline(), '/test/%/more', 'The pattern outline was correct.'); + } + }