Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with
or
.
Download ZIP
Browse files

merged branch Tobion/collection-api (PR #6022)

This PR was squashed before being merged into the master branch (closes #6022).

Commits
-------

8c7a169 [Routing] clean up of RouteCollection API

Discussion
----------

[Routing] clean up of RouteCollection API

BC break: only the internal behavior of addPrefix()
Deprecations:
- some params of addCollection and addPrefix that still work but should not be used anymore
- getPrefix (you cannot rely on it how my added test shows that failed previously but was fixed with https://github.com/symfony/symfony/pull/6120/files#L5L109
and it's also useless since we dont have a tree anymore)

Reasoning see commits and changelog.

---------------------------------------------------------------------------

by Tobion at 2012-12-06T19:15:53Z

@fabpot this is finished and rebased. I switched from `addConfigs` to addDefaults(), addRequirements(), and addOptions() as you suggested. I also deprecated getPrefix(). Reasoning see above and in the changelog.
  • Loading branch information...
commit 51bcb6e7ee0e7f6aa128750e1e786308fc773791 2 parents 7464833 + 8c7a169
@fabpot fabpot authored
View
23 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
View
26 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
View
15 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));
View
17 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);
}
View
108 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 @@ 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
@@ -185,12 +181,24 @@ public function addCollection(RouteCollection $collection, $prefix = '', $defaul
// 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);
+ }
}
}
View
16 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();
Please sign in to comment.
Something went wrong with that request. Please try again.