From f695b3d39107d574a491216b6d7c38b83978854a Mon Sep 17 00:00:00 2001 From: Tobias Schultze Date: Fri, 6 Apr 2012 22:56:30 +0200 Subject: [PATCH 1/4] performance improvement in PhpMatcherDumper 56c1e31e8a3cab8376a for 2.0 --- .../Matcher/Dumper/PhpMatcherDumper.php | 113 ++++++++++-------- .../Component/Routing/RouteCollection.php | 2 +- .../Routing/Fixtures/dumper/url_matcher1.php | 2 - .../Routing/Fixtures/dumper/url_matcher2.php | 12 +- .../Matcher/Dumper/PhpMatcherDumperTest.php | 6 +- 5 files changed, 66 insertions(+), 69 deletions(-) diff --git a/src/Symfony/Component/Routing/Matcher/Dumper/PhpMatcherDumper.php b/src/Symfony/Component/Routing/Matcher/Dumper/PhpMatcherDumper.php index 6ec216d324ca3..9df017640793d 100644 --- a/src/Symfony/Component/Routing/Matcher/Dumper/PhpMatcherDumper.php +++ b/src/Symfony/Component/Routing/Matcher/Dumper/PhpMatcherDumper.php @@ -55,7 +55,7 @@ public function dump(array $options = array()) private function addMatcher($supportsRedirections) { // we need to deep clone the routes as we will modify the structure to optimize the dump - $code = implode("\n", $this->compileRoutes(clone $this->getRoutes(), $supportsRedirections)); + $code = rtrim($this->compileRoutes(clone $this->getRoutes(), $supportsRedirections), "\n"); return <<getIterator(); $keys = array_keys($routeIterator->getArrayCopy()); @@ -86,10 +87,11 @@ private function compileRoutes(RouteCollection $routes, $supportsRedirections, $ if ($route instanceof RouteCollection) { $prefix = $route->getPrefix(); $optimizable = $prefix && count($route->all()) > 1 && false === strpos($route->getPrefix(), '{'); - $indent = ''; + $optimized = false; + if ($optimizable) { for ($j = $i; $j < $keysCount; $j++) { - if ($keys[$j] === null) { + if (null === $keys[$j]) { continue; } @@ -113,28 +115,25 @@ private function compileRoutes(RouteCollection $routes, $supportsRedirections, $ } if ($prefix !== $parentPrefix) { - $code[] = sprintf(" if (0 === strpos(\$pathinfo, %s)) {", var_export($prefix, true)); - $indent = ' '; + $code .= sprintf(" if (0 === strpos(\$pathinfo, %s)) {\n", var_export($prefix, true)); + $optimized = true; } } - foreach ($this->compileRoutes($route, $supportsRedirections, $prefix) as $line) { - foreach (explode("\n", $line) as $l) { - if ($l) { - $code[] = $indent.$l; - } else { - $code[] = $l; + if ($optimized) { + foreach (explode("\n", $this->compileRoutes($route, $supportsRedirections, $prefix)) as $line) { + if ('' !== $line) { + $code .= ' '; // apply extra indention } + $code .= $line."\n"; } - } - - if ($optimizable && $prefix !== $parentPrefix) { - $code[] = " }\n"; + $code = substr($code, 0, -2); // remove redundant last two line breaks + $code .= " }\n\n"; + } else { + $code .= $this->compileRoutes($route, $supportsRedirections, $prefix); } } else { - foreach ($this->compileRoute($route, $name, $supportsRedirections, $parentPrefix) as $line) { - $code[] = $line; - } + $code .= $this->compileRoute($route, $name, $supportsRedirections, $parentPrefix)."\n"; } } @@ -143,13 +142,25 @@ private function compileRoutes(RouteCollection $routes, $supportsRedirections, $ private function compileRoute(Route $route, $name, $supportsRedirections, $parentPrefix = null) { - $code = array(); + $code = ''; $compiledRoute = $route->compile(); $conditions = array(); $hasTrailingSlash = false; $matches = false; + $methods = array(); + + if ($req = $route->getRequirement('_method')) { + $methods = explode('|', strtoupper($req)); + // GET and HEAD are equivalent + if (in_array('GET', $methods) && !in_array('HEAD', $methods)) { + $methods[] = 'HEAD'; + } + } + + $supportsTrailingSlash = $supportsRedirections && (!$methods || in_array('HEAD', $methods)); + if (!count($compiledRoute->getVariables()) && false !== preg_match('#^(.)\^(?P.*?)\$\1#', str_replace(array("\n", ' '), '', $compiledRoute->getRegex()), $m)) { - if ($supportsRedirections && substr($m['url'], -1) === '/') { + if ($supportsTrailingSlash && substr($m['url'], -1) === '/') { $conditions[] = sprintf("rtrim(\$pathinfo, '/') === %s", var_export(rtrim(str_replace('\\', '', $m['url']), '/'), true)); $hasTrailingSlash = true; } else { @@ -161,7 +172,7 @@ private function compileRoute(Route $route, $name, $supportsRedirections, $paren } $regex = str_replace(array("\n", ' '), '', $compiledRoute->getRegex()); - if ($supportsRedirections && $pos = strpos($regex, '/$')) { + if ($supportsTrailingSlash && $pos = strpos($regex, '/$')) { $regex = substr($regex, 0, $pos).'/?$'.substr($regex, $pos + 2); $hasTrailingSlash = true; } @@ -172,79 +183,75 @@ private function compileRoute(Route $route, $name, $supportsRedirections, $paren $conditions = implode(' && ', $conditions); - $gotoname = 'not_'.preg_replace('/[^A-Za-z0-9_]/', '', $name); - - $code[] = <<getRequirement('_method')) { - $methods = explode('|', strtoupper($req)); - // GET and HEAD are equivalent - if (in_array('GET', $methods) && !in_array('HEAD', $methods)) { - $methods[] = 'HEAD'; - } + if ($methods) { + $gotoname = 'not_'.preg_replace('/[^A-Za-z0-9_]/', '', $name); + if (1 === count($methods)) { - $code[] = <<context->getMethod() != '$methods[0]') { \$allow[] = '$methods[0]'; goto $gotoname; } + EOF; } else { - $methods = implode('\', \'', $methods); - $code[] = <<context->getMethod(), array('$methods'))) { \$allow = array_merge(\$allow, array('$methods')); goto $gotoname; } + EOF; } } if ($hasTrailingSlash) { - $code[] = sprintf(<<redirect(\$pathinfo.'/', '%s'); + return \$this->redirect(\$pathinfo.'/', '$name'); } -EOF - , $name); + +EOF; } if ($scheme = $route->getRequirement('_scheme')) { if (!$supportsRedirections) { - throw new \LogicException('The "_scheme" requirement is only supported for route dumper that implements RedirectableUrlMatcherInterface.'); + throw new \LogicException('The "_scheme" requirement is only supported for URL matchers that implement RedirectableUrlMatcherInterface.'); } - $code[] = sprintf(<<context->getScheme() !== '$scheme') { - return \$this->redirect(\$pathinfo, '%s', '$scheme'); + return \$this->redirect(\$pathinfo, '$name', '$scheme'); } -EOF - , $name); + +EOF; } // optimize parameters array if (true === $matches && $compiledRoute->getDefaults()) { - $code[] = sprintf(" return array_merge(\$this->mergeDefaults(\$matches, %s), array('_route' => '%s'));" + $code .= sprintf(" return array_merge(\$this->mergeDefaults(\$matches, %s), array('_route' => '%s'));\n" , str_replace("\n", '', var_export($compiledRoute->getDefaults(), true)), $name); } elseif (true === $matches) { - $code[] = sprintf(" \$matches['_route'] = '%s';", $name); - $code[] = sprintf(" return \$matches;", $name); + $code .= sprintf(" \$matches['_route'] = '%s';\n", $name); + $code .= " return \$matches;\n"; } elseif ($compiledRoute->getDefaults()) { - $code[] = sprintf(' return %s;', str_replace("\n", '', var_export(array_merge($compiledRoute->getDefaults(), array('_route' => $name)), true))); + $code .= sprintf(" return %s;\n", str_replace("\n", '', var_export(array_merge($compiledRoute->getDefaults(), array('_route' => $name)), true))); } else { - $code[] = sprintf(" return array('_route' => '%s');", $name); + $code .= sprintf(" return array('_route' => '%s');\n", $name); } - $code[] = " }"; + $code .= " }\n"; - if ($req) { - $code[] = " $gotoname:"; + if ($methods) { + $code .= " $gotoname:\n"; } - $code[] = ''; - return $code; } diff --git a/src/Symfony/Component/Routing/RouteCollection.php b/src/Symfony/Component/Routing/RouteCollection.php index 0b2955d1631bf..ce48e343e093d 100644 --- a/src/Symfony/Component/Routing/RouteCollection.php +++ b/src/Symfony/Component/Routing/RouteCollection.php @@ -73,7 +73,7 @@ public function setParent(RouteCollection $parent) } /** - * Gets the current RouteCollection as an Iterator. + * Gets the current RouteCollection as an Iterator that includes all routes and child route collections. * * @return \ArrayIterator An \ArrayIterator interface */ diff --git a/tests/Symfony/Tests/Component/Routing/Fixtures/dumper/url_matcher1.php b/tests/Symfony/Tests/Component/Routing/Fixtures/dumper/url_matcher1.php index 7932944a34506..6367119def646 100644 --- a/tests/Symfony/Tests/Component/Routing/Fixtures/dumper/url_matcher1.php +++ b/tests/Symfony/Tests/Component/Routing/Fixtures/dumper/url_matcher1.php @@ -131,7 +131,6 @@ public function match($pathinfo) $matches['_route'] = 'bar2'; return $matches; } - } // overriden @@ -149,7 +148,6 @@ public function match($pathinfo) $matches['_route'] = 'foo4'; return $matches; } - } // foo3 diff --git a/tests/Symfony/Tests/Component/Routing/Fixtures/dumper/url_matcher2.php b/tests/Symfony/Tests/Component/Routing/Fixtures/dumper/url_matcher2.php index d0f1e31381804..257a3160ef2b1 100644 --- a/tests/Symfony/Tests/Component/Routing/Fixtures/dumper/url_matcher2.php +++ b/tests/Symfony/Tests/Component/Routing/Fixtures/dumper/url_matcher2.php @@ -80,28 +80,22 @@ public function match($pathinfo) } // baz5 - if (0 === strpos($pathinfo, '/test') && preg_match('#^/test/(?P[^/]+?)/?$#xs', $pathinfo, $matches)) { + if (0 === strpos($pathinfo, '/test') && preg_match('#^/test/(?P[^/]+?)/$#xs', $pathinfo, $matches)) { if ($this->context->getMethod() != 'POST') { $allow[] = 'POST'; goto not_baz5; } - if (substr($pathinfo, -1) !== '/') { - return $this->redirect($pathinfo.'/', 'baz5'); - } $matches['_route'] = 'baz5'; return $matches; } not_baz5: // baz.baz6 - if (0 === strpos($pathinfo, '/test') && preg_match('#^/test/(?P[^/]+?)/?$#xs', $pathinfo, $matches)) { + if (0 === strpos($pathinfo, '/test') && preg_match('#^/test/(?P[^/]+?)/$#xs', $pathinfo, $matches)) { if ($this->context->getMethod() != 'PUT') { $allow[] = 'PUT'; goto not_bazbaz6; } - if (substr($pathinfo, -1) !== '/') { - return $this->redirect($pathinfo.'/', 'baz.baz6'); - } $matches['_route'] = 'baz.baz6'; return $matches; } @@ -143,7 +137,6 @@ public function match($pathinfo) $matches['_route'] = 'bar2'; return $matches; } - } // overriden @@ -161,7 +154,6 @@ public function match($pathinfo) $matches['_route'] = 'foo4'; return $matches; } - } // foo3 diff --git a/tests/Symfony/Tests/Component/Routing/Matcher/Dumper/PhpMatcherDumperTest.php b/tests/Symfony/Tests/Component/Routing/Matcher/Dumper/PhpMatcherDumperTest.php index ec8b6688d64b0..5d4f11708bdce 100644 --- a/tests/Symfony/Tests/Component/Routing/Matcher/Dumper/PhpMatcherDumperTest.php +++ b/tests/Symfony/Tests/Component/Routing/Matcher/Dumper/PhpMatcherDumperTest.php @@ -20,11 +20,11 @@ class PhpMatcherDumperTest extends \PHPUnit_Framework_TestCase { public function testDump() { - $dumper = new PhpMatcherDumper($this->getRouteCollection(), new RequestContext()); + $collection = $this->getRouteCollection(); - $this->assertStringEqualsFile(__DIR__.'/../../Fixtures/dumper/url_matcher1.php', $dumper->dump(), '->dump() dumps basic routes to the correct PHP file.'); + $dumper = new PhpMatcherDumper($collection, new RequestContext()); - $collection = $this->getRouteCollection(); + $this->assertStringEqualsFile(__DIR__.'/../../Fixtures/dumper/url_matcher1.php', $dumper->dump(), '->dump() dumps basic routes to the correct PHP file.'); // force HTTPS redirection $collection->add('secure', new Route( From eaa1f48e9d7c3b06141a1523d8de01806b04a1cb Mon Sep 17 00:00:00 2001 From: Tobias Schultze Date: Sun, 1 Apr 2012 12:32:28 +0200 Subject: [PATCH 2/4] fix bugs and improvements in RouteCollection b5f51d00598 for 2.0 --- .../Component/Routing/RouteCollection.php | 80 +++++++++++-------- .../Routing/Matcher/UrlMatcherTest.php | 2 +- 2 files changed, 48 insertions(+), 34 deletions(-) diff --git a/src/Symfony/Component/Routing/RouteCollection.php b/src/Symfony/Component/Routing/RouteCollection.php index ce48e343e093d..abbbdda287019 100644 --- a/src/Symfony/Component/Routing/RouteCollection.php +++ b/src/Symfony/Component/Routing/RouteCollection.php @@ -17,9 +17,10 @@ * A RouteCollection represents a set of Route instances. * * When adding a route, it overrides existing routes with the - * same name defined in theinstance or its children and parents. + * same name defined in the instance or its children and parents. * * @author Fabien Potencier + * @author Tobias Schultze * * @api */ @@ -55,7 +56,7 @@ public function __clone() /** * Gets the parent RouteCollection. * - * @return RouteCollection The parent RouteCollection + * @return RouteCollection|null The parent RouteCollection or null when it's the root */ public function getParent() { @@ -98,20 +99,13 @@ public function add($name, Route $route) throw new \InvalidArgumentException(sprintf('The provided route name "%s" contains non valid characters. A route name must only contain digits (0-9), letters (a-z and A-Z), underscores (_) and dots (.).', $name)); } - $parent = $this; - while ($parent->getParent()) { - $parent = $parent->getParent(); - } - - if ($parent) { - $parent->remove($name); - } + $this->removeCompletely($name); $this->routes[$name] = $route; } /** - * Returns the array of routes. + * Returns all routes in this collection and its children. * * @return array An array of routes */ @@ -130,44 +124,58 @@ public function all() } /** - * Gets a route by name. + * Gets a route by name defined in this collection or its children. * - * @param string $name The route name + * @param string $name The route name * - * @return Route $route A Route instance + * @return Route|null $route A Route instance or null when not found */ public function get($name) { - // get the latest defined route - foreach (array_reverse($this->routes) as $routes) { - if (!$routes instanceof RouteCollection) { - continue; - } - - if (null !== $route = $routes->get($name)) { - return $route; - } - } - if (isset($this->routes[$name])) { return $this->routes[$name]; + } else { + foreach ($this->routes as $routes) { + if ($routes instanceof RouteCollection && null !== $route = $routes->get($name)) { + return $route; + } + } } + + return null; } /** - * Removes a route by name. + * Removes a route by name from all connected collections (this instance and all parents and children). + * + * @param string $name The route name + */ + public function removeCompletely($name) + { + $parent = $this; + while ($parent->getParent()) { + $parent = $parent->getParent(); + } + + $parent->remove($name); + } + + /** + * Removes a route by name from this collection and its children. * * @param string $name The route name */ public function remove($name) { + // the route can only be in this RouteCollection or one of its children (not both) because the + // adders (->add and ->addCollection) make sure there is only one route per name in all collections if (isset($this->routes[$name])) { unset($this->routes[$name]); - } - - foreach ($this->routes as $routes) { - if ($routes instanceof RouteCollection) { - $routes->remove($name); + } else { + foreach ($this->routes as $routes) { + if ($routes instanceof RouteCollection) { + $routes->remove($name); + } } } } @@ -187,7 +195,7 @@ public function addCollection(RouteCollection $collection, $prefix = '') // remove all routes with the same name in all existing collections foreach (array_keys($collection->all()) as $name) { - $this->remove($name); + $this->removeCompletely($name); } $this->routes[] = $collection; @@ -210,7 +218,7 @@ public function addPrefix($prefix) } // a prefix must start with a slash - if ('/' !== $prefix[0]) { + if ('' !== $prefix && '/' !== $prefix[0]) { $prefix = '/'.$prefix; } @@ -225,6 +233,12 @@ public function addPrefix($prefix) } } + /** + * Returns the prefix that may contain placeholders. + * When given, it must start with a slash and must not end with a slash. + * + * @return string The prefix + */ public function getPrefix() { return $this->prefix; diff --git a/tests/Symfony/Tests/Component/Routing/Matcher/UrlMatcherTest.php b/tests/Symfony/Tests/Component/Routing/Matcher/UrlMatcherTest.php index 8acff1686d7b3..536803f0e0066 100644 --- a/tests/Symfony/Tests/Component/Routing/Matcher/UrlMatcherTest.php +++ b/tests/Symfony/Tests/Component/Routing/Matcher/UrlMatcherTest.php @@ -71,7 +71,7 @@ public function testMethodNotAllowedAggregatesAllowedMethods() public function testMatch() { - // test the patterns are matched are parameters are returned + // test the patterns are matched and parameters are returned $collection = new RouteCollection(); $collection->add('foo', new Route('/foo/{bar}')); $matcher = new UrlMatcher($collection, new RequestContext(), array()); From 2d00c29192ab3d83381dc2ed0fff6e266a4f9743 Mon Sep 17 00:00:00 2001 From: Tobias Schultze Date: Thu, 5 Apr 2012 00:23:32 +0200 Subject: [PATCH 3/4] fixed bugs and added tests for Routing cf4f39a91388 for 2.0 --- .../Matcher/Dumper/PhpMatcherDumper.php | 136 +++++----------- src/Symfony/Component/Routing/Route.php | 12 +- .../Component/Routing/RouteCollection.php | 150 ++++++++++++------ .../Routing/Fixtures/dumper/url_matcher1.php | 61 +++++-- .../Routing/Fixtures/dumper/url_matcher2.php | 64 ++++++-- .../Matcher/Dumper/PhpMatcherDumperTest.php | 29 +++- .../Component/Routing/RouteCollectionTest.php | 93 ++++++++++- .../Tests/Component/Routing/RouteTest.php | 18 +++ 8 files changed, 392 insertions(+), 171 deletions(-) diff --git a/src/Symfony/Component/Routing/Matcher/Dumper/PhpMatcherDumper.php b/src/Symfony/Component/Routing/Matcher/Dumper/PhpMatcherDumper.php index 9df017640793d..f5d62d383bd5d 100644 --- a/src/Symfony/Component/Routing/Matcher/Dumper/PhpMatcherDumper.php +++ b/src/Symfony/Component/Routing/Matcher/Dumper/PhpMatcherDumper.php @@ -18,6 +18,7 @@ * PhpMatcherDumper creates a PHP class able to match URLs for a given set of routes. * * @author Fabien Potencier + * @author Tobias Schultze */ class PhpMatcherDumper extends MatcherDumper { @@ -44,21 +45,40 @@ public function dump(array $options = array()) $interfaces = class_implements($options['base_class']); $supportsRedirections = isset($interfaces['Symfony\Component\Routing\Matcher\RedirectableUrlMatcherInterface']); - return - $this->startClass($options['class'], $options['base_class']). - $this->addConstructor(). - $this->addMatcher($supportsRedirections). - $this->endClass() - ; + return <<context = \$context; + } + +{$this->generateMatchMethod($supportsRedirections)} +} + +EOF; } - private function addMatcher($supportsRedirections) + private function generateMatchMethod($supportsRedirections) { - // we need to deep clone the routes as we will modify the structure to optimize the dump - $code = rtrim($this->compileRoutes(clone $this->getRoutes(), $supportsRedirections), "\n"); + $code = rtrim($this->compileRoutes($this->getRoutes(), $supportsRedirections), "\n"); return <<getIterator(); - $keys = array_keys($routeIterator->getArrayCopy()); - $keysCount = count($keys); - - $i = 0; - foreach ($routeIterator as $name => $route) { - $i++; - + foreach ($routes as $name => $route) { if ($route instanceof RouteCollection) { $prefix = $route->getPrefix(); - $optimizable = $prefix && count($route->all()) > 1 && false === strpos($route->getPrefix(), '{'); - $optimized = false; - - if ($optimizable) { - for ($j = $i; $j < $keysCount; $j++) { - if (null === $keys[$j]) { - continue; - } - - $testRoute = $routeIterator->offsetGet($keys[$j]); - $isCollection = ($testRoute instanceof RouteCollection); - - $testPrefix = $isCollection ? $testRoute->getPrefix() : $testRoute->getPattern(); - - if (0 === strpos($testPrefix, $prefix)) { - $routeIterator->offsetUnset($keys[$j]); - - if ($isCollection) { - $route->addCollection($testRoute); - } else { - $route->add($keys[$j], $testRoute); - } - - $i++; - $keys[$j] = null; - } - } - - if ($prefix !== $parentPrefix) { - $code .= sprintf(" if (0 === strpos(\$pathinfo, %s)) {\n", var_export($prefix, true)); - $optimized = true; - } - } + $countWorth = count($route->all()) > 1; + // whether it's optimizable + if ('' !== $prefix && $prefix !== $parentPrefix && $countWorth && false === strpos($prefix, '{')) { + $code .= sprintf(" if (0 === strpos(\$pathinfo, %s)) {\n", var_export($prefix, true)); - if ($optimized) { foreach (explode("\n", $this->compileRoutes($route, $supportsRedirections, $prefix)) as $line) { if ('' !== $line) { $code .= ' '; // apply extra indention } $code .= $line."\n"; } + $code = substr($code, 0, -2); // remove redundant last two line breaks $code .= " }\n\n"; } else { - $code .= $this->compileRoutes($route, $supportsRedirections, $prefix); + $code .= $this->compileRoutes($route, $supportsRedirections, $countWorth ? $prefix : null); } } else { $code .= $this->compileRoute($route, $name, $supportsRedirections, $parentPrefix)."\n"; @@ -167,7 +150,7 @@ private function compileRoute(Route $route, $name, $supportsRedirections, $paren $conditions[] = sprintf("\$pathinfo === %s", var_export(str_replace('\\', '', $m['url']), true)); } } else { - if ($compiledRoute->getStaticPrefix() && $compiledRoute->getStaticPrefix() != $parentPrefix) { + if ($compiledRoute->getStaticPrefix() && $compiledRoute->getStaticPrefix() !== $parentPrefix) { $conditions[] = sprintf("0 === strpos(\$pathinfo, %s)", var_export($compiledRoute->getStaticPrefix(), true)); } @@ -254,47 +237,4 @@ private function compileRoute(Route $route, $name, $supportsRedirections, $paren return $code; } - - private function startClass($class, $baseClass) - { - return <<context = \$context; - } - -EOF; - } - - private function endClass() - { - return <<parent = $parent; + $parent = $this; + while ($parent->getParent()) { + $parent = $parent->getParent(); + } + + return $parent; } /** @@ -99,7 +104,7 @@ public function add($name, Route $route) throw new \InvalidArgumentException(sprintf('The provided route name "%s" contains non valid characters. A route name must only contain digits (0-9), letters (a-z and A-Z), underscores (_) and dots (.).', $name)); } - $this->removeCompletely($name); + $this->remove($name); $this->routes[$name] = $route; } @@ -133,50 +138,34 @@ public function all() public function get($name) { if (isset($this->routes[$name])) { - return $this->routes[$name]; - } else { - foreach ($this->routes as $routes) { - if ($routes instanceof RouteCollection && null !== $route = $routes->get($name)) { - return $route; - } - } + return $this->routes[$name] instanceof RouteCollection ? null : $this->routes[$name]; } - - return null; - } - /** - * Removes a route by name from all connected collections (this instance and all parents and children). - * - * @param string $name The route name - */ - public function removeCompletely($name) - { - $parent = $this; - while ($parent->getParent()) { - $parent = $parent->getParent(); + foreach ($this->routes as $routes) { + if ($routes instanceof RouteCollection && null !== $route = $routes->get($name)) { + return $route; + } } - $parent->remove($name); + return null; } - + /** - * Removes a route by name from this collection and its children. + * Removes a route or an array of routes by name from all connected + * collections (this instance and all parents and children). * - * @param string $name The route name + * @param string|array $name The route name or an array of route names */ public function remove($name) { - // the route can only be in this RouteCollection or one of its children (not both) because the - // adders (->add and ->addCollection) make sure there is only one route per name in all collections - if (isset($this->routes[$name])) { - unset($this->routes[$name]); - } else { - foreach ($this->routes as $routes) { - if ($routes instanceof RouteCollection) { - $routes->remove($name); - } + $root = $this->getRoot(); + + if (is_array($name)) { + foreach ($name as $n) { + $root->removeRecursively($n); } + } else { + $root->removeRecursively($name); } } @@ -186,18 +175,25 @@ public function remove($name) * @param RouteCollection $collection A RouteCollection instance * @param string $prefix An optional prefix to add before each pattern of the route collection * + * @throws \InvalidArgumentException When the RouteCollection already exists in the tree + * * @api */ public function addCollection(RouteCollection $collection, $prefix = '') { - $collection->setParent($this); - $collection->addPrefix($prefix); - - // remove all routes with the same name in all existing collections - foreach (array_keys($collection->all()) as $name) { - $this->removeCompletely($name); + // prevent infinite loops by recursive referencing + $root = $this->getRoot(); + if ($root === $collection || $root->existsSubCollection($collection)) { + throw new \InvalidArgumentException('The RouteCollection already exists in the tree.'); } + // remove all routes with the same names in all existing collections + $this->remove(array_keys($collection->all())); + + $collection->setParent($this); + // the sub-collection must have the prefix of the parent (current instance) prepended because it it does not + // necessarily already have it applied (depending on the order RouteCollections are added to each other) + $collection->addPrefix($this->getPrefix() . $prefix); $this->routes[] = $collection; } @@ -234,7 +230,7 @@ public function addPrefix($prefix) } /** - * Returns the prefix that may contain placeholders. + * Returns the prefix that may contain placeholders. * When given, it must start with a slash and must not end with a slash. * * @return string The prefix @@ -270,4 +266,66 @@ public function addResource(ResourceInterface $resource) { $this->resources[] = $resource; } + + /** + * Sets the parent RouteCollection. It's only used internally from one RouteCollection + * to another. It makes no sense to be available as part of the public API. + * + * @param RouteCollection $parent The parent RouteCollection + */ + private function setParent(RouteCollection $parent) + { + $this->parent = $parent; + } + + /** + * Removes a route by name from this collection and its children recursively. + * + * @param string $name The route name + * + * @return Boolean true when found + */ + private function removeRecursively($name) + { + // It is ensured by the adders (->add and ->addCollection) that there can + // only be one route per name in all connected collections. So we can stop + // interating recursively on the first hit. + if (isset($this->routes[$name])) { + unset($this->routes[$name]); + + return true; + } + + foreach ($this->routes as $routes) { + if ($routes instanceof RouteCollection) { + if ($routes->removeRecursively($name)) { + return true; + } + } + } + + return false; + } + + /** + * Checks whether the given RouteCollection is already set in any child of the current instance. + * + * @param RouteCollection $collection A RouteCollection instance + * + * @return Boolean + */ + private function existsSubCollection(RouteCollection $collection) + { + foreach ($this->routes as $routes) { + if ($routes === $collection) { + return true; + } elseif ($routes instanceof RouteCollection) { + if ($routes->existsSubCollection($collection)) { + return true; + } + } + } + + return false; + } } diff --git a/tests/Symfony/Tests/Component/Routing/Fixtures/dumper/url_matcher1.php b/tests/Symfony/Tests/Component/Routing/Fixtures/dumper/url_matcher1.php index 6367119def646..3550360e6883f 100644 --- a/tests/Symfony/Tests/Component/Routing/Fixtures/dumper/url_matcher1.php +++ b/tests/Symfony/Tests/Component/Routing/Fixtures/dumper/url_matcher1.php @@ -119,7 +119,15 @@ public function match($pathinfo) $matches['_route'] = 'bar1'; return $matches; } + } + // overriden + if (preg_match('#^/a/(?P.*)$#xs', $pathinfo, $matches)) { + $matches['_route'] = 'overriden'; + return $matches; + } + + if (0 === strpos($pathinfo, '/a/b\'b')) { // foo2 if (preg_match('#^/a/b\'b/(?P[^/]+?)$#xs', $pathinfo, $matches)) { $matches['_route'] = 'foo2'; @@ -132,21 +140,22 @@ public function match($pathinfo) return $matches; } } + } - // overriden - if ($pathinfo === '/a/overriden2') { - return array('_route' => 'overriden'); + if (0 === strpos($pathinfo, '/multi')) { + // helloWorld + if (0 === strpos($pathinfo, '/multi/hello') && preg_match('#^/multi/hello(?:/(?P[^/]+?))?$#xs', $pathinfo, $matches)) { + return array_merge($this->mergeDefaults($matches, array ( 'who' => 'World!',)), array('_route' => 'helloWorld')); } - // ababa - if ($pathinfo === '/ababa') { - return array('_route' => 'ababa'); + // overriden2 + if ($pathinfo === '/multi/new') { + return array('_route' => 'overriden2'); } - // foo4 - if (preg_match('#^/aba/(?P[^/]+?)$#xs', $pathinfo, $matches)) { - $matches['_route'] = 'foo4'; - return $matches; + // hey + if ($pathinfo === '/multi/hey/') { + return array('_route' => 'hey'); } } @@ -162,6 +171,38 @@ public function match($pathinfo) return $matches; } + // ababa + if ($pathinfo === '/ababa') { + return array('_route' => 'ababa'); + } + + // foo4 + if (0 === strpos($pathinfo, '/aba') && preg_match('#^/aba/(?P[^/]+?)$#xs', $pathinfo, $matches)) { + $matches['_route'] = 'foo4'; + return $matches; + } + + if (0 === strpos($pathinfo, '/a')) { + // a + if ($pathinfo === '/a/a...') { + return array('_route' => 'a'); + } + + if (0 === strpos($pathinfo, '/a/b')) { + // b + if (preg_match('#^/a/b/(?P[^/]+?)$#xs', $pathinfo, $matches)) { + $matches['_route'] = 'b'; + return $matches; + } + + // c + if (0 === strpos($pathinfo, '/a/b/c') && preg_match('#^/a/b/c/(?P[^/]+?)$#xs', $pathinfo, $matches)) { + $matches['_route'] = 'c'; + return $matches; + } + } + } + throw 0 < count($allow) ? new MethodNotAllowedException(array_unique($allow)) : new ResourceNotFoundException(); } } diff --git a/tests/Symfony/Tests/Component/Routing/Fixtures/dumper/url_matcher2.php b/tests/Symfony/Tests/Component/Routing/Fixtures/dumper/url_matcher2.php index 257a3160ef2b1..524830bce8c57 100644 --- a/tests/Symfony/Tests/Component/Routing/Fixtures/dumper/url_matcher2.php +++ b/tests/Symfony/Tests/Component/Routing/Fixtures/dumper/url_matcher2.php @@ -125,7 +125,15 @@ public function match($pathinfo) $matches['_route'] = 'bar1'; return $matches; } + } + // overriden + if (preg_match('#^/a/(?P.*)$#xs', $pathinfo, $matches)) { + $matches['_route'] = 'overriden'; + return $matches; + } + + if (0 === strpos($pathinfo, '/a/b\'b')) { // foo2 if (preg_match('#^/a/b\'b/(?P[^/]+?)$#xs', $pathinfo, $matches)) { $matches['_route'] = 'foo2'; @@ -138,21 +146,25 @@ public function match($pathinfo) return $matches; } } + } - // overriden - if ($pathinfo === '/a/overriden2') { - return array('_route' => 'overriden'); + if (0 === strpos($pathinfo, '/multi')) { + // helloWorld + if (0 === strpos($pathinfo, '/multi/hello') && preg_match('#^/multi/hello(?:/(?P[^/]+?))?$#xs', $pathinfo, $matches)) { + return array_merge($this->mergeDefaults($matches, array ( 'who' => 'World!',)), array('_route' => 'helloWorld')); } - // ababa - if ($pathinfo === '/ababa') { - return array('_route' => 'ababa'); + // overriden2 + if ($pathinfo === '/multi/new') { + return array('_route' => 'overriden2'); } - // foo4 - if (preg_match('#^/aba/(?P[^/]+?)$#xs', $pathinfo, $matches)) { - $matches['_route'] = 'foo4'; - return $matches; + // hey + if (rtrim($pathinfo, '/') === '/multi/hey') { + if (substr($pathinfo, -1) !== '/') { + return $this->redirect($pathinfo.'/', 'hey'); + } + return array('_route' => 'hey'); } } @@ -168,6 +180,38 @@ public function match($pathinfo) return $matches; } + // ababa + if ($pathinfo === '/ababa') { + return array('_route' => 'ababa'); + } + + // foo4 + if (0 === strpos($pathinfo, '/aba') && preg_match('#^/aba/(?P[^/]+?)$#xs', $pathinfo, $matches)) { + $matches['_route'] = 'foo4'; + return $matches; + } + + if (0 === strpos($pathinfo, '/a')) { + // a + if ($pathinfo === '/a/a...') { + return array('_route' => 'a'); + } + + if (0 === strpos($pathinfo, '/a/b')) { + // b + if (preg_match('#^/a/b/(?P[^/]+?)$#xs', $pathinfo, $matches)) { + $matches['_route'] = 'b'; + return $matches; + } + + // c + if (0 === strpos($pathinfo, '/a/b/c') && preg_match('#^/a/b/c/(?P[^/]+?)$#xs', $pathinfo, $matches)) { + $matches['_route'] = 'c'; + return $matches; + } + } + } + // secure if ($pathinfo === '/secure') { if ($this->context->getScheme() !== 'https') { diff --git a/tests/Symfony/Tests/Component/Routing/Matcher/Dumper/PhpMatcherDumperTest.php b/tests/Symfony/Tests/Component/Routing/Matcher/Dumper/PhpMatcherDumperTest.php index 5d4f11708bdce..37ecfc8a53252 100644 --- a/tests/Symfony/Tests/Component/Routing/Matcher/Dumper/PhpMatcherDumperTest.php +++ b/tests/Symfony/Tests/Component/Routing/Matcher/Dumper/PhpMatcherDumperTest.php @@ -131,13 +131,25 @@ protected function getRouteCollection() $collection1->add('bar1', new Route('/{bar}')); $collection2 = new RouteCollection(); $collection2->addCollection($collection1, '/b\'b'); - $collection2->add('overriden', new Route('/overriden2')); + $collection2->add('overriden', new Route('/{var}', array(), array('var' => '.*'))); $collection1 = new RouteCollection(); $collection1->add('foo2', new Route('/{foo1}')); $collection1->add('bar2', new Route('/{bar1}')); $collection2->addCollection($collection1, '/b\'b'); $collection->addCollection($collection2, '/a'); + // overriden through addCollection() and multiple sub-collections with no own prefix + $collection1 = new RouteCollection(); + $collection1->add('overriden2', new Route('/old')); + $collection1->add('helloWorld', new Route('/hello/{who}', array('who' => 'World!'))); + $collection2 = new RouteCollection(); + $collection3 = new RouteCollection(); + $collection3->add('overriden2', new Route('/new')); + $collection3->add('hey', new Route('/hey/')); + $collection1->addCollection($collection2); + $collection2->addCollection($collection3); + $collection->addCollection($collection1, '/multi'); + // "dynamic" prefix $collection1 = new RouteCollection(); $collection1->add('foo3', new Route('/{foo}')); @@ -146,13 +158,26 @@ protected function getRouteCollection() $collection2->addCollection($collection1, '/b'); $collection->addCollection($collection2, '/{_locale}'); + // route between collections $collection->add('ababa', new Route('/ababa')); - // some more prefixes + // collection with static prefix but only one route $collection1 = new RouteCollection(); $collection1->add('foo4', new Route('/{foo}')); $collection->addCollection($collection1, '/aba'); + // multiple sub-collections with a single route and a prefix each + $collection1 = new RouteCollection(); + $collection1->add('a', new Route('/a...')); + $collection2 = new RouteCollection(); + $collection2->add('b', new Route('/{var}')); + $collection3 = new RouteCollection(); + $collection3->add('c', new Route('/{var}')); + + $collection1->addCollection($collection2, '/b'); + $collection2->addCollection($collection3, '/c'); + $collection->addCollection($collection1, '/a'); + return $collection; } } diff --git a/tests/Symfony/Tests/Component/Routing/RouteCollectionTest.php b/tests/Symfony/Tests/Component/Routing/RouteCollectionTest.php index f0569065c1ce4..ca4f8d70fa61a 100644 --- a/tests/Symfony/Tests/Component/Routing/RouteCollectionTest.php +++ b/tests/Symfony/Tests/Component/Routing/RouteCollectionTest.php @@ -28,7 +28,7 @@ public function testRoute() } /** - * @expectedException InvalidArgumentException + * @expectedException \InvalidArgumentException */ public function testAddInvalidRoute() { @@ -130,4 +130,95 @@ public function testResource() $collection->addResource($foo = new FileResource(__DIR__.'/Fixtures/foo.xml')); $this->assertEquals(array($foo), $collection->getResources(), '->addResources() adds a resource'); } + + public function testUniqueRouteWithGivenName() + { + $collection1 = new RouteCollection(); + $collection1->add('foo', $old = new Route('/old')); + $collection2 = new RouteCollection(); + $collection3 = new RouteCollection(); + $collection3->add('foo', $new = new Route('/new')); + + $collection1->addCollection($collection2); + $collection2->addCollection($collection3); + + $this->assertSame($new, $collection1->get('foo'), '->get() returns new route that overrode previous one'); + $p = new \ReflectionProperty('Symfony\Component\Routing\RouteCollection', 'routes'); + $p->setAccessible(true); + // size of 1 because collection1 contains collection2 but not $old anymore + $this->assertCount(1, $p->getValue($collection1), '->addCollection() removes previous routes when adding new routes with the same name'); + } + + public function testGet() + { + $collection1 = new RouteCollection(); + $collection1->add('a', $a = new Route('/a')); + $collection2 = new RouteCollection(); + $collection2->add('b', $b = new Route('/b')); + $collection1->addCollection($collection2); + + $this->assertSame($b, $collection1->get('b'), '->get() returns correct route in child collection'); + $this->assertNull($collection2->get('a'), '->get() does not return the route defined in parent collection'); + $this->assertNull($collection1->get('non-existent'), '->get() returns null when route does not exist'); + $this->assertNull($collection1->get(0), '->get() does not disclose internal child RouteCollection'); + } + + /** + * @expectedException \InvalidArgumentException + */ + public function testCannotSelfJoinCollection() + { + $collection = new RouteCollection(); + + $collection->addCollection($collection); + } + + /** + * @expectedException \InvalidArgumentException + */ + public function testCannotAddExistingCollectionToTree() + { + $collection1 = new RouteCollection(); + $collection2 = new RouteCollection(); + $collection3 = new RouteCollection(); + + $collection1->addCollection($collection2); + $collection1->addCollection($collection3); + $collection2->addCollection($collection3); + } + + public function testDefinitionOrderDoesNotMatter() + { + $collection1 = new RouteCollection(); + $collection1->add('a', new Route('/a...')); + $collection2 = new RouteCollection(); + $collection2->add('b', new Route('/b...')); + $collection3 = new RouteCollection(); + $collection3->add('c', new Route('/c...')); + + $rootCollection_A = new RouteCollection(); + $collection2->addCollection($collection3, '/c'); + $collection1->addCollection($collection2, '/b'); + $rootCollection_A->addCollection($collection1, '/a'); + + // above should mean the same as below + + $collection1 = new RouteCollection(); + $collection1->add('a', new Route('/a...')); + $collection2 = new RouteCollection(); + $collection2->add('b', new Route('/b...')); + $collection3 = new RouteCollection(); + $collection3->add('c', new Route('/c...')); + + $rootCollection_B = new RouteCollection(); + $collection1->addCollection($collection2, '/b'); + $collection2->addCollection($collection3, '/c'); + $rootCollection_B->addCollection($collection1, '/a'); + + // test it + + $p = new \ReflectionProperty('Symfony\Component\Routing\RouteCollection', 'routes'); + $p->setAccessible(true); + $this->assertEquals($p->getValue($rootCollection_A), $p->getValue($rootCollection_B)); + } } diff --git a/tests/Symfony/Tests/Component/Routing/RouteTest.php b/tests/Symfony/Tests/Component/Routing/RouteTest.php index 831bd915ba1cf..3e4642b09e21b 100644 --- a/tests/Symfony/Tests/Component/Routing/RouteTest.php +++ b/tests/Symfony/Tests/Component/Routing/RouteTest.php @@ -89,6 +89,24 @@ public function testRequirement() $this->assertEquals('\d+', $route->getRequirement('foo'), '->setRequirement() removes ^ and $ from the pattern'); } + /** + * @dataProvider getInvalidRequirements + * @expectedException \InvalidArgumentException + */ + public function testSetInvalidRequirement($req) + { + $route = new Route('/{foo}'); + $route->setRequirement('foo', $req); + } + + public function getInvalidRequirements() + { + return array( + array(''), + array(array()) + ); + } + public function testCompile() { $route = new Route('/{foo}'); From 8da6d607e38ea39c19f0e0c99b00d010fb1ef52a Mon Sep 17 00:00:00 2001 From: Tobias Schultze Date: Fri, 6 Apr 2012 00:34:52 +0200 Subject: [PATCH 4/4] fixed CS and minor tweaks 6bd80a7f0 for 2.0 --- .../Component/Routing/RouteCollection.php | 17 +++++++-------- .../Component/Routing/RouteCollectionTest.php | 21 +++++++++++-------- 2 files changed, 19 insertions(+), 19 deletions(-) diff --git a/src/Symfony/Component/Routing/RouteCollection.php b/src/Symfony/Component/Routing/RouteCollection.php index b6c229396ab52..d85d461bda8a8 100644 --- a/src/Symfony/Component/Routing/RouteCollection.php +++ b/src/Symfony/Component/Routing/RouteCollection.php @@ -191,7 +191,7 @@ public function addCollection(RouteCollection $collection, $prefix = '') $this->remove(array_keys($collection->all())); $collection->setParent($this); - // the sub-collection must have the prefix of the parent (current instance) prepended because it it does not + // the sub-collection must have the prefix of the parent (current instance) prepended because it does not // necessarily already have it applied (depending on the order RouteCollections are added to each other) $collection->addPrefix($this->getPrefix() . $prefix); $this->routes[] = $collection; @@ -297,10 +297,8 @@ private function removeRecursively($name) } foreach ($this->routes as $routes) { - if ($routes instanceof RouteCollection) { - if ($routes->removeRecursively($name)) { - return true; - } + if ($routes instanceof RouteCollection && $routes->removeRecursively($name)) { + return true; } } @@ -310,7 +308,7 @@ private function removeRecursively($name) /** * Checks whether the given RouteCollection is already set in any child of the current instance. * - * @param RouteCollection $collection A RouteCollection instance + * @param RouteCollection $collection A RouteCollection instance * * @return Boolean */ @@ -319,10 +317,9 @@ private function existsSubCollection(RouteCollection $collection) foreach ($this->routes as $routes) { if ($routes === $collection) { return true; - } elseif ($routes instanceof RouteCollection) { - if ($routes->existsSubCollection($collection)) { - return true; - } + } + if ($routes instanceof RouteCollection && $routes->existsSubCollection($collection)) { + return true; } } diff --git a/tests/Symfony/Tests/Component/Routing/RouteCollectionTest.php b/tests/Symfony/Tests/Component/Routing/RouteCollectionTest.php index ca4f8d70fa61a..bf62abf3d33b4 100644 --- a/tests/Symfony/Tests/Component/Routing/RouteCollectionTest.php +++ b/tests/Symfony/Tests/Component/Routing/RouteCollectionTest.php @@ -134,7 +134,7 @@ public function testResource() public function testUniqueRouteWithGivenName() { $collection1 = new RouteCollection(); - $collection1->add('foo', $old = new Route('/old')); + $collection1->add('foo', new Route('/old')); $collection2 = new RouteCollection(); $collection3 = new RouteCollection(); $collection3->add('foo', $new = new Route('/new')); @@ -142,11 +142,16 @@ public function testUniqueRouteWithGivenName() $collection1->addCollection($collection2); $collection2->addCollection($collection3); + $collection1->add('stay', new Route('/stay')); + + $iterator = $collection1->getIterator(); + $this->assertSame($new, $collection1->get('foo'), '->get() returns new route that overrode previous one'); - $p = new \ReflectionProperty('Symfony\Component\Routing\RouteCollection', 'routes'); - $p->setAccessible(true); - // size of 1 because collection1 contains collection2 but not $old anymore - $this->assertCount(1, $p->getValue($collection1), '->addCollection() removes previous routes when adding new routes with the same name'); + // size of 2 because collection1 contains collection2 and /stay but not /old anymore + $this->assertCount(2, $iterator, '->addCollection() removes previous routes when adding new routes with the same name'); + $this->assertInstanceOf('Symfony\Component\Routing\RouteCollection', $iterator->current(), '->getIterator returns both Routes and RouteCollections'); + $iterator->next(); + $this->assertInstanceOf('Symfony\Component\Routing\Route', $iterator->current(), '->getIterator returns both Routes and RouteCollections'); } public function testGet() @@ -215,10 +220,8 @@ public function testDefinitionOrderDoesNotMatter() $collection2->addCollection($collection3, '/c'); $rootCollection_B->addCollection($collection1, '/a'); - // test it + // test it now - $p = new \ReflectionProperty('Symfony\Component\Routing\RouteCollection', 'routes'); - $p->setAccessible(true); - $this->assertEquals($p->getValue($rootCollection_A), $p->getValue($rootCollection_B)); + $this->assertEquals($rootCollection_A, $rootCollection_B); } }