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.
This commit is contained in:
commit
51bcb6e7ee
@ -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
|
||||
|
||||
|
@ -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
|
||||
|
@ -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));
|
||||
|
@ -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);
|
||||
}
|
||||
|
@ -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);
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
|
@ -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();
|
||||
|
Reference in New Issue
Block a user