-
Notifications
You must be signed in to change notification settings - Fork 11.2k
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 our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[5.4] Bring back an old behaviour in resolving controller method dependencies #18646
Merged
Merged
Changes from all commits
Commits
Show all changes
9 commits
Select commit
Hold shift + click to select a range
fe50c35
bring back the old behaviour while fixing the issue
themsaid 0e4787b
fix style
themsaid 57878d5
fix test to make more sense
themsaid f859dcd
fix test to make more sense
themsaid 2a56a80
cover the scenarios with tests
themsaid 0c307db
fix style
themsaid 5f382e9
add test to cover model bindings
themsaid 6a15b83
fix style
themsaid 66bf8a9
clean tests
themsaid File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why was this test removed? Makes me a little nervous that perhaps we are breaking something?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Here you go: https://github.com/laravel/framework/pull/18646/files#diff-4480aa925f3c087f5bd9c0603d1eb0fcR405
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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?