Skip to content

[Routing] removed tree structure from RouteCollection #6120

Merged
merged 3 commits into from Dec 6, 2012
View
36 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`,
View
20 src/Symfony/Bundle/FrameworkBundle/Routing/Router.php
@@ -84,19 +84,15 @@ public function warmUp($cacheDir)
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()));
}
}
View
33 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
View
34 src/Symfony/Component/Routing/Matcher/Dumper/PhpMatcherDumper.php
@@ -111,7 +111,6 @@ private function compileRoutes(RouteCollection $routes, $supportsRedirections)
{
$fetchedHostname = false;
- $routes = $this->flattenRouteCollection($routes);
$groups = $this->groupRoutesByHostnameRegex($routes);
$code = '';
@@ -322,31 +321,6 @@ private function compileRoute(Route $route, $name, $supportsRedirections, $paren
}
/**
- * 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.
*
* The results is a collection of collections of routes having the same hostname regex.
@@ -355,22 +329,22 @@ private function flattenRouteCollection(RouteCollection $routes, DumperCollectio
*
* @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();
$currentGroup = new DumperCollection();
$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;
View
8 src/Symfony/Component/Routing/Matcher/TraceableUrlMatcher.php
@@ -44,14 +44,6 @@ public function getTraces($pathinfo)
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)) {
View
12 src/Symfony/Component/Routing/Matcher/UrlMatcher.php
@@ -105,18 +105,6 @@ public function match($pathinfo)
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
View
185 src/Symfony/Component/Routing/RouteCollection.php
@@ -14,10 +14,11 @@
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 <fabien@symfony.com>
* @author Tobias Schultze <http://tobion.de>
@@ -27,7 +28,7 @@
class RouteCollection implements \IteratorAggregate, \Countable
{
/**
- * @var (RouteCollection|Route)[]
+ * @var Route[]
*/
private $routes = array();
@@ -43,33 +44,35 @@ class RouteCollection implements \IteratorAggregate, \Countable
/**
* @var RouteCollection|null
+ * @deprecated since version 2.2, will be removed in 2.3
*/
private $parent;
public function __clone()
{
foreach ($this->routes as $name => $route) {
$this->routes[$name] = clone $route;
- if ($route instanceof RouteCollection) {
- $this->routes[$name]->setParent($this);
- }
}
}
/**
* 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()
{
return $this->parent;
}
/**
- * 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 @@ public function getRoot()
}
/**
- * 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.
+ *
+ * @see all()
*
- * @return \ArrayIterator An \ArrayIterator interface
+ * @return \ArrayIterator An \ArrayIterator object for iterating over routes
*/
public function getIterator()
{
@@ -94,16 +101,11 @@ public function getIterator()
/**
* 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,69 +124,55 @@ 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->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
*
* @return Route|null A Route instance or null when not found
*/
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 @@ public function remove($name)
* @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.');
- }
-
- // remove all routes with the same names in all existing collections
- $this->remove(array_keys($collection->all()));
+ // 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;
- $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 @@ public function addCollection(RouteCollection $collection, $prefix = '', $defaul
$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 @@ public function addPrefix($prefix, $defaults = array(), $requirements = array(),
}
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 @@ public function getPrefix()
}
/**
- * Sets the hostname pattern on all child routes.
+ * Sets the hostname pattern on all routes.
*
* @param string $pattern The pattern
*/
@@ -287,14 +271,7 @@ public function setHostnamePattern($pattern)
*/
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 @@ 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
- // 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;
- }
}
View
4 src/Symfony/Component/Routing/Tests/Matcher/Dumper/PhpMatcherDumperTest.php
@@ -135,8 +135,8 @@ public function getRouteCollections()
$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 @@ public function getRouteCollections()
$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 */
View
74 src/Symfony/Component/Routing/Tests/RouteCollectionTest.php
@@ -70,8 +70,8 @@ public function testIteratorWithOverriddenRoutes()
$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 @@ public function testCount()
$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 @@ public function testUniqueRouteWithGivenName()
$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 @@ public function testGet()
$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();
Something went wrong with that request. Please try again.