[12.x] fix method dependencies order

[Seb33300]

Hello,

Fixes #56047

In the documentation, we can read:

Laravel automatically resolves Eloquent models defined in routes or controller actions whose type-hinted variable names match a route segment name.

What I understand:
As long as our controller action variable names matches with the routes segment names, the order of variables should not matter.

Unfortunately, this is not how it currently works, if our controller actions does not declare variables in the same order as the route, this simple example will fail:

class UsersController extends Controller
{
    /**
     * Note: $company is nullable, so we can use the same action with multiple routes
     */
    public function show(User $user, ?Company $company = null)
    {
        var_dump($user);
        var_dump($company);
    }
}

With the following routes :

// That route works as expected
Route::get('users/{user}/details', 'UsersController@show');

// That route trigger an Exception
// Note that the company param is placed first in the URL, while it's in second position in the controller action
Route::get('companies/{company}/users/{user}/details', 'UsersController@show');

UsersController::show(): Argument #1 ($user) must be of type User, Company given, called in ...

The exception shows that the params are not passed in the correct order, even if I am using the same param names.

With my PR, that issue is fixed by trying to resolve dependencies by param name first.

[Seb33300]

I replaced assertEquals by assertSame because assertEquals is NOT checking the order of array items.
While assertSame is checking that both values and order are identical.

When we call the controller action, the order of param is important so we must check it.
In that example, the order of params must be reversed if they are reversed in the controller action.

[shaedrich]

fyi: Another way to solve this would be using contextual attributes

[Seb33300]

Creating a new ContextualAttribute is not really convenient.
Also, the benefit to rely on the router when binding models, especially in the example provided, is that we can easily couple it with scope binding.

[taylorotwell]

@stancl what do you think of this?

[shaedrich]

Creating a new ContextualAttribute is not really convenient.

I didn't mean a new one. There already is \Illuminate\Container\Attributes\RouteParameter

[stancl]

@taylorotwell From the perspective of my work on the route generation logic, I think route generation should always only be concerned with route strings, not the route actions they point to. Meaning, if you have a route like this:

Route::get('companies/{company}/users/{user}/details', function (User $user, Company $company) {
    dd($user, $company);
})->name('foo');

You'd generate routes as:

• route('foo', ['company' => $company, 'user' => $user]); (or route('foo', ['user' => $user, 'company' => $company]); since parameter order is irrelevant with named parameters)
• route('foo', [$company, $user]) since that's the order specified in the route string

Maybe you could add logic that'd reorder [$user, $company] to [$company, $user] based on types, but the logic is already pretty complex and I'm afraid this would add a ton of edge cases that'd be hard to predict upfront.

Types aren't always sufficient information. If you have a route like /mergeUsers/from/{user}/to/{user} you'd use a (User $from, User $to) signature. But the signature could be (User $to, User $from). Presumably if multiple parameters have the same type, they wouldn't be reordered, but in some sense that'd be inconsistent with the reordering implemented here.

It's difficult to come up with realistic edge cases on the spot, but with how the route generation logic can now sort of fill parameters "from the middle" if too few are passed (e.g. there are some with defaults at the start, some optional ones at the end, so it just fills the required ones in the middle) the behavior would also drastically change if made type-aware, I'm not sure how realistic that concern is but it seems undesirable. For instance if you had:

1. Route::get('/{tenant}/teams/{team}/addUser/{user}/{role?}', ...)->name('bar') with tenant having a default value
2. route('bar', [$tenant, $team, $user]) with all being model instances, would pass arguments as expected, but if perhaps during a refactor you end up with just the keys and not model instances anymore, the call would be reinterpreted as:
3. route('bar', ['team' => $tenant, 'user' => $team, 'role' => $user])

As I'm writing this I'm also realizing mere reordering would not work if you wanted to make route generation type-aware. Point 2) would actually require adding keys based on types because in cases of too few arguments, the logic treats parameters with different precedence: required > optional > with defaults.

In short, from quickly thinking of some cases I think this might be painful to handle in route generation logic too. Maybe it would turn out to be not as bad as I'm imagining, but you'd still be left with the potential scenario where a seemingly innocuous refactor, that changes whether some variables are model instances, could change how routes are generated.

From that my conclusion is that as far as route generation is concerned, it should only ever care about parameter order or names, not types.

Cases like Route::get('/team/{team}/swapRoles/from/{user}/to/{user}', ...) would just make this magic too ambiguous/inconsistent IMO. You could have (Team $team, User $from, User $to), (User $from, Team $team, User $to), (User $from, User $to, Team $team) but not (Team $team, User $to, User $from). So where now you have a single rule — order dictates everything — you'd get two rules: both types and order are considered.

If route model binding didn't require parameters to match their model names, and you could have {from} and {to} for the User models, this might be a different story.

---

Considering the feature overall, this change could make sense and would perhaps feel more consistent with dependency injection, my concerns would just be:

• Making routing inconsistent with route generation unless the changes above would be made. Right now route generation, route strings, and route actions all follow the same order when no names are passed. This change would make some parts of this type-aware and others not.
• The issues mentioned above with refactors potentially having unexpected effects.
• Route string parameter order always matched the route action parameter order as far as I know, so maybe there might be other consequences of this change that I'm not seeing right now.

So the main benefit I see would be making this feel more like dependency injection, but I'm worried about some inconsistencies this would introduce and potential edge cases, given that routing has always expected developers to be strict about parameter order.

Personally I feel like being able to rely on developers having "parameter order discipline" simplifies things, so I'd probably favor just updating the docs.

Right now routing feels like a "function call" with the dependence on parameter order, type-aware magic autowiring would feel more Laravely, I'm just not sure if it's worth the potential complications.

[Seb33300]

Hi @stancl

Please let me provide additional context:

Currently, if an object with the required typehint is found in route parameters, it's skipped and we assume it's at the correct position when the name does not match the route param (same as before), resulting in an exception if not.

Maybe you could add logic that'd reorder [$user, $company] to [$company, $user] based on types, but the logic is already pretty complex and I'm afraid this would add a ton of edge cases that'd be hard to predict upfront.

I thought about it too, but i did not implemented yet. I can give it a try if you want?

Types aren't always sufficient information. If you have a route like /mergeUsers/from/{user}/to/{user} you'd use a (User $from, User $to) signature. But the signature could be (User $to, User $from).

That case is not possible:

Route pattern "/mergeUsers/from/{user}/to/{user}" cannot reference variable name "user" more than once.

That's why i decided to go with reordering by param names only for now.
It will probably cover most use cases, and it does not change the previous logic in case param names are not matching with controller action param names.

Route string parameter order always matched the route action parameter order as far as I know

Not strictly, even before my change it was possible to inject a service in between 2 route parameters (eg: injecting the Request at any position) by using spliceIntoParameters()

framework/src/Illuminate/Routing/ResolvesRouteDependencies.php

Lines 42 to 66 in 46b66aa

	public function resolveMethodDependencies(array $parameters, ReflectionFunctionAbstract $reflector)
	{
	$instanceCount = 0;
	$values = array_values($parameters);
	$skippableValue = new stdClass;
	foreach ($reflector->getParameters() as $key => $parameter) {
	$instance = $this->transformDependency($parameter, $parameters, $skippableValue);
	if ($instance !== $skippableValue) {
	$instanceCount++;
	$this->spliceIntoParameters($parameters, $key, $instance);
	} elseif (! isset($values[$key - $instanceCount]) &&
	$parameter->isDefaultValueAvailable()) {
	$this->spliceIntoParameters($parameters, $key, $parameter->getDefaultValue());
	}
	$this->container->fireAfterResolvingAttributeCallbacks($parameter->getAttributes(), $instance);
	}
	return $parameters;
	}

routing has always expected developers to be strict about parameter order

Maybe you are referring to Laravel router?
In symfony, the order of route parameters does not matter:
https://symfony.com/doc/current/routing.html#route-parameters

[stancl]

Currently, if an object with the required typehint is found in route parameters, it's skipped and we assume it's at the correct position when the name does not match the route param (same as before), resulting in an exception if not.

Not sure what you mean by this, positions seem to always be the only thing the router looks at regardless of types being present or not.

That case is not possible

Interesting. I'd expect that to work since you do see routes that accept e.g. multiple users in some APIs (though probably not Laravel APIs in that case?).

Above I mentioned:

If route model binding didn't require parameters to match their model names, and you could have {from} and {to} for the User models, this might be a different story.

If you could specify parameters something like {from@user}/{to@user}, where the router could match both types and names, maybe I could like this more. Honestly, I'm not sure how common such routes (with multiple parameters of the same type) are, in most cases you'd just put the second user into the request payload, but such approach would end up actually expanding what the routing logic can do rather than just adding reordering.

Not strictly, even before my change it was possible to inject a service in between 2 route parameters

I know, but if you ignore the injected dependency parameters, the order of parameters coming from the routing logic always matches what's used in the route string as well as route generation.

Maybe you are referring to Laravel router?

Yes, I don't think most Laravel developers are exposed to Symfony internals much.

Anyway - if it's impossible to use two parameters of the same (model) type, then I don't have as many concerns as expressed in the original comment, but I'm still not sure how I'd feel about the inconsistency between this logic and route generation (latter requiring the correct order). Maybe you can try making the route generation changes here to see how complex (or not!) it'd end up being 👍

[Seb33300]

if it's impossible to use two parameters of the same (model) type

Just to clarify, I think we have some confusion here.

In your example, I am not sure if by {user} you are referring to an "user typed param", or reusing the same param name twice.

We cannot reuse the same segment name in the route:

Route::get('/mergeUsers/from/{user}/to/{user}', ...)

But yes, it's possible to have two parameters of the same (model) type if the segment names are different:

Route::get('/mergeUsers/from/{user_from}/to/{user_to}', ...)

class UsersController extends Controller
{
    // same order
    public function show(User $user_from, User $user_to)
    {
        // This works
    }

    // different param name
    public function show(User $a, User $b)
    {
        // This also works, not the same name, so it fallback on declared order of route params
        // $a contains {user_from}
        // $b contains {user_to}
    }

    // reversed:
    public function show(User $user_to, User $user_from)
    {
        // Without my PR: $user_to contains {user_from}
        // With my PR: $user_to contains {user_to}
    }
}

[shaedrich]

If you could specify parameters something like {from@user}/{to@user}, where the router could match both types and names, maybe I could like this more. Honestly, I'm not sure how common such routes (with multiple parameters of the same type) are, in most cases you'd just put the second user into the request payload, but such approach would end up actually expanding what the routing logic can do rather than just adding reordering.

Definitely sounds interesting 🤔

fyi: Another way to solve this would be using contextual attributes

Creating a new ContextualAttribute is not really convenient.

I didn't mean a new one. There already is \Illuminate\Container\Attributes\RouteParameter

@Seb33300 Isn't that working for you?

[Seb33300]

@stancl I am not sure if you really understood the changes I did in that PR.

In my PR, I modified the resolveMethodDependencies(array $parameters, ReflectionFunctionAbstract $reflector) method.
This is the method called just before calling the controller action.

That method has the $parameters argument which receive the list of parameters from the route already and resolved by the router.
The router is not modified at all in my PR.

resolveMethodDependencies() is in charge to use the parameters resolved by the router + add missing service dependencies (like the Request).

On top of that, I added the ability to reorder the params based on their name to match the order of controller action just before calling it

[Seb33300]

@shaedrich from what I understand about contextual attribute, is that they are not resolved by the router. So it is not possible to use them with scope binding.

[stancl]

Hm interesting, looks like the {from@user} syntax would not even be needed. I always thought route model binding required parameter names to match the model names (given the example here https://laravel.com/docs/11.x/routing#explicit-binding) but this works just fine:

Route::get('/test1/{foo}', function (User $foo) {
    // true
    dd($foo->exists);
});

So yeah you can have {from}, {to} both typehinted to User, the action parameter name just needs to match the route string parameter name.

@Seb33300 Pretty much all my feedback has been about whether reordering is desired or not. The rest is just considering edge cases (some of which I see aren't really an issue).

My main point of feedback remains that as Laravel works now, all of:

• route strings
• route actions
• route generation

uses the same order. Without updating route generation this would add some inconsistency, unless it's specifically decided that route generation should only look at route strings and follow the parameter order there.

Given how this works:

Route::get('/test3/{foo}/{bar}', function (User $foo, User $bar) {
    // true, true
    dd($foo->exists, $bar->exists);
});

(Route model binding works by matching the route string param names to the action param names, then reading the typehint) I understand why you think the order of parameters should not matter. I'm just not sure if it'd be a good change.

In the example you gave originally, you essentially reused the same route action for two different routes. Can you maybe comment on how the action() helper should handle that? Also https://github.com/laravel/wayfinder.

[Seb33300]

So yeah you can have {from}, {to} both typehinted to User,

Yes

the action parameter name just needs to match the route string parameter name.

No, there is no check at all on the parameter name.
The current logic is: we pass the parameters in the same order as they are defined in the route. No check on names.

This works

Route::get('/test1/{a}', function (User $b) {
    // true
    dd($b->exists);
});

And my PR tries to add that the check on the names to find a match and reorder them if not in the correct order.
If no match is found, same logic as before is used (use route order)

I need to investigate action() to check how it works.

And about wayfinder. I never used it, so I have no idea about how it works. If it's based on routes definition, this should not have any side effect on it.
Maybe @joetannenbaum can have a look and check if this PR can break something?

[stancl]

This works

Does it?

Route::get('/test1/{foo}', function (User $foo) {
    // true
    dd($foo->exists);
});

Route::get('/test2/{foo}', function (User $different_name) {
    // false
    dd($different_name->exists);
});

If I change {foo} in test2 to {different_name}, exists gives me true. This is consistent with:

Laravel automatically resolves Eloquent models defined in routes or controller actions whose type-hinted variable names match a route segment name

https://laravel.com/docs/11.x/routing#implicit-binding

Regarding wayfinder, I think it's just going to be the same story as action(). The reason I'm bringing that up is that as your original post shows, I feel not requiring developers to use a strict parameter order could incentivize reusing route actions more.

[Seb33300]

Does it?

Right my bad. Using a different name works only for non object params (string, int...)

I will try to take time tomorrow to test the action() helper.
Thanks for catching this.

[shaedrich]

@shaedrich from what I understand about contextual attribute, is that they are not resolved by the router. So it is not possible to use them with scope binding.

In other words: They work fine as long as you don't need scope binding. This in turn means, your PR will primarily benefit the case, the attribute doesn't cover. Gotcha!

[Seb33300]

I just did the following test on my local environment with my PR and it looks good:

Route::get('/from/{from}/to/{to}',  ...)

class UsersController extends Controller
{
    public function show(User $from, User $to)
    action([UsersController::class, 'show'], ['from' => 1, 'to' => 2]); # /from/1/to/2
    action([UsersController::class, 'show'], ['to' => 2, 'from' => 1]); # /from/1/to/2

    public function show(User $to, User $from)
    action([UsersController::class, 'show'], ['from' => 1, 'to' => 2]); # /from/1/to/2
    action([UsersController::class, 'show'], ['to' => 2, 'from' => 1]); # /from/1/to/2

    public function show(User $to, Request $request, User $from)
    action([UsersController::class, 'show'], ['from' => 1, 'to' => 2]); # /from/1/to/2
    action([UsersController::class, 'show'], ['to' => 2, 'from' => 1]); # /from/1/to/2
}