[12.x] Various URL generation bugfixes

[stancl]

Fixes #54796.

This PR addresses several issues in the route URL generator by adding a mechanism for turning [$positional, $parameters] into ['named' => $parameters]. The logic is a little complex with some notable edge cases so I've included an extensive number of assertions (that's the bulk of the diff) testing all edge case scenarios I could think of.

I realize the added method for mapping positional parameters to their respective names is quite complex and may seem error-prone, but while working on this I've become confident this is the right approach here. There are many assumptions made in the URL generation logic about parameters being associative arrays, despite the API not requiring it. Code along the lines of route('profile', $user) is very common, so complex cases should be handled correctly as well.

While originally reporting #54796 I've noticed a bunch of other bugs, all of which are perfectly solved by turning positional parameters into named parameters. On top of that, there was a TODO for a past bug, marking a test as skipped (I've confirmed it fails on 12.x) which passes with my changes:

framework/tests/Routing/RoutingUrlGeneratorTest.php

Lines 431 to 456 in 5645879

	/**
	* @todo Fix bug related to route keys
	*
	* @link https://github.com/laravel/framework/pull/42425
	*/
	public function testRoutableInterfaceRoutingWithSeparateBindingFieldOnlyForSecondParameter()
	{
	$this->markTestSkipped('See https://github.com/laravel/framework/pull/43255');
	$url = new UrlGenerator(
	$routes = new RouteCollection,
	Request::create('http://www.foo.com/')
	);
	$route = new Route(['GET'], 'foo/{bar}/{baz:slug}', ['as' => 'routable']);
	$routes->add($route);
	$model1 = new RoutableInterfaceStub;
	$model1->key = 'routable-1';
	$model2 = new RoutableInterfaceStub;
	$model2->key = 'routable-2';
	$this->assertSame('/foo/routable-1/test-slug', $url->route('routable', ['bar' => $model1, 'baz' => $model2], false));
	$this->assertSame('/foo/routable-1/test-slug', $url->route('routable', [$model1, $model2], false));
	}

Implementation details

The added formatParameters() method essentially works like this:

1. Loop over route parameters, move any named route parameters to $namedParameters and track parameters that don't have a default value OR a named parameter (i.e. required parameters)
2. Loop over $parameters (modified above) and move any remaining parameters with string values into $namedQueryParameters as these parameters do not exist on the route
3. Special case: If the count of remaining parameters exactly matches the count of "required parameters", fill these with priority (in reverse order). This covers cases where you may have a default parameter after a required parameter. This is atypical but Laravel currently supports it (to some extent) so this small section just makes the support more robust.
4. The main logic: for remaining passed parameters (all of which are positional by now) match them to route parameters:
   • If the count of ALL passed parameters is less than the number of route parameters, we match them in reverse order, expecting defaults to cover the remaining leftmost parameters. The step above handles scenarios where that is not the case.
   • If the count of ALL passed parameters is greater (technically >= would work but the current implementation reads better) than the total number of route parameters, we know the parameter list may include positional query strings (?foo) so we can't do the reverse mapping. On the other hand, we for sure have enough parameters to cover all route parameters, so we just match them in normal order.

This PR tries to centralize logic related to handling URL generation to routes in the RouteUrlGenerator, while not moving too much stuff around to minimize breaking changes. The logic may seem quite long and complex, but this way issues are prevented upfront rather than each method in this class having to worry about all possible edge cases.

The code likely requires some cleanup, maybe some variables renamed, comments aren't perfectly aligned etc, but it should be good enough for a review now.

Defaults overriding passed parameters

If URL::defaults() is used, and parameters are passed "positionally" rather than with names, this issue occurs:

Route::get('/test/{foo}/{user}', fn () => 'foo')->name('abc');
URL::defaults(['foo' => 'bar']);

$user = User::first();

// http://foo.com/test/bar/baz?1
dd(route('abc', ['baz', $user]));

// http://foo.com/test/baz/1
dd(route('abc', ['foo' => 'baz', 'user' => $user]));

Using the default value for the {user} parameter instead of the provided one and pushing the user parameter to the query string.

This is because methods such as:

protected function replaceNamedParameters($path, &$parameters)
{
    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];
    }, $path);
}

Expect named parameters. Here the first check fails because the parameters are passed as [0] and [1], so it uses the second branch — the default — for the parameter, turning the passed positional parameters into query string parameters.

Binding fields breaking defaults

Route::get('/{foo:slug}/example/{bar}', ...)->name('example');
URL::defaults(['foo' => 'some_value']);

// Missing required parameter
route('example', 'baz');

// Produces /some_value/baz
route('example', ['bar' => 'baz']);

The issue comes from UrlGenerator::toRoute():

public function toRoute($route, $parameters, $absolute)
{
    // $parameters = [$bar]
    $parameters = Collection::wrap($parameters)->map(function ($value, $key) use ($route) {
        // $value = $bar, $key = 0
        //     true since it's a model       && true because field '0' (foo:slug) has a binding field (slug)
        return $value instanceof UrlRoutable && $route->bindingFieldFor($key)
                ? $value->{$route->bindingFieldFor($key)}
                : $value;
    })->all();

    // $parameters = [null because we've incorrectly done $bar->slug]

    array_walk_recursive($parameters, function (&$item) {
        if ($item instanceof BackedEnum) {
            $item = $item->value;
        }
    });

    return $this->routeUrl()->to(
        $route, $this->formatParameters($parameters), $absolute
    );
}

The default parameter only gets used in $this->routeUrl() (RouteUrlGenerator), but by that point the UrlGenerator has broken the provided parameters, passing a [null] array.

This PR fixes this issue by converting positional arguments into named ones. That at first look fixes the bug described here but there's actually a second bug — we actually don't want to use the default value for 'foo' because we are using a {foo:slug} parameter. Those can have different values — that's the point of binding fields, you don't override getRouteKeyName(), you can set this on a per-route basis.

The next section addresses this.

URL::defaults() support for binding fields

This PR adds support for:

URL::defaults(['tenant:slug' => $tenant->slug]);

This is done in the same method that handles converting positional arguments to named parameters. In other words, in the RouteUrlGenerator. That means this won't work in e.g. replaceRootParameters() but honestly I'm not sure if that'd be worth adding even more complexity to this PR.

Resolve existing bug (stop skipping a test)

The test mentioned above:

framework/tests/Routing/RoutingUrlGeneratorTest.php

Lines 431 to 456 in 5645879

	/**
	* @todo Fix bug related to route keys
	*
	* @link https://github.com/laravel/framework/pull/42425
	*/
	public function testRoutableInterfaceRoutingWithSeparateBindingFieldOnlyForSecondParameter()
	{
	$this->markTestSkipped('See https://github.com/laravel/framework/pull/43255');
	$url = new UrlGenerator(
	$routes = new RouteCollection,
	Request::create('http://www.foo.com/')
	);
	$route = new Route(['GET'], 'foo/{bar}/{baz:slug}', ['as' => 'routable']);
	$routes->add($route);
	$model1 = new RoutableInterfaceStub;
	$model1->key = 'routable-1';
	$model2 = new RoutableInterfaceStub;
	$model2->key = 'routable-2';
	$this->assertSame('/foo/routable-1/test-slug', $url->route('routable', ['bar' => $model1, 'baz' => $model2], false));
	$this->assertSame('/foo/routable-1/test-slug', $url->route('routable', [$model1, $model2], false));
	}

Now passes since it's essentially a subset of what I'm testing in the added tests. Notable difference is that it doesn't deal with defaults() at all. I'll also note that I'm taking a very different approach to solving the bug in the test than the original approaches (to my understanding) and only noticed that the bug is fixed after trying to un-skip the test. This makes me think converting positional parameters to named ones really is the right solution to all these seemingly unrelated bugs in the URL generation logic.

Related PRs: #42942 #43255. Any feedback from @driesvints or @ksassnowski would be greatly appreciated.

---

Finally I'll say that I do know this is a big PR, however it's:

• less than say 50 LOC of actual logical code
• ~90% added tests with 80+ assertions

I came up with the mechanism by going over all the edge cases I could think of and tweaking the code until it seemed to work right. Then I added tests for those edge cases and all were passing.

So while this is a PR adding complex logic to an area of the codebase that was bug-prone in previous PRs (see above), I'm fairly confident in this approach and don't think it should cause any issues. If it does cause some issues in edge cases I didn't think of, hopefully it can be addressed by just updating the logic in the method I added, rather than any large scale changes or reverts.

[github-actions]

Thanks for submitting a PR!

Note that draft PR's are not reviewed. If you would like a review, please mark your pull request as ready for review in the GitHub user interface.

Pull requests that are abandoned in draft may be closed due to inactivity.

[shaedrich]

You could use $passedParameterCount here

Suggested change

	if (count($parameters) == count($routeParametersWithoutDefaultsOrNamedParameters)) {
	if ($passedParameterCount == count($routeParametersWithoutDefaultsOrNamedParameters)) {

[stancl]

In this case it's using the count of $parameters after some values are removed. It isn't $passedParameterCount and switching to that makes tests fail.

[shaedrich]

Oh, okay—thanks for the explanation 👍🏻 Nevermind

[stancl]

It's why I back up the original count of the passed parameters. It might help to rename $parameters to something like $remainingParameters or $positionalParameters later to make this more clear.

[shaedrich]

Can we define this a little more precise?

[stancl]

We cannot. URL generator:

/**
 * Get the URL for a given route instance.
 *
 * @param  \Illuminate\Routing\Route  $route
 * @param  mixed  $parameters
 * @param  bool  $absolute
 * @return string
 *
 * @throws \Illuminate\Routing\Exceptions\UrlGenerationException
 */
public function toRoute($route, $parameters, $absolute)
{
    return $this->routeUrl()->to(
        $route, $parameters, $absolute
    );
}

$parameters are mixed. They're passed without any transformations to routeUrl()->to(), which is:

/**
 * Generate a URL for the given route.
 *
 * @param  \Illuminate\Routing\Route  $route
 * @param  array  $parameters
 * @param  bool  $absolute
 * @return string
 *
 * @throws \Illuminate\Routing\Exceptions\UrlGenerationException
 */
public function to($route, $parameters = [], $absolute = false)
{
    $parameters = $this->formatParameters($route, $parameters);
    $domain = $this->getRouteDomain($route, $parameters);

Here the $parameters docblock technically says array is required but in practice mixed is passed from the URL generator.

[shaedrich]

I didn't mean the $parameters param but the formatParams() return value. Sorry for the confusion