diff --git a/UPGRADE-2.2.md b/UPGRADE-2.2.md index 2cf26fee14..825e7f2570 100644 --- a/UPGRADE-2.2.md +++ b/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 diff --git a/src/Symfony/Component/Routing/CHANGELOG.md b/src/Symfony/Component/Routing/CHANGELOG.md index e7fec1b0b3..fba7d41a40 100644 --- a/src/Symfony/Component/Routing/CHANGELOG.md +++ b/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 diff --git a/src/Symfony/Component/Routing/Loader/XmlFileLoader.php b/src/Symfony/Component/Routing/Loader/XmlFileLoader.php index 1b3eb0c15f..d5a7bf3f93 100644 --- a/src/Symfony/Component/Routing/Loader/XmlFileLoader.php +++ b/src/Symfony/Component/Routing/Loader/XmlFileLoader.php @@ -78,7 +78,7 @@ class XmlFileLoader extends FileLoader $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 @@ class XmlFileLoader extends FileLoader } $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)); diff --git a/src/Symfony/Component/Routing/Loader/YamlFileLoader.php b/src/Symfony/Component/Routing/Loader/YamlFileLoader.php index 4a530fbb6a..326414e78e 100644 --- a/src/Symfony/Component/Routing/Loader/YamlFileLoader.php +++ b/src/Symfony/Component/Routing/Loader/YamlFileLoader.php @@ -66,14 +66,25 @@ class YamlFileLoader extends FileLoader 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); } diff --git a/src/Symfony/Component/Routing/RouteCollection.php b/src/Symfony/Component/Routing/RouteCollection.php index 2164d32d8a..4df6f7d145 100644 --- a/src/Symfony/Component/Routing/RouteCollection.php +++ b/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,15 +170,10 @@ class RouteCollection implements \IteratorAggregate, \Countable * 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 @@ -185,12 +181,24 @@ class RouteCollection implements \IteratorAggregate, \Countable // 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 @@ class RouteCollection implements \IteratorAggregate, \Countable } /** - * 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 @@ class RouteCollection implements \IteratorAggregate, \Countable * 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 @@ class RouteCollection implements \IteratorAggregate, \Countable /** * 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); + } } } diff --git a/src/Symfony/Component/Routing/Tests/Matcher/UrlMatcherTest.php b/src/Symfony/Component/Routing/Tests/Matcher/UrlMatcherTest.php index 64ef2d339b..18e11ebcfb 100644 --- a/src/Symfony/Component/Routing/Tests/Matcher/UrlMatcherTest.php +++ b/src/Symfony/Component/Routing/Tests/Matcher/UrlMatcherTest.php @@ -338,6 +338,22 @@ class UrlMatcherTest extends \PHPUnit_Framework_TestCase $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();