fix bugs and improvements in RouteCollection

b5f51d00598 for 2.0
symfony · Tobion · Apr 6, 2012 · Apr 1, 2012 · Apr 4, 2012 · Apr 5, 2012
commit eaa1f48e9d7c3b06141a1523d8de01806b04a1cb
diff --git 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 <fabien@symfony.com>
+ * @author Tobias Schultze <http://tobion.de>
  *
  * @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
  
10000
    */
     public function get($name)
     {
-        // get the latest defined route
-        foreach (array_reverse($this->routes) as $routes) {
-            if (!$routes instanceof RouteCollection) {
-                continue;
-            }
-
-            if (null !== $route = $routes->get($name)) {
-                return $route;
-            }
-        }
-
         if (isset($this->routes[$name])) {
             return $this->routes[$name];
+        } else {
+            foreach ($this->routes as $routes) {
+                if ($routes instanceof RouteCollection && null !== $route = $routes->get($name)) {
+                    return $route;
+                }
+            }
         }
+
+        return null;
     }
 
     /**
-     * Removes a route by name.
+     * Removes a route by name from all connected collections (this instance and all parents and children).
+     *
+     * @param string $name The route name
+     */
+    public function removeCompletely($name)
+    {
+        $parent = $this;
+        while ($parent->getParent()) {
+            $parent = $parent->getParent();
+        }
+
+        $parent->remove($name);
+    }
+
+    /**
+     * Removes a route by name from this collection and its children.
      *
      * @param string $name The route name
      */
     public function remove($name)
     {
+        // the route can only be in this RouteCollection or one of its children (not both) because the
+        // adders (->add and ->addCollection) make sure there is only one route per name in all collections     
         if (isset($this->routes[$name])) {
             unset($this->routes[$name]);
-        }
-
-        foreach ($this->routes as $routes) {
-            if ($routes instanceof RouteCollection) {
-                $routes->remove($name);
+        } else {
+            foreach ($this->routes as $routes) {
+                if ($routes instanceof RouteCollection) {
+                    $routes->remove($name);
+                }
             }
         }
     }
@@ -187,7 +195,7 @@ public function addCollection(RouteCollection $collection, $prefix = '')
 
         // remove all routes with the same name in all existing collections
         foreach (array_keys($collection->all()) as $name) {
-            $this->remove($name);
+            $this->removeCompletely($name);
         }
 
         $this->routes[] = $collection;
@@ -210,7 +218,7 @@ public function addPrefix($prefix)
         }
 
         // a prefix must start with a slash
-        if ('/' !== $prefix[0]) {
+        if ('' !== $prefix && '/' !== $prefix[0]) {
             $prefix = '/'.$prefix;
         }
 
@@ -225,6 +233,12 @@ public function addPrefix($prefix)
         }
     }
 
+    /**
+     * Returns the prefix that may contain placeholders. 
+     * When given, it must start with a slash and must not end with a slash.
+     *
+     * @return string The prefix
+     */
     public function getPrefix()
     {
         return $this->prefix;

diff --git a/tests/Symfony/Tests/Component/Routing/Matcher/UrlMatcherTest.php b/tests/Symfony/Tests/Component/Routing/Matcher/UrlMatcherTest.php
@@ -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());