From a3147e9f134067932f1e896930f1c64fe142adc5 Mon Sep 17 00:00:00 2001 From: Tobias Schultze Date: Sat, 8 Sep 2012 12:51:33 +0200 Subject: [PATCH] [Routing] default requirement of variable should not disallow previous char, but the slash '/' (closes #5469) --- .../Component/Routing/RouteCompiler.php | 20 +++++------ .../Tests/Generator/UrlGeneratorTest.php | 27 +++++++++++++++ .../Routing/Tests/Matcher/UrlMatcherTest.php | 33 +++++++++++++++++++ .../Routing/Tests/RouteCompilerTest.php | 12 +++---- 4 files changed, 76 insertions(+), 16 deletions(-) diff --git a/src/Symfony/Component/Routing/RouteCompiler.php b/src/Symfony/Component/Routing/RouteCompiler.php index e132e24085..919a649fe7 100644 --- a/src/Symfony/Component/Routing/RouteCompiler.php +++ b/src/Symfony/Component/Routing/RouteCompiler.php @@ -42,7 +42,6 @@ class RouteCompiler implements RouteCompilerInterface $variables = array(); $matches = array(); $pos = 0; - $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. @@ -68,17 +67,18 @@ class RouteCompiler implements RouteCompilerInterface $tokens[] = array('text', $precedingText); } - // 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 ($isSeparator) { - $lastSeparator = $precedingChar; - } - $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)); + $followingPattern = (string) substr($pattern, $pos); + // Find the next static character after the variable that functions as a separator. By default, this separator and '/' + // are disallowed for the variable. This default requirement makes sure that optional variables can be matched at all + // and that the generating-matching-combination of URLs unambiguous, i.e. the params used for generating the URL are + // the same that will be matched. Example: new Route('/{page}.{_format}', array('_format' => 'html')) + // If {page} would also match the separating dot, {_format} would never match as {page} will eagerly consume everything. + // Also even if {_format} was not optional the requirement prevents that {page} matches something that was originally + // part of {_format} when generating the URL, e.g. _format = 'mobile.html'. + $nextSeparator = $this->findNextSeparator($followingPattern); + $regexp = sprintf('[^/%s]+', '/' !== $nextSeparator && '' !== $nextSeparator ? preg_quote($nextSeparator, self::REGEX_DELIMITER) : ''); } $tokens[] = array('variable', $isSeparator ? $precedingChar : '', $regexp, $varName); diff --git a/src/Symfony/Component/Routing/Tests/Generator/UrlGeneratorTest.php b/src/Symfony/Component/Routing/Tests/Generator/UrlGeneratorTest.php index d417a25e21..9333e759e2 100644 --- a/src/Symfony/Component/Routing/Tests/Generator/UrlGeneratorTest.php +++ b/src/Symfony/Component/Routing/Tests/Generator/UrlGeneratorTest.php @@ -285,6 +285,33 @@ class UrlGeneratorTest extends \PHPUnit_Framework_TestCase $this->assertSame('/app.php/getSitesSuffix', $generator->generate('test', array('what' => 'Sites'))); } + public function testDefaultRequirementOfVariable() + { + $routes = $this->getRoutes('test', new Route('/{page}.{_format}')); + $generator = $this->getGenerator($routes); + + $this->assertSame('/app.php/index.mobile.html', $generator->generate('test', array('page' => 'index', '_format' => 'mobile.html'))); + } + + /** + * @expectedException Symfony\Component\Routing\Exception\InvalidParameterException + */ + public function testDefaultRequirementOfVariableDisallowsSlash() + { + $routes = $this->getRoutes('test', new Route('/{page}.{_format}')); + $this->getGenerator($routes)->generate('test', array('page' => 'index', '_format' => 'sl/ash')); + } + + /** + * @expectedException Symfony\Component\Routing\Exception\InvalidParameterException + */ + public function testDefaultRequirementOfVariableDisallowsNextSeparator() + { + + $routes = $this->getRoutes('test', new Route('/{page}.{_format}')); + $this->getGenerator($routes)->generate('test', array('page' => 'do.t', '_format' => 'html')); + } + 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 312c44863c..0927c75044 100644 --- a/src/Symfony/Component/Routing/Tests/Matcher/UrlMatcherTest.php +++ b/src/Symfony/Component/Routing/Tests/Matcher/UrlMatcherTest.php @@ -276,6 +276,39 @@ class UrlMatcherTest extends \PHPUnit_Framework_TestCase $this->assertEquals(array('what' => 'Sites', '_route' => 'test'), $matcher->match('/getSitesSuffix')); } + public function testDefaultRequirementOfVariable() + { + $coll = new RouteCollection(); + $coll->add('test', new Route('/{page}.{_format}')); + $matcher = new UrlMatcher($coll, new RequestContext()); + + $this->assertEquals(array('page' => 'index', '_format' => 'mobile.html', '_route' => 'test'), $matcher->match('/index.mobile.html')); + } + + /** + * @expectedException Symfony\Component\Routing\Exception\ResourceNotFoundException + */ + public function testDefaultRequirementOfVariableDisallowsSlash() + { + $coll = new RouteCollection(); + $coll->add('test', new Route('/{page}.{_format}')); + $matcher = new UrlMatcher($coll, new RequestContext()); + + $matcher->match('/index.sl/ash'); + } + + /** + * @expectedException Symfony\Component\Routing\Exception\ResourceNotFoundException + */ + public function testDefaultRequirementOfVariableDisallowsNextSeparator() + { + $coll = new RouteCollection(); + $coll->add('test', new Route('/{page}.{_format}', array(), array('_format' => 'html|xml'))); + $matcher = new UrlMatcher($coll, new RequestContext()); + + $matcher->match('/do.t.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 4e371a2cfd..f800047525 100644 --- a/src/Symfony/Component/Routing/Tests/RouteCompilerTest.php +++ b/src/Symfony/Component/Routing/Tests/RouteCompilerTest.php @@ -115,8 +115,8 @@ class RouteCompilerTest extends \PHPUnit_Framework_TestCase array( 'Route with a variable in last position', array('/foo-{bar}'), - '/foo', '#^/foo\-(?[^\-]+)$#s', array('bar'), array( - array('variable', '-', '[^\-]+', 'bar'), + '/foo', '#^/foo\-(?[^/]+)$#s', array('bar'), array( + array('variable', '-', '[^/]+', 'bar'), array('text', '/foo'), )), @@ -132,8 +132,8 @@ class RouteCompilerTest extends \PHPUnit_Framework_TestCase 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'), + '', '#^/(?[^/\.]+)(?[^/\.]+)(?(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'), @@ -143,8 +143,8 @@ class RouteCompilerTest extends \PHPUnit_Framework_TestCase array( 'Route with a format', array('/foo/{bar}.{_format}'), - '/foo', '#^/foo/(?[^/\.]+)\.(?<_format>[^\.]+)$#s', array('bar', '_format'), array( - array('variable', '.', '[^\.]+', '_format'), + '/foo', '#^/foo/(?[^/\.]+)\.(?<_format>[^/]+)$#s', array('bar', '_format'), array( + array('variable', '.', '[^/]+', '_format'), array('variable', '/', '[^/\.]+', 'bar'), array('text', '/foo'), )),