Movatterモバイル変換


[0]ホーム

URL:


Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

Sign up
Appearance settings

[12.x] fix method dependencies order#56062

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to ourterms of service andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Draft
Seb33300 wants to merge1 commit intolaravel:12.x
base:12.x
Choose a base branch
Loading
fromSeb33300:param-order

Conversation

Seb33300
Copy link

@Seb33300Seb33300 commentedJun 17, 2025
edited
Loading

Hello,

Fixes#56047

Inthe 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 UsersControllerextends Controller{/**     * Note: $company is nullable, so we can use the same action with multiple routes     */publicfunctionshow(User$user, ?Company$company =null)    {var_dump($user);var_dump($company);    }}

With the following routes :

// That route works as expectedRoute::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 actionRoute::get('companies/{company}/users/{user}/details','UsersController@show');

UsersController::show(): Argument#1 ($user) must be of typeUser,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.

$this->assertEquals(['one' => 'one', 'two' => 'two'], $_SERVER['__test.controller_callAction_parameters']);
$this->assertSame(['two' => 'two', 'one' => 'one'], $_SERVER['__test.controller_callAction_parameters']);
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

I replacedassertEquals byassertSame becauseassertEquals is NOT checking the order of array items.
WhileassertSame 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
Copy link
Contributor

fyi: Another way to solve this would be usingcontextual attributes

@Seb33300
Copy link
Author

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
Copy link
Member

@stancl what do you think of this?

@taylorotwelltaylorotwell marked this pull request as draftJune 17, 2025 12:59
@shaedrich
Copy link
Contributor

Creating a new ContextualAttribute is not really convenient.

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

@stancl
Copy link
Contributor

@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]); (orroute('foo', ['user' => $user, 'company' => $company]); since parameter order isirrelevant with named parameters)
  • route('foo', [$company, $user]) since that's the order specified in the route string

Maybe youcould 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') withtenant 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 realizingmere reordering would not work if you wanted to make route generation type-aware. Point 2) would actually require adding keysbased 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 thatas far as route generation is concerned, it should only ever care about parameter order or names, not types.

Cases likeRoute::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 gettwo 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 theUser 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 routegeneration 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 orderalways 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
Copy link
Author

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 theRequest at any position) by usingspliceIntoParameters()

publicfunctionresolveMethodDependencies(array$parameters,ReflectionFunctionAbstract$reflector)
{
$instanceCount =0;
$values =array_values($parameters);
$skippableValue =newstdClass;
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
Copy link
Contributor

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 theUser models, this might be a different story.

If you could specify parameters something like{from@user}/{to@user}, where the router could match both typesand 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
Copy link
Author

Seb33300 commentedJun 18, 2025
edited
Loading

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 UsersControllerextends Controller{// same orderpublicfunctionshow(User$user_from,User$user_to)    {// This works    }// different param namepublicfunctionshow(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:publicfunctionshow(User$user_to,User$user_from)    {// Without my PR: $user_to contains {user_from}// With my PR: $user_to contains {user_to}    }}

@shaedrich
Copy link
Contributor

If you could specify parameters something like{from@user}/{to@user}, where the router could match both typesand 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 usingcontextual 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
Copy link
Author

Seb33300 commentedJun 18, 2025
edited
Loading

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

In my PR, I modified theresolveMethodDependencies(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 theRequest).

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
Copy link
Author

@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
Copy link
Contributor

stancl commentedJun 18, 2025
edited
Loading

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

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

So yeah you can have{from},{to} both typehinted toUser, theaction parameter name just needs to match theroute 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, truedd($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 theaction() helper should handle that? Alsohttps://github.com/laravel/wayfinder.

@Seb33300
Copy link
Author

Seb33300 commentedJun 18, 2025
edited
Loading

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) {// truedd($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 investigateaction() to check how it works.

And aboutwayfinder. 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
Copy link
Contributor

This works

Does it?

Route::get('/test1/{foo}',function (User$foo) {// truedd($foo->exists);});Route::get('/test2/{foo}',function (User$different_name) {// falsedd($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-hintedvariable 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 asaction(). 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
Copy link
Author

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 theaction() helper.
Thanks for catching this.

@shaedrich
Copy link
Contributor

@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
Copy link
Author

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{    publicfunctionshow(User$from, User$to)action([UsersController::class,'show'], ['from' =>1,'to' =>2]);# /from/1/to/2action([UsersController::class,'show'], ['to' =>2,'from' =>1]);# /from/1/to/2publicfunction show(User$to,User$from)action([UsersController::class,'show'], ['from' =>1,'to' =>2]);# /from/1/to/2action([UsersController::class,'show'], ['to' =>2,'from' =>1]);# /from/1/to/2publicfunction show(User$to,Request$request,User$from)action([UsersController::class,'show'], ['from' =>1,'to' =>2]);# /from/1/to/2action([UsersController::class,'show'], ['to' =>2,'from' =>1]);# /from/1/to/2}

Copy link

@Ayokanmi-AdejolaAyokanmi-Adejola left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

@Seb33300
Copy link
Author

Hi@stancl, not sure if you saw my last message (I forgot to tag you)

@stancl
Copy link
Contributor

My question was about the behavior of action() when multiple routes point to one controller method, since that's the example given in the original post and something this feature could incentivize (if you don't share route actions it's not a problem or really even an inconvenience that you have to use consistent parameter order).

@Seb33300
Copy link
Author

Hi@stancl,

I used multiple routes pointing to one controller method as example, because I think it shows a real use case that makes sense to not declare controller action params in the same order as route params.
And it's already possible to declare multiple routes with different params pointing to one controller method in Laravel, this is not a new feature introduced by my PR.

Regarding theaction() helper, I did some tests and the route used by the helper depends on the order routes are declared in route config files.action() seems to always use the lastest route declared for the specified controller action.
If the last route declared contains only 1 param, the result will be:/from/1?to=2

If you want to fixaction() to match the most appropriate route based on params provided, this should be implemented in another PR.
My PR, is only fixing the order of param before calling the controller action. There nothing changed related to theaction() helper.

Sign up for freeto join this conversation on GitHub. Already have an account?Sign in to comment
Reviewers

@Ayokanmi-AdejolaAyokanmi-AdejolaAyokanmi-Adejola left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Implicit binding with omitted param
5 participants
@Seb33300@shaedrich@taylorotwell@stancl@Ayokanmi-Adejola

[8]ページ先頭

©2009-2025 Movatter.jp