[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 keys 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.

[taylorotwell]

Thanks!

[stancl]

Thanks for merging! This had to be a pain to review 😅

[tabuna]

It seems like this PR might be causing an issue with route generation on my end. When I try to run a test with the following code:

public function testRouteSystemsRolesEdit(): void
{
    $response = $this
        ->get(route('platform.systems.roles.edit', 1));
    // ...
}

I get the following error:

Illuminate\Routing\Exceptions\UrlGenerationException: 

  Missing required parameter for the route:
  [Route: platform.systems.roles.edit] 
  [URI: dashboard/roles/{role}/edit/{method?}]
  
  Missing parameter: 'role'.

However, when I revert to version v12.3.0 or revert only this specific commit, everything works as expected and the error doesn’t appear.

It looks like the parameters in the Illuminate\Routing\RouteUrlGenerator::to method are initially:

array:1 [
  0 => 1
]

But after passing through the $this->formatParameters($route, $parameters); method, they change to:

array:2 [
  "role" => ""
  "method" => 1
]

Here's a minimal test example for tests/Routing/RoutingUrlGeneratorTest.php, which you can use to reproduce the issue:

public function testUrlGenerationWithOptionalParameter(): void
{
    $url = new UrlGenerator(
        $routes = new RouteCollection,
        Request::create('https://www.foo.com/')
    );

    $route = new Route(['GET'], 'tenantPost/{post}/{method?}', ['as' => 'tenantPost', fn() => '']);
    $routes->add($route);

    $this->assertSame(
        'https://www.foo.com/tenantPost/1',
        $url->route('tenantPost', 1),
    );
}

[shengslogar]

Also seeing a regression on v12.4; restoring two files (UrlGenerator.php, RouteUrlGenerator.php) from v12.3 solves this so, without reading too closely, fairly certain it stems from this PR:

Route::name('foo')->get('bar/{param1?}/{param2?}')
route('foo', 'baz')

In 12.3 (and other versions of Laravel): http://example.test/bar/baz
In 12.4: http://example.test/bar//baz <- Note the redundant forward slash

[stancl]

Sorry about that and thanks for the reproduction cases. The optional parameters seem like the common denominator here, I missed those in the tests here, it should be an easy fix though. Will have a PR open in a bit.

[stancl]

Managed to reproduce this in tests. Just from observing the routes you suggested, the flaw is in the "reversing" logic where we try to match parameters in reverse order if there are some parameters with default values in front and not enough parameters were supplied to cover all parameters.

In the cases above, the opposite is wanted: required parameters first, followed by optional parameters, with a lower amount of passed parameters than total parameters.

This however creates a scenario where a decision has to be made about Laravel's behavior. Consider a more complex route:

/{defaultParam}/{requiredParam}/{optionalParam?}

If you pass two positional parameters, ['a', 'b'], which of these three should be filled? I'm going to go with ['requiredParam', 'optionalParam'] since in my opinion that's what you're most likely to think of when thinking of the route, since a default would usually be in some group prefix. No perfect solution here but this seems the most reasonable.

[stancl]

After going over some sample scenarios, I think the right approach to take here is not to reverse parameters, but instead use an offset. So it's always left-to-right, just offset by some leading default parameters if they're not specified. Should simplify the implementation too.

Edge cases where default parameters are interleaved by a required parameter can be handled using the count($x) === count($y) loop we have now.

[rodrigopedra]

@stancl here is another related issue, to use as an additional test case:

#55209

[stancl]

Think I got it working, writing detailed tests for optional parameters now 👍

[efc]

Yes, I can confirm that 12.4.1 has fixed the problem I was seeing in Statamic. Thank you @stancl!

[shengslogar]

Also confirming. Thanks for the quick fix, @stancl!!