From 0706d18adc5ecc234b872418bfb17fe3e97b3d50 Mon Sep 17 00:00:00 2001 From: Tobias Schultze Date: Sun, 5 Aug 2012 05:35:48 +0200 Subject: [PATCH] [Routing] fixed 4 bugs in the UrlGenerator --- .../Routing/Generator/UrlGenerator.php | 42 ++++++++---------- .../Tests/Generator/UrlGeneratorTest.php | 43 +++++++++++++++++-- 2 files changed, 58 insertions(+), 27 deletions(-) diff --git a/src/Symfony/Component/Routing/Generator/UrlGenerator.php b/src/Symfony/Component/Routing/Generator/UrlGenerator.php index 18edd160dd597..307b5a588f842 100644 --- a/src/Symfony/Component/Routing/Generator/UrlGenerator.php +++ b/src/Symfony/Component/Routing/Generator/UrlGenerator.php @@ -23,6 +23,7 @@ * UrlGenerator generates a URL based on a set of routes. * * @author Fabien Potencier + * @author Tobias Schultze * * @api */ @@ -132,13 +133,10 @@ 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)))); } @@ -146,30 +144,26 @@ protected function doGenerate($variables, $defaults, $requirements, $tokens, $pa $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; } diff --git a/src/Symfony/Component/Routing/Tests/Generator/UrlGeneratorTest.php b/src/Symfony/Component/Routing/Tests/Generator/UrlGeneratorTest.php index 69c06a44ea71a..651da617f0c9a 100644 --- a/src/Symfony/Component/Routing/Tests/Generator/UrlGeneratorTest.php +++ b/src/Symfony/Component/Routing/Tests/Generator/UrlGeneratorTest.php @@ -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'))); + $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)