Permalink
Browse files

[Routing] default requirement of variable should not disallow previou…

…s char, but the slash '/' (closes #5469)
  • Loading branch information...
1 parent 4868bec commit a3147e9f134067932f1e896930f1c64fe142adc5 @Tobion Tobion committed with fabpot Sep 8, 2012
View
20 src/Symfony/Component/Routing/RouteCompiler.php
@@ -42,7 +42,6 @@ public function compile(Route $route)
$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 @@ public function compile(Route $route)
$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);
View
27 src/Symfony/Component/Routing/Tests/Generator/UrlGeneratorTest.php
@@ -285,6 +285,33 @@ public function testRequiredVariableWithNoRealSeparator()
$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');
View
33 src/Symfony/Component/Routing/Tests/Matcher/UrlMatcherTest.php
@@ -276,6 +276,39 @@ public function testRequiredVariableWithNoRealSeparator()
$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
*/
View
12 src/Symfony/Component/Routing/Tests/RouteCompilerTest.php
@@ -115,8 +115,8 @@ public function provideCompileData()
array(
'Route with a variable in last position',
array('/foo-{bar}'),
- '/foo', '#^/foo\-(?<bar>[^\-]+)$#s', array('bar'), array(
- array('variable', '-', '[^\-]+', 'bar'),
+ '/foo', '#^/foo\-(?<bar>[^/]+)$#s', array('bar'), array(
+ array('variable', '-', '[^/]+', 'bar'),
array('text', '/foo'),
)),
@@ -132,8 +132,8 @@ public function provideCompileData()
array(
'Route without separator between variables',
array('/{w}{x}{y}{z}.{_format}', array('z' => 'default-z', '_format' => 'html'), array('y' => '(y|Y)')),
- '', '#^/(?<w>[^/\.]+)(?<x>[^/\.]+)(?<y>(y|Y))(?:(?<z>[^/\.]+)(?:\.(?<_format>[^\.]+))?)?$#s', array('w', 'x', 'y', 'z', '_format'), array(
- array('variable', '.', '[^\.]+', '_format'),
+ '', '#^/(?<w>[^/\.]+)(?<x>[^/\.]+)(?<y>(y|Y))(?:(?<z>[^/\.]+)(?:\.(?<_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 @@ public function provideCompileData()
array(
'Route with a format',
array('/foo/{bar}.{_format}'),
- '/foo', '#^/foo/(?<bar>[^/\.]+)\.(?<_format>[^\.]+)$#s', array('bar', '_format'), array(
- array('variable', '.', '[^\.]+', '_format'),
+ '/foo', '#^/foo/(?<bar>[^/\.]+)\.(?<_format>[^/]+)$#s', array('bar', '_format'), array(
+ array('variable', '.', '[^/]+', '_format'),
array('variable', '/', '[^/\.]+', 'bar'),
array('text', '/foo'),
)),

0 comments on commit a3147e9

Please sign in to comment.