[6.x] Fix RouteUrlGenerator accepting empty string for required param…

…enter (#30714) * Fix RouteUrlGenerator accepting empty string for required paramenter * Fixed optional parameter passed without key, added tests * StyleCI fixes
laravel · Dec 2, 2019 · dfb4c1c · dfb4c1c
1 parent 767bc1b
commit dfb4c1c
Show file tree

Hide file tree

Showing 2 changed files with 19 additions and 4 deletions.
diff --git a/src/Illuminate/Routing/RouteUrlGenerator.php b/src/Illuminate/Routing/RouteUrlGenerator.php
@@ -217,11 +217,13 @@ protected function replaceRouteParameters($path, array &$parameters)
      */
     protected function replaceNamedParameters($path, &$parameters)
     {
-        return preg_replace_callback('/\{(.*?)\??\}/', function ($m) use (&$parameters) {
-            if (isset($parameters[$m[1]])) {
+        return preg_replace_callback('/\{(.*?)(\?)?\}/', function ($m) use (&$parameters) {
+            if (isset($parameters[$m[1]]) && $parameters[$m[1]] != '') {
                 return Arr::pull($parameters, $m[1]);
             } elseif (isset($this->defaultParameters[$m[1]])) {
                 return $this->defaultParameters[$m[1]];
+            } elseif (isset($parameters[$m[1]])) {
+                Arr::pull($parameters, $m[1]);
             }
 
             return $m[0];

diff --git a/tests/Routing/RoutingUrlGeneratorTest.php b/tests/Routing/RoutingUrlGeneratorTest.php
@@ -260,6 +260,7 @@ public function testBasicRouteGeneration()
         $this->assertSame('http://www.foo.com/foo/bar/taylor/breeze/otwell?wall&woz', $url->route('bar', ['taylor', 'otwell', 'wall', 'woz']));
         $this->assertSame('http://www.foo.com/foo/bar', $url->route('optional'));
         $this->assertSame('http://www.foo.com/foo/bar', $url->route('optional', ['baz' => null]));
+        $this->assertSame('http://www.foo.com/foo/bar', $url->route('optional', ['baz' => '']));
         $this->assertSame('http://www.foo.com/foo/bar/taylor', $url->route('optional', 'taylor'));
         $this->assertSame('http://www.foo.com/foo/bar/taylor', $url->route('optional', ['taylor']));
         $this->assertSame('http://www.foo.com/foo/bar/taylor?breeze', $url->route('optional', ['taylor', 'breeze']));
@@ -501,7 +502,19 @@ public function testRoutesWithDomainsThroughProxy()
         $this->assertSame('http://sub.foo.com/foo/bar', $url->route('foo'));
     }
 
-    public function testUrlGenerationForControllersRequiresPassingOfRequiredParameters()
+    public function providerRouteParameters()
+    {
+        return [
+            [['test' => 123]],
+            [['one' => null, 'test' => 123]],
+            [['one' => '', 'test' => 123]],
+        ];
+    }
+
+    /**
+     * @dataProvider providerRouteParameters
+     */
+    public function testUrlGenerationForControllersRequiresPassingOfRequiredParameters($parameters)
     {
         $this->expectException(UrlGenerationException::class);
 
@@ -515,7 +528,7 @@ public function testUrlGenerationForControllersRequiresPassingOfRequiredParamete
         }]);
         $routes->add($route);
 
-        $this->assertSame('http://www.foo.com:8080/foo?test=123', $url->route('foo', ['test' => 123]));
+        $this->assertSame('http://www.foo.com:8080/foo?test=123', $url->route('foo', $parameters));
     }
 
     public function testForceRootUrl()