From 005a9a3c5bd137acdec7f222a0e0dc101e513f41 Mon Sep 17 00:00:00 2001 From: Tobias Schultze Date: Tue, 8 May 2012 10:14:10 +0200 Subject: [PATCH] [Routing] fixed RouteCompiler for adjacent and nested placeholders --- .../Component/Routing/RouteCompiler.php | 77 ++++++++++++------- .../Tests/Generator/UrlGeneratorTest.php | 13 ++++ .../Routing/Tests/Matcher/UrlMatcherTest.php | 20 +++++ .../Routing/Tests/RouteCompilerTest.php | 20 +++++ 4 files changed, 104 insertions(+), 26 deletions(-) diff --git a/src/Symfony/Component/Routing/RouteCompiler.php b/src/Symfony/Component/Routing/RouteCompiler.php index 5c16813943..76cd4f2df9 100644 --- a/src/Symfony/Component/Routing/RouteCompiler.php +++ b/src/Symfony/Component/Routing/RouteCompiler.php @@ -15,6 +15,7 @@ namespace Symfony\Component\Routing; * RouteCompiler compiles Route instances to CompiledRoute instances. * * @author Fabien Potencier + * @author Tobias Schultze */ class RouteCompiler implements RouteCompilerInterface { @@ -30,45 +31,50 @@ class RouteCompiler implements RouteCompilerInterface public function compile(Route $route) { $pattern = $route->getPattern(); - $len = strlen($pattern); $tokens = array(); $variables = array(); + $matches = array(); $pos = 0; - preg_match_all('#.\{(\w+)\}#', $pattern, $matches, PREG_OFFSET_CAPTURE | PREG_SET_ORDER); + $lastSeparator = ''; + + // Match all variables enclosed in "{}" and iterate over them. But we only want to match the innermost variable + // in case of nested "{}", e.g. {foo{bar}}. This in ensured because \w does not match "{" or "}" itself. + preg_match_all('#\{\w+\}#', $pattern, $matches, PREG_OFFSET_CAPTURE | PREG_SET_ORDER); foreach ($matches as $match) { - if ($text = substr($pattern, $pos, $match[0][1] - $pos)) { - $tokens[] = array('text', $text); - } - + $varName = substr($match[0][0], 1, -1); + // get all static text preceding the current variable + $precedingText = substr($pattern, $pos, $match[0][1] - $pos); $pos = $match[0][1] + strlen($match[0][0]); - $var = $match[1][0]; + $precedingChar = strlen($precedingText) > 0 ? substr($precedingText, -1) : ''; - if (null !== $req = $route->getRequirement($var)) { - $regexp = $req; - } else { - // Use the character preceding the variable as a separator - $separators = array($match[0][0][0]); - - if ($pos !== $len) { - // Use the character following the variable as the separator when available - $separators[] = $pattern[$pos]; - } - $regexp = sprintf('[^%s]+', preg_quote(implode('', array_unique($separators)), self::REGEX_DELIMITER)); + if (is_numeric($varName)) { + throw new \DomainException(sprintf('Variable name "%s" cannot be numeric in route pattern "%s". Please use a different name.', $varName, $pattern)); + } + if (in_array($varName, $variables)) { + throw new \LogicException(sprintf('Route pattern "%s" cannot reference variable name "%s" more than once.', $pattern, $varName)); } - $tokens[] = array('variable', $match[0][0][0], $regexp, $var); - - if (is_numeric($var)) { - throw new \DomainException(sprintf('Variable name "%s" cannot be numeric in route pattern "%s". Please use a different name.', $var, $route->getPattern())); + if (strlen($precedingText) > 1) { + $tokens[] = array('text', substr($precedingText, 0, -1)); } - if (in_array($var, $variables)) { - throw new \LogicException(sprintf('Route pattern "%s" cannot reference variable name "%s" more than once.', $route->getPattern(), $var)); + // use the character preceding the variable as a separator + // save it for later use as default separator for variables that follow directly without having a preceding char e.g. "/{x}{y}" + if ('' !== $precedingChar) { + $lastSeparator = $precedingChar; } - $variables[] = $var; + $regexp = $route->getRequirement($varName); + if (null === $regexp) { + // use the character following the variable (ignoring other placeholders) as a separator when it's not the same as the preceding separator + $nextSeparator = $this->findNextSeparator(substr($pattern, $pos)); + $regexp = sprintf('[^%s]+', preg_quote($lastSeparator !== $nextSeparator ? $lastSeparator.$nextSeparator : $lastSeparator, self::REGEX_DELIMITER)); + } + + $tokens[] = array('variable', $precedingChar, $regexp, $varName); + $variables[] = $varName; } - if ($pos < $len) { + if ($pos < strlen($pattern)) { $tokens[] = array('text', substr($pattern, $pos)); } @@ -97,6 +103,25 @@ class RouteCompiler implements RouteCompilerInterface ); } + /** + * Returns the next static character in the Route pattern that will serve as a separator. + * + * @param string $pattern The route pattern + * + * @return string The next static character (or empty string when none available) + */ + private function findNextSeparator($pattern) + { + if ('' == $pattern) { + // return empty string if pattern is empty or false (false which can be returned by substr) + return ''; + } + // first remove all placeholders from the pattern so we can find the next real static character + $pattern = preg_replace('#\{\w+\}#', '', $pattern); + + return isset($pattern[0]) ? $pattern[0] : ''; + } + /** * Computes the regexp used to match a specific token. It can be static text or a subpattern. * diff --git a/src/Symfony/Component/Routing/Tests/Generator/UrlGeneratorTest.php b/src/Symfony/Component/Routing/Tests/Generator/UrlGeneratorTest.php index f001067a53..e40285defe 100644 --- a/src/Symfony/Component/Routing/Tests/Generator/UrlGeneratorTest.php +++ b/src/Symfony/Component/Routing/Tests/Generator/UrlGeneratorTest.php @@ -255,6 +255,19 @@ class UrlGeneratorTest extends \PHPUnit_Framework_TestCase $this->assertSame('/app.php/a./.a/a../..a/...', $this->getGenerator($routes)->generate('test')); } + public function testAdjacentVariables() + { + $routes = $this->getRoutes('test', new Route('/{x}{y}{z}.{_format}', array('z' => 'default-z', '_format' => 'html'), array('y' => '\d+'))); + $generator = $this->getGenerator($routes); + $this->assertSame('/app.php/foo123', $generator->generate('test', array('x' => 'foo', 'y' => '123'))); + $this->assertSame('/app.php/foo123bar.xml', $generator->generate('test', array('x' => 'foo', 'y' => '123', 'z' => 'bar', '_format' => 'xml'))); + + // The default requirement for 'x' should not allow the separator '.' in this case because it would otherwise match everything + // and following optional variables like _format could never match. + $this->setExpectedException('Symfony\Component\Routing\Exception\InvalidParameterException'); + $generator->generate('test', array('x' => 'do.t', 'y' => '123', 'z' => 'bar', '_format' => 'xml')); + } + protected function getGenerator(RouteCollection $routes, array $parameters = array(), $logger = null) { $context = new RequestContext('/app.php'); diff --git a/src/Symfony/Component/Routing/Tests/Matcher/UrlMatcherTest.php b/src/Symfony/Component/Routing/Tests/Matcher/UrlMatcherTest.php index eb18e5068d..2fde2d8864 100644 --- a/src/Symfony/Component/Routing/Tests/Matcher/UrlMatcherTest.php +++ b/src/Symfony/Component/Routing/Tests/Matcher/UrlMatcherTest.php @@ -232,6 +232,26 @@ class UrlMatcherTest extends \PHPUnit_Framework_TestCase $this->assertEquals(array('foo' => 'text1-text2-text3', 'bar' => 'text4', '_route' => 'test'), $matcher->match('/text1-text2-text3-text4-')); } + public function testAdjacentVariables() + { + $coll = new RouteCollection(); + $coll->add('test', new Route('/{w}{x}{y}{z}.{_format}', array('z' => 'default-z', '_format' => 'html'), array('y' => 'y|Y'))); + + $matcher = new UrlMatcher($coll, new RequestContext()); + // 'w' eagerly matches as much as possible and the other variables match the remaining chars. + // This also shows that the variables w-z must all exclude the separating char (the dot '.' in this case) by default requirement. + // Otherwise they would also comsume '.xml' and _format would never match as it's an optional variable. + $this->assertEquals(array('w' => 'wwwww', 'x' => 'x', 'y' => 'Y', 'z' => 'Z','_format' => 'xml', '_route' => 'test'), $matcher->match('/wwwwwxYZ.xml')); + // As 'y' has custom requirement and can only be of value 'y|Y', it will leave 'ZZZ' to variable z. + // So with carefully chosen requirements adjacent variables, can be useful. + $this->assertEquals(array('w' => 'wwwww', 'x' => 'x', 'y' => 'y', 'z' => 'ZZZ','_format' => 'html', '_route' => 'test'), $matcher->match('/wwwwwxyZZZ')); + // z and _format are optional. + $this->assertEquals(array('w' => 'wwwww', 'x' => 'x', 'y' => 'y', 'z' => 'default-z','_format' => 'html', '_route' => 'test'), $matcher->match('/wwwwwxy')); + + $this->setExpectedException('Symfony\Component\Routing\Exception\ResourceNotFoundException'); + $matcher->match('/wxy.html'); + } + /** * @expectedException Symfony\Component\Routing\Exception\ResourceNotFoundException */ diff --git a/src/Symfony/Component/Routing/Tests/RouteCompilerTest.php b/src/Symfony/Component/Routing/Tests/RouteCompilerTest.php index a71b4993ad..284739c10a 100644 --- a/src/Symfony/Component/Routing/Tests/RouteCompilerTest.php +++ b/src/Symfony/Component/Routing/Tests/RouteCompilerTest.php @@ -120,6 +120,26 @@ class RouteCompilerTest extends \PHPUnit_Framework_TestCase array('text', '/foo'), )), + array( + 'Route with nested placeholders', + array('/{static{var}static}'), + '/{stati', '#^/\{static(?[^cs]+)static\}$#s', array('var'), array( + array('text', 'static}'), + array('variable', 'c', '[^cs]+', 'var'), + array('text', '/{stati'), + )), + + array( + 'Route without separator between variables', + array('/{w}{x}{y}{z}.{_format}', array('z' => 'default-z', '_format' => 'html'), array('y' => '(y|Y)')), + '', '#^/(?[^/\.]+)(?[^/\.]+)(?(y|Y))(?:(?[^/\.]+)(?:\.(?<_format>[^\.]+))?)?$#s', array('w', 'x', 'y', 'z', '_format'), array( + array('variable', '.', '[^\.]+', '_format'), + array('variable', '', '[^/\.]+', 'z'), + array('variable', '', '(y|Y)', 'y'), + array('variable', '', '[^/\.]+', 'x'), + array('variable', '/', '[^/\.]+', 'w'), + )), + array( 'Route with a format', array('/foo/{bar}.{_format}'),