From 9307f5b33bd424d3fd9bd8d88597b249c28f42de Mon Sep 17 00:00:00 2001 From: Tobias Schultze Date: Sun, 1 Apr 2012 12:32:28 +0200 Subject: [PATCH] [Routing] Implement bug fixes and enhancements --- CHANGELOG-2.1.md | 8 +- .../Matcher/Dumper/PhpMatcherDumper.php | 221 +++++++++--------- src/Symfony/Component/Routing/Route.php | 22 +- .../Component/Routing/RouteCollection.php | 155 ++++++++---- .../Tests/Fixtures/dumper/url_matcher1.php | 67 +++++- .../Tests/Fixtures/dumper/url_matcher2.php | 70 +++++- .../Tests/Fixtures/dumper/url_matcher3.php | 44 ++++ .../Matcher/Dumper/PhpMatcherDumperTest.php | 103 +++++--- .../Routing/Tests/Matcher/UrlMatcherTest.php | 2 +- .../Routing/Tests/RouteCollectionTest.php | 98 +++++++- .../Component/Routing/Tests/RouteTest.php | 24 +- 11 files changed, 590 insertions(+), 224 deletions(-) create mode 100644 src/Symfony/Component/Routing/Tests/Fixtures/dumper/url_matcher3.php diff --git a/CHANGELOG-2.1.md b/CHANGELOG-2.1.md index a6278bedf5..9711d587d7 100644 --- a/CHANGELOG-2.1.md +++ b/CHANGELOG-2.1.md @@ -363,9 +363,11 @@ To get the diff between two versions, go to https://github.com/symfony/symfony/c * added a TraceableUrlMatcher * added the possibility to define options, default values and requirements for placeholders in prefix, including imported routes * added RouterInterface::getRouteCollection - * [BC BREAK] the UrlMatcher urldecodes the route parameters only once, they were - decoded twice before. Note that the `urldecode()` calls have been change for a - single `rawurldecode()` in order to support `+` for input paths. + * [BC BREAK] the UrlMatcher urldecodes the route parameters only once, they were decoded twice before. + Note that the `urldecode()` calls have been changed for a single `rawurldecode()` in order to support `+` for input paths. + * added RouteCollection::getRoot method to retrieve the root of a RouteCollection tree + * [BC BREAK] made RouteCollection::setParent private which could not have been used anyway without creating inconsistencies + * [BC BREAK] RouteCollection::remove also removes a route from parent collections (not only from its children) ### Security diff --git a/src/Symfony/Component/Routing/Matcher/Dumper/PhpMatcherDumper.php b/src/Symfony/Component/Routing/Matcher/Dumper/PhpMatcherDumper.php index dd7fbe3544..5fb3e3fd82 100644 --- a/src/Symfony/Component/Routing/Matcher/Dumper/PhpMatcherDumper.php +++ b/src/Symfony/Component/Routing/Matcher/Dumper/PhpMatcherDumper.php @@ -18,6 +18,7 @@ use Symfony\Component\Routing\RouteCollection; * PhpMatcherDumper creates a PHP class able to match URLs for a given set of routes. * * @author Fabien Potencier + * @author Tobias Schultze */ class PhpMatcherDumper extends MatcherDumper { @@ -42,23 +43,49 @@ class PhpMatcherDumper extends MatcherDumper // trailing slash support is only enabled if we know how to redirect the user $interfaces = class_implements($options['base_class']); - $supportsRedirections = isset($interfaces['Symfony\Component\Routing\Matcher\RedirectableUrlMatcherInterface']); - - return - $this->startClass($options['class'], $options['base_class']). - $this->addConstructor(). - $this->addMatcher($supportsRedirections). - $this->endClass() - ; - } - - private function addMatcher($supportsRedirections) - { - // we need to deep clone the routes as we will modify the structure to optimize the dump - $code = rtrim($this->compileRoutes(clone $this->getRoutes(), $supportsRedirections), "\n"); + $supportsRedirections = isset($interfaces['Symfony\\Component\\Routing\\Matcher\\RedirectableUrlMatcherInterface']); return <<context = \$context; + } + +{$this->generateMatchMethod($supportsRedirections)} +} + +EOF; + } + + /** + * Generates the code for the match method implementing UrlMatcherInterface. + * + * @param Boolean $supportsRedirections Whether redirections are supported by the base class + * + * @return string Match method as PHP code + */ + private function generateMatchMethod($supportsRedirections) + { + $code = rtrim($this->compileRoutes($this->getRoutes(), $supportsRedirections), "\n"); + + return <<getIterator(); - $keys = array_keys($routeIterator->getArrayCopy()); - $keysCount = count($keys); + $prefix = $routes->getPrefix(); + $countDirectChildRoutes = $this->countDirectChildRoutes($routes); + $countAllChildRoutes = count($routes->all()); + // Can the matching be optimized by wrapping it with the prefix condition + // - no need to optimize if current prefix is the same as the parent prefix + // - if $countDirectChildRoutes === 0, the sub-collections can do their own optimizations (in case there are any) + // - it's not worth wrapping a single child route + // - prefixes with variables cannot be optimized because routes within the collection might have different requirements for the same variable + $optimizable = '' !== $prefix && $prefix !== $parentPrefix && $countDirectChildRoutes > 0 && $countAllChildRoutes > 1 && false === strpos($prefix, '{'); + if ($optimizable) { + $code .= sprintf(" if (0 === strpos(\$pathinfo, %s)) {\n", var_export($prefix, true)); + } - $i = 0; - foreach ($routeIterator as $name => $route) { - $i++; - - if ($route instanceof RouteCollection) { - $prefix = $route->getPrefix(); - $optimizable = $prefix && count($route->all()) > 1 && false === strpos($route->getPrefix(), '{'); - $optimized = false; - - if ($optimizable) { - for ($j = $i; $j < $keysCount; $j++) { - if (null === $keys[$j]) { - continue; - } - - $testRoute = $routeIterator->offsetGet($keys[$j]); - $isCollection = ($testRoute instanceof RouteCollection); - - $testPrefix = $isCollection ? $testRoute->getPrefix() : $testRoute->getPattern(); - - if (0 === strpos($testPrefix, $prefix)) { - $routeIterator->offsetUnset($keys[$j]); - - if ($isCollection) { - $route->addCollection($testRoute); - } else { - $route->add($keys[$j], $testRoute); - } - - $i++; - $keys[$j] = null; - } - } - - if ($prefix !== $parentPrefix) { - $code .= sprintf(" if (0 === strpos(\$pathinfo, %s)) {\n", var_export($prefix, true)); - $optimized = true; - } - } - - if ($optimized) { - foreach (explode("\n", $this->compileRoutes($route, $supportsRedirections, $prefix)) as $line) { - if ('' !== $line) { - $code .= ' '; // apply extra indention - } - $code .= $line."\n"; - } - $code = substr($code, 0, -2); // remove redundant last two line breaks - $code .= " }\n\n"; - } else { - $code .= $this->compileRoutes($route, $supportsRedirections, $prefix); - } - } else { - $code .= $this->compileRoute($route, $name, $supportsRedirections, $parentPrefix)."\n"; + foreach ($routes as $name => $route) { + if ($route instanceof Route) { + // a single route in a sub-collection is not wrapped so it should do its own optimization in ->compileRoute with $parentPrefix = null + $code .= $this->compileRoute($route, $name, $supportsRedirections, 1 === $countAllChildRoutes ? null : $prefix)."\n"; + } elseif ($countAllChildRoutes - $countDirectChildRoutes > 0) { // we can stop iterating recursively if we already know there are no more routes + $code .= $this->compileRoutes($route, $supportsRedirections, $prefix); } } + if ($optimizable) { + $code .= " }\n\n"; + // apply extra indention at each line (except empty ones) + $code = preg_replace('/^.{2,}$/m', ' $0', $code); + } + return $code; } + + /** + * Compiles a single Route to PHP code used to match it against the path info. + * + * @param Route $routes A Route instance + * @param string $name The name of the Route + * @param Boolean $supportsRedirections Whether redirections are supported by the base class + * @param string|null $parentPrefix The prefix of the parent collection used to optimize the code + * + * @return string PHP code + */ private function compileRoute(Route $route, $name, $supportsRedirections, $parentPrefix = null) { $code = ''; @@ -167,7 +199,7 @@ EOF; $conditions[] = sprintf("\$pathinfo === %s", var_export(str_replace('\\', '', $m['url']), true)); } } else { - if ($compiledRoute->getStaticPrefix() && $compiledRoute->getStaticPrefix() != $parentPrefix) { + if ($compiledRoute->getStaticPrefix() && $compiledRoute->getStaticPrefix() !== $parentPrefix) { $conditions[] = sprintf("0 === strpos(\$pathinfo, %s)", var_export($compiledRoute->getStaticPrefix(), true)); } @@ -254,47 +286,4 @@ EOF; return $code; } - - private function startClass($class, $baseClass) - { - return <<context = \$context; - } - -EOF; - } - - private function endClass() - { - return <<pattern = trim($pattern); // a route must start with a slash - if (empty($this->pattern) || '/' !== $this->pattern[0]) { + if ('' === $this->pattern || '/' !== $this->pattern[0]) { $this->pattern = '/'.$this->pattern; } + $this->compiled = null; + return $this; } @@ -128,6 +130,7 @@ class Route foreach ($options as $name => $option) { $this->options[(string) $name] = $option; } + $this->compiled = null; return $this; } @@ -147,6 +150,7 @@ class Route public function setOption($name, $value) { $this->options[$name] = $value; + $this->compiled = null; return $this; } @@ -203,6 +207,7 @@ class Route foreach ($defaults as $name => $default) { $this->defaults[(string) $name] = $default; } + $this->compiled = null; return $this; } @@ -244,6 +249,7 @@ class Route public function setDefault($name, $default) { $this->defaults[(string) $name] = $default; + $this->compiled = null; return $this; } @@ -288,6 +294,7 @@ class Route foreach ($requirements as $key => $regex) { $this->requirements[$key] = $this->sanitizeRequirement($key, $regex); } + $this->compiled = null; return $this; } @@ -317,6 +324,7 @@ class Route public function setRequirement($key, $regex) { $this->requirements[$key] = $this->sanitizeRequirement($key, $regex); + $this->compiled = null; return $this; } @@ -343,15 +351,19 @@ class Route private function sanitizeRequirement($key, $regex) { - if (is_array($regex)) { - throw new \InvalidArgumentException(sprintf('Routing requirements must be a string, array given for "%s"', $key)); + if (!is_string($regex)) { + throw new \InvalidArgumentException(sprintf('Routing requirement for "%s" must be a string', $key)); } - if ('^' == $regex[0]) { + if ('' === $regex) { + throw new \InvalidArgumentException(sprintf('Routing requirement for "%s" cannot be empty', $key)); + } + + if ('^' === $regex[0]) { $regex = substr($regex, 1); } - if ('$' == substr($regex, -1)) { + if ('$' === substr($regex, -1)) { $regex = substr($regex, 0, -1); } diff --git a/src/Symfony/Component/Routing/RouteCollection.php b/src/Symfony/Component/Routing/RouteCollection.php index f049d5985b..85acf331ba 100644 --- a/src/Symfony/Component/Routing/RouteCollection.php +++ b/src/Symfony/Component/Routing/RouteCollection.php @@ -14,12 +14,13 @@ namespace Symfony\Component\Routing; use Symfony\Component\Config\Resource\ResourceInterface; /** - * A RouteCollection represents a set of Route instances. + * A RouteCollection represents a set of Route instances as a tree structure. * * When adding a route, it overrides existing routes with the - * same name defined in theinstance or its children and parents. + * same name defined in the instance or its children and parents. * * @author Fabien Potencier + * @author Tobias Schultze * * @api */ @@ -55,7 +56,7 @@ class RouteCollection implements \IteratorAggregate /** * Gets the parent RouteCollection. * - * @return RouteCollection The parent RouteCollection + * @return RouteCollection|null The parent RouteCollection or null when it's the root */ public function getParent() { @@ -63,13 +64,18 @@ class RouteCollection implements \IteratorAggregate } /** - * Sets the parent RouteCollection. + * Gets the root RouteCollection of the tree. * - * @param RouteCollection $parent The parent RouteCollection + * @return RouteCollection The root RouteCollection */ - public function setParent(RouteCollection $parent) + public function getRoot() { - $this->parent = $parent; + $parent = $this; + while ($parent->getParent()) { + $parent = $parent->getParent(); + } + + return $parent; } /** @@ -98,20 +104,13 @@ class RouteCollection implements \IteratorAggregate throw new \InvalidArgumentException(sprintf('The provided route name "%s" contains non valid characters. A route name must only contain digits (0-9), letters (a-z and A-Z), underscores (_) and dots (.).', $name)); } - $parent = $this; - while ($parent->getParent()) { - $parent = $parent->getParent(); - } - - if ($parent) { - $parent->remove($name); - } + $this->remove($name); $this->routes[$name] = $route; } /** - * Returns the array of routes. + * Returns all routes in this collection and its children. * * @return array An array of routes */ @@ -130,45 +129,39 @@ class RouteCollection implements \IteratorAggregate } /** - * Gets a route by name. + * Gets a route by name defined in this collection or its children. * - * @param string $name The route name + * @param string $name The route name * - * @return Route $route A Route instance + * @return Route|null A Route instance or null when not found */ public function get($name) { - // get the latest defined route - foreach (array_reverse($this->routes) as $routes) { - if (!$routes instanceof RouteCollection) { - continue; - } + if (isset($this->routes[$name])) { + return $this->routes[$name] instanceof RouteCollection ? null : $this->routes[$name]; + } - if (null !== $route = $routes->get($name)) { + foreach ($this->routes as $routes) { + if ($routes instanceof RouteCollection && null !== $route = $routes->get($name)) { return $route; } } - if (isset($this->routes[$name])) { - return $this->routes[$name]; - } + return null; } /** - * Removes a route by name. + * Removes a route or an array of routes by name from all connected + * collections (this instance and all parents and children). * - * @param string $name The route name + * @param string|array $name The route name or an array of route names */ public function remove($name) { - if (isset($this->routes[$name])) { - unset($this->routes[$name]); - } + $root = $this->getRoot(); - foreach ($this->routes as $routes) { - if ($routes instanceof RouteCollection) { - $routes->remove($name); - } + foreach ((array) $name as $n) { + $root->removeRecursively($n); } } @@ -181,18 +174,25 @@ class RouteCollection implements \IteratorAggregate * @param array $requirements An array of requirements * @param array $options An array of options * + * @throws \InvalidArgumentException When the RouteCollection already exists in the tree + * * @api */ public function addCollection(RouteCollection $collection, $prefix = '', $defaults = array(), $requirements = array(), $options = array()) { - $collection->setParent($this); - $collection->addPrefix($prefix, $defaults, $requirements, $options); - - // remove all routes with the same name in all existing collections - foreach (array_keys($collection->all()) as $name) { - $this->remove($name); + // prevent infinite loops by recursive referencing + $root = $this->getRoot(); + if ($root === $collection || $root->hasCollection($collection)) { + throw new \InvalidArgumentException('The RouteCollection already exists in the tree.'); } + // remove all routes with the same names in all existing collections + $this->remove(array_keys($collection->all())); + + $collection->setParent($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); $this->routes[] = $collection; } @@ -211,14 +211,18 @@ class RouteCollection implements \IteratorAggregate // a prefix must not end with a slash $prefix = rtrim($prefix, '/'); + if ('' === $prefix && empty($defaults) && empty($requirements) && empty($options)) { + return; + } + // a prefix must start with a slash - if ($prefix && '/' !== $prefix[0]) { + if ('' !== $prefix && '/' !== $prefix[0]) { $prefix = '/'.$prefix; } $this->prefix = $prefix.$this->prefix; - foreach ($this->routes as $name => $route) { + foreach ($this->routes as $route) { if ($route instanceof RouteCollection) { $route->addPrefix($prefix, $defaults, $requirements, $options); } else { @@ -230,6 +234,11 @@ class RouteCollection implements \IteratorAggregate } } + /** + * Returns the prefix that may contain placeholders. + * + * @return string The prefix + */ public function getPrefix() { return $this->prefix; @@ -261,4 +270,60 @@ class RouteCollection implements \IteratorAggregate { $this->resources[] = $resource; } + + /** + * Sets the parent RouteCollection. It's only used internally from one RouteCollection + * to another. It makes no sense to be available as part of the public API. + * + * @param RouteCollection $parent The parent RouteCollection + */ + private function setParent(RouteCollection $parent) + { + $this->parent = $parent; + } + + /** + * Removes a route by name from this collection and its children recursively. + * + * @param string $name The route name + * + * @return Boolean true when found + */ + private function removeRecursively($name) + { + // It is ensured by the adders (->add and ->addCollection) that there can + // only be one route per name in all connected collections. So we can stop + // interating recursively on the first hit. + if (isset($this->routes[$name])) { + unset($this->routes[$name]); + + return true; + } + + foreach ($this->routes as $routes) { + if ($routes instanceof RouteCollection && $routes->removeRecursively($name)) { + return true; + } + } + + return false; + } + + /** + * Checks whether the given RouteCollection is already set in any child of the current instance. + * + * @param RouteCollection $collection A RouteCollection instance + * + * @return Boolean + */ + private function hasCollection(RouteCollection $collection) + { + foreach ($this->routes as $routes) { + if ($routes === $collection || $routes instanceof RouteCollection && $routes->hasCollection($collection)) { + return true; + } + } + + return false; + } } diff --git a/src/Symfony/Component/Routing/Tests/Fixtures/dumper/url_matcher1.php b/src/Symfony/Component/Routing/Tests/Fixtures/dumper/url_matcher1.php index 98d8b0ef90..4753410ffa 100644 --- a/src/Symfony/Component/Routing/Tests/Fixtures/dumper/url_matcher1.php +++ b/src/Symfony/Component/Routing/Tests/Fixtures/dumper/url_matcher1.php @@ -125,6 +125,15 @@ class ProjectUrlMatcher extends Symfony\Component\Routing\Matcher\UrlMatcher return $matches; } + } + + // overriden + if (preg_match('#^/a/(?P.*)$#s', $pathinfo, $matches)) { + $matches['_route'] = 'overriden'; + return $matches; + } + + if (0 === strpos($pathinfo, '/a/b\'b')) { // foo2 if (preg_match('#^/a/b\'b/(?P[^/]+?)$#s', $pathinfo, $matches)) { $matches['_route'] = 'foo2'; @@ -136,23 +145,27 @@ class ProjectUrlMatcher extends Symfony\Component\Routing\Matcher\UrlMatcher $matches['_route'] = 'bar2'; return $matches; } + } - // overriden - if ($pathinfo === '/a/overriden2') { - return array('_route' => 'overriden'); + } + + if (0 === strpos($pathinfo, '/multi')) { + // helloWorld + if (0 === strpos($pathinfo, '/multi/hello') && preg_match('#^/multi/hello(?:/(?P[^/]+?))?$#s', $pathinfo, $matches)) { + return array_merge($this->mergeDefaults($matches, array ( 'who' => 'World!',)), array('_route' => 'helloWorld')); } - // ababa - if ($pathinfo === '/ababa') { - return array('_route' => 'ababa'); + // overriden2 + if ($pathinfo === '/multi/new') { + return array('_route' => 'overriden2'); } - // foo4 - if (preg_match('#^/aba/(?P[^/]+?)$#s', $pathinfo, $matches)) { - $matches['_route'] = 'foo4'; - return $matches; + // hey + if ($pathinfo === '/multi/hey/') { + return array('_route' => 'hey'); } + } // foo3 @@ -167,6 +180,40 @@ class ProjectUrlMatcher extends Symfony\Component\Routing\Matcher\UrlMatcher return $matches; } + // ababa + if ($pathinfo === '/ababa') { + return array('_route' => 'ababa'); + } + + // foo4 + if (0 === strpos($pathinfo, '/aba') && preg_match('#^/aba/(?P[^/]+?)$#s', $pathinfo, $matches)) { + $matches['_route'] = 'foo4'; + return $matches; + } + + if (0 === strpos($pathinfo, '/a')) { + // a + if ($pathinfo === '/a/a...') { + return array('_route' => 'a'); + } + + if (0 === strpos($pathinfo, '/a/b')) { + // b + if (preg_match('#^/a/b/(?P[^/]+?)$#s', $pathinfo, $matches)) { + $matches['_route'] = 'b'; + return $matches; + } + + // c + if (0 === strpos($pathinfo, '/a/b/c') && preg_match('#^/a/b/c/(?P[^/]+?)$#s', $pathinfo, $matches)) { + $matches['_route'] = 'c'; + return $matches; + } + + } + + } + throw 0 < count($allow) ? new MethodNotAllowedException(array_unique($allow)) : new ResourceNotFoundException(); } } diff --git a/src/Symfony/Component/Routing/Tests/Fixtures/dumper/url_matcher2.php b/src/Symfony/Component/Routing/Tests/Fixtures/dumper/url_matcher2.php index 390acfbd24..dbfd28ab0a 100644 --- a/src/Symfony/Component/Routing/Tests/Fixtures/dumper/url_matcher2.php +++ b/src/Symfony/Component/Routing/Tests/Fixtures/dumper/url_matcher2.php @@ -131,6 +131,15 @@ class ProjectUrlMatcher extends Symfony\Component\Routing\Tests\Fixtures\Redirec return $matches; } + } + + // overriden + if (preg_match('#^/a/(?P.*)$#s', $pathinfo, $matches)) { + $matches['_route'] = 'overriden'; + return $matches; + } + + if (0 === strpos($pathinfo, '/a/b\'b')) { // foo2 if (preg_match('#^/a/b\'b/(?P[^/]+?)$#s', $pathinfo, $matches)) { $matches['_route'] = 'foo2'; @@ -142,23 +151,30 @@ class ProjectUrlMatcher extends Symfony\Component\Routing\Tests\Fixtures\Redirec $matches['_route'] = 'bar2'; return $matches; } + } - // overriden - if ($pathinfo === '/a/overriden2') { - return array('_route' => 'overriden'); + } + + if (0 === strpos($pathinfo, '/multi')) { + // helloWorld + if (0 === strpos($pathinfo, '/multi/hello') && preg_match('#^/multi/hello(?:/(?P[^/]+?))?$#s', $pathinfo, $matches)) { + return array_merge($this->mergeDefaults($matches, array ( 'who' => 'World!',)), array('_route' => 'helloWorld')); } - // ababa - if ($pathinfo === '/ababa') { - return array('_route' => 'ababa'); + // overriden2 + if ($pathinfo === '/multi/new') { + return array('_route' => 'overriden2'); } - // foo4 - if (preg_match('#^/aba/(?P[^/]+?)$#s', $pathinfo, $matches)) { - $matches['_route'] = 'foo4'; - return $matches; + // hey + if (rtrim($pathinfo, '/') === '/multi/hey') { + if (substr($pathinfo, -1) !== '/') { + return $this->redirect($pathinfo.'/', 'hey'); + } + return array('_route' => 'hey'); } + } // foo3 @@ -173,6 +189,40 @@ class ProjectUrlMatcher extends Symfony\Component\Routing\Tests\Fixtures\Redirec return $matches; } + // ababa + if ($pathinfo === '/ababa') { + return array('_route' => 'ababa'); + } + + // foo4 + if (0 === strpos($pathinfo, '/aba') && preg_match('#^/aba/(?P[^/]+?)$#s', $pathinfo, $matches)) { + $matches['_route'] = 'foo4'; + return $matches; + } + + if (0 === strpos($pathinfo, '/a')) { + // a + if ($pathinfo === '/a/a...') { + return array('_route' => 'a'); + } + + if (0 === strpos($pathinfo, '/a/b')) { + // b + if (preg_match('#^/a/b/(?P[^/]+?)$#s', $pathinfo, $matches)) { + $matches['_route'] = 'b'; + return $matches; + } + + // c + if (0 === strpos($pathinfo, '/a/b/c') && preg_match('#^/a/b/c/(?P[^/]+?)$#s', $pathinfo, $matches)) { + $matches['_route'] = 'c'; + return $matches; + } + + } + + } + // secure if ($pathinfo === '/secure') { if ($this->context->getScheme() !== 'https') { diff --git a/src/Symfony/Component/Routing/Tests/Fixtures/dumper/url_matcher3.php b/src/Symfony/Component/Routing/Tests/Fixtures/dumper/url_matcher3.php new file mode 100644 index 0000000000..2dc0be9886 --- /dev/null +++ b/src/Symfony/Component/Routing/Tests/Fixtures/dumper/url_matcher3.php @@ -0,0 +1,44 @@ +context = $context; + } + + public function match($pathinfo) + { + $allow = array(); + $pathinfo = rawurldecode($pathinfo); + + if (0 === strpos($pathinfo, '/rootprefix')) { + // static + if ($pathinfo === '/rootprefix/test') { + return array('_route' => 'static'); + } + + // dynamic + if (preg_match('#^/rootprefix/(?P[^/]+?)$#s', $pathinfo, $matches)) { + $matches['_route'] = 'dynamic'; + return $matches; + } + + } + + throw 0 < count($allow) ? new MethodNotAllowedException(array_unique($allow)) : new ResourceNotFoundException(); + } +} diff --git a/src/Symfony/Component/Routing/Tests/Matcher/Dumper/PhpMatcherDumperTest.php b/src/Symfony/Component/Routing/Tests/Matcher/Dumper/PhpMatcherDumperTest.php index 293348cc15..0aea0396b6 100644 --- a/src/Symfony/Component/Routing/Tests/Matcher/Dumper/PhpMatcherDumperTest.php +++ b/src/Symfony/Component/Routing/Tests/Matcher/Dumper/PhpMatcherDumperTest.php @@ -18,33 +18,6 @@ use Symfony\Component\Routing\RequestContext; class PhpMatcherDumperTest extends \PHPUnit_Framework_TestCase { - public function testDump() - { - $collection = $this->getRouteCollection(); - - $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') - )); - - $dumper = new PhpMatcherDumper($collection, new RequestContext()); - - $this->assertStringEqualsFile(__DIR__.'/../../Fixtures/dumper/url_matcher2.php', $dumper->dump(array('base_class' => 'Symfony\Component\Routing\Tests\Fixtures\RedirectableUrlMatcher')), '->dump() dumps basic routes to the correct PHP file.'); - } - /** * @expectedException \LogicException */ @@ -60,8 +33,22 @@ class PhpMatcherDumperTest extends \PHPUnit_Framework_TestCase $dumper->dump(); } - protected function getRouteCollection() + /** + * @dataProvider getRouteCollections + */ + public function testDump(RouteCollection $collection, $fixture, $options = array()) { + $basePath = __DIR__.'/../../Fixtures/dumper/'; + + $dumper = new PhpMatcherDumper($collection, new RequestContext()); + + $this->assertStringEqualsFile($basePath.$fixture, $dumper->dump($options), '->dump() correctly dumps routes as optimized PHP code.'); + } + + public function getRouteCollections() + { + /* test case 1 */ + $collection = new RouteCollection(); $collection->add('overriden', new Route('/overriden')); @@ -135,13 +122,25 @@ class PhpMatcherDumperTest extends \PHPUnit_Framework_TestCase $collection1->add('bar1', new Route('/{bar}')); $collection2 = new RouteCollection(); $collection2->addCollection($collection1, '/b\'b'); - $collection2->add('overriden', new Route('/overriden2')); + $collection2->add('overriden', new Route('/{var}', array(), array('var' => '.*'))); $collection1 = new RouteCollection(); $collection1->add('foo2', new Route('/{foo1}')); $collection1->add('bar2', new Route('/{bar1}')); $collection2->addCollection($collection1, '/b\'b'); $collection->addCollection($collection2, '/a'); + // overriden through addCollection() and multiple sub-collections with no own prefix + $collection1 = new RouteCollection(); + $collection1->add('overriden2', new Route('/old')); + $collection1->add('helloWorld', new Route('/hello/{who}', array('who' => 'World!'))); + $collection2 = new RouteCollection(); + $collection3 = new RouteCollection(); + $collection3->add('overriden2', new Route('/new')); + $collection3->add('hey', new Route('/hey/')); + $collection1->addCollection($collection2); + $collection2->addCollection($collection3); + $collection->addCollection($collection1, '/multi'); + // "dynamic" prefix $collection1 = new RouteCollection(); $collection1->add('foo3', new Route('/{foo}')); @@ -150,13 +149,55 @@ class PhpMatcherDumperTest extends \PHPUnit_Framework_TestCase $collection2->addCollection($collection1, '/b'); $collection->addCollection($collection2, '/{_locale}'); + // route between collections $collection->add('ababa', new Route('/ababa')); - // some more prefixes + // collection with static prefix but only one route $collection1 = new RouteCollection(); $collection1->add('foo4', new Route('/{foo}')); $collection->addCollection($collection1, '/aba'); - return $collection; + // multiple sub-collections with a single route and a prefix each + $collection1 = new RouteCollection(); + $collection1->add('a', new Route('/a...')); + $collection2 = new RouteCollection(); + $collection2->add('b', new Route('/{var}')); + $collection3 = new RouteCollection(); + $collection3->add('c', new Route('/{var}')); + + $collection1->addCollection($collection2, '/b'); + $collection2->addCollection($collection3, '/c'); + $collection->addCollection($collection1, '/a'); + + /* test case 2 */ + + $redirectCollection = clone $collection; + + // force HTTPS redirection + $redirectCollection->add('secure', new Route( + '/secure', + array(), + array('_scheme' => 'https') + )); + + // force HTTP redirection + $redirectCollection->add('nonsecure', new Route( + '/nonsecure', + array(), + array('_scheme' => 'http') + )); + + /* test case 3 */ + + $rootprefixCollection = new RouteCollection(); + $rootprefixCollection->add('static', new Route('/test')); + $rootprefixCollection->add('dynamic', new Route('/{var}')); + $rootprefixCollection->addPrefix('rootprefix'); + + return array( + array($collection, 'url_matcher1.php', array()), + array($redirectCollection, 'url_matcher2.php', array('base_class' => 'Symfony\Component\Routing\Tests\Fixtures\RedirectableUrlMatcher')), + array($rootprefixCollection, 'url_matcher3.php', array()) + ); } } diff --git a/src/Symfony/Component/Routing/Tests/Matcher/UrlMatcherTest.php b/src/Symfony/Component/Routing/Tests/Matcher/UrlMatcherTest.php index 1d782b889d..e38c75a030 100644 --- a/src/Symfony/Component/Routing/Tests/Matcher/UrlMatcherTest.php +++ b/src/Symfony/Component/Routing/Tests/Matcher/UrlMatcherTest.php @@ -71,7 +71,7 @@ class UrlMatcherTest extends \PHPUnit_Framework_TestCase public function testMatch() { - // test the patterns are matched are parameters are returned + // test the patterns are matched and parameters are returned $collection = new RouteCollection(); $collection->add('foo', new Route('/foo/{bar}')); $matcher = new UrlMatcher($collection, new RequestContext(), array()); diff --git a/src/Symfony/Component/Routing/Tests/RouteCollectionTest.php b/src/Symfony/Component/Routing/Tests/RouteCollectionTest.php index 3e73065d0b..aee0f9d681 100644 --- a/src/Symfony/Component/Routing/Tests/RouteCollectionTest.php +++ b/src/Symfony/Component/Routing/Tests/RouteCollectionTest.php @@ -28,7 +28,7 @@ class RouteCollectionTest extends \PHPUnit_Framework_TestCase } /** - * @expectedException InvalidArgumentException + * @expectedException \InvalidArgumentException */ public function testAddInvalidRoute() { @@ -144,6 +144,8 @@ class RouteCollectionTest extends \PHPUnit_Framework_TestCase array('foo' => 'bar', 'compiler_class' => 'Symfony\\Component\\Routing\\RouteCompiler'), $collection->get('bar')->getOptions(), '->addPrefix() adds an option to all routes' ); + $collection->addPrefix('0'); + $this->assertEquals('/0/{admin}', $collection->getPrefix(), '->addPrefix() ensures a prefix must start with a slash and must not end with a slash'); } public function testAddPrefixOverridesDefaultsAndRequirements() @@ -180,4 +182,98 @@ class RouteCollectionTest extends \PHPUnit_Framework_TestCase $collection->addResource($foo = new FileResource(__DIR__.'/Fixtures/foo.xml')); $this->assertEquals(array($foo), $collection->getResources(), '->addResources() adds a resource'); } + + public function testUniqueRouteWithGivenName() + { + $collection1 = new RouteCollection(); + $collection1->add('foo', new Route('/old')); + $collection2 = new RouteCollection(); + $collection3 = new RouteCollection(); + $collection3->add('foo', $new = new Route('/new')); + + $collection1->addCollection($collection2); + $collection2->addCollection($collection3); + + $collection1->add('stay', new Route('/stay')); + + $iterator = $collection1->getIterator(); + + $this->assertSame($new, $collection1->get('foo'), '->get() returns new route that overrode previous one'); + // size of 2 because collection1 contains collection2 and /stay but not /old anymore + $this->assertCount(2, $iterator, '->addCollection() removes previous routes when adding new routes with the same name'); + $this->assertInstanceOf('Symfony\Component\Routing\RouteCollection', $iterator->current(), '->getIterator returns both Routes and RouteCollections'); + $iterator->next(); + $this->assertInstanceOf('Symfony\Component\Routing\Route', $iterator->current(), '->getIterator returns both Routes and RouteCollections'); + } + + public function testGet() + { + $collection1 = new RouteCollection(); + $collection1->add('a', $a = new Route('/a')); + $collection2 = new RouteCollection(); + $collection2->add('b', $b = new Route('/b')); + $collection1->addCollection($collection2); + + $this->assertSame($b, $collection1->get('b'), '->get() returns correct route in child collection'); + $this->assertNull($collection2->get('a'), '->get() does not return the route defined in parent collection'); + $this->assertNull($collection1->get('non-existent'), '->get() returns null when route does not exist'); + $this->assertNull($collection1->get(0), '->get() does not disclose internal child RouteCollection'); + } + + /** + * @expectedException \InvalidArgumentException + */ + public function testCannotSelfJoinCollection() + { + $collection = new RouteCollection(); + + $collection->addCollection($collection); + } + + /** + * @expectedException \InvalidArgumentException + */ + public function testCannotAddExistingCollectionToTree() + { + $collection1 = new RouteCollection(); + $collection2 = new RouteCollection(); + $collection3 = new RouteCollection(); + + $collection1->addCollection($collection2); + $collection1->addCollection($collection3); + $collection2->addCollection($collection3); + } + + public function testPatternDoesNotChangeWhenDefinitionOrderChanges() + { + $collection1 = new RouteCollection(); + $collection1->add('a', new Route('/a...')); + $collection2 = new RouteCollection(); + $collection2->add('b', new Route('/b...')); + $collection3 = new RouteCollection(); + $collection3->add('c', new Route('/c...')); + + $rootCollection_A = new RouteCollection(); + $collection2->addCollection($collection3, '/c'); + $collection1->addCollection($collection2, '/b'); + $rootCollection_A->addCollection($collection1, '/a'); + + // above should mean the same as below + + $collection1 = new RouteCollection(); + $collection1->add('a', new Route('/a...')); + $collection2 = new RouteCollection(); + $collection2->add('b', new Route('/b...')); + $collection3 = new RouteCollection(); + $collection3->add('c', new Route('/c...')); + + $rootCollection_B = new RouteCollection(); + $collection1->addCollection($collection2, '/b'); + $collection2->addCollection($collection3, '/c'); + $rootCollection_B->addCollection($collection1, '/a'); + + // test it now + + $this->assertEquals($rootCollection_A, $rootCollection_B); + } } diff --git a/src/Symfony/Component/Routing/Tests/RouteTest.php b/src/Symfony/Component/Routing/Tests/RouteTest.php index 1295bc43c0..8e466826da 100644 --- a/src/Symfony/Component/Routing/Tests/RouteTest.php +++ b/src/Symfony/Component/Routing/Tests/RouteTest.php @@ -98,10 +98,30 @@ class RouteTest extends \PHPUnit_Framework_TestCase $this->assertEquals('\d+', $route->getRequirement('foo'), '->setRequirement() removes ^ and $ from the pattern'); } + /** + * @dataProvider getInvalidRequirements + * @expectedException \InvalidArgumentException + */ + public function testSetInvalidRequirement($req) + { + $route = new Route('/{foo}'); + $route->setRequirement('foo', $req); + } + + public function getInvalidRequirements() + { + return array( + array(''), + array(array()) + ); + } + public function testCompile() { $route = new Route('/{foo}'); - $this->assertEquals('Symfony\\Component\\Routing\\CompiledRoute', get_class($compiled = $route->compile()), '->compile() returns a compiled route'); - $this->assertEquals($compiled, $route->compile(), '->compile() only compiled the route once'); + $this->assertInstanceOf('Symfony\Component\Routing\CompiledRoute', $compiled = $route->compile(), '->compile() returns a compiled route'); + $this->assertSame($compiled, $route->compile(), '->compile() only compiled the route once if unchanged'); + $route->setRequirement('foo', '.*'); + $this->assertNotSame($compiled, $route->compile(), '->compile() recompiles if the route was modified'); } }