[Routing] fixed route overriden mechanism when using embedded collect…

…ions (closes symfony#2139)
lsmith77 · Sep 30, 2011 · 5c8a2fb · 5c8a2fb
1 parent 245d57a
commit 5c8a2fb
Show file tree

Hide file tree

Showing 7 changed files with 225 additions and 72 deletions.
diff --git a/src/Symfony/Component/Routing/Matcher/Dumper/PhpMatcherDumper.php b/src/Symfony/Component/Routing/Matcher/Dumper/PhpMatcherDumper.php
@@ -91,7 +91,7 @@ private function compileRoutes(RouteCollection $routes, $supportsRedirections, $
                 if ($optimizable) {
                     for ($j = $i; $j < $keysCount; $j++) {
                         if ($keys[$j] === null) {
-                          continue;
+                            continue;
                         }
 
                         $testRoute = $routeIterator->offsetGet($keys[$j]);

diff --git a/src/Symfony/Component/Routing/RouteCollection.php b/src/Symfony/Component/Routing/RouteCollection.php
@@ -16,6 +16,9 @@
 /**
  * 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.
+ *
  * @author Fabien Potencier <fabien@symfony.com>
  *
  * @api
@@ -25,6 +28,7 @@ class RouteCollection implements \IteratorAggregate
     private $routes;
     private $resources;
     private $prefix;
+    private $parent;
 
     /**
      * Constructor.
@@ -38,6 +42,31 @@ public function __construct()
         $this->prefix = '';
     }
 
+    /**
+     * Gets the parent RouteCollection.
+     *
+     * @return RouteCollection The parent RouteCollection
+     */
+    public function getParent()
+    {
+        return $this->parent;
+    }
+
+    /**
+     * Sets the parent RouteCollection.
+     *
+     * @param RouteCollection $parent The parent RouteCollection
+     */
+    public function setParent(RouteCollection $parent)
+    {
+        $this->parent = $parent;
+    }
+
+    /**
+     * Gets the current RouteCollection as an Iterator.
+     *
+     * @return \ArrayIterator An \ArrayIterator interface
+     */
     public function getIterator()
     {
         return new \ArrayIterator($this->routes);
@@ -59,6 +88,15 @@ public function add($name, Route $route)
             throw new \InvalidArgumentException(sprintf('Name "%s" contains non valid characters for a route name.', $name));
         }
 
+        $parent = $this;
+        while ($parent->getParent()) {
+            $parent = $parent->getParent();
+        }
+
+        if ($parent) {
+            $parent->remove($name);
+        }
+
         $this->routes[$name] = $route;
     }
 
@@ -106,6 +144,24 @@ public function get($name)
         }
     }
 
+    /**
+     * Removes a route by name.
+     *
+     * @param string $name The route name
+     */
+    public function remove($name)
+    {
+        if (isset($this->routes[$name])) {
+            unset($this->routes[$name]);
+        }
+
+        foreach ($this->routes as $routes) {
+            if ($routes instanceof RouteCollection) {
+                $routes->remove($name);
+            }
+        }
+    }
+
     /**
      * Adds a route collection to the current set of routes (at the end of the current set).
      *
@@ -116,8 +172,14 @@ public function get($name)
      */
     public function addCollection(RouteCollection $collection, $prefix = '')
     {
+        $collection->setParent($this);
         $collection->addPrefix($prefix);
 
+        // remove all routes with the same name in all existing collections
+        foreach (array_keys($collection->all()) as $name) {
+            $this->remove($name);
+        }
+
         $this->routes[] = $collection;
     }
 

diff --git a/tests/Symfony/Tests/Component/Routing/Fixtures/dumper/url_matcher1.php b/tests/Symfony/Tests/Component/Routing/Fixtures/dumper/url_matcher1.php
@@ -108,54 +108,59 @@ public function match($pathinfo)
 
         if (0 === strpos($pathinfo, '/a')) {
             if (0 === strpos($pathinfo, '/a/b\'b')) {
-                // foo
+                // foo1
                 if (preg_match('#^/a/b\'b/(?P<foo>[^/]+?)$#xs', $pathinfo, $matches)) {
-                    $matches['_route'] = 'foo';
+                    $matches['_route'] = 'foo1';
                     return $matches;
                 }
 
-                // bar
+                // bar1
                 if (preg_match('#^/a/b\'b/(?P<bar>[^/]+?)$#xs', $pathinfo, $matches)) {
-                    $matches['_route'] = 'bar';
+                    $matches['_route'] = 'bar1';
                     return $matches;
                 }
 
-                // foo1
+                // foo2
                 if (preg_match('#^/a/b\'b/(?P<foo1>[^/]+?)$#xs', $pathinfo, $matches)) {
-                    $matches['_route'] = 'foo1';
+                    $matches['_route'] = 'foo2';
                     return $matches;
                 }
 
-                // bar1
+                // bar2
                 if (preg_match('#^/a/b\'b/(?P<bar1>[^/]+?)$#xs', $pathinfo, $matches)) {
-                    $matches['_route'] = 'bar1';
+                    $matches['_route'] = 'bar2';
                     return $matches;
                 }
 
             }
 
+            // overriden
+            if ($pathinfo === '/a/overriden2') {
+                return array('_route' => 'overriden');
+            }
+
             // ababa
             if ($pathinfo === '/ababa') {
                 return array('_route' => 'ababa');
             }
 
-            // foo
+            // foo4
             if (preg_match('#^/aba/(?P<foo>[^/]+?)$#xs', $pathinfo, $matches)) {
-                $matches['_route'] = 'foo';
+                $matches['_route'] = 'foo4';
                 return $matches;
             }
 
         }
 
-        // foo
+        // foo3
         if (preg_match('#^/(?P<_locale>[^/]+?)/b/(?P<foo>[^/]+?)$#xs', $pathinfo, $matches)) {
-            $matches['_route'] = 'foo';
+            $matches['_route'] = 'foo3';
             return $matches;
         }
 
-        // bar
+        // bar3
         if (preg_match('#^/(?P<_locale>[^/]+?)/b/(?P<bar>[^/]+?)$#xs', $pathinfo, $matches)) {
-            $matches['_route'] = 'bar';
+            $matches['_route'] = 'bar3';
             return $matches;
         }
 

diff --git a/tests/Symfony/Tests/Component/Routing/Fixtures/dumper/url_matcher2.php b/tests/Symfony/Tests/Component/Routing/Fixtures/dumper/url_matcher2.php
@@ -120,54 +120,59 @@ public function match($pathinfo)
 
         if (0 === strpos($pathinfo, '/a')) {
             if (0 === strpos($pathinfo, '/a/b\'b')) {
-                // foo
+                // foo1
                 if (preg_match('#^/a/b\'b/(?P<foo>[^/]+?)$#xs', $pathinfo, $matches)) {
-                    $matches['_route'] = 'foo';
+                    $matches['_route'] = 'foo1';
                     return $matches;
                 }
 
-                // bar
+                // bar1
                 if (preg_match('#^/a/b\'b/(?P<bar>[^/]+?)$#xs', $pathinfo, $matches)) {
-                    $matches['_route'] = 'bar';
+                    $matches['_route'] = 'bar1';
                     return $matches;
                 }
 
-                // foo1
+                // foo2
                 if (preg_match('#^/a/b\'b/(?P<foo1>[^/]+?)$#xs', $pathinfo, $matches)) {
-                    $matches['_route'] = 'foo1';
+                    $matches['_route'] = 'foo2';
                     return $matches;
                 }
 
-                // bar1
+                // bar2
                 if (preg_match('#^/a/b\'b/(?P<bar1>[^/]+?)$#xs', $pathinfo, $matches)) {
-                    $matches['_route'] = 'bar1';
+                    $matches['_route'] = 'bar2';
                     return $matches;
                 }
 
             }
 
+            // overriden
+            if ($pathinfo === '/a/overriden2') {
+                return array('_route' => 'overriden');
+            }
+
             // ababa
             if ($pathinfo === '/ababa') {
                 return array('_route' => 'ababa');
             }
 
-            // foo
+            // foo4
             if (preg_match('#^/aba/(?P<foo>[^/]+?)$#xs', $pathinfo, $matches)) {
-                $matches['_route'] = 'foo';
+                $matches['_route'] = 'foo4';
                 return $matches;
             }
 
         }
 
-        // foo
+        // foo3
         if (preg_match('#^/(?P<_locale>[^/]+?)/b/(?P<foo>[^/]+?)$#xs', $pathinfo, $matches)) {
-            $matches['_route'] = 'foo';
+            $matches['_route'] = 'foo3';
             return $matches;
         }
 
-        // bar
+        // bar3
         if (preg_match('#^/(?P<_locale>[^/]+?)/b/(?P<bar>[^/]+?)$#xs', $pathinfo, $matches)) {
-            $matches['_route'] = 'bar';
+            $matches['_route'] = 'bar3';
             return $matches;
         }
 

diff --git a/tests/Symfony/Tests/Component/Routing/Matcher/Dumper/PhpMatcherDumperTest.php b/tests/Symfony/Tests/Component/Routing/Matcher/Dumper/PhpMatcherDumperTest.php
@@ -19,9 +19,53 @@
 class PhpMatcherDumperTest extends \PHPUnit_Framework_TestCase
 {
     public function testDump()
+    {
+        $dumper = new PhpMatcherDumper($this->getRouteCollection(), new RequestContext());
+
+        $this->assertStringEqualsFile(__DIR__.'/../../Fixtures/dumper/url_matcher1.php', $dumper->dump(), '->dump() dumps basic routes to the correct PHP file.');
+
+        $collection = $this->getRouteCollection();
+
+        // 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\Tests\Component\Routing\Fixtures\RedirectableUrlMatcher')), '->dump() dumps basic routes to the correct PHP file.');
+    }
+
+    /**
+     * @expectedException \LogicException
+     */
+    public function testDumpWhenSchemeIsUsedWithoutAProperDumper()
+    {
+        $collection = new RouteCollection();
+        $collection->add('secure', new Route(
+            '/secure',
+            array(),
+            array('_scheme' => 'https')
+        ));
+        $dumper = new PhpMatcherDumper($collection, new RequestContext());
+        $dumper->dump();
+    }
+
+    protected function getRouteCollection()
     {
         $collection = new RouteCollection();
 
+        $collection->add('overriden', new Route('/overriden'));
+
         // defaults and requirements
         $collection->add('foo', new Route(
             '/foo/{bar}',
@@ -82,20 +126,22 @@ public function testDump()
 
         // prefixes
         $collection1 = new RouteCollection();
-        $collection1->add('foo', new Route('/{foo}'));
-        $collection1->add('bar', new Route('/{bar}'));
+        $collection1->add('overriden', new Route('/overriden1'));
+        $collection1->add('foo1', new Route('/{foo}'));
+        $collection1->add('bar1', new Route('/{bar}'));
         $collection2 = new RouteCollection();
         $collection2->addCollection($collection1, '/b\'b');
+        $collection2->add('overriden', new Route('/overriden2'));
         $collection1 = new RouteCollection();
-        $collection1->add('foo1', new Route('/{foo1}'));
-        $collection1->add('bar1', new Route('/{bar1}'));
+        $collection1->add('foo2', new Route('/{foo1}'));
+        $collection1->add('bar2', new Route('/{bar1}'));
         $collection2->addCollection($collection1, '/b\'b');
         $collection->addCollection($collection2, '/a');
 
         // "dynamic" prefix
         $collection1 = new RouteCollection();
-        $collection1->add('foo', new Route('/{foo}'));
-        $collection1->add('bar', new Route('/{bar}'));
+        $collection1->add('foo3', new Route('/{foo}'));
+        $collection1->add('bar3', new Route('/{bar}'));
         $collection2 = new RouteCollection();
         $collection2->addCollection($collection1, '/b');
         $collection->addCollection($collection2, '/{_locale}');
@@ -104,42 +150,9 @@ public function testDump()
 
         // some more prefixes
         $collection1 = new RouteCollection();
-        $collection1->add('foo', new Route('/{foo}'));
+        $collection1->add('foo4', new Route('/{foo}'));
         $collection->addCollection($collection1, '/aba');
 
-        $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')
-        ));
-
-        $this->assertStringEqualsFile(__DIR__.'/../../Fixtures/dumper/url_matcher2.php', $dumper->dump(array('base_class' => 'Symfony\Tests\Component\Routing\Fixtures\RedirectableUrlMatcher')), '->dump() dumps basic routes to the correct PHP file.');
-    }
-
-    /**
-     * @expectedException \LogicException
-     */
-    public function testDumpWhenSchemeIsUsedWithoutAProperDumper()
-    {
-        $collection = new RouteCollection();
-        $collection->add('secure', new Route(
-            '/secure',
-            array(),
-            array('_scheme' => 'https')
-        ));
-        $dumper = new PhpMatcherDumper($collection, new RequestContext());
-        $dumper->dump();
+        return $collection;
     }
 }