8000 merged branch Tobion/uselessparamdefaults (PR #5400) · sunqipenglib/symfony@0f61b2e · GitHub
[go: up one dir, main page]
Skip to content

Commit 0f61b2e

Browse files
fabpotfabpot
committed
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.
2 parents 5e7723f + cb7e3f5 commit 0f61b2e

File tree

3 files changed

+29
-8
lines changed

3 files changed

+29
-8
lines changed

‎src/Symfony/Component/Routing/RouteCompiler.php

Lines changed: 14 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -23,7 +23,8 @@ class RouteCompiler implements RouteCompilerInterface
2323
/**
2424
* {@inheritDoc}
2525
*
26-
* @throws \LogicException If a variable is referenced more than once
26+
* @throws \LogicException If a variable is referenced more than once or if a required variable
27+
* has a default value that doesn't meet its own requirement.
2728
*/
2829
public function compile(Route $route)
2930
{
@@ -67,14 +68,23 @@ public function compile(Route $route)
6768
$tokens[] = array('text', substr($pattern, $pos));
6869
}
6970

70-
// find the first optional token
71+
// find the first optional token and validate the default values for non-optional variables
72+
$optional = true;
7173
$firstOptional = INF;
7274
for ($i = count($tokens) - 1; $i >= 0; $i--) {
7375
$token = $tokens[$i];
7476
if ('variable' === $token[0] && $route->hasDefault($token[3])) {
75-
$firstOptional = $i;
77+
if ($optional) {
78+
$firstOptional = $i;
79+
} elseif (!preg_match('#^'.$token[2].'$#', $route->getDefault($token[3]))) {
80+
throw new \LogicException(sprintf('The default value "%s" of the required variable "%s" in pattern "%s" does not match the requirement "%s". ' .
81+
'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. ' .
82+
'You should change the default to something that meets the requirement or remove it.',
83+
$route->getDefault($token[3]), $token[3], $route->getPattern(), $token[2]
84+
));
85+
}
7686
} else {
77-
break;
87+
$optional = false;
7888
}
7989
}
8090

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

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -75,13 +75,13 @@ public function testRelativeUrlWithNullParameter()
7575
}
7676

7777
/**
78-
* @expectedException Symfony\Component\Routing\Exception\InvalidParameterException
78+
* @expectedException \LogicException
7979
*/
8080
public function testRelativeUrlWithNullParameterButNotOptional()
8181
{
8282
$routes = $this->getRoutes('test', new Route('/testing/{foo}/bar', array('foo' => null)));
83-
// This must raise an exception because the default requirement for "foo" is "[^/]+" which is not met with these params.
84-
// Generating path "/testing//bar" would be wrong as matching this route would fail.
83+
// It should raise an exception when compiling this route because the given default value is absolutely
84+
// irrelevant for both matching and generating URLs.
8585
$this->getGenerator($routes)->generate('test', array(), false);
8686
}
8787

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

Lines changed: 12 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -131,6 +131,17 @@ public function testRouteWithSameVariableTwice()
131131
{
132132
$route = new Route('/{name}/{name}');
133133

134-
$compiled = $route->compile();
134+
$route->compile();
135+
}
136+
137+
/**
138+
* @expectedException \LogicException
139+
*/
140+
public function testRouteWithRequiredVariableAndBadDefault()
141+
{
142+
$route = new Route('/{foo}/', array('foo' => null));
143+
// It should raise an exception when compiling this route because the given default value is absolutely
144+
// irrelevant for both matching and generating URLs.
145+
$route->compile();
135146
}
136147
}

0 commit comments

Comments
 (0)
0