10000 merged branch Tobion/strictrequirements (PR #5181) · symfony/symfony@2da2a44 · GitHub
[go: up one dir, main page]
Skip to content

Commit 2da2a44

Browse files
fabpotfabpot
committed
merged branch Tobion/strictrequirements (PR #5181)
Commits ------- 0706d18 [Routing] fixed 4 bugs in the UrlGenerator Discussion ---------- [Routing] UrlGenerator: fixed missing query param and some ignored requirements This was pretty hard to figure out. I could fix 4 bugs and refactor the code to safe 2 variables and several assignments. Sorry for doing this in one commit, but they were highly interdependent. See the added tests for what was fixed. The most obvious bug was that a query param was ignored if it had by accident the same name as a default param (but wasn't used in the path). In 3 cases it generated the wrong URL that wouldn't match this route. The generator wrongly ignored either the requirements or the passed parameter. I had to adjust one test that was asserting something wrong (see comments). --------------------------------------------------------------------------- by Tobion at 2012-08-13T14:22:35Z ping @fabpot --------------------------------------------------------------------------- by Tobion at 2012-08-29T17:53:07Z @fabpot I think it's important to merge this before 2.1 final.
2 parents 5885547 + 0706d18 commit 2da2a44

File tree

2 files changed

+58
-27
lines changed

2 files changed

+58
-27
lines changed

‎src/Symfony/Component/Routing/Generator/UrlGenerator.php

Lines changed: 18 additions & 24 deletions
Original file line numberDiff line numberDiff line change
@@ -23,6 +23,7 @@
2323
* UrlGenerator generates a URL based on a set of routes.
2424
*
2525
* @author Fabien Potencier <fabien@symfony.com>
26+
* @author Tobias Schultze <http://tobion.de>
2627
*
2728
* @api
2829
*/
@@ -132,44 +133,37 @@ public function generate($name, $parameters = array(), $absolute = false)
132133
protected function doGenerate($variables, $defaults, $requirements, $tokens, $parameters, $name, $absolute)
133134
{
134135
$variables = array_flip($variables);
135-
136-
$originParameters = $parameters;
137-
$parameters = array_replace($this->context->getParameters(), $parameters);
138-
$tparams = array_replace($defaults, $parameters);
136+
$mergedParams = array_replace($this->context->getParameters(), $defaults, $parameters);
139137

140138
// all params must be given
141-
if ($diff = array_diff_key($variables, $tparams)) {
139+
if ($diff = array_diff_key($variables, $mergedParams)) {
142140
throw new MissingMandatoryParametersException(sprintf('The "%s" route has some missing mandatory parameters ("%s").', $name, implode('", "', array_keys($diff))));
143141
}
144142

145143
$url = '';
146144
$optional = true;
147145
foreach ($tokens as $token) {
148146
if ('variable' === $token[0]) {
149-
if (false === $optional || !array_key_exists($token[3], $defaults) || (isset($parameters[$token[3]]) && (string) $parameters[$token[3]] != (string) $defaults[$token[3]])) {
150-
if (!$isEmpty = in_array($tparams[$token[3]], array(null, '', false), true)) {
151-
// check requirement
152-
if ($tparams[$token[3]] && !preg_match('#^'.$token[2].'$#', $tparams[$token[3]])) {
153-
$message = sprintf('Parameter "%s" for route "%s" must match "%s" ("%s" given).', $token[3], $name, $token[2], $tparams[$token[3]]);
154-
if ($this->strictRequirements) {
155-
throw new InvalidParameterException($message);
156-
}
157-
158-
if ($this->logger) {
159-
$this->logger->err($message);
160-
}
161-
162-
return null;
147+
if (!$optional || !array_key_exists($token[3], $defaults) || (string) $mergedParams[$token[3]] !== (string) $defaults[$token[3]]) {
148+
// check requirement
149+
if (!preg_match('#^'.$token[2].'$#', $mergedParams[$token[3]])) {
150+
$message = sprintf('Parameter "%s" for route "%s" must match "%s" ("%s" given).', $token[3], $name, $token[2], $mergedParams[$token[3]]);
151+
if ($this->strictRequirements) {
152+
throw new InvalidParameterException($message);
153+
}
154+
155+
if ($this->logger) {
156+
$this->logger->err($message);
163157
}
164-
}
165158

166-
if (!$isEmpty || !$optional) {
167-
$url = $token[1].$tparams[$token[3]].$url;
159+
return null;
168160
}
169161

162+
$url = $token[1].$mergedParams[$token[3]].$url;
170163
$optional = false;
171164
}
172-
} elseif ('text' === $token[0]) {
165+
} else {
166+
// static text
173167
$url = $token[1].$url;
174168
$optional = false;
175169
}
@@ -193,7 +187,7 @@ protected function doGenerate($variables, $defaults, $requirements, $tokens, $pa
193187
}
194188

195189
// add a query string if needed
196-
$extra = array_diff_key($originParameters, $variables, $defaults);
190+
$extra = array_diff_key($parameters, $variables);
197191
if ($extra && $query = http_build_query($extra, '', '&')) {
198192
$url .= '?'.$query;
199193
}

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

Lines changed: 40 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -74,12 +74,15 @@ public function testRelativeUrlWithNullParameter()
7474
$this->assertEquals('/app.php/testing', $url);
7575
}
7676

77+
/**
78+
* @expectedException Symfony\Component\Routing\Exception\InvalidParameterException
79+
*/
7780
public function testRelativeUrlWithNullParameterButNotOptional()
7881
{
7982
$routes = $this->getRoutes('test', new Route('/testing/{foo}/bar', array('foo' => null)));
80-
$url = $this->getGenerator($routes)->generate('test', array(), false);
81-
82-
$this->assertEquals('/app.php/testing//bar', $url);
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.
85+
$this->getGenerator($routes)->generate('test', array(), false);
8386
}
8487

8588
public function testRelativeUrlWithOptionalZeroParameter()
@@ -90,6 +93,13 @@ public function testRelativeUrlWithOptionalZeroParameter()
9093
$this->assertEquals('/app.php/testing/0', $url);
9194
}
9295

96+
public function testNotPassedOptionalParameterInBetween()
97+
{
98+
$routes = $this->getRoutes('test', new Route('/{slug}/{page}', array('slug' => 'index', 'page' => 0)));
99+
$this->assertSame('/app.php/index/1', $this->getGenerator($routes)->generate('test', array('page' => 1)));
100+
$this->assertSame('/app.php/', $this->getGenerator($routes)->generate('test'));
101+
}
102+
93103
public function testRelativeUrlWithExtraParameters()
94104
{
95105
$routes = $this->getRoutes('test', new Route('/testing'));
@@ -165,6 +175,15 @@ public function testGenerateForRouteWithInvalidOptionalParameter()
165175
$this->getGenerator($routes)->generate('test', array('foo' => 'bar'), true);
166176
}
167177

178+
/**
179+
* @expectedException Symfony\Component\Routing\Exception\InvalidParameterException
180+
*/
181+
public function testGenerateForRouteWithInvalidParameter()
182+
{
183+
$routes = $this->getRoutes('test', new Route('/testing/{foo}', array(), array('foo' => '1|2')));
184+
$this->getGenerator($routes)->generate('test', array('foo' => '0'), true);
185+
}
186+
168187
public function testGenerateForRouteWithInvalidOptionalParameterNonStrict()
169188
{
170189
$routes = $this->getRoutes('test', new Route('/testing/{foo}', array('foo' => '1'), array('foo' => 'd+')));
@@ -196,6 +215,15 @@ public function testGenerateForRouteWithInvalidMandatoryParameter()
196215
$routes = $this->getRoutes('test', new Route('/testing/{foo}', array(), array('foo' => 'd+')));
197216
$this->getGenerator($routes)->generate('test', array('foo' => 'bar'), true);
198217
}
218+
219+
/**
220+
* @expectedException Symfony\Component\Routing\Exception\InvalidParameterException
221+
*/
222+
public function testRequiredParamAndEmptyPassed()
223+
{
224+
$routes = $this->getRoutes('test', new Route('/{slug}', array(), array('slug' => '.+')));
225+
$this->getGenerator($routes)->generate('test', array('slug' => ''));
226+
}
199227

200228
public function testSchemeRequirementDoesNothingIfSameCurrentScheme()
201229
{
@@ -229,6 +257,15 @@ public function testWithAnIntegerAsADefaultValue()
229257
$this->assertEquals('/app.php/foo', $this->getGenerator($routes)->generate('test', array('default' => 'foo')));
230258
}
231259

260+
public function testQueryParamSameAsDefault()
261+
{
262+
$routes = $this->getRoutes('test', new Route('/test', array('default' => 'value')));
263+
264+
$this->assertSame('/app.php/test?default=foo', $this->getGenerator($routes)->generate('test', array('default' => 'foo')));
265+
$this->assertSame('/app.php/test?default=value', $this->getGenerator($routes)->generate('test', array('default' => 'value')));
266+
$this->assertSame('/app.php/test', $this->getGenerator($routes)->generate('test'));
267+
}
268+
232269
public function testUrlEncoding()
233270
{
234271
// This tests the encoding of reserved characters that are used for delimiting of URI components (defined in RFC 3986)

0 commit comments

Comments
 (0)
0