[5.4] Revert breaking changes in ControllerDispatcher::resolveMethodDependencies

[sisve]

This fixes all known (so far) breaking changes mentioned in issue #18593. It also includes tests for these cases to avoid a regression in the future.

It has one change of existing behavior back to the 5.4.12 ways; a certain type will only be resolved once. One test introduced with the breaking 5.4.13 changes, testClassesAndVariablesCanBeInjectedIntoRoutes, has to be updated to reflect this: In this case it still works because the second parameter of type stdClass also has a default value, and the default value is passed into the method instead.

I believe that this is the correct behavior based on a comment in RouteDependencyResolverTrait::transformDependency and the way that pre-5.4.13 ensured that a specific type was only resolved once.

If the parameter has a type-hinted class, we will check to see if it is already in
the list of parameters. If it is we will just skip it as it is probably a model
binding and we do not want to mess with those; otherwise, we resolve it here.

Assuming I understand this change correctly; this only affects people that uses the same interface twice (or more) in the same action signature. The test now fails because it has two parameters of the same type, and no model binding involved. I believe the test to be odd and have updated it to verify the null value instead.

[themsaid]

Based on your reporting, you say that a route like:

Route::get('test/{name}/{age}', 'SomeController@test');

// You hit `/test/12`

class SomeController extends Controller{
    public function test($age){
        dd($age);
    }

    public function callAction($method, $parameters)
    {
        dd($parameters);
        // You expect the parameters to be ['age' => 12]
    }
}

You expect the parameters to be ['age' => 12]? But that doesn't work under the changes in your PR still, you get a NotFoundHttpException!

Can you please explain your concerns with real life example? Like show the route defined, the method, how you override callAction, what you expect, and what you get. This is more clear than sharing such tests.

[sisve]

I've been unclear in our Slack discussions. Using that exact controller, action and route I expect /test/12 to not match the route, but /test/sisve/12 to result in an array of ["name" => "sisve", "age" => 12] passed to callAction, even if the target action (the test method) does not have a $name parameter. This was the 5.4.12 behavior.

The current (5.4.17) behavior passes a ["age" => "sisve"] to callAction. I consider this wrong.

Regarding the actual invocation of the method, both the old and the new behavior sends in "sisve" as the $age parameter since it's the first value. There's no change in behavior in this case by this PR.

Anything else, and what I've mentioned about $age=12, was me explaining how I've overridden my callAction to match parameter names action with route segment names. That is not something that I am trying to change or impose with this PR.

[taylorotwell]

Aren't we simply introducing another bug by reverting this bug? Can we not make sure both are fixed?

[themsaid]

@taylorotwell I'm working on it, and yes reverting will introduce the bug that the PR fixed originally and that was like a month ago so I guess it might break some other app if reverted.

[themsaid]

Opened another PR that should handle the issues mentioned here while keeping the fixes: #18646