Permalink
Browse files

[Routing] fixed 4 bugs in the UrlGenerator

  • Loading branch information...
1 parent 79ea562 commit c66d1f9de30fd1b6a86cca10dd79d12c9ba9ff25 @Tobion Tobion committed Aug 5, 2012
Showing with 58 additions and 27 deletions.
  1. +18 −24 Generator/UrlGenerator.php
  2. +40 −3 Tests/Generator/UrlGeneratorTest.php
@@ -23,6 +23,7 @@
* UrlGenerator generates a URL based on a set of routes.
*
* @author Fabien Potencier <fabien@symfony.com>
+ * @author Tobias Schultze <http://tobion.de>
*
* @api
*/
@@ -132,44 +133,37 @@ public function generate($name, $parameters = array(), $absolute = false)
protected function doGenerate($variables, $defaults, $requirements, $tokens, $parameters, $name, $absolute)
{
$variables = array_flip($variables);
-
- $originParameters = $parameters;
- $parameters = array_replace($this->context->getParameters(), $parameters);
- $tparams = array_replace($defaults, $parameters);
+ $mergedParams = array_replace($this->context->getParameters(), $defaults, $parameters);
// all params must be given
- if ($diff = array_diff_key($variables, $tparams)) {
+ if ($diff = array_diff_key($variables, $mergedParams)) {
throw new MissingMandatoryParametersException(sprintf('The "%s" route has some missing mandatory parameters ("%s").', $name, implode('", "', array_keys($diff))));
}
$url = '';
$optional = true;
foreach ($tokens as $token) {
if ('variable' === $token[0]) {
- if (false === $optional || !array_key_exists($token[3], $defaults) || (isset($parameters[$token[3]]) && (string) $parameters[$token[3]] != (string) $defaults[$token[3]])) {
- if (!$isEmpty = in_array($tparams[$token[3]], array(null, '', false), true)) {
- // check requirement
- if ($tparams[$token[3]] && !preg_match('#^'.$token[2].'$#', $tparams[$token[3]])) {
- $message = sprintf('Parameter "%s" for route "%s" must match "%s" ("%s" given).', $token[3], $name, $token[2], $tparams[$token[3]]);
- if ($this->strictRequirements) {
- throw new InvalidParameterException($message);
- }
-
- if ($this->logger) {
- $this->logger->err($message);
- }
-
- return null;
+ if (!$optional || !array_key_exists($token[3], $defaults) || (string) $mergedParams[$token[3]] !== (string) $defaults[$token[3]]) {
+ // check requirement
+ if (!preg_match('#^'.$token[2].'$#', $mergedParams[$token[3]])) {
+ $message = sprintf('Parameter "%s" for route "%s" must match "%s" ("%s" given).', $token[3], $name, $token[2], $mergedParams[$token[3]]);
+ if ($this->strictRequirements) {
+ throw new InvalidParameterException($message);
+ }
+
+ if ($this->logger) {
+ $this->logger->err($message);
}
- }
- if (!$isEmpty || !$optional) {
- $url = $token[1].$tparams[$token[3]].$url;
+ return null;
}
+ $url = $token[1].$mergedParams[$token[3]].$url;
$optional = false;
}
- } elseif ('text' === $token[0]) {
+ } else {
+ // static text
$url = $token[1].$url;
$optional = false;
}
@@ -193,7 +187,7 @@ protected function doGenerate($variables, $defaults, $requirements, $tokens, $pa
}
// add a query string if needed
- $extra = array_diff_key($originParameters, $variables, $defaults);
+ $extra = array_diff_key($parameters, $variables);
if ($extra && $query = http_build_query($extra, '', '&')) {
$url .= '?'.$query;
}
@@ -74,12 +74,15 @@ public function testRelativeUrlWithNullParameter()
$this->assertEquals('/app.php/testing', $url);
}
+ /**
+ * @expectedException Symfony\Component\Routing\Exception\InvalidParameterException
+ */
public function testRelativeUrlWithNullParameterButNotOptional()
{
$routes = $this->getRoutes('test', new Route('/testing/{foo}/bar', array('foo' => null)));
- $url = $this->getGenerator($routes)->generate('test', array(), false);
-
- $this->assertEquals('/app.php/testing//bar', $url);
+ // This must raise an exception because the default requirement for "foo" is "[^/]+" which is not met with these params.
+ // Generating path "/testing//bar" would be wrong as matching this route would fail.
+ $this->getGenerator($routes)->generate('test', array(), false);
}
public function testRelativeUrlWithOptionalZeroParameter()
@@ -90,6 +93,13 @@ public function testRelativeUrlWithOptionalZeroParameter()
$this->assertEquals('/app.php/testing/0', $url);
}
+ public function testNotPassedOptionalParameterInBetween()
+ {
+ $routes = $this->getRoutes('test', new Route('/{slug}/{page}', array('slug' => 'index', 'page' => 0)));
+ $this->assertSame('/app.php/index/1', $this->getGenerator($routes)->generate('test', array('page' => 1)));
+ $this->assertSame('/app.php/', $this->getGenerator($routes)->generate('test'));
+ }
+
public function testRelativeUrlWithExtraParameters()
{
$routes = $this->getRoutes('test', new Route('/testing'));
@@ -165,6 +175,15 @@ public function testGenerateForRouteWithInvalidOptionalParameter()
$this->getGenerator($routes)->generate('test', array('foo' => 'bar'), true);
}
+ /**
+ * @expectedException Symfony\Component\Routing\Exception\InvalidParameterException
+ */
+ public function testGenerateForRouteWithInvalidParameter()
+ {
+ $routes = $this->getRoutes('test', new Route('/testing/{foo}', array(), array('foo' => '1|2')));
+ $this->getGenerator($routes)->generate('test', array('foo' => '0'), true);
+ }
+
public function testGenerateForRouteWithInvalidOptionalParameterNonStrict()
{
$routes = $this->getRoutes('test', new Route('/testing/{foo}', array('foo' => '1'), array('foo' => 'd+')));
@@ -196,6 +215,15 @@ public function testGenerateForRouteWithInvalidMandatoryParameter()
$routes = $this->getRoutes('test', new Route('/testing/{foo}', array(), array('foo' => 'd+')));
$this->getGenerator($routes)->generate('test', array('foo' => 'bar'), true);
}
+
+ /**
+ * @expectedException Symfony\Component\Routing\Exception\InvalidParameterException
+ */
+ public function testRequiredParamAndEmptyPassed()
+ {
+ $routes = $this->getRoutes('test', new Route('/{slug}', array(), array('slug' => '.+')));
+ $this->getGenerator($routes)->generate('test', array('slug' => ''));
+ }
public function testSchemeRequirementDoesNothingIfSameCurrentScheme()
{
@@ -229,6 +257,15 @@ public function testWithAnIntegerAsADefaultValue()
$this->assertEquals('/app.php/foo', $this->getGenerator($routes)->generate('test', array('default' => 'foo')));
}
+ public function testQueryParamSameAsDefault()
+ {
+ $routes = $this->getRoutes('test', new Route('/test', array('default' => 'value')));
+
+ $this->assertSame('/app.php/test?default=foo', $this->getGenerator($routes)->generate('test', array('default' => 'foo')));
+ $this->assertSame('/app.php/test?default=value', $this->getGenerator($routes)->generate('test', array('default' => 'value')));
@schmittjoh
schmittjoh Mar 18, 2013

Why shouldn't this url be /app.php/test? On a related note, does this not lead to duplicate content, e.g.

/app.php/test
/app.php/test?default=value

having the same content?

@Tobion
Tobion Mar 18, 2013 Symfony member

Well, placeholders and query params are not related. The idea is that any given parameter to the generate method that is not a placeholder will be added as query string. If we change it, the query params would depend on what defaults are defined on the route which IMO is not expected.

Your argument about duplicate content seems not valid here because you could add any query param and say it's duplicate content because they are not matched at all. /app.php/test == /app.php/test?asdjflk=aksdjf

@Tobion
Tobion Mar 18, 2013 Symfony member

If we really what that behavior it would make the query generation much more complex as we would first need to filter out params that equal the default. And one cannot simply use array_intersect_assoc because it fails for nested arrays which are allowed for query params.

+ $this->assertSame('/app.php/test', $this->getGenerator($routes)->generate('test'));
+ }
+
public function testUrlEncoding()
{
// This tests the encoding of reserved characters that are used for delimiting of URI components (defined in RFC 3986)

0 comments on commit c66d1f9

Please sign in to comment.