From 30839fb09686d1f907f8a077772bbf6396891ebd Mon Sep 17 00:00:00 2001 From: Tobias Schultze Date: Sun, 1 Apr 2012 12:32:28 +0200 Subject: [PATCH 1/7] fix bugs and improvements in RouteCollection --- .../Component/Routing/RouteCollection.php | 80 +++++++++++-------- .../Routing/Tests/Matcher/UrlMatcherTest.php | 2 +- .../Routing/Tests/RouteCollectionTest.php | 30 +++++++ 3 files changed, 78 insertions(+), 34 deletions(-) diff --git a/src/Symfony/Component/Routing/RouteCollection.php b/src/Symfony/Component/Routing/RouteCollection.php index f049d5985b1c8..4070ce92f1b01 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); + } } } } @@ -190,7 +198,7 @@ public function addCollection(RouteCollection $collection, $prefix = '', $defaul // 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; @@ -212,7 +220,7 @@ public function addPrefix($prefix, $defaults = array(), $requirements = array(), $prefix = rtrim($prefix, '/'); // a prefix must start with a slash - if ($prefix && '/' !== $prefix[0]) { + if ('' !== $prefix && '/' !== $prefix[0]) { $prefix = '/'.$prefix; } @@ -230,6 +238,12 @@ public function addPrefix($prefix, $defaults = array(), $requirements = array(), } } + /** + * 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/src/Symfony/Component/Routing/Tests/Matcher/UrlMatcherTest.php b/src/Symfony/Component/Routing/Tests/Matcher/UrlMatcherTest.php index 1d782b889d1c3..e38c75a0303f2 100644 --- a/src/Symfony/Component/Routing/Tests/Matcher/UrlMatcherTest.php +++ b/src/Symfony/Component/Routing/Tests/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()); diff --git a/src/Symfony/Component/Routing/Tests/RouteCollectionTest.php b/src/Symfony/Component/Routing/Tests/RouteCollectionTest.php index 3e73065d0b7ca..da9339b88ec0e 100644 --- a/src/Symfony/Component/Routing/Tests/RouteCollectionTest.php +++ b/src/Symfony/Component/Routing/Tests/RouteCollectionTest.php @@ -144,6 +144,8 @@ public function testAddPrefix() array('foo' => 'bar', 'compiler_class' => 'Symfony\\Component\\Routing\\RouteCompiler'), $collection->get('bar')->getOptions(), '->addPrefix() adds an option to all routes' ); + $collection->addPrefix('0'); + $this->assertEquals('/0/{admin}', $collection->getPrefix(), '->addPrefix() ensures a prefix must start with a slash and must not end with a slash'); } public function testAddPrefixOverridesDefaultsAndRequirements() @@ -180,4 +182,32 @@ public function testResource() $collection->addResource($foo = new FileResource(__DIR__.'/Fixtures/foo.xml')); $this->assertEquals(array($foo), $collection->getResources(), '->addResources() adds a resource'); } + + public function testSingleRouteWithGivenName() + { + $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(); + $collection2 = new RouteCollection(); + $collection1->addCollection($collection2); + + $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'); + } } From 55b4482079339cdfa3f56de05ef6b5875f4b7991 Mon Sep 17 00:00:00 2001 From: Tobias Schultze Date: Thu, 5 Apr 2012 00:23:32 +0200 Subject: [PATCH 2/7] fixed bugs and added tests for Routing --- .../Matcher/Dumper/PhpMatcherDumper.php | 136 +++++----------- src/Symfony/Component/Routing/Route.php | 12 +- .../Component/Routing/RouteCollection.php | 150 ++++++++++++------ .../Tests/Fixtures/dumper/url_matcher1.php | 61 +++++-- .../Tests/Fixtures/dumper/url_matcher2.php | 64 ++++++-- .../Matcher/Dumper/PhpMatcherDumperTest.php | 29 +++- .../Routing/Tests/RouteCollectionTest.php | 67 +++++++- .../Component/Routing/Tests/RouteTest.php | 18 +++ 8 files changed, 365 insertions(+), 172 deletions(-) diff --git a/src/Symfony/Component/Routing/Matcher/Dumper/PhpMatcherDumper.php b/src/Symfony/Component/Routing/Matcher/Dumper/PhpMatcherDumper.php index dd7fbe35442af..166d5ded8a811 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); } } @@ -189,18 +178,25 @@ public function remove($name) * @param array $requirements An array of requirements * @param array $options An array of options * + * @throws \InvalidArgumentException When the RouteCollection already exists in the tree + * * @api */ public function addCollection(RouteCollection $collection, $prefix = '', $defaults = array(), $requirements = array(), $options = array()) { - $collection->setParent($this); - $collection->addPrefix($prefix, $defaults, $requirements, $options); - - // 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, $defaults, $requirements); $this->routes[] = $collection; } @@ -239,7 +235,7 @@ public function addPrefix($prefix, $defaults = array(), $requirements = array(), } /** - * 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 @@ -275,4 +271,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/src/Symfony/Component/Routing/Tests/Fixtures/dumper/url_matcher1.php b/src/Symfony/Component/Routing/Tests/Fixtures/dumper/url_matcher1.php index 98d8b0ef90b16..abf50c2f473a1 100644 --- a/src/Symfony/Component/Routing/Tests/Fixtures/dumper/url_matcher1.php +++ b/src/Symfony/Component/Routing/Tests/Fixtures/dumper/url_matcher1.php @@ -124,7 +124,15 @@ public function match($pathinfo) $matches['_route'] = 'bar1'; return $matches; } + } + // overriden + if (preg_match('#^/a/(?P.*)$#s', $pathinfo, $matches)) { + $matches['_route'] = 'overriden'; + return $matches; + } + + if (0 === strpos($pathinfo, '/a/b\'b')) { // foo2 if (preg_match('#^/a/b\'b/(?P[^/]+?)$#s', $pathinfo, $matches)) { $matches['_route'] = 'foo2'; @@ -137,21 +145,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[^/]+?))?$#s', $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[^/]+?)$#s', $pathinfo, $matches)) { - $matches['_route'] = 'foo4'; - return $matches; + // hey + if ($pathinfo === '/multi/hey/') { + return array('_route' => 'hey'); } } @@ -167,6 +176,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[^/]+?)$#s', $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[^/]+?)$#s', $pathinfo, $matches)) { + $matches['_route'] = 'b'; + return $matches; + } + + // c + if (0 === strpos($pathinfo, '/a/b/c') && preg_match('#^/a/b/c/(?P[^/]+?)$#s', $pathinfo, $matches)) { + $matches['_route'] = 'c'; + return $matches; + } + } + } + throw 0 < count($allow) ? new MethodNotAllowedException(array_unique($allow)) : new ResourceNotFoundException(); } } diff --git a/src/Symfony/Component/Routing/Tests/Fixtures/dumper/url_matcher2.php b/src/Symfony/Component/Routing/Tests/Fixtures/dumper/url_matcher2.php index 390acfbd245b4..77d1a00a72335 100644 --- a/src/Symfony/Component/Routing/Tests/Fixtures/dumper/url_matcher2.php +++ b/src/Symfony/Component/Routing/Tests/Fixtures/dumper/url_matcher2.php @@ -130,7 +130,15 @@ public function match($pathinfo) $matches['_route'] = 'bar1'; return $matches; } + } + // overriden + if (preg_match('#^/a/(?P.*)$#s', $pathinfo, $matches)) { + $matches['_route'] = 'overriden'; + return $matches; + } + + if (0 === strpos($pathinfo, '/a/b\'b')) { // foo2 if (preg_match('#^/a/b\'b/(?P[^/]+?)$#s', $pathinfo, $matches)) { $matches['_route'] = 'foo2'; @@ -143,21 +151,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[^/]+?))?$#s', $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[^/]+?)$#s', $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'); } } @@ -173,6 +185,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[^/]+?)$#s', $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[^/]+?)$#s', $pathinfo, $matches)) { + $matches['_route'] = 'b'; + return $matches; + } + + // c + if (0 === strpos($pathinfo, '/a/b/c') && preg_match('#^/a/b/c/(?P[^/]+?)$#s', $pathinfo, $matches)) { + $matches['_route'] = 'c'; + return $matches; + } + } + } + // secure if ($pathinfo === '/secure') { if ($this->context->getScheme() !== 'https') { diff --git a/src/Symfony/Component/Routing/Tests/Matcher/Dumper/PhpMatcherDumperTest.php b/src/Symfony/Component/Routing/Tests/Matcher/Dumper/PhpMatcherDumperTest.php index 293348cc15201..b32e47af83c0c 100644 --- a/src/Symfony/Component/Routing/Tests/Matcher/Dumper/PhpMatcherDumperTest.php +++ b/src/Symfony/Component/Routing/Tests/Matcher/Dumper/PhpMatcherDumperTest.php @@ -135,13 +135,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}')); @@ -150,13 +162,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/src/Symfony/Component/Routing/Tests/RouteCollectionTest.php b/src/Symfony/Component/Routing/Tests/RouteCollectionTest.php index da9339b88ec0e..c0fbf05c9e659 100644 --- a/src/Symfony/Component/Routing/Tests/RouteCollectionTest.php +++ b/src/Symfony/Component/Routing/Tests/RouteCollectionTest.php @@ -28,7 +28,7 @@ public function testRoute() } /** - * @expectedException InvalidArgumentException + * @expectedException \InvalidArgumentException */ public function testAddInvalidRoute() { @@ -183,7 +183,7 @@ public function testResource() $this->assertEquals(array($foo), $collection->getResources(), '->addResources() adds a resource'); } - public function testSingleRouteWithGivenName() + public function testUniqueRouteWithGivenName() { $collection1 = new RouteCollection(); $collection1->add('foo', $old = new Route('/old')); @@ -204,10 +204,73 @@ public function testSingleRouteWithGivenName() 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/src/Symfony/Component/Routing/Tests/RouteTest.php b/src/Symfony/Component/Routing/Tests/RouteTest.php index 1295bc43c0b8c..2cf9a0d7728a4 100644 --- a/src/Symfony/Component/Routing/Tests/RouteTest.php +++ b/src/Symfony/Component/Routing/Tests/RouteTest.php @@ -98,6 +98,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 d4a5f9f04bfef1b338aae169279d65889cfc25de Mon Sep 17 00:00:00 2001 From: Tobias Schultze Date: Fri, 6 Apr 2012 00:34:52 +0200 Subject: [PATCH 3/7] fixed CS and minor tweaks --- .../Component/Routing/RouteCollection.php | 19 +++++++---------- .../Routing/Tests/RouteCollectionTest.php | 21 +++++++++++-------- 2 files changed, 20 insertions(+), 20 deletions(-) diff --git a/src/Symfony/Component/Routing/RouteCollection.php b/src/Symfony/Component/Routing/RouteCollection.php index 97f7d3b38042e..5e41a9b3ae7f0 100644 --- a/src/Symfony/Component/Routing/RouteCollection.php +++ b/src/Symfony/Component/Routing/RouteCollection.php @@ -194,9 +194,9 @@ public function addCollection(RouteCollection $collection, $prefix = '', $defaul $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, $defaults, $requirements); + $collection->addPrefix($this->getPrefix() . $prefix, $defaults, $requirements, $options); $this->routes[] = $collection; } @@ -302,10 +302,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; } } @@ -315,7 +313,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 */ @@ -324,10 +322,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/src/Symfony/Component/Routing/Tests/RouteCollectionTest.php b/src/Symfony/Component/Routing/Tests/RouteCollectionTest.php index c0fbf05c9e659..f5e84124ed36e 100644 --- a/src/Symfony/Component/Routing/Tests/RouteCollectionTest.php +++ b/src/Symfony/Component/Routing/Tests/RouteCollectionTest.php @@ -186,7 +186,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')); @@ -194,11 +194,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() @@ -267,10 +272,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); } } From 46434e0528c561f89f182e164b8689e5431bae44 Mon Sep 17 00:00:00 2001 From: Tobias Schultze Date: Wed, 11 Apr 2012 00:45:22 +0200 Subject: [PATCH 4/7] fixed dumping of a root collection with prefix --- .../Matcher/Dumper/PhpMatcherDumper.php | 90 ++++++++++++++----- .../Component/Routing/RouteCollection.php | 4 + .../Tests/Fixtures/dumper/url_matcher1.php | 6 ++ .../Tests/Fixtures/dumper/url_matcher2.php | 6 ++ .../Tests/Fixtures/dumper/url_matcher3.php | 44 +++++++++ .../Matcher/Dumper/PhpMatcherDumperTest.php | 77 ++++++++++------ 6 files changed, 177 insertions(+), 50 deletions(-) create mode 100644 src/Symfony/Component/Routing/Tests/Fixtures/dumper/url_matcher3.php diff --git a/src/Symfony/Component/Routing/Matcher/Dumper/PhpMatcherDumper.php b/src/Symfony/Component/Routing/Matcher/Dumper/PhpMatcherDumper.php index 166d5ded8a811..447a51fd7f688 100644 --- a/src/Symfony/Component/Routing/Matcher/Dumper/PhpMatcherDumper.php +++ b/src/Symfony/Component/Routing/Matcher/Dumper/PhpMatcherDumper.php @@ -74,6 +74,13 @@ public function __construct(RequestContext \$context) EOF; } + /** + * Generates the code for the match method implementing UrlMatcherInterface. + * + * @param Boolean $supportsRedirections Whether redirections are supported by the base class + * + * @return string Match method as PHP code + */ private function generateMatchMethod($supportsRedirections) { $code = rtrim($this->compileRoutes($this->getRoutes(), $supportsRedirections), "\n"); @@ -91,38 +98,79 @@ public function match(\$pathinfo) EOF; } + /** + * Counts the number of routes as direct child of the RouteCollection. + * + * @param RouteCollection $routes A RouteCollection instance + * + * @return integer Number of Routes + */ + private function countDirectChildRoutes(RouteCollection $routes) + { + $count = 0; + foreach ($routes as $route) { + if ($route instanceof Route) { + $count++; + } + } + + return $count; + } + + /** + * Generates PHP code recursively to match a RouteCollection with all child routes and child collections. + * + * @param RouteCollection $routes A RouteCollection instance + * @param Boolean $supportsRedirections Whether redirections are supported by the base class + * @param string|null $parentPrefix The prefix of the parent collection used to optimize the code + * + * @return string PHP code + */ private function compileRoutes(RouteCollection $routes, $supportsRedirections, $parentPrefix = null) { $code = ''; + $prefix = $routes->getPrefix(); + $countDirectChildRoutes = $this->countDirectChildRoutes($routes); + $countAllChildRoutes = count($routes->all()); + $optimizable = '' !== $prefix && $prefix !== $parentPrefix && $countDirectChildRoutes > 0 && $countAllChildRoutes > 1 && false === strpos($prefix, '{'); + // whether the matching of routes within the collection can be optimized by wrapping it with the prefix condition + // - no need to optimize if current prefix is the same as the parent prefix + // - if $countDirectChildRoutes === 0, the sub-collections can do their own optimizations (in case there are any) + // - it's not worth wrapping a single child route + // - prefixes with variables cannot be optimized because routes within the collection might have different requirements for the same variable + if ($optimizable) { + $code .= sprintf(" if (0 === strpos(\$pathinfo, %s)) {\n", var_export($prefix, true)); + } + foreach ($routes as $name => $route) { - if ($route instanceof RouteCollection) { - $prefix = $route->getPrefix(); - $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)); - - 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, $countWorth ? $prefix : null); - } - } else { - $code .= $this->compileRoute($route, $name, $supportsRedirections, $parentPrefix)."\n"; + if ($route instanceof Route) { + $code .= $this->compileRoute($route, $name, $supportsRedirections, 1 === $countAllChildRoutes ? null : $prefix)."\n"; + // a single route in a sub-collection is not wrapped so it should do its own optimization in ->compileRoute with $parentPrefix = null + } elseif ($countAllChildRoutes - $countDirectChildRoutes > 0) { // we can stop iterating recursively if we already know there are no more routes + $code .= $this->compileRoutes($route, $supportsRedirections, $prefix); } } + if ($optimizable) { + $code .= " }\n\n"; + $code = preg_replace('/^.{2,}$/m', ' $0', $code); // apply extra indention at each line (except empty ones) + } + return $code; } + + /** + * Compiles a single Route to PHP code used to match it against the path info. + * + * @param Route $routes A Route instance + * @param string $name The name of the Route + * @param Boolean $supportsRedirections Whether redirections are supported by the base class + * @param string|null $parentPrefix The prefix of the parent collection used to optimize the code + * + * @return string PHP code + */ private function compileRoute(Route $route, $name, $supportsRedirections, $parentPrefix = null) { $code = ''; diff --git a/src/Symfony/Component/Routing/RouteCollection.php b/src/Symfony/Component/Routing/RouteCollection.php index 5e41a9b3ae7f0..301849086a19d 100644 --- a/src/Symfony/Component/Routing/RouteCollection.php +++ b/src/Symfony/Component/Routing/RouteCollection.php @@ -215,6 +215,10 @@ public function addPrefix($prefix, $defaults = array(), $requirements = array(), // a prefix must not end with a slash $prefix = rtrim($prefix, '/'); + if ('' === $prefix && empty($defaults) && empty($requirements) && empty($options)) { + return; + } + // a prefix must start with a slash if ('' !== $prefix && '/' !== $prefix[0]) { $prefix = '/'.$prefix; diff --git a/src/Symfony/Component/Routing/Tests/Fixtures/dumper/url_matcher1.php b/src/Symfony/Component/Routing/Tests/Fixtures/dumper/url_matcher1.php index abf50c2f473a1..4753410ffa557 100644 --- a/src/Symfony/Component/Routing/Tests/Fixtures/dumper/url_matcher1.php +++ b/src/Symfony/Component/Routing/Tests/Fixtures/dumper/url_matcher1.php @@ -124,6 +124,7 @@ public function match($pathinfo) $matches['_route'] = 'bar1'; return $matches; } + } // overriden @@ -144,7 +145,9 @@ public function match($pathinfo) $matches['_route'] = 'bar2'; return $matches; } + } + } if (0 === strpos($pathinfo, '/multi')) { @@ -162,6 +165,7 @@ public function match($pathinfo) if ($pathinfo === '/multi/hey/') { return array('_route' => 'hey'); } + } // foo3 @@ -205,7 +209,9 @@ public function match($pathinfo) $matches['_route'] = 'c'; return $matches; } + } + } throw 0 < count($allow) ? new MethodNotAllowedException(array_unique($allow)) : new ResourceNotFoundException(); diff --git a/src/Symfony/Component/Routing/Tests/Fixtures/dumper/url_matcher2.php b/src/Symfony/Component/Routing/Tests/Fixtures/dumper/url_matcher2.php index 77d1a00a72335..dbfd28ab0a9ba 100644 --- a/src/Symfony/Component/Routing/Tests/Fixtures/dumper/url_matcher2.php +++ b/src/Symfony/Component/Routing/Tests/Fixtures/dumper/url_matcher2.php @@ -130,6 +130,7 @@ public function match($pathinfo) $matches['_route'] = 'bar1'; return $matches; } + } // overriden @@ -150,7 +151,9 @@ public function match($pathinfo) $matches['_route'] = 'bar2'; return $matches; } + } + } if (0 === strpos($pathinfo, '/multi')) { @@ -171,6 +174,7 @@ public function match($pathinfo) } return array('_route' => 'hey'); } + } // foo3 @@ -214,7 +218,9 @@ public function match($pathinfo) $matches['_route'] = 'c'; return $matches; } + } + } // secure diff --git a/src/Symfony/Component/Routing/Tests/Fixtures/dumper/url_matcher3.php b/src/Symfony/Component/Routing/Tests/Fixtures/dumper/url_matcher3.php new file mode 100644 index 0000000000000..e67aa98b33a34 --- /dev/null +++ b/src/Symfony/Component/Routing/Tests/Fixtures/dumper/url_matcher3.php @@ -0,0 +1,44 @@ +context = $context; + } + + public function match($pathinfo) + { + $allow = array(); + $pathinfo = urldecode($pathinfo); + + if (0 === strpos($pathinfo, '/rootprefix')) { + // static + if ($pathinfo === '/rootprefix/test') { + return array('_route' => 'static'); + } + + // dynamic + if (preg_match('#^/rootprefix/(?P[^/]+?)$#xs', $pathinfo, $matches)) { + $matches['_route'] = 'dynamic'; + return $matches; + } + + } + + throw 0 < count($allow) ? new MethodNotAllowedException(array_unique($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 b32e47af83c0c..f21a35566341a 100644 --- a/src/Symfony/Component/Routing/Tests/Matcher/Dumper/PhpMatcherDumperTest.php +++ b/src/Symfony/Component/Routing/Tests/Matcher/Dumper/PhpMatcherDumperTest.php @@ -18,33 +18,6 @@ class PhpMatcherDumperTest extends \PHPUnit_Framework_TestCase { - public function testDump() - { - $collection = $this->getRouteCollection(); - - $dumper = new PhpMatcherDumper($collection, new RequestContext()); - - $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( - '/secure', - array(), - array('_scheme' => 'https') - )); - - // force HTTP redirection - $collection->add('nonsecure', new Route( - '/nonsecure', - array(), - array('_scheme' => 'http') - )); - - $dumper = new PhpMatcherDumper($collection, new RequestContext()); - - $this->assertStringEqualsFile(__DIR__.'/../../Fixtures/dumper/url_matcher2.php', $dumper->dump(array('base_class' => 'Symfony\Component\Routing\Tests\Fixtures\RedirectableUrlMatcher')), '->dump() dumps basic routes to the correct PHP file.'); - } - /** * @expectedException \LogicException */ @@ -60,8 +33,22 @@ public function testDumpWhenSchemeIsUsedWithoutAProperDumper() $dumper->dump(); } - protected function getRouteCollection() + /** + * @dataProvider getRouteCollections + */ + public function testDump(RouteCollection $collection, $fixture, $options = array()) + { + $basePath = __DIR__.'/../../Fixtures/dumper/'; + + $dumper = new PhpMatcherDumper($collection, new RequestContext()); + + $this->assertStringEqualsFile($basePath.$fixture, $dumper->dump($options), '->dump() correctly dumps routes as optimized PHP code.'); + } + + public function getRouteCollections() { + /* test case 1 */ + $collection = new RouteCollection(); $collection->add('overriden', new Route('/overriden')); @@ -182,6 +169,38 @@ protected function getRouteCollection() $collection2->addCollection($collection3, '/c'); $collection->addCollection($collection1, '/a'); - return $collection; + + /* test case 2 */ + + $redirectCollection = clone $collection; + + // force HTTPS redirection + $redirectCollection->add('secure', new Route( + '/secure', + array(), + array('_scheme' => 'https') + )); + + // force HTTP redirection + $redirectCollection->add('nonsecure', new Route( + '/nonsecure', + array(), + array('_scheme' => 'http') + )); + + + /* test case 3 */ + + $rootprefixCollection = new RouteCollection(); + $rootprefixCollection->add('static', new Route('/test')); + $rootprefixCollection->add('dynamic', new Route('/{var}')); + $rootprefixCollection->addPrefix('rootprefix'); + + + return array( + array($collection, 'url_matcher1.php', array()), + array($redirectCollection, 'url_matcher2.php', array('base_class' => 'Symfony\Component\Routing\Tests\Fixtures\RedirectableUrlMatcher')), + array($rootprefixCollection, 'url_matcher3.php', array()) + ); } } From a6b8d54c6723dec610fda70aaad7fa34376e7a50 Mon Sep 17 00:00:00 2001 From: Tobias Schultze Date: Wed, 11 Apr 2012 01:10:51 +0200 Subject: [PATCH 5/7] fixed CS and added notes in changelog --- CHANGELOG-2.1.md | 8 ++++--- .../Component/Routing/RouteCollection.php | 21 +++++++------------ 2 files changed, 12 insertions(+), 17 deletions(-) diff --git a/CHANGELOG-2.1.md b/CHANGELOG-2.1.md index e87a58f4a4d63..f217f7c1627ee 100644 --- a/CHANGELOG-2.1.md +++ b/CHANGELOG-2.1.md @@ -363,9 +363,11 @@ To get the diff between two versions, go to https://github.com/symfony/symfony/c * added a TraceableUrlMatcher * added the possibility to define options, default values and requirements for placeholders in prefix, including imported routes * added RouterInterface::getRouteCollection - * [BC BREAK] the UrlMatcher urldecodes the route parameters only once, they were - decoded twice before. Note that the `urldecode()` calls have been change for a - single `rawurldecode()` in order to support `+` for input paths. + * [BC BREAK] the UrlMatcher urldecodes the route parameters only once, they were decoded twice before. + Note that the `urldecode()` calls have been changed for a single `rawurldecode()` in order to support `+` for input paths. + * added RouteCollection::getRoot method to retrieve the root of a RouteCollection tree + * [BC BREAK] made RouteCollection::setParent private which could not have been used anyway without creating inconsistencies + * [BC BREAK] RouteCollection::remove also removes a route from parent collections (not only from its children) ### Security diff --git a/src/Symfony/Component/Routing/RouteCollection.php b/src/Symfony/Component/Routing/RouteCollection.php index 301849086a19d..59a00b0e9b056 100644 --- a/src/Symfony/Component/Routing/RouteCollection.php +++ b/src/Symfony/Component/Routing/RouteCollection.php @@ -131,9 +131,9 @@ public function all() /** * 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|null $route A Route instance or null when not found + * @return Route|null A Route instance or null when not found */ public function get($name) { @@ -160,12 +160,8 @@ public function remove($name) { $root = $this->getRoot(); - if (is_array($name)) { - foreach ($name as $n) { - $root->removeRecursively($n); - } - } else { - $root->removeRecursively($name); + foreach ((array) $name as $n) { + $root->removeRecursively($n); } } @@ -186,7 +182,7 @@ public function addCollection(RouteCollection $collection, $prefix = '', $defaul { // prevent infinite loops by recursive referencing $root = $this->getRoot(); - if ($root === $collection || $root->existsSubCollection($collection)) { + if ($root === $collection || $root->hasCollection($collection)) { throw new \InvalidArgumentException('The RouteCollection already exists in the tree.'); } @@ -321,13 +317,10 @@ private function removeRecursively($name) * * @return Boolean */ - private function existsSubCollection(RouteCollection $collection) + private function hasCollection(RouteCollection $collection) { foreach ($this->routes as $routes) { - if ($routes === $collection) { - return true; - } - if ($routes instanceof RouteCollection && $routes->existsSubCollection($collection)) { + if ($routes === $collection || $routes instanceof RouteCollection && $routes->hasCollection($collection)) { return true; } } From a4a3f4476f53336d22dc7ac2d17fecad8c3ac850 Mon Sep 17 00:00:00 2001 From: Tobias Schultze Date: Wed, 11 Apr 2012 01:45:48 +0200 Subject: [PATCH 6/7] fix that Route::compile needs to recompile once route is modified --- src/Symfony/Component/Routing/Route.php | 10 +++++++++- src/Symfony/Component/Routing/Tests/RouteTest.php | 6 ++++-- 2 files changed, 13 insertions(+), 3 deletions(-) diff --git a/src/Symfony/Component/Routing/Route.php b/src/Symfony/Component/Routing/Route.php index 771fcb29e5681..05d8a74e82739 100644 --- a/src/Symfony/Component/Routing/Route.php +++ b/src/Symfony/Component/Routing/Route.php @@ -79,10 +79,12 @@ public function setPattern($pattern) $this->pattern = trim($pattern); // a route must start with a slash - if (empty($this->pattern) || '/' !== $this->pattern[0]) { + if ('' === $this->pattern || '/' !== $this->pattern[0]) { $this->pattern = '/'.$this->pattern; } + $this->compiled = null; + return $this; } @@ -128,6 +130,7 @@ public function addOptions(array $options) foreach ($options as $name => $option) { $this->options[(string) $name] = $option; } + $this->compiled = null; return $this; } @@ -147,6 +150,7 @@ public function addOptions(array $options) public function setOption($name, $value) { $this->options[$name] = $value; + $this->compiled = null; return $this; } @@ -203,6 +207,7 @@ public function addDefaults(array $defaults) foreach ($defaults as $name => $default) { $this->defaults[(string) $name] = $default; } + $this->compiled = null; return $this; } @@ -244,6 +249,7 @@ public function hasDefault($name) public function setDefault($name, $default) { $this->defaults[(string) $name] = $default; + $this->compiled = null; return $this; } @@ -288,6 +294,7 @@ public function addRequirements(array $requirements) foreach ($requirements as $key => $regex) { $this->requirements[$key] = $this->sanitizeRequirement($key, $regex); } + $this->compiled = null; return $this; } @@ -317,6 +324,7 @@ public function getRequirement($key) public function setRequirement($key, $regex) { $this->requirements[$key] = $this->sanitizeRequirement($key, $regex); + $this->compiled = null; return $this; } diff --git a/src/Symfony/Component/Routing/Tests/RouteTest.php b/src/Symfony/Component/Routing/Tests/RouteTest.php index 2cf9a0d7728a4..8e466826dae35 100644 --- a/src/Symfony/Component/Routing/Tests/RouteTest.php +++ b/src/Symfony/Component/Routing/Tests/RouteTest.php @@ -119,7 +119,9 @@ public function getInvalidRequirements() public function testCompile() { $route = new Route('/{foo}'); - $this->assertEquals('Symfony\\Component\\Routing\\CompiledRoute', get_class($compiled = $route->compile()), '->compile() returns a compiled route'); - $this->assertEquals($compiled, $route->compile(), '->compile() only compiled the route once'); + $this->assertInstanceOf('Symfony\Component\Routing\CompiledRoute', $compiled = $route->compile(), '->compile() returns a compiled route'); + $this->assertSame($compiled, $route->compile(), '->compile() only compiled the route once if unchanged'); + $route->setRequirement('foo', '.*'); + $this->assertNotSame($compiled, $route->compile(), '->compile() recompiles if the route was modified'); } } From d4f63d3a899eea8e7a5c80a2c607b307ca30eebd Mon Sep 17 00:00:00 2001 From: Tobias Schultze Date: Wed, 11 Apr 2012 10:57:31 +0200 Subject: [PATCH 7/7] apply required changes caused by upstream --- .../Component/Routing/Tests/Fixtures/dumper/url_matcher3.php | 4 ++-- .../Routing/Tests/Matcher/Dumper/PhpMatcherDumperTest.php | 2 -- 2 files changed, 2 insertions(+), 4 deletions(-) diff --git a/src/Symfony/Component/Routing/Tests/Fixtures/dumper/url_matcher3.php b/src/Symfony/Component/Routing/Tests/Fixtures/dumper/url_matcher3.php index e67aa98b33a34..2dc0be9886012 100644 --- a/src/Symfony/Component/Routing/Tests/Fixtures/dumper/url_matcher3.php +++ b/src/Symfony/Component/Routing/Tests/Fixtures/dumper/url_matcher3.php @@ -23,7 +23,7 @@ public function __construct(RequestContext $context) public function match($pathinfo) { $allow = array(); - $pathinfo = urldecode($pathinfo); + $pathinfo = rawurldecode($pathinfo); if (0 === strpos($pathinfo, '/rootprefix')) { // static @@ -32,7 +32,7 @@ public function match($pathinfo) } // dynamic - if (preg_match('#^/rootprefix/(?P[^/]+?)$#xs', $pathinfo, $matches)) { + if (preg_match('#^/rootprefix/(?P[^/]+?)$#s', $pathinfo, $matches)) { $matches['_route'] = 'dynamic'; return $matches; } diff --git a/src/Symfony/Component/Routing/Tests/Matcher/Dumper/PhpMatcherDumperTest.php b/src/Symfony/Component/Routing/Tests/Matcher/Dumper/PhpMatcherDumperTest.php index f21a35566341a..b4dde1d4803ec 100644 --- a/src/Symfony/Component/Routing/Tests/Matcher/Dumper/PhpMatcherDumperTest.php +++ b/src/Symfony/Component/Routing/Tests/Matcher/Dumper/PhpMatcherDumperTest.php @@ -188,7 +188,6 @@ public function getRouteCollections() array('_scheme' => 'http') )); - /* test case 3 */ $rootprefixCollection = new RouteCollection(); @@ -196,7 +195,6 @@ public function getRouteCollections() $rootprefixCollection->add('dynamic', new Route('/{var}')); $rootprefixCollection->addPrefix('rootprefix'); - return array( array($collection, 'url_matcher1.php', array()), array($redirectCollection, 'url_matcher2.php', array('base_class' => 'Symfony\Component\Routing\Tests\Fixtures\RedirectableUrlMatcher')),