merged branch Tobion/uselessparamdefaults (PR symfony#5400)

Commits ------- cb7e3f5 [Routing] added route compile check to identify a default value of a required variable that does not match the requirement Discussion ---------- [Routing] added route compile check to identify a bad default value BC break: yes but only for strange route definitions See the exception message in code for the reasoning. An exception is thrown for a __required__ variable that __has a default__ that __doesn't match__ the requirement. So optional variables can of course still have a default that don't meet the requirement, which is useful. This helps to identify useless route definitions at compile time instead of when generating or matching a URL.

Author: fabpot

src/Symfony/Component/Routing/RouteCompiler.php

@@ -23,7 +23,8 @@ class RouteCompiler implements RouteCompilerInterface
  /**
  * {@inheritDoc}
  *
- * @throws \LogicException If a variable is referenced more than once
+ * @throws \LogicException If a variable is referenced more than once or if a required variable
+ * has a default value that doesn't meet its own requirement.
  */
  public function compile(Route $route)
  {
@@ -67,14 +68,23 @@ public function compile(Route $route)
  $tokens[] = array('text', substr($pattern, $pos));
  }
 
- // find the first optional token
+ // find the first optional token and validate the default values for non-optional variables
+ $optional = true;
  $firstOptional = INF;
  for ($i = count($tokens) - 1; $i >= 0; $i--) {
  $token = $tokens[$i];
  if ('variable' === $token[0] && $route->hasDefault($token[3])) {
- $firstOptional = $i;
+ if ($optional) {
+ $firstOptional = $i;
+ } elseif (!preg_match('#^'.$token[2].'$#', $route->getDefault($token[3]))) {
+ throw new \LogicException(sprintf('The default value "%s" of the required variable "%s" in pattern "%s" does not match the requirement "%s". ' .
+ 'This route definition makes no sense because this default can neither be used as default for generating URLs nor can it ever be returned by the matching process. ' .
+ 'You should change the default to something that meets the requirement or remove it.',
+ $route->getDefault($token[3]), $token[3], $route->getPattern(), $token[2]
+ ));
+ }
  } else {
- break;
+ $optional = false;
  }
  }
 

src/Symfony/Component/Routing/Tests/Generator/UrlGeneratorTest.php

@@ -75,13 +75,13 @@ public function testRelativeUrlWithNullParameter()
  }
 
  /**
- * @expectedException Symfony\Component\Routing\Exception\InvalidParameterException
+ * @expectedException \LogicException
  */
  public function testRelativeUrlWithNullParameterButNotOptional()
  {
  $routes = $this->getRoutes('test', new Route('/testing/{foo}/bar', array('foo' => null)));
- // 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.
+ // It should raise an exception when compiling this route because the given default value is absolutely
+ // irrelevant for both matching and generating URLs.
  $this->getGenerator($routes)->generate('test', array(), false);
  }
 

src/Symfony/Component/Routing/Tests/RouteCompilerTest.php

@@ -131,6 +131,17 @@ public function testRouteWithSameVariableTwice()
  {
  $route = new Route('/{name}/{name}');
 
- $compiled = $route->compile();
+ $route->compile();
+ }
+
+ /**
+ * @expectedException \LogicException
+ */
+ public function testRouteWithRequiredVariableAndBadDefault()
+ {
+ $route = new Route('/{foo}/', array('foo' => null));
+ // It should raise an exception when compiling this route because the given default value is absolutely
+ // irrelevant for both matching and generating URLs.
+ $route->compile();
  }
 }