From 928d3500ecd2cbb41cee402359fa4baf3e8fd5aa Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Samuel=20=C5=A0tancl?= Date: Sun, 30 Mar 2025 16:44:52 +0200 Subject: [PATCH 1/3] Fix URL generation with optional parameters (regression in #54811) This reworks the logic from conditionally reversing passed parameters to instead computing an offset and filling parameters left-to-right from there. --- src/Illuminate/Routing/Route.php | 4 +- src/Illuminate/Routing/RouteUrlGenerator.php | 74 +++++++-- tests/Routing/RoutingUrlGeneratorTest.php | 161 +++++++++++++++++++ 3 files changed, 221 insertions(+), 18 deletions(-) diff --git a/src/Illuminate/Routing/Route.php b/src/Illuminate/Routing/Route.php index 22008aa091de..7dd19b552ddc 100755 --- a/src/Illuminate/Routing/Route.php +++ b/src/Illuminate/Routing/Route.php @@ -1321,9 +1321,9 @@ public function toSymfonyRoute() /** * Get the optional parameter names for the route. * - * @return array + * @return array */ - protected function getOptionalParameterNames() + public function getOptionalParameterNames() { preg_match_all('/\{(\w+?)\?\}/', $this->uri(), $matches); diff --git a/src/Illuminate/Routing/RouteUrlGenerator.php b/src/Illuminate/Routing/RouteUrlGenerator.php index c82324363418..4402d203c518 100644 --- a/src/Illuminate/Routing/RouteUrlGenerator.php +++ b/src/Illuminate/Routing/RouteUrlGenerator.php @@ -183,13 +183,12 @@ protected function formatParameters(Route $route, $parameters) { $parameters = Arr::wrap($parameters); - $passedParameterCount = count($parameters); - $namedParameters = []; $namedQueryParameters = []; - $routeParametersWithoutDefaultsOrNamedParameters = []; + $requiredRouteParametersWithoutDefaultsOrNamedParameters = []; $routeParameters = $route->parameterNames(); + $optionalParameters = $route->getOptionalParameterNames(); foreach ($routeParameters as $name) { if (isset($parameters[$name])) { @@ -198,9 +197,9 @@ protected function formatParameters(Route $route, $parameters) unset($parameters[$name]); continue; - } elseif (! isset($this->defaultParameters[$name])) { - // No named parameter or default value, try to match to positional parameter below... - array_push($routeParametersWithoutDefaultsOrNamedParameters, $name); + } elseif (! isset($this->defaultParameters[$name]) && ! isset($optionalParameters[$name])) { + // No named parameter or default value for a required parameter, try to match to positional parameter below... + array_push($requiredRouteParametersWithoutDefaultsOrNamedParameters, $name); } $namedParameters[$name] = ''; @@ -216,8 +215,8 @@ protected function formatParameters(Route $route, $parameters) } // Match positional parameters to the route parameters that didn't have a value in order... - if (count($parameters) == count($routeParametersWithoutDefaultsOrNamedParameters)) { - foreach (array_reverse($routeParametersWithoutDefaultsOrNamedParameters) as $name) { + if (count($parameters) == count($requiredRouteParametersWithoutDefaultsOrNamedParameters)) { + foreach (array_reverse($requiredRouteParametersWithoutDefaultsOrNamedParameters) as $name) { if (count($parameters) === 0) { break; } @@ -226,19 +225,62 @@ protected function formatParameters(Route $route, $parameters) } } - // If there are extra parameters, just fill left to right... if not, fill right to left and try to use defaults... - $extraParameters = $passedParameterCount > count($routeParameters); + $offset = 0; + $emptyParameters = array_filter($namedParameters, static fn ($val) => $val === ''); + if ( + count($requiredRouteParametersWithoutDefaultsOrNamedParameters) !== 0 && + count($parameters) !== count($emptyParameters) + ) { + // Find the index of the first required parameter... + $offset = array_search($requiredRouteParametersWithoutDefaultsOrNamedParameters[0], array_keys($namedParameters)); + + // If more empty parameters remain, adjust the offset... + $remaining = count($emptyParameters) - $offset - count($parameters); + + if ($remaining < 0) { + // Effectively subtract the remaining count since it's negative + $offset += $remaining; + } - foreach ($extraParameters ? $namedParameters : array_reverse($namedParameters) as $key => $value) { - $bindingField = $route->bindingFieldFor($key); + // Correct offset if it goes below zero... + if ($offset < 0) { + $offset = 0; + } + } else if (count($requiredRouteParametersWithoutDefaultsOrNamedParameters) === 0 && count($parameters) !== 0) { + // Handle the case where all passed parameters are for parameters that have default values... + $remainingCount = count($parameters); + + // Loop over empty parameters backwards and stop when we run out of passed parameters... + for ($i = count($namedParameters) - 1; $i >= 0; $i--) { + if ($namedParameters[array_keys($namedParameters)[$i]] === '') { + $offset = $i; + $remainingCount--; + + if ($remainingCount === 0) { + // If there are no more passed parameters, we stop here + break; + } + } + } + } - $defaultParameterKey = $bindingField ? "$key:$bindingField" : $key; + // Starting from the offset, match any passed parameters from left to right... + for ($i = $offset; $i < count($namedParameters); $i++) { + $key = array_keys($namedParameters)[$i]; - if ($value !== '') { + if ($namedParameters[$key] !== '') { continue; } elseif (! empty($parameters)) { - $namedParameters[$key] = $extraParameters ? array_shift($parameters) : array_pop($parameters); - } elseif (isset($this->defaultParameters[$defaultParameterKey])) { + $namedParameters[$key] = array_shift($parameters); + } + } + + // Fill leftmost parameters with defaults if the loop above was offset... + foreach ($namedParameters as $key => $value) { + $bindingField = $route->bindingFieldFor($key); + $defaultParameterKey = $bindingField ? "$key:$bindingField" : $key; + + if ($value === '' && isset($this->defaultParameters[$defaultParameterKey])) { $namedParameters[$key] = $this->defaultParameters[$defaultParameterKey]; } } diff --git a/tests/Routing/RoutingUrlGeneratorTest.php b/tests/Routing/RoutingUrlGeneratorTest.php index 28bb67a49423..bfc0ac69e412 100755 --- a/tests/Routing/RoutingUrlGeneratorTest.php +++ b/tests/Routing/RoutingUrlGeneratorTest.php @@ -1821,6 +1821,167 @@ public function testDefaultsCanBeCombinedWithExtraQueryParameters() $url->route('tenantPostUser', ['concretePost', 'extra' => 'query']), ); } + + public function testUrlGenerationWithOptionalParameters(): void + { + $url = new UrlGenerator( + $routes = new RouteCollection, + Request::create('https://www.foo.com/') + ); + + $url->defaults([ + 'tenant' => 'defaultTenant', + 'user' => 'defaultUser', + ]); + + /** + * Route with one required parameter and one optional parameter. + */ + $route = new Route(['GET'], 'postOptionalMethod/{post}/{method?}', ['as' => 'postOptionalMethod', fn() => '']); + $routes->add($route); + + $this->assertSame( + 'https://www.foo.com/postOptionalMethod/1', + $url->route('postOptionalMethod', 1), + ); + + $this->assertSame( + 'https://www.foo.com/postOptionalMethod/1/2', + $url->route('postOptionalMethod', [1, 2]), + ); + + /** + * Route with two optional parameters. + */ + $route = new Route(['GET'], 'optionalPostOptionalMethod/{post}/{method?}', ['as' => 'optionalPostOptionalMethod', fn() => '']); + $routes->add($route); + + $this->assertSame( + 'https://www.foo.com/optionalPostOptionalMethod/1', + $url->route('optionalPostOptionalMethod', 1), + ); + + $this->assertSame( + 'https://www.foo.com/optionalPostOptionalMethod/1/2', + $url->route('optionalPostOptionalMethod', [1, 2]), + ); + + /** + * Route with one default parameter, one required parameter, and one optional parameter. + */ + $route = new Route(['GET'], 'tenantPostOptionalMethod/{tenant}/{post}/{method?}', ['as' => 'tenantPostOptionalMethod', fn() => '']); + $routes->add($route); + + // Passing one parameter + $this->assertSame( + 'https://www.foo.com/tenantPostOptionalMethod/defaultTenant/concretePost', + $url->route('tenantPostOptionalMethod', ['concretePost']), + ); + + // Passing two parameters: optional parameter is prioritized over parameter with a default value + $this->assertSame( + 'https://www.foo.com/tenantPostOptionalMethod/defaultTenant/concretePost/concreteMethod', + $url->route('tenantPostOptionalMethod', ['concretePost', 'concreteMethod']), + ); + + // Passing all three parameters + $this->assertSame( + 'https://www.foo.com/tenantPostOptionalMethod/concreteTenant/concretePost/concreteMethod', + $url->route('tenantPostOptionalMethod', ['concreteTenant', 'concretePost', 'concreteMethod']), + ); + + /** + * Route with two default parameters, one required parameter, and one optional parameter. + */ + $route = new Route(['GET'], 'tenantUserPostOptionalMethod/{tenant}/{user}/{post}/{method?}', ['as' => 'tenantUserPostOptionalMethod', fn() => '']); + $routes->add($route); + + // Passing one parameter + $this->assertSame( + 'https://www.foo.com/tenantUserPostOptionalMethod/defaultTenant/defaultUser/concretePost', + $url->route('tenantUserPostOptionalMethod', ['concretePost']), + ); + + // Passing two parameters: optional parameter is prioritized over parameters with default values + $this->assertSame( + 'https://www.foo.com/tenantUserPostOptionalMethod/defaultTenant/defaultUser/concretePost/concreteMethod', + $url->route('tenantUserPostOptionalMethod', ['concretePost', 'concreteMethod']), + ); + + // Passing three parameters: only the leftmost parameter with a default value uses its default value + $this->assertSame( + 'https://www.foo.com/tenantUserPostOptionalMethod/defaultTenant/concreteUser/concretePost/concreteMethod', + $url->route('tenantUserPostOptionalMethod', ['concreteUser', 'concretePost', 'concreteMethod']), + ); + + // Same as the assertion above, but using some named parameters + $this->assertSame( + 'https://www.foo.com/tenantUserPostOptionalMethod/defaultTenant/concreteUser/concretePost/concreteMethod', + $url->route('tenantUserPostOptionalMethod', ['user' => 'concreteUser', 'concretePost', 'concreteMethod']), + ); + + // Also using a named parameter, but this time for the post parameter + $this->assertSame( + 'https://www.foo.com/tenantUserPostOptionalMethod/defaultTenant/concreteUser/concretePost/concreteMethod', + $url->route('tenantUserPostOptionalMethod', ['concreteUser', 'post' => 'concretePost', 'concreteMethod']), + ); + + // Also using a named parameter, but this time for the optional method parameter + $this->assertSame( + 'https://www.foo.com/tenantUserPostOptionalMethod/defaultTenant/concreteUser/concretePost/concreteMethod', + $url->route('tenantUserPostOptionalMethod', ['concreteUser', 'concretePost', 'method' => 'concreteMethod']), + ); + + // Passing all four parameters + $this->assertSame( + 'https://www.foo.com/tenantUserPostOptionalMethod/concreteTenant/concreteUser/concretePost/concreteMethod', + $url->route('tenantUserPostOptionalMethod', ['concreteTenant', 'concreteUser', 'concretePost', 'concreteMethod']), + ); + + /** + * Route with a default parameter, a required parameter, another default parameter, and finally an optional parameter. + * + * This tests interleaved default parameters when combined with optional parameters. + */ + $route = new Route(['GET'], 'tenantPostUserOptionalMethod/{tenant}/{post}/{user}/{method?}', ['as' => 'tenantPostUserOptionalMethod', fn() => '']); + $routes->add($route); + + // Passing one parameter + $this->assertSame( + 'https://www.foo.com/tenantPostUserOptionalMethod/defaultTenant/concretePost/defaultUser', + $url->route('tenantPostUserOptionalMethod', ['concretePost']), + ); + + // Passing two parameters: optional parameter is prioritized over parameters with default values + $this->assertSame( + 'https://www.foo.com/tenantPostUserOptionalMethod/defaultTenant/concretePost/defaultUser/concreteMethod', + $url->route('tenantPostUserOptionalMethod', ['concretePost', 'concreteMethod']), + ); + + // Same as the assertion above, but using some named parameters + $this->assertSame( + 'https://www.foo.com/tenantPostUserOptionalMethod/defaultTenant/concretePost/defaultUser/concreteMethod', + $url->route('tenantPostUserOptionalMethod', ['post' => 'concretePost', 'concreteMethod']), + ); + + // Also using a named parameter, but this time for the optional parameter + $this->assertSame( + 'https://www.foo.com/tenantPostUserOptionalMethod/defaultTenant/concretePost/defaultUser/concreteMethod', + $url->route('tenantPostUserOptionalMethod', ['concretePost', 'method' => 'concreteMethod']), + ); + + // Passing three parameters: only the leftmost parameter with a default value uses its default value + $this->assertSame( + 'https://www.foo.com/tenantPostUserOptionalMethod/defaultTenant/concretePost/concreteUser/concreteMethod', + $url->route('tenantPostUserOptionalMethod', ['concretePost', 'concreteUser', 'concreteMethod']), + ); + + // Passing all four parameters + $this->assertSame( + 'https://www.foo.com/tenantPostUserOptionalMethod/concreteTenant/concretePost/concreteUser/concreteMethod', + $url->route('tenantPostUserOptionalMethod', ['concreteTenant', 'concretePost', 'concreteUser', 'concreteMethod']), + ); + } } class RoutableInterfaceStub implements UrlRoutable From dd33ae14eabd874b20c54e1c52fc629dc06f54d2 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Samuel=20=C5=A0tancl?= Date: Sun, 30 Mar 2025 16:52:42 +0200 Subject: [PATCH 2/3] Apply StyleCI diff --- src/Illuminate/Routing/RouteUrlGenerator.php | 2 +- tests/Routing/RoutingUrlGeneratorTest.php | 10 +++++----- 2 files changed, 6 insertions(+), 6 deletions(-) diff --git a/src/Illuminate/Routing/RouteUrlGenerator.php b/src/Illuminate/Routing/RouteUrlGenerator.php index 4402d203c518..049d24738802 100644 --- a/src/Illuminate/Routing/RouteUrlGenerator.php +++ b/src/Illuminate/Routing/RouteUrlGenerator.php @@ -246,7 +246,7 @@ protected function formatParameters(Route $route, $parameters) if ($offset < 0) { $offset = 0; } - } else if (count($requiredRouteParametersWithoutDefaultsOrNamedParameters) === 0 && count($parameters) !== 0) { + } elseif (count($requiredRouteParametersWithoutDefaultsOrNamedParameters) === 0 && count($parameters) !== 0) { // Handle the case where all passed parameters are for parameters that have default values... $remainingCount = count($parameters); diff --git a/tests/Routing/RoutingUrlGeneratorTest.php b/tests/Routing/RoutingUrlGeneratorTest.php index bfc0ac69e412..f03e4b10bb8c 100755 --- a/tests/Routing/RoutingUrlGeneratorTest.php +++ b/tests/Routing/RoutingUrlGeneratorTest.php @@ -1837,7 +1837,7 @@ public function testUrlGenerationWithOptionalParameters(): void /** * Route with one required parameter and one optional parameter. */ - $route = new Route(['GET'], 'postOptionalMethod/{post}/{method?}', ['as' => 'postOptionalMethod', fn() => '']); + $route = new Route(['GET'], 'postOptionalMethod/{post}/{method?}', ['as' => 'postOptionalMethod', fn () => '']); $routes->add($route); $this->assertSame( @@ -1853,7 +1853,7 @@ public function testUrlGenerationWithOptionalParameters(): void /** * Route with two optional parameters. */ - $route = new Route(['GET'], 'optionalPostOptionalMethod/{post}/{method?}', ['as' => 'optionalPostOptionalMethod', fn() => '']); + $route = new Route(['GET'], 'optionalPostOptionalMethod/{post}/{method?}', ['as' => 'optionalPostOptionalMethod', fn () => '']); $routes->add($route); $this->assertSame( @@ -1869,7 +1869,7 @@ public function testUrlGenerationWithOptionalParameters(): void /** * Route with one default parameter, one required parameter, and one optional parameter. */ - $route = new Route(['GET'], 'tenantPostOptionalMethod/{tenant}/{post}/{method?}', ['as' => 'tenantPostOptionalMethod', fn() => '']); + $route = new Route(['GET'], 'tenantPostOptionalMethod/{tenant}/{post}/{method?}', ['as' => 'tenantPostOptionalMethod', fn () => '']); $routes->add($route); // Passing one parameter @@ -1893,7 +1893,7 @@ public function testUrlGenerationWithOptionalParameters(): void /** * Route with two default parameters, one required parameter, and one optional parameter. */ - $route = new Route(['GET'], 'tenantUserPostOptionalMethod/{tenant}/{user}/{post}/{method?}', ['as' => 'tenantUserPostOptionalMethod', fn() => '']); + $route = new Route(['GET'], 'tenantUserPostOptionalMethod/{tenant}/{user}/{post}/{method?}', ['as' => 'tenantUserPostOptionalMethod', fn () => '']); $routes->add($route); // Passing one parameter @@ -1943,7 +1943,7 @@ public function testUrlGenerationWithOptionalParameters(): void * * This tests interleaved default parameters when combined with optional parameters. */ - $route = new Route(['GET'], 'tenantPostUserOptionalMethod/{tenant}/{post}/{user}/{method?}', ['as' => 'tenantPostUserOptionalMethod', fn() => '']); + $route = new Route(['GET'], 'tenantPostUserOptionalMethod/{tenant}/{post}/{user}/{method?}', ['as' => 'tenantPostUserOptionalMethod', fn () => '']); $routes->add($route); // Passing one parameter From 1a070e0cad922a66f452a4514d021c0eae22c558 Mon Sep 17 00:00:00 2001 From: Taylor Otwell Date: Sun, 30 Mar 2025 11:23:44 -0500 Subject: [PATCH 3/3] formatting --- src/Illuminate/Routing/RouteUrlGenerator.php | 11 +++++------ 1 file changed, 5 insertions(+), 6 deletions(-) diff --git a/src/Illuminate/Routing/RouteUrlGenerator.php b/src/Illuminate/Routing/RouteUrlGenerator.php index 049d24738802..cd23162c015f 100644 --- a/src/Illuminate/Routing/RouteUrlGenerator.php +++ b/src/Illuminate/Routing/RouteUrlGenerator.php @@ -227,10 +227,9 @@ protected function formatParameters(Route $route, $parameters) $offset = 0; $emptyParameters = array_filter($namedParameters, static fn ($val) => $val === ''); - if ( - count($requiredRouteParametersWithoutDefaultsOrNamedParameters) !== 0 && - count($parameters) !== count($emptyParameters) - ) { + + if (count($requiredRouteParametersWithoutDefaultsOrNamedParameters) !== 0 && + count($parameters) !== count($emptyParameters)) { // Find the index of the first required parameter... $offset = array_search($requiredRouteParametersWithoutDefaultsOrNamedParameters[0], array_keys($namedParameters)); @@ -238,7 +237,7 @@ protected function formatParameters(Route $route, $parameters) $remaining = count($emptyParameters) - $offset - count($parameters); if ($remaining < 0) { - // Effectively subtract the remaining count since it's negative + // Effectively subtract the remaining count since it's negative... $offset += $remaining; } @@ -257,7 +256,7 @@ protected function formatParameters(Route $route, $parameters) $remainingCount--; if ($remainingCount === 0) { - // If there are no more passed parameters, we stop here + // If there are no more passed parameters, we stop here... break; } }