[Routing] fixed 4 bugs in the UrlGenerator

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

@@ -23,6 +23,7 @@ use Symfony\Component\HttpKernel\Log\LoggerInterface;
  * UrlGenerator generates a URL based on a set of routes.
  *
  * @author Fabien Potencier <fabien@symfony.com>
+ * @author Tobias Schultze <http://tobion.de>
  *
  * @api
  */
@@ -132,13 +133,10 @@ class UrlGenerator implements UrlGeneratorInterface, ConfigurableRequirementsInt
     protected function doGenerate($variables, $defaults, $requirements, $tokens, $parameters, $name, $absolute)
     {
         $variables = array_flip($variables);
-
+        $mergedParams = array_replace($this->context->getParameters(), $defaults, $parameters);
-        $originParameters = $parameters;
-        $parameters = array_replace($this->context->getParameters(), $parameters);
-        $tparams = array_replace($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 @@ class UrlGenerator implements UrlGeneratorInterface, ConfigurableRequirementsInt
         $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 (!$optional || !array_key_exists($token[3], $defaults) || (string) $mergedParams[$token[3]] !== (string) $defaults[$token[3]]) {
-                    if (!$isEmpty = in_array($tparams[$token[3]], array(null, '', false), true)) {
+                    // check requirement
-                        // check requirement
+                    if (!preg_match('#^'.$token[2].'$#', $mergedParams[$token[3]])) {
-                        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], $mergedParams[$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) {
-                            if ($this->strictRequirements) {
+                            throw new InvalidParameterException($message);
-                                throw new InvalidParameterException($message);
-                            }
-
-                            if ($this->logger) {
-                                $this->logger->err($message);
-                            }
-
-                            return null;
                         }
+
+                        if ($this->logger) {
+                            $this->logger->err($message);
+                        }
+
+                        return null;
                     }
 
-                    if (!$isEmpty || !$optional) {
+                    $url = $token[1].$mergedParams[$token[3]].$url;
-                        $url = $token[1].$tparams[$token[3]].$url;
-                    }
-
                     $optional = false;
                 }
-            } elseif ('text' === $token[0]) {
+            } else {
+                // static text
                 $url = $token[1].$url;
                 $optional = false;
             }
@@ -193,7 +187,7 @@ class UrlGenerator implements UrlGeneratorInterface, ConfigurableRequirementsInt
         }
 
         // 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;
         }

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

@@ -74,12 +74,15 @@ class UrlGeneratorTest extends \PHPUnit_Framework_TestCase
         $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 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->assertEquals('/app.php/testing//bar', $url);
+        $this->getGenerator($routes)->generate('test', array(), false);
     }
 
     public function testRelativeUrlWithOptionalZeroParameter()
@@ -90,6 +93,13 @@ class UrlGeneratorTest extends \PHPUnit_Framework_TestCase
         $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 @@ class UrlGeneratorTest extends \PHPUnit_Framework_TestCase
         $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 @@ class UrlGeneratorTest extends \PHPUnit_Framework_TestCase
         $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 @@ class UrlGeneratorTest extends \PHPUnit_Framework_TestCase
         $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)