[Routing] clean up of RouteCollection API

UPGRADE-2.2.md

@@ -74,6 +74,29 @@
 
  * The methods `RouteCollection::getParent()` and `RouteCollection::getRoot()`
    have been deprecated and will be removed in Symfony 2.3.
+ * Misusing the `RouteCollection::addPrefix` method to add defaults, requirements
+   or options without adding a prefix is not supported anymore. So if you called `addPrefix`
+   with an empty prefix or `/` only (both have no relevance), like
+   `addPrefix('', $defaultsArray, $requirementsArray, $optionsArray)`
+   you need to use the new dedicated methods `addDefaults($defaultsArray)`,
+   `addRequirements($requirementsArray)` or `addOptions($optionsArray)` instead.
+ * The `$options` parameter to `RouteCollection::addPrefix()` has been deprecated
+   because adding options has nothing to do with adding a path prefix. If you want to add options
+   to all child routes of a RouteCollection, you can use `addOptions()`.
+ * The method `RouteCollection::getPrefix()` has been deprecated
+   because it suggested that all routes in the collection would have this prefix, which is
+   not necessarily true. On top of that, since there is no tree structure anymore, this method
+   is also useless.
+ * `RouteCollection::addCollection(RouteCollection $collection)` should now only be
+   used with a single parameter. The other params `$prefix`, `$default`, `$requirements` and `$options`
+   will still work, but have been deprecated. The `addPrefix` method should be used for this
+   use-case instead.
+   Before: `$parentCollection->addCollection($collection, '/prefix', array(...), array(...))`
+   After:
+   ```
+   $collection->addPrefix('/prefix', array(...), array(...));
+   $parentCollection->addCollection($collection);
+   ```
 
 ### Validator
 

src/Symfony/Component/Routing/CHANGELOG.md

@@ -35,8 +35,32 @@ CHANGELOG
    $rootCollection->addCollection($childCollection);
    ```
 
- * The methods `RouteCollection::getParent()` and `RouteCollection::getRoot()`
+ * [DEPRECATION] The methods `RouteCollection::getParent()` and `RouteCollection::getRoot()`
    have been deprecated and will be removed in Symfony 2.3.
+ * [BC BREAK] Misusing the `RouteCollection::addPrefix` method to add defaults, requirements
+   or options without adding a prefix is not supported anymore. So if you called `addPrefix`
+   with an empty prefix or `/` only (both have no relevance), like
+   `addPrefix('', $defaultsArray, $requirementsArray, $optionsArray)`
+   you need to use the new dedicated methods `addDefaults($defaultsArray)`,
+   `addRequirements($requirementsArray)` or `addOptions($optionsArray)` instead.
+ * [DEPRECATION] The `$options` parameter to `RouteCollection::addPrefix()` has been deprecated
+   because adding options has nothing to do with adding a path prefix. If you want to add options
+   to all child routes of a RouteCollection, you can use `addOptions()`.
+ * [DEPRECATION] The method `RouteCollection::getPrefix()` has been deprecated
+   because it suggested that all routes in the collection would have this prefix, which is
+   not necessarily true. On top of that, since there is no tree structure anymore, this method
+   is also useless. Don't worry about performance, prefix optimization for matching is still done
+   in the dumper, which was also improved in 2.2.0 to find even more grouping possibilities.
+ * [DEPRECATION] `RouteCollection::addCollection(RouteCollection $collection)` should now only be
+   used with a single parameter. The other params `$prefix`, `$default`, `$requirements` and `$options`
+   will still work, but have been deprecated. The `addPrefix` method should be used for this
+   use-case instead.
+   Before: `$parentCollection->addCollection($collection, '/prefix', array(...), array(...))`
+   After:
+   ```
+   $collection->addPrefix('/prefix', array(...), array(...));
+   $parentCollection->addCollection($collection);
+   ```
  * 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

src/Symfony/Component/Routing/Loader/XmlFileLoader.php

@@ -78,7 +78,7 @@ protected function parseNode(RouteCollection $collection, \DOMElement $node, $pa
                 $resource = $node->getAttribute('resource');
                 $type = $node->getAttribute('type');
                 $prefix = $node->getAttribute('prefix');
-                $hostnamePattern = $node->getAttribute('hostname-pattern');
+                $hostnamePattern = $node->hasAttribute('hostname-pattern') ? $node->getAttribute('hostname-pattern') : null;
 
                 $defaults = array();
                 $requirements = array();
@@ -105,7 +105,18 @@ protected function parseNode(RouteCollection $collection, \DOMElement $node, $pa
                 }
 
                 $this->setCurrentDir(dirname($path));
-                $collection->addCollection($this->import($resource, ('' !== $type ? $type : null), false, $file), $prefix, $defaults, $requirements, $options, $hostnamePattern);
+
+                $subCollection = $this->import($resource, ('' !== $type ? $type : null), false, $file);
+                /* @var $subCollection RouteCollection */
+                $subCollection->addPrefix($prefix);
+                if (null !== $hostnamePattern) {
+                    $subCollection->setHostnamePattern($hostnamePattern);
+                }
+                $subCollection->addDefaults($defaults);
+                $subCollection->addRequirements($requirements);
+                $subCollection->addOptions($options);
+
+                $collection->addCollection($subCollection);
                 break;
             default:
                 throw new \InvalidArgumentException(sprintf('Unable to parse tag "%s"', $node->tagName));

src/Symfony/Component/Routing/Loader/YamlFileLoader.php

@@ -66,14 +66,25 @@ public function load($file, $type = null)
 
             if (isset($config['resource'])) {
                 $type = isset($config['type']) ? $config['type'] : null;
-                $prefix = isset($config['prefix']) ? $config['prefix'] : null;
+                $prefix = isset($config['prefix']) ? $config['prefix'] : '';
                 $defaults = isset($config['defaults']) ? $config['defaults'] : array();
                 $requirements = isset($config['requirements']) ? $config['requirements'] : array();
                 $options = isset($config['options']) ? $config['options'] : array();
-                $hostnamePattern = isset($config['hostname_pattern']) ? $config['hostname_pattern'] : '';
+                $hostnamePattern = isset($config['hostname_pattern']) ? $config['hostname_pattern'] : null;
 
                 $this->setCurrentDir(dirname($path));
-                $collection->addCollection($this->import($config['resource'], $type, false, $file), $prefix, $defaults, $requirements, $options, $hostnamePattern);
+
+                $subCollection = $this->import($config['resource'], $type, false, $file);
+                /* @var $subCollection RouteCollection */
+                $subCollection->addPrefix($prefix);
+                if (null !== $hostnamePattern) {
+                    $subCollection->setHostnamePattern($hostnamePattern);
+                }
+                $subCollection->addDefaults($defaults);
+                $subCollection->addRequirements($requirements);
+                $subCollection->addOptions($options);
+
+                $collection->addCollection($subCollection);
             } else {
                 $this->parseRoute($collection, $name, $config, $path);
             }

src/Symfony/Component/Routing/RouteCollection.php

@@ -39,6 +39,7 @@ class RouteCollection implements \IteratorAggregate, \Countable
 
     /**
      * @var string
+     * @deprecated since version 2.2, will be removed in 2.3
      */
     private $prefix = '';
 
@@ -169,28 +170,35 @@ public function remove($name)
      * 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
-     * @param array           $defaults        An array of default values
-     * @param array           $requirements    An array of requirements
-     * @param array           $options         An array of options
-     * @param string          $hostnamePattern Hostname pattern
      *
      * @api
      */
-    public function addCollection(RouteCollection $collection, $prefix = '', $defaults = array(), $requirements = array(), $options = array(), $hostnamePattern = '')
+    public function addCollection(RouteCollection $collection)
     {
         // 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;
 
-        // 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);
-
-        if ('' !== $hostnamePattern) {
-            $collection->setHostnamePattern($hostnamePattern);
+        // this is to keep BC
+        $numargs = func_num_args();
+        if ($numargs > 1) {
+            $collection->addPrefix($this->prefix . func_get_arg(1));
+            if ($numargs > 2) {
+                $collection->addDefaults(func_get_arg(2));
+                if ($numargs > 3) {
+                    $collection->addRequirements(func_get_arg(3));
+                    if ($numargs > 4) {
+                        $collection->addOptions(func_get_arg(4));
+                    }
+                }
+            }
+        } else {
+            // 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)
+            // this will be removed when the BC layer for getPrefix() is removed
+            $collection->addPrefix($this->prefix);
         }
 
         // we need to remove all routes with the same names first because just replacing them
@@ -204,32 +212,30 @@ public function addCollection(RouteCollection $collection, $prefix = '', $defaul
     }
 
     /**
-     * Adds a prefix to all routes in the current set.
+     * Adds a prefix to the path of all child routes.
      *
      * @param string $prefix       An optional prefix to add before each pattern of the route collection
      * @param array  $defaults     An array of default values
      * @param array  $requirements An array of requirements
-     * @param array  $options      An array of options
      *
      * @api
      */
-    public function addPrefix($prefix, $defaults = array(), $requirements = array(), $options = array())
+    public function addPrefix($prefix, array $defaults = array(), array $requirements = array())
     {
         $prefix = trim(trim($prefix), '/');
 
-        if ('' === $prefix && empty($defaults) && empty($requirements) && empty($options)) {
+        if ('' === $prefix) {
             return;
         }
 
         // a prefix must start with a single slash and must not end with a slash
-        if ('' !== $prefix) {
-            $this->prefix = '/' . $prefix . $this->prefix;
-        }
+        $this->prefix = '/' . $prefix . $this->prefix;
+
+        // this is to keep BC
+        $options = func_num_args() > 3 ? func_get_arg(3) : array();
 
         foreach ($this->routes as $route) {
-            if ('' !== $prefix) {
-                $route->setPattern('/' . $prefix . $route->getPattern());
-            }
+            $route->setPattern('/' . $prefix . $route->getPattern());
             $route->addDefaults($defaults);
             $route->addRequirements($requirements);
             $route->addOptions($options);
@@ -240,6 +246,8 @@ public function addPrefix($prefix, $defaults = array(), $requirements = array(),
      * Returns the prefix that may contain placeholders.
      *
      * @return string The prefix
+     *
+     * @deprecated since version 2.2, will be removed in 2.3
      */
     public function getPrefix()
     {
@@ -249,12 +257,64 @@ public function getPrefix()
     /**
      * Sets the hostname pattern on all routes.
      *
-     * @param string $pattern The pattern
+     * @param string $pattern      The pattern
+     * @param array  $defaults     An array of default values
+     * @param array  $requirements An array of requirements
      */
-    public function setHostnamePattern($pattern)
+    public function setHostnamePattern($pattern, array $defaults = array(), array $requirements = array())
     {
         foreach ($this->routes as $route) {
             $route->setHostnamePattern($pattern);
+            $route->addDefaults($defaults);
+            $route->addRequirements($requirements);
+        }
+    }
+
+    /**
+     * Adds defaults to all routes.
+     *
+     * An existing default value under the same name in a route will be overridden.
+     *
+     * @param array $defaults An array of default values
+     */
+    public function addDefaults(array $defaults)
+    {
+        if ($defaults) {
+            foreach ($this->routes as $route) {
+                $route->addDefaults($defaults);
+            }
+        }
+    }
+
+    /**
+     * Adds requirements to all routes.
+     *
+     * An existing requirement under the same name in a route will be overridden.
+     *
+     * @param array $requirements An array of requirements
+     */
+    public function addRequirements(array $requirements)
+    {
+        if ($requirements) {
+            foreach ($this->routes as $route) {
+                $route->addRequirements($requirements);
+            }
+        }
+    }
+
+    /**
+     * Adds options to all routes.
+     *
+     * An existing option value under the same name in a route will be overridden.
+     *
+     * @param array $options An array of options
+     */
+    public function addOptions(array $options)
+    {
+        if ($options) {
+            foreach ($this->routes as $route) {
+                $route->addOptions($options);
+            }
         }
     }
 

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

@@ -338,6 +338,22 @@ public function testDecodeOnce()
         $this->assertEquals(array('foo' => 'bar%23', '_route' => 'foo'), $matcher->match('/foo/bar%2523'));
     }
 
+    public function testCannotRelyOnPrefix()
+    {
+        $coll = new RouteCollection();
+
+        $subColl = new RouteCollection();
+        $subColl->add('bar', new Route('/bar'));
+        $subColl->addPrefix('/prefix');
+        // overwrite the pattern, so the prefix is not valid anymore for this route in the collection
+        $subColl->get('bar')->setPattern('/new');
+
+        $coll->addCollection($subColl);
+
+        $matcher = new UrlMatcher($coll, new RequestContext());
+        $this->assertEquals(array('_route' => 'bar'), $matcher->match('/new'));
+    }
+
     public function testWithHostname()
     {
         $coll = new RouteCollection();