From 98f3ca83959e71f91e54315d7f50923a86ba3a45 Mon Sep 17 00:00:00 2001 From: Tobias Schultze Date: Mon, 26 Nov 2012 18:28:37 +0100 Subject: [PATCH 1/3] [Routing] removed tree structure from RouteCollection --- .../Bundle/FrameworkBundle/Routing/Router.php | 20 +- .../Matcher/Dumper/PhpMatcherDumper.php | 34 +--- .../Routing/Matcher/TraceableUrlMatcher.php | 8 - .../Component/Routing/Matcher/UrlMatcher.php | 12 -- .../Component/Routing/RouteCollection.php | 185 +++++------------- 5 files changed, 65 insertions(+), 194 deletions(-) diff --git a/src/Symfony/Bundle/FrameworkBundle/Routing/Router.php b/src/Symfony/Bundle/FrameworkBundle/Routing/Router.php index 31fbc63ef6..cc6df98a41 100644 --- a/src/Symfony/Bundle/FrameworkBundle/Routing/Router.php +++ b/src/Symfony/Bundle/FrameworkBundle/Routing/Router.php @@ -84,19 +84,15 @@ class Router extends BaseRouter implements WarmableInterface private function resolveParameters(RouteCollection $collection) { foreach ($collection as $route) { - if ($route instanceof RouteCollection) { - $this->resolveParameters($route); - } else { - foreach ($route->getDefaults() as $name => $value) { - $route->setDefault($name, $this->resolve($value)); - } - - foreach ($route->getRequirements() as $name => $value) { - $route->setRequirement($name, $this->resolve($value)); - } - - $route->setPattern($this->resolve($route->getPattern())); + foreach ($route->getDefaults() as $name => $value) { + $route->setDefault($name, $this->resolve($value)); } + + foreach ($route->getRequirements() as $name => $value) { + $route->setRequirement($name, $this->resolve($value)); + } + + $route->setPattern($this->resolve($route->getPattern())); } } diff --git a/src/Symfony/Component/Routing/Matcher/Dumper/PhpMatcherDumper.php b/src/Symfony/Component/Routing/Matcher/Dumper/PhpMatcherDumper.php index 454e26ca7b..2c86761b63 100644 --- a/src/Symfony/Component/Routing/Matcher/Dumper/PhpMatcherDumper.php +++ b/src/Symfony/Component/Routing/Matcher/Dumper/PhpMatcherDumper.php @@ -111,7 +111,6 @@ EOF; { $fetchedHostname = false; - $routes = $this->flattenRouteCollection($routes); $groups = $this->groupRoutesByHostnameRegex($routes); $code = ''; @@ -321,31 +320,6 @@ EOF; return $code; } - /** - * Flattens a tree of routes to a single collection. - * - * @param RouteCollection $routes Collection of routes - * @param DumperCollection|null $to A DumperCollection to add routes to - * - * @return DumperCollection - */ - private function flattenRouteCollection(RouteCollection $routes, DumperCollection $to = null) - { - if (null === $to) { - $to = new DumperCollection(); - } - - foreach ($routes as $name => $route) { - if ($route instanceof RouteCollection) { - $this->flattenRouteCollection($route, $to); - } else { - $to->add(new DumperRoute($name, $route)); - } - } - - return $to; - } - /** * Groups consecutive routes having the same hostname regex. * @@ -355,7 +329,7 @@ EOF; * * @return DumperCollection A collection with routes grouped by hostname regex in sub-collections */ - private function groupRoutesByHostnameRegex(DumperCollection $routes) + private function groupRoutesByHostnameRegex(RouteCollection $routes) { $groups = new DumperCollection(); @@ -363,14 +337,14 @@ EOF; $currentGroup->setAttribute('hostname_regex', null); $groups->add($currentGroup); - foreach ($routes as $route) { - $hostnameRegex = $route->getRoute()->compile()->getHostnameRegex(); + foreach ($routes as $name => $route) { + $hostnameRegex = $route->compile()->getHostnameRegex(); if ($currentGroup->getAttribute('hostname_regex') !== $hostnameRegex) { $currentGroup = new DumperCollection(); $currentGroup->setAttribute('hostname_regex', $hostnameRegex); $groups->add($currentGroup); } - $currentGroup->add($route); + $currentGroup->add(new DumperRoute($name, $route)); } return $groups; diff --git a/src/Symfony/Component/Routing/Matcher/TraceableUrlMatcher.php b/src/Symfony/Component/Routing/Matcher/TraceableUrlMatcher.php index 1e2491c488..6ef846dd69 100644 --- a/src/Symfony/Component/Routing/Matcher/TraceableUrlMatcher.php +++ b/src/Symfony/Component/Routing/Matcher/TraceableUrlMatcher.php @@ -44,14 +44,6 @@ class TraceableUrlMatcher extends UrlMatcher protected function matchCollection($pathinfo, RouteCollection $routes) { foreach ($routes as $name => $route) { - if ($route instanceof RouteCollection) { - if (!$ret = $this->matchCollection($pathinfo, $route)) { - continue; - } - - return true; - } - $compiledRoute = $route->compile(); if (!preg_match($compiledRoute->getRegex(), $pathinfo, $matches)) { diff --git a/src/Symfony/Component/Routing/Matcher/UrlMatcher.php b/src/Symfony/Component/Routing/Matcher/UrlMatcher.php index 5c6985447c..7a12dd16b9 100644 --- a/src/Symfony/Component/Routing/Matcher/UrlMatcher.php +++ b/src/Symfony/Component/Routing/Matcher/UrlMatcher.php @@ -105,18 +105,6 @@ class UrlMatcher implements UrlMatcherInterface protected function matchCollection($pathinfo, RouteCollection $routes) { foreach ($routes as $name => $route) { - if ($route instanceof RouteCollection) { - if (false === strpos($route->getPrefix(), '{') && $route->getPrefix() !== substr($pathinfo, 0, strlen($route->getPrefix()))) { - continue; - } - - if (!$ret = $this->matchCollection($pathinfo, $route)) { - continue; - } - - return $ret; - } - $compiledRoute = $route->compile(); // check the static prefix of the URL first. Only use the more expensive preg_match when it matches diff --git a/src/Symfony/Component/Routing/RouteCollection.php b/src/Symfony/Component/Routing/RouteCollection.php index ebbf236b58..91efdf4a71 100644 --- a/src/Symfony/Component/Routing/RouteCollection.php +++ b/src/Symfony/Component/Routing/RouteCollection.php @@ -14,10 +14,11 @@ namespace Symfony\Component\Routing; use Symfony\Component\Config\Resource\ResourceInterface; /** - * A RouteCollection represents a set of Route instances as a tree structure. + * A RouteCollection represents a set of Route instances. * - * When adding a route, it overrides existing routes with the - * same name defined in the instance or its children and parents. + * When adding a route at the end of the collection, an existing route + * with the same name is removed first. So there can only be one route + * with a given name. * * @author Fabien Potencier * @author Tobias Schultze @@ -27,7 +28,7 @@ use Symfony\Component\Config\Resource\ResourceInterface; class RouteCollection implements \IteratorAggregate, \Countable { /** - * @var (RouteCollection|Route)[] + * @var Route[] */ private $routes = array(); @@ -43,6 +44,7 @@ class RouteCollection implements \IteratorAggregate, \Countable /** * @var RouteCollection|null + * @deprecated since version 2.2, will be removed in 2.3 */ private $parent; @@ -50,9 +52,6 @@ class RouteCollection implements \IteratorAggregate, \Countable { foreach ($this->routes as $name => $route) { $this->routes[$name] = clone $route; - if ($route instanceof RouteCollection) { - $this->routes[$name]->setParent($this); - } } } @@ -60,6 +59,8 @@ class RouteCollection implements \IteratorAggregate, \Countable * Gets the parent RouteCollection. * * @return RouteCollection|null The parent RouteCollection or null when it's the root + * + * @deprecated since version 2.2, will be removed in 2.3 */ public function getParent() { @@ -67,9 +68,11 @@ class RouteCollection implements \IteratorAggregate, \Countable } /** - * Gets the root RouteCollection of the tree. + * Gets the root RouteCollection. * * @return RouteCollection The root RouteCollection + * + * @deprecated since version 2.2, will be removed in 2.3 */ public function getRoot() { @@ -82,9 +85,13 @@ class RouteCollection implements \IteratorAggregate, \Countable } /** - * Gets the current RouteCollection as an Iterator that includes all routes and child route collections. + * Gets the current RouteCollection as an Iterator that includes all routes. + * + * It implements \IteratorAggregate. * - * @return \ArrayIterator An \ArrayIterator interface + * @see all() + * + * @return \ArrayIterator An \ArrayIterator object for iterating over routes */ public function getIterator() { @@ -94,16 +101,11 @@ class RouteCollection implements \IteratorAggregate, \Countable /** * Gets the number of Routes in this collection. * - * @return int The number of routes in this collection, including nested collections + * @return int The number of routes */ public function count() { - $count = 0; - foreach ($this->routes as $route) { - $count += $route instanceof RouteCollection ? count($route) : 1; - } - - return $count; + return count($this->routes); } /** @@ -122,32 +124,23 @@ class RouteCollection implements \IteratorAggregate, \Countable 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->remove($name); + unset($this->routes[$name]); $this->routes[$name] = $route; } /** - * Returns all routes in this collection and its children. + * Returns all routes in this collection. * * @return Route[] An array of routes */ public function all() { - $routes = array(); - foreach ($this->routes as $name => $route) { - if ($route instanceof RouteCollection) { - $routes = array_merge($routes, $route->all()); - } else { - $routes[$name] = $route; - } - } - - return $routes; + return $this->routes; } /** - * Gets a route by name defined in this collection or its children. + * Gets a route by name. * * @param string $name The route name * @@ -155,36 +148,31 @@ class RouteCollection implements \IteratorAggregate, \Countable */ public function get($name) { - if (isset($this->routes[$name])) { - return $this->routes[$name] instanceof RouteCollection ? null : $this->routes[$name]; - } - - foreach ($this->routes as $routes) { - if ($routes instanceof RouteCollection && null !== $route = $routes->get($name)) { - return $route; - } - } - - return null; + return isset($this->routes[$name]) ? $this->routes[$name] : null; } /** - * Removes a route or an array of routes by name from all connected - * collections (this instance and all parents and children). + * Removes a route or an array of routes by name from the collection + * + * For BC it's also removed from the root, which will not be the case in 2.3 + * as the RouteCollection won't be a tree structure. * * @param string|array $name The route name or an array of route names */ public function remove($name) { + // just for BC $root = $this->getRoot(); foreach ((array) $name as $n) { - $root->removeRecursively($n); + unset($root->routes[$n]); + unset($this->routes[$n]); } } /** - * Adds a route collection to the current set of routes (at the end of the current set). + * Adds a route collection at the end of the current set by appending all + * routes of the added collection. * * @param RouteCollection $collection A RouteCollection instance * @param string $prefix An optional prefix to add before each pattern of the route collection @@ -193,22 +181,16 @@ class RouteCollection implements \IteratorAggregate, \Countable * @param array $options An array of options * @param string $hostnamePattern Hostname pattern * - * @throws \InvalidArgumentException When the RouteCollection already exists in the tree - * * @api */ public function addCollection(RouteCollection $collection, $prefix = '', $defaults = array(), $requirements = array(), $options = array(), $hostnamePattern = '') { - // prevent infinite loops by recursive referencing - $root = $this->getRoot(); - if ($root === $collection || $root->hasCollection($collection)) { - throw new \InvalidArgumentException('The RouteCollection already exists in the tree.'); - } + // This is to keep BC for getParent() and getRoot(). It does not prevent + // infinite loops by recursive referencing. But we don't need that logic + // anymore as the tree logic has been deprecated and we are just widening + // the accepted range. + $collection->parent = $this; - // 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 does not // necessarily already have it applied (depending on the order RouteCollections are added to each other) $collection->addPrefix($this->getPrefix() . $prefix, $defaults, $requirements, $options); @@ -217,7 +199,14 @@ class RouteCollection implements \IteratorAggregate, \Countable $collection->setHostnamePattern($hostnamePattern); } - $this->routes[] = $collection; + // we need to remove all routes with the same names first because just replacing them + // would not place the new route at the end of the merged array + foreach ($collection->all() as $name => $route) { + unset($this->routes[$name]); + $this->routes[$name] = $route; + } + + $this->resources = array_merge($this->resources, $collection->getResources()); } /** @@ -244,17 +233,12 @@ class RouteCollection implements \IteratorAggregate, \Countable } foreach ($this->routes as $route) { - if ($route instanceof RouteCollection) { - // we add the slashes so the prefix is not lost by trimming in the sub-collection - $route->addPrefix('/' . $prefix . '/', $defaults, $requirements, $options); - } else { - if ('' !== $prefix) { - $route->setPattern('/' . $prefix . $route->getPattern()); - } - $route->addDefaults($defaults); - $route->addRequirements($requirements); - $route->addOptions($options); + if ('' !== $prefix) { + $route->setPattern('/' . $prefix . $route->getPattern()); } + $route->addDefaults($defaults); + $route->addRequirements($requirements); + $route->addOptions($options); } } @@ -269,7 +253,7 @@ class RouteCollection implements \IteratorAggregate, \Countable } /** - * Sets the hostname pattern on all child routes. + * Sets the hostname pattern on all routes. * * @param string $pattern The pattern */ @@ -287,14 +271,7 @@ class RouteCollection implements \IteratorAggregate, \Countable */ public function getResources() { - $resources = $this->resources; - foreach ($this->routes as $routes) { - if ($routes instanceof RouteCollection) { - $resources = array_merge($resources, $routes->getResources()); - } - } - - return array_unique($resources); + return array_unique($this->resources); } /** @@ -306,60 +283,4 @@ class RouteCollection implements \IteratorAggregate, \Countable { $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 - // iterating 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 && $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 hasCollection(RouteCollection $collection) - { - foreach ($this->routes as $routes) { - if ($routes === $collection || $routes instanceof RouteCollection && $routes->hasCollection($collection)) { - return true; - } - } - - return false; - } } From 50e625962c004650e03c8e2481591e7ae8170b42 Mon Sep 17 00:00:00 2001 From: Tobias Schultze Date: Mon, 26 Nov 2012 18:29:17 +0100 Subject: [PATCH 2/3] adjusted tests --- .../Matcher/Dumper/PhpMatcherDumperTest.php | 4 +- .../Routing/Tests/RouteCollectionTest.php | 74 ++----------------- 2 files changed, 7 insertions(+), 71 deletions(-) diff --git a/src/Symfony/Component/Routing/Tests/Matcher/Dumper/PhpMatcherDumperTest.php b/src/Symfony/Component/Routing/Tests/Matcher/Dumper/PhpMatcherDumperTest.php index 9c15be5642..e04310cc9a 100644 --- a/src/Symfony/Component/Routing/Tests/Matcher/Dumper/PhpMatcherDumperTest.php +++ b/src/Symfony/Component/Routing/Tests/Matcher/Dumper/PhpMatcherDumperTest.php @@ -135,8 +135,8 @@ class PhpMatcherDumperTest extends \PHPUnit_Framework_TestCase $collection3 = new RouteCollection(); $collection3->add('overridden2', new Route('/new')); $collection3->add('hey', new Route('/hey/')); - $collection1->addCollection($collection2); $collection2->addCollection($collection3); + $collection1->addCollection($collection2); $collection->addCollection($collection1, '/multi'); // "dynamic" prefix @@ -216,8 +216,8 @@ class PhpMatcherDumperTest extends \PHPUnit_Framework_TestCase $collection3 = new RouteCollection(); $collection3->add('c', new Route('/{var}')); - $collection1->addCollection($collection2, '/b'); $collection2->addCollection($collection3, '/c'); + $collection1->addCollection($collection2, '/b'); $collection->addCollection($collection1, '/a'); /* test case 2 */ diff --git a/src/Symfony/Component/Routing/Tests/RouteCollectionTest.php b/src/Symfony/Component/Routing/Tests/RouteCollectionTest.php index 16d193e34b..840a1d0dc6 100644 --- a/src/Symfony/Component/Routing/Tests/RouteCollectionTest.php +++ b/src/Symfony/Component/Routing/Tests/RouteCollectionTest.php @@ -70,8 +70,8 @@ class RouteCollectionTest extends \PHPUnit_Framework_TestCase $collection->add('foo', new Route('/foo')); $collection1 = new RouteCollection(); - $collection->addCollection($collection1); $collection1->add('foo', new Route('/foo1')); + $collection->addCollection($collection1); $this->assertEquals('/foo1', $this->getFirstNamedRoute($collection, 'foo')->getPattern()); } @@ -82,8 +82,8 @@ class RouteCollectionTest extends \PHPUnit_Framework_TestCase $collection->add('foo', new Route('/foo')); $collection1 = new RouteCollection(); - $collection->addCollection($collection1); $collection1->add('foo1', new Route('/foo1')); + $collection->addCollection($collection1); $this->assertCount(2, $collection); } @@ -212,19 +212,12 @@ class RouteCollectionTest extends \PHPUnit_Framework_TestCase $collection3 = new RouteCollection(); $collection3->add('foo', $new = new Route('/new')); - $collection1->addCollection($collection2); $collection2->addCollection($collection3); - - $collection1->add('stay', new Route('/stay')); - - $iterator = $collection1->getIterator(); + $collection1->addCollection($collection2); $this->assertSame($new, $collection1->get('foo'), '->get() returns new route that overrode previous one'); - // 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'); + // size of 1 because collection1 contains /new but not /old anymore + $this->assertCount(1, $collection1->getIterator(), '->addCollection() removes previous routes when adding new routes with the same name'); } public function testGet() @@ -241,63 +234,6 @@ class RouteCollectionTest extends \PHPUnit_Framework_TestCase $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 testPatternDoesNotChangeWhenDefinitionOrderChanges() - { - $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 now - - $this->assertEquals($rootCollection_A, $rootCollection_B); - } - public function testSetHostnamePattern() { $collection = new RouteCollection(); From 51223c05fff207b6a1b954c0be0575470fc16279 Mon Sep 17 00:00:00 2001 From: Tobias Schultze Date: Tue, 27 Nov 2012 20:03:51 +0100 Subject: [PATCH 3/3] added upgrade instructions --- UPGRADE-2.2.md | 36 ++++++++++++++++++++++ src/Symfony/Component/Routing/CHANGELOG.md | 33 ++++++++++++++++++++ 2 files changed, 69 insertions(+) diff --git a/UPGRADE-2.2.md b/UPGRADE-2.2.md index 8025bab73b..43cff13cb7 100644 --- a/UPGRADE-2.2.md +++ b/UPGRADE-2.2.md @@ -36,6 +36,42 @@ * The PasswordType is now not trimmed by default. +### Routing + + * RouteCollection does not behave like a tree structure anymore but as a flat + array of Routes. So when using PHP to build the RouteCollection, you must + make sure to add routes to the sub-collection before adding it to the parent + collection (this is not relevant when using YAML or XML for Route definitions). + + Before: + + ``` + $rootCollection = new RouteCollection(); + $subCollection = new RouteCollection(); + $rootCollection->addCollection($subCollection); + $subCollection->add('foo', new Route('/foo')); + ``` + + After: + + ``` + $rootCollection = new RouteCollection(); + $subCollection = new RouteCollection(); + $subCollection->add('foo', new Route('/foo')); + $rootCollection->addCollection($subCollection); + ``` + + Also one must call `addCollection` from the bottom to the top hierarchy. + So the correct sequence is the following (and not the reverse): + + ``` + $childCollection->->addCollection($grandchildCollection); + $rootCollection->addCollection($childCollection); + ``` + + * The methods `RouteCollection::getParent()` and `RouteCollection::getRoot()` + have been deprecated and will be removed in Symfony 2.3. + ### Validator * Interfaces were created for created for the classes `ConstraintViolation`, diff --git a/src/Symfony/Component/Routing/CHANGELOG.md b/src/Symfony/Component/Routing/CHANGELOG.md index 92a460dd14..39bf2b88b3 100644 --- a/src/Symfony/Component/Routing/CHANGELOG.md +++ b/src/Symfony/Component/Routing/CHANGELOG.md @@ -4,6 +4,39 @@ CHANGELOG 2.2.0 ----- + * [BC BREAK] RouteCollection does not behave like a tree structure anymore but as + a flat array of Routes. So when using PHP to build the RouteCollection, you must + make sure to add routes to the sub-collection before adding it to the parent + collection (this is not relevant when using YAML or XML for Route definitions). + + Before: + + ``` + $rootCollection = new RouteCollection(); + $subCollection = new RouteCollection(); + $rootCollection->addCollection($subCollection); + $subCollection->add('foo', new Route('/foo')); + ``` + + After: + + ``` + $rootCollection = new RouteCollection(); + $subCollection = new RouteCollection(); + $subCollection->add('foo', new Route('/foo')); + $rootCollection->addCollection($subCollection); + ``` + + Also one must call `addCollection` from the bottom to the top hierarchy. + So the correct sequence is the following (and not the reverse): + + ``` + $childCollection->->addCollection($grandchildCollection); + $rootCollection->addCollection($childCollection); + ``` + + * The methods `RouteCollection::getParent()` and `RouteCollection::getRoot()` + have been deprecated and will be removed in Symfony 2.3. * added support for the method default argument values when defining a @Route * Adjacent placeholders without separator work now, e.g. `/{x}{y}{z}.{_format}`. * Characters that function as separator between placeholders are now whitelisted