[9.x] Use model route key when route parameter does not specifiy custom binding field but a different parameter does

[ksassnowski]

Note: I'm unsure to which branch I should send this bugfix since the implementation of RouteUri has changed between 8.x and 9.x. 9.x uses str_contains instead of strpos. I might have to send two separate PRs to each respective branch? Help? 😅

Summary

This PR fixes a special circumstance where the wrong model route key would get used when substituting route parameters. For a tl;dr of what the bug is, check out the test I added to RoutingUrlGeneratorTest.php that reproduces the error. For the longer explanation, keep reading.

Reproducing the bug

Let's say I have two models, User and Post. The User model overrides the getRouteKeyName method so the user's username field gets used during route generation instead of the id. The Post model is kept vanilla.

class User extends Model
{
    public function getRouteKeyName()
    {
        return 'username';
    } 
}

class Post extends Model
{
}

Now, let's define a route like this.

Route::get('/{user}/posts/{post:id}', UserPostsController::class)
    ->name('users.posts.show');

We specify an explicit binding field for the post so it gets scoped on the user. For the user, we don't specify an explicit binding key, so we expect it to use the username like we've configured. This works as expected if we generate the route url and pass the parameters as an associative array.

$user = new User(['username' => 'kai']);
$post = new Post(['id' => 5]);

route('users.posts.show', ['user' => $user, 'post' => $post], false);
// => /kai/posts/5

However, if we pass the parameters without explicit keys it breaks.

$user = new User(['username' => 'kai']);
$post = new Post(['id' => 5]);

route('users.posts.show', [$user, $post], false);
// => /1/posts/5

If we explicitly define the binding field for the user parameter as well, it works again as expected.

Route::get('/{user:username}/posts/{post:id}', UserPostsController::class)
    ->name('users.posts.show');

route('users.posts.show', [$user, $post], false);
// => /kai/posts/5

I have added a test to the RoutingUrlGeneratorTest file that reproduces this error.

Why the bug happens

When the RouteUri class parses the URI, it extracts all binding fields from the URI. This array then gets saved on the Route object's $bindingFields property. So our URI of

/{user}/posts/{post:id}

gets turned into

['post' => 'id']

So far so good, but note that the user parameter is missing since it doesn't define a an explicit binding field (no spoilers, but this is important).

When the UrlGenerator tries to resolve the route parameters, it loops of the parameters and calls the bindingFieldFor method of the Route object for each of them (https://github.com/laravel/framework/blob/9.x/src/Illuminate/Routing/UrlGenerator.php#L483).

This method is passed the key of the current parameter we're looping over. It then checks if that key is an integer, and if so, extracts the binding field from the route's bindingFields array by index.

// Routing/Route.php#536

public function bindingFieldFor($parameter)
{
    $fields = is_int($parameter) ? array_values($this->bindingFields) : $this->bindingFields;

    return $fields[$parameter] ?? null;
}

Going back to our route above, the bindingFields array currently looks like this.

['post' => 'id']

But since it's trying to look up the binding field by index, it ends doing this instead.

array_values(['post' => 'id'])
// => [0 => 'id']

Since user is the first parameter we passed to the route call, it tries to look up the binding field with index 0 in this array. Which is why the id field gets mistakenly used for the user parameter.

Also, at this point "binding fields" has stopped sounding like a word.

Changes

To fix this bug, I changed the parse method of the RouteUri class to keep track of all parameters, instead of only parameters with an explicit binding field. So when we first parse our URI, we get back this instead:

"/{user}/posts/{post:id}"

// turns into
['user' => null, 'post' => 'id']

Before returning, I check if this array contains any values that are not null, meaning we have at least one explicit binDinG fIeLd. In that case, we keep the whole array—including null values—so the index positions line up as expected when calling array_values on it. In any other case, we simply return an empty array (like before).

Other changes

I've refactored the tests for the RouteUri class to use a data provider instead of having all assertions in a single test. That made figuring out which test case actually broke quite a bit easier.

[taylorotwell]

Thanks!

[driesvints]

FYI: #43255

[ksassnowski]

Looks good. Sorry about all the issues this PR has caused 😞

[driesvints]

@ksassnowski don't worry about it! :)