[5.4] Bring back an old behaviour in resolving controller method dependencies #18646
Conversation
themsaid
changed the title
|
Could this include all the unit tests written in #18644 ? |
|
Yeah it might be nice to add a couple of tests @sisve added to make sure we have it covered. |
|
Added tests to cover the cases in the other PR + an integration test to cover the issue that we originally wanted to fix. |
|
While looking at the tests I notice that the one calling withModels has some model binding. The keys (bar, baz) does not match anything. Changing the keys breaks nothing, and removing those two lines to This PR fixes all issues I've got regarding the changes in resolveMethodDependencies. |
| @@ -309,24 +309,6 @@ public function testClassesCanBeInjectedIntoRoutes() | |||
| unset($_SERVER['__test.route_inject']); | |||
| } | |||
|
|
|||
| public function testClassesAndVariablesCanBeInjectedIntoRoutes() | |||
There was a problem hiding this comment.
Why was this test removed? Makes me a little nervous that perhaps we are breaking something?
There was a problem hiding this comment.
To transform a route method dependency we first check if the class already exists in the parameters passed to the route if so then we assume it's a Model so we pass it as is.
The given test contains two type-hinted stdClass arguments, checking for the second stdClass we'll find that it's already in parameters when the first stdClass was resolved, as a result we won't resolve it and we pass the default value instead.
That test used to work before because we were building a brand new array of results instead of mutating the $parameters array, and thus when we check for the second stdClass in $parameters it won't exist since the first resolved one was stored in the $results array not the $parameters array.
For example before this PR you could have function show(Request $request, $id, Request $sameRequest) and you get two arguments with a Request instance, which isn't considered a breaking change because it actually doesn't make sense to resolve the same method dependency twice.
The fix that removed test was supposed to check is now tested here: https://github.com/laravel/framework/pull/18646/files#diff-4480aa925f3c087f5bd9c0603d1eb0fcR439
There was a problem hiding this comment.
But that test was the only test introduced #17973 that started my issue. What test is now covering the changes in that PR?
There was a problem hiding this comment.
There was a problem hiding this comment.
A majority of those tests are just rewritten tests from my PR. They ensure the 5.4.12 behavior, they do not verify the new functionality in 5.4.13. Is it the invocation of withModels that is the new behavior?
tests/Routing/RoutingRouteTest.php
Outdated
| 'uses' => 'Illuminate\Tests\Routing\RouteTestAnotherControllerWithParameterStub@withModels', | ||
| ]); | ||
| $router->model('bar', 'Illuminate\Tests\Routing\RoutingTestUserModel'); | ||
| $router->model('baz', 'Illuminate\Tests\Routing\RoutingTestTeamModel'); |
There was a problem hiding this comment.
As mentioned in the issue, these two calls to $router->model() seems out of place. The keys seems irrelevant, and the test still works if I remove them. Should they be there?
| @@ -309,24 +309,6 @@ public function testClassesCanBeInjectedIntoRoutes() | |||
| unset($_SERVER['__test.route_inject']); | |||
| } | |||
|
|
|||
| public function testClassesAndVariablesCanBeInjectedIntoRoutes() | |||
There was a problem hiding this comment.
A majority of those tests are just rewritten tests from my PR. They ensure the 5.4.12 behavior, they do not verify the new functionality in 5.4.13. Is it the invocation of withModels that is the new behavior?
|
Where are we with this? Do we have the tests we need in place? |
|
There are tests available for the 5.4.12 behavior (the array keys). I do not know what new functionality was introduced in 5.4.13 or what state those tests are in. |
This PR will handle the breaking changes reported in #18593 while keeping the fix in #17973