[Routing] fixed route overriden mechanism when using embedded collections (closes #2139)

src/Symfony/Component/Routing/Matcher/Dumper/PhpMatcherDumper.php

@@ -91,7 +91,7 @@ EOF;
                 if ($optimizable) {
                     for ($j = $i; $j < $keysCount; $j++) {
                         if ($keys[$j] === null) {
-                          continue;
+                            continue;
                         }
 
                         $testRoute = $routeIterator->offsetGet($keys[$j]);

src/Symfony/Component/Routing/RouteCollection.php

@@ -16,6 +16,9 @@ use Symfony\Component\Config\Resource\ResourceInterface;
 /**
  * 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 @@ class RouteCollection implements \IteratorAggregate
         $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 @@ class RouteCollection implements \IteratorAggregate
             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 @@ class RouteCollection implements \IteratorAggregate
         }
     }
 
+    /**
+     * 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 @@ class RouteCollection implements \IteratorAggregate
      */
     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;
     }
 

tests/Symfony/Tests/Component/Routing/Fixtures/dumper/url_matcher1.php

@@ -108,54 +108,59 @@ class ProjectUrlMatcher extends Symfony\Component\Routing\Matcher\UrlMatcher
 
         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;
         }
 

tests/Symfony/Tests/Component/Routing/Fixtures/dumper/url_matcher2.php

@@ -120,54 +120,59 @@ class ProjectUrlMatcher extends Symfony\Tests\Component\Routing\Fixtures\Redirec
 
         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;
         }
 

tests/Symfony/Tests/Component/Routing/Matcher/Dumper/PhpMatcherDumperTest.php

@@ -19,9 +19,53 @@ use Symfony\Component\Routing\RequestContext;
 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 @@ class PhpMatcherDumperTest extends \PHPUnit_Framework_TestCase
 
         // 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 @@ class PhpMatcherDumperTest extends \PHPUnit_Framework_TestCase
 
         // 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;
     }
 }

tests/Symfony/Tests/Component/Routing/Matcher/UrlMatcherTest.php

@@ -170,6 +170,23 @@ class UrlMatcherTest extends \PHPUnit_Framework_TestCase
         $this->assertEquals(array('_route' => 'foo', 'foo' => "\n"), $matcher->match('/'.urlencode("\n").'/bar'), 'linefeed character is matched');
     }
 
+    public function testMatchOverridenRoute()
+    {
+        $collection = new RouteCollection();
+        $collection->add('foo', new Route('/foo'));
+
+        $collection1 = new RouteCollection();
+        $collection1->add('foo', new Route('/foo1'));
+
+        $collection->addCollection($collection1);
+
+        $matcher = new UrlMatcher($collection, new RequestContext(), array());
+
+        $this->assertEquals(array('_route' => 'foo'), $matcher->match('/foo1'));
+        $this->setExpectedException('Symfony\Component\Routing\Exception\ResourceNotFoundException');
+        $this->assertEquals(array(), $matcher->match('/foo'));
+    }
+
     public function testMatchRegression()
     {
         $coll = new RouteCollection();
@@ -188,4 +205,4 @@ class UrlMatcherTest extends \PHPUnit_Framework_TestCase
         } catch (ResourceNotFoundException $e) {
         }
     }
-}
+}

tests/Symfony/Tests/Component/Routing/RouteCollectionTest.php

@@ -28,7 +28,6 @@ class RouteCollectionTest extends \PHPUnit_Framework_TestCase
     }
 
     /**
-     * @covers Symfony\Component\Routing\RouteCollection::add
      * @expectedException InvalidArgumentException
      */
     public function testAddInvalidRoute()
@@ -38,6 +37,58 @@ class RouteCollectionTest extends \PHPUnit_Framework_TestCase
         $collection->add('f o o', $route);
     }
 
+    public function testOverridenRoute()
+    {
+        $collection = new RouteCollection();
+        $collection->add('foo', new Route('/foo'));
+        $collection->add('foo', new Route('/foo1'));
+
+        $this->assertEquals('/foo1', $collection->get('foo')->getPattern());
+    }
+
+    public function testDeepOverridenRoute()
+    {
+        $collection = new RouteCollection();
+        $collection->add('foo', new Route('/foo'));
+
+        $collection1 = new RouteCollection();
+        $collection1->add('foo', new Route('/foo1'));
+
+        $collection2 = new RouteCollection();
+        $collection2->add('foo', new Route('/foo2'));
+
+        $collection1->addCollection($collection2);
+        $collection->addCollection($collection1);
+
+        $this->assertEquals('/foo2', $collection1->get('foo')->getPattern());
+        $this->assertEquals('/foo2', $collection->get('foo')->getPattern());
+    }
+
+    public function testIteratorWithOverridenRoutes()
+    {
+        $collection = new RouteCollection();
+        $collection->add('foo', new Route('/foo'));
+
+        $collection1 = new RouteCollection();
+        $collection->addCollection($collection1);
+        $collection1->add('foo', new Route('/foo1'));
+
+        $this->assertEquals('/foo1', $this->getFirstNamedRoute($collection, 'foo')->getPattern());
+    }
+
+    protected function getFirstNamedRoute(RouteCollection $routeCollection, $name)
+    {
+        foreach ($routeCollection as $key => $route) {
+            if ($route instanceof RouteCollection) {
+                return $this->getFirstNamedRoute($route, $name);
+            }
+
+            if ($name === $key) {
+                return $route;
+            }
+        }
+    }
+
     public function testAddCollection()
     {
         $collection = new RouteCollection();