Skip to content

Commit 2730140

Browse files
stancltaylorotwell
stancl
and
taylorotwell
authored
[12.x] Fix URL generation with optional parameters (regression in #54811) (#55213)
* 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. * Apply StyleCI diff * formatting --------- Co-authored-by: Taylor Otwell <[email protected]>
1 parent f2f55a2 commit 2730140

File tree

3 files changed

+220
-18
lines changed

3 files changed

+220
-18
lines changed

Diff for: ‎src/Illuminate/Routing/Route.php

+2-2
Original file line numberDiff line numberDiff line change
@@ -1321,9 +1321,9 @@ public function toSymfonyRoute()
13211321
/**
13221322
* Get the optional parameter names for the route.
13231323
*
1324-
* @return array
1324+
* @return array<string, null>
13251325
*/
1326-
protected function getOptionalParameterNames()
1326+
public function getOptionalParameterNames()
13271327
{
13281328
preg_match_all('/\{(\w+?)\?\}/', $this->uri(), $matches);
13291329

Diff for: ‎src/Illuminate/Routing/RouteUrlGenerator.php

+57-16
Original file line numberDiff line numberDiff line change
@@ -183,13 +183,12 @@ protected function formatParameters(Route $route, $parameters)
183183
{
184184
$parameters = Arr::wrap($parameters);
185185

186-
$passedParameterCount = count($parameters);
187-
188186
$namedParameters = [];
189187
$namedQueryParameters = [];
190-
$routeParametersWithoutDefaultsOrNamedParameters = [];
188+
$requiredRouteParametersWithoutDefaultsOrNamedParameters = [];
191189

192190
$routeParameters = $route->parameterNames();
191+
$optionalParameters = $route->getOptionalParameterNames();
193192

194193
foreach ($routeParameters as $name) {
195194
if (isset($parameters[$name])) {
@@ -198,9 +197,9 @@ protected function formatParameters(Route $route, $parameters)
198197
unset($parameters[$name]);
199198

200199
continue;
201-
} elseif (! isset($this->defaultParameters[$name])) {
202-
// No named parameter or default value, try to match to positional parameter below...
203-
array_push($routeParametersWithoutDefaultsOrNamedParameters, $name);
200+
} elseif (! isset($this->defaultParameters[$name]) && ! isset($optionalParameters[$name])) {
201+
// No named parameter or default value for a required parameter, try to match to positional parameter below...
202+
array_push($requiredRouteParametersWithoutDefaultsOrNamedParameters, $name);
204203
}
205204

206205
$namedParameters[$name] = '';
@@ -216,8 +215,8 @@ protected function formatParameters(Route $route, $parameters)
216215
}
217216

218217
// Match positional parameters to the route parameters that didn't have a value in order...
219-
if (count($parameters) == count($routeParametersWithoutDefaultsOrNamedParameters)) {
220-
foreach (array_reverse($routeParametersWithoutDefaultsOrNamedParameters) as $name) {
218+
if (count($parameters) == count($requiredRouteParametersWithoutDefaultsOrNamedParameters)) {
219+
foreach (array_reverse($requiredRouteParametersWithoutDefaultsOrNamedParameters) as $name) {
221220
if (count($parameters) === 0) {
222221
break;
223222
}
@@ -226,19 +225,61 @@ protected function formatParameters(Route $route, $parameters)
226225
}
227226
}
228227

229-
// If there are extra parameters, just fill left to right... if not, fill right to left and try to use defaults...
230-
$extraParameters = $passedParameterCount > count($routeParameters);
228+
$offset = 0;
229+
$emptyParameters = array_filter($namedParameters, static fn ($val) => $val === '');
231230

232-
foreach ($extraParameters ? $namedParameters : array_reverse($namedParameters) as $key => $value) {
233-
$bindingField = $route->bindingFieldFor($key);
231+
if (count($requiredRouteParametersWithoutDefaultsOrNamedParameters) !== 0 &&
232+
count($parameters) !== count($emptyParameters)) {
233+
// Find the index of the first required parameter...
234+
$offset = array_search($requiredRouteParametersWithoutDefaultsOrNamedParameters[0], array_keys($namedParameters));
234235

235-
$defaultParameterKey = $bindingField ? "$key:$bindingField" : $key;
236+
// If more empty parameters remain, adjust the offset...
237+
$remaining = count($emptyParameters) - $offset - count($parameters);
238+
239+
if ($remaining < 0) {
240+
// Effectively subtract the remaining count since it's negative...
241+
$offset += $remaining;
242+
}
236243

237-
if ($value !== '') {
244+
// Correct offset if it goes below zero...
245+
if ($offset < 0) {
246+
$offset = 0;
247+
}
248+
} elseif (count($requiredRouteParametersWithoutDefaultsOrNamedParameters) === 0 && count($parameters) !== 0) {
249+
// Handle the case where all passed parameters are for parameters that have default values...
250+
$remainingCount = count($parameters);
251+
252+
// Loop over empty parameters backwards and stop when we run out of passed parameters...
253+
for ($i = count($namedParameters) - 1; $i >= 0; $i--) {
254+
if ($namedParameters[array_keys($namedParameters)[$i]] === '') {
255+
$offset = $i;
256+
$remainingCount--;
257+
258+
if ($remainingCount === 0) {
259+
// If there are no more passed parameters, we stop here...
260+
break;
261+
}
262+
}
263+
}
264+
}
265+
266+
// Starting from the offset, match any passed parameters from left to right...
267+
for ($i = $offset; $i < count($namedParameters); $i++) {
268+
$key = array_keys($namedParameters)[$i];
269+
270+
if ($namedParameters[$key] !== '') {
238271
continue;
239272
} elseif (! empty($parameters)) {
240-
$namedParameters[$key] = $extraParameters ? array_shift($parameters) : array_pop($parameters);
241-
} elseif (isset($this->defaultParameters[$defaultParameterKey])) {
273+
$namedParameters[$key] = array_shift($parameters);
274+
}
275+
}
276+
277+
// Fill leftmost parameters with defaults if the loop above was offset...
278+
foreach ($namedParameters as $key => $value) {
279+
$bindingField = $route->bindingFieldFor($key);
280+
$defaultParameterKey = $bindingField ? "$key:$bindingField" : $key;
281+
282+
if ($value === '' && isset($this->defaultParameters[$defaultParameterKey])) {
242283
$namedParameters[$key] = $this->defaultParameters[$defaultParameterKey];
243284
}
244285
}

Diff for: ‎tests/Routing/RoutingUrlGeneratorTest.php

+161
Original file line numberDiff line numberDiff line change
@@ -1821,6 +1821,167 @@ public function testDefaultsCanBeCombinedWithExtraQueryParameters()
18211821
$url->route('tenantPostUser', ['concretePost', 'extra' => 'query']),
18221822
);
18231823
}
1824+
1825+
public function testUrlGenerationWithOptionalParameters(): void
1826+
{
1827+
$url = new UrlGenerator(
1828+
$routes = new RouteCollection,
1829+
Request::create('https://www.foo.com/')
1830+
);
1831+
1832+
$url->defaults([
1833+
'tenant' => 'defaultTenant',
1834+
'user' => 'defaultUser',
1835+
]);
1836+
1837+
/**
1838+
* Route with one required parameter and one optional parameter.
1839+
*/
1840+
$route = new Route(['GET'], 'postOptionalMethod/{post}/{method?}', ['as' => 'postOptionalMethod', fn () => '']);
1841+
$routes->add($route);
1842+
1843+
$this->assertSame(
1844+
'https://www.foo.com/postOptionalMethod/1',
1845+
$url->route('postOptionalMethod', 1),
1846+
);
1847+
1848+
$this->assertSame(
1849+
'https://www.foo.com/postOptionalMethod/1/2',
1850+
$url->route('postOptionalMethod', [1, 2]),
1851+
);
1852+
1853+
/**
1854+
* Route with two optional parameters.
1855+
*/
1856+
$route = new Route(['GET'], 'optionalPostOptionalMethod/{post}/{method?}', ['as' => 'optionalPostOptionalMethod', fn () => '']);
1857+
$routes->add($route);
1858+
1859+
$this->assertSame(
1860+
'https://www.foo.com/optionalPostOptionalMethod/1',
1861+
$url->route('optionalPostOptionalMethod', 1),
1862+
);
1863+
1864+
$this->assertSame(
1865+
'https://www.foo.com/optionalPostOptionalMethod/1/2',
1866+
$url->route('optionalPostOptionalMethod', [1, 2]),
1867+
);
1868+
1869+
/**
1870+
* Route with one default parameter, one required parameter, and one optional parameter.
1871+
*/
1872+
$route = new Route(['GET'], 'tenantPostOptionalMethod/{tenant}/{post}/{method?}', ['as' => 'tenantPostOptionalMethod', fn () => '']);
1873+
$routes->add($route);
1874+
1875+
// Passing one parameter
1876+
$this->assertSame(
1877+
'https://www.foo.com/tenantPostOptionalMethod/defaultTenant/concretePost',
1878+
$url->route('tenantPostOptionalMethod', ['concretePost']),
1879+
);
1880+
1881+
// Passing two parameters: optional parameter is prioritized over parameter with a default value
1882+
$this->assertSame(
1883+
'https://www.foo.com/tenantPostOptionalMethod/defaultTenant/concretePost/concreteMethod',
1884+
$url->route('tenantPostOptionalMethod', ['concretePost', 'concreteMethod']),
1885+
);
1886+
1887+
// Passing all three parameters
1888+
$this->assertSame(
1889+
'https://www.foo.com/tenantPostOptionalMethod/concreteTenant/concretePost/concreteMethod',
1890+
$url->route('tenantPostOptionalMethod', ['concreteTenant', 'concretePost', 'concreteMethod']),
1891+
);
1892+
1893+
/**
1894+
* Route with two default parameters, one required parameter, and one optional parameter.
1895+
*/
1896+
$route = new Route(['GET'], 'tenantUserPostOptionalMethod/{tenant}/{user}/{post}/{method?}', ['as' => 'tenantUserPostOptionalMethod', fn () => '']);
1897+
$routes->add($route);
1898+
1899+
// Passing one parameter
1900+
$this->assertSame(
1901+
'https://www.foo.com/tenantUserPostOptionalMethod/defaultTenant/defaultUser/concretePost',
1902+
$url->route('tenantUserPostOptionalMethod', ['concretePost']),
1903+
);
1904+
1905+
// Passing two parameters: optional parameter is prioritized over parameters with default values
1906+
$this->assertSame(
1907+
'https://www.foo.com/tenantUserPostOptionalMethod/defaultTenant/defaultUser/concretePost/concreteMethod',
1908+
$url->route('tenantUserPostOptionalMethod', ['concretePost', 'concreteMethod']),
1909+
);
1910+
1911+
// Passing three parameters: only the leftmost parameter with a default value uses its default value
1912+
$this->assertSame(
1913+
'https://www.foo.com/tenantUserPostOptionalMethod/defaultTenant/concreteUser/concretePost/concreteMethod',
1914+
$url->route('tenantUserPostOptionalMethod', ['concreteUser', 'concretePost', 'concreteMethod']),
1915+
);
1916+
1917+
// Same as the assertion above, but using some named parameters
1918+
$this->assertSame(
1919+
'https://www.foo.com/tenantUserPostOptionalMethod/defaultTenant/concreteUser/concretePost/concreteMethod',
1920+
$url->route('tenantUserPostOptionalMethod', ['user' => 'concreteUser', 'concretePost', 'concreteMethod']),
1921+
);
1922+
1923+
// Also using a named parameter, but this time for the post parameter
1924+
$this->assertSame(
1925+
'https://www.foo.com/tenantUserPostOptionalMethod/defaultTenant/concreteUser/concretePost/concreteMethod',
1926+
$url->route('tenantUserPostOptionalMethod', ['concreteUser', 'post' => 'concretePost', 'concreteMethod']),
1927+
);
1928+
1929+
// Also using a named parameter, but this time for the optional method parameter
1930+
$this->assertSame(
1931+
'https://www.foo.com/tenantUserPostOptionalMethod/defaultTenant/concreteUser/concretePost/concreteMethod',
1932+
$url->route('tenantUserPostOptionalMethod', ['concreteUser', 'concretePost', 'method' => 'concreteMethod']),
1933+
);
1934+
1935+
// Passing all four parameters
1936+
$this->assertSame(
1937+
'https://www.foo.com/tenantUserPostOptionalMethod/concreteTenant/concreteUser/concretePost/concreteMethod',
1938+
$url->route('tenantUserPostOptionalMethod', ['concreteTenant', 'concreteUser', 'concretePost', 'concreteMethod']),
1939+
);
1940+
1941+
/**
1942+
* Route with a default parameter, a required parameter, another default parameter, and finally an optional parameter.
1943+
*
1944+
* This tests interleaved default parameters when combined with optional parameters.
1945+
*/
1946+
$route = new Route(['GET'], 'tenantPostUserOptionalMethod/{tenant}/{post}/{user}/{method?}', ['as' => 'tenantPostUserOptionalMethod', fn () => '']);
1947+
$routes->add($route);
1948+
1949+
// Passing one parameter
1950+
$this->assertSame(
1951+
'https://www.foo.com/tenantPostUserOptionalMethod/defaultTenant/concretePost/defaultUser',
1952+
$url->route('tenantPostUserOptionalMethod', ['concretePost']),
1953+
);
1954+
1955+
// Passing two parameters: optional parameter is prioritized over parameters with default values
1956+
$this->assertSame(
1957+
'https://www.foo.com/tenantPostUserOptionalMethod/defaultTenant/concretePost/defaultUser/concreteMethod',
1958+
$url->route('tenantPostUserOptionalMethod', ['concretePost', 'concreteMethod']),
1959+
);
1960+
1961+
// Same as the assertion above, but using some named parameters
1962+
$this->assertSame(
1963+
'https://www.foo.com/tenantPostUserOptionalMethod/defaultTenant/concretePost/defaultUser/concreteMethod',
1964+
$url->route('tenantPostUserOptionalMethod', ['post' => 'concretePost', 'concreteMethod']),
1965+
);
1966+
1967+
// Also using a named parameter, but this time for the optional parameter
1968+
$this->assertSame(
1969+
'https://www.foo.com/tenantPostUserOptionalMethod/defaultTenant/concretePost/defaultUser/concreteMethod',
1970+
$url->route('tenantPostUserOptionalMethod', ['concretePost', 'method' => 'concreteMethod']),
1971+
);
1972+
1973+
// Passing three parameters: only the leftmost parameter with a default value uses its default value
1974+
$this->assertSame(
1975+
'https://www.foo.com/tenantPostUserOptionalMethod/defaultTenant/concretePost/concreteUser/concreteMethod',
1976+
$url->route('tenantPostUserOptionalMethod', ['concretePost', 'concreteUser', 'concreteMethod']),
1977+
);
1978+
1979+
// Passing all four parameters
1980+
$this->assertSame(
1981+
'https://www.foo.com/tenantPostUserOptionalMethod/concreteTenant/concretePost/concreteUser/concreteMethod',
1982+
$url->route('tenantPostUserOptionalMethod', ['concreteTenant', 'concretePost', 'concreteUser', 'concreteMethod']),
1983+
);
1984+
}
18241985
}
18251986

18261987
class RoutableInterfaceStub implements UrlRoutable

0 commit comments

Comments
 (0)