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
Fix finder compatibilty with 4.x #17659
Conversation
|
||
$secondParamIsOptions = ( | ||
count($params) === 2 && | ||
$secondParam?->name === 'options' && |
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.
Are there other variable names we should consider? The docs, app and bake all use options
but there are likely other commonly used names.
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.
Supporting only the "official" variable name should suffice IMO.
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.
I think only options
is good. It's fairly standardized.
We have rector rules to update the callsites of finders to use named parameters. What we lack is rector operations to upgrade finder declarations to use named parameters. Because this is an impossible task, we should have better compatibility for new style finder calls with old style options arrays. This helps improve finder ergonomics (less typing) for more folks. This would have saved me a bunch of work when upgraded an application recently and making the upgrade smoother is important to me.
43219fa
to
31aecbd
Compare
deprecationWarning( | ||
'5.0.0', | ||
'Using options array for the `find()` call is deprecated.' | ||
. ' Use named arguments instead.' | ||
); |
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.
I don't think this should be deprecated right now. It creates a lot of noise to deal with when upgrading. If others feel strongly about this I'm happy to put it back.
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.
If we don't push people to update the methods they will very likely continue using the same and removing this temporary support will become difficult even for the next major.
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.
Fair point, I can add this deprecation back.
We have rector rules to update the callsites of finders to use named parameters. What we lack is rector operations to upgrade finder declarations to use named parameters. Because this is an impossible task, we should have better compatibility for new style finder calls with old style options arrays. This helps improve finder ergonomics (less typing) for more folks.
This would have saved me a bunch of work when upgraded an application recently and making the upgrade smoother is important to me.