-
-
Notifications
You must be signed in to change notification settings - Fork 488
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
✨ New WordPress.WP.GetMetaSingle sniff #2465
✨ New WordPress.WP.GetMetaSingle sniff #2465
Conversation
This sniff warns when get_*_meta() and get_metadata*() functions are used with the $meta_key/$key param, but without the $single parameter. This could lead to unexpected behavior as an array will be returned, but a string might be expected.
We reviewed this in a pairing session, formal review will follow once the next iteration is up. |
d60856e
to
ad4f238
Compare
ad4f238
to
d189e65
Compare
@jrfnl, I implemented the changes that you suggested during our pairing session, and this PR is ready for a final review. |
When we checked this PR together, you suggested that I change the title of the XML documentation from |
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.
He @rodrigoprimo, thanks for updating the PR!
Mostly looking good. I did leave some remarks inline, mostly small/nitpicky things, but a few to do with precision which really should be fixed.
When we checked this PR together, you suggested that I change the title of the XML documentation from
Get Meta Single
toGet Meta Function Single Parameter
. I opted to follow the same pattern and rename the sniff as well fromGetMetaSingle
toGetMetaFunctionSingleParameter
. Let me know if you prefer the new name or the old name.
I think the new name is very long and wordy and, like I said when we discussed this before, I think I prefer a name which is in line with prior art in this repo, which largely uses the FunctionNameParameterNameSniff
pattern, which this sniff now doesn't comply with.
Examples of prior art:
CurrentTimeTimestampSniff
PregQuoteDelimiterSniff
StrictInArraySniff
(wrong order, but that's historical, principle is still the same)
Thanks for your review, @jrfnl. I added new commits addressing all the points that you raised. Could you please take another look?
That makes sense. I reverted to using |
ceb3d3b
to
c73162e
Compare
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.
@rodrigoprimo Thanks for updating the sniff, including the updates made during our video call today in which we did a final review of the sniff.
As discussed, I've now also added a metric to the sniff, just in case someone finds it useful (like when checking the state of things this sniff checks for in plugins in the plugin repo).
As far as I'm concerned, this is ready for merge.
@dingo-d @GaryJones Do you still want to have a look or can I merge this ?
Note: this should be squashed-merged!
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.
Looks good to me
This sniff warns when get_*_meta() and get_metadata*() functions are used with the $meta_key/$key param, but without the $single parameter (here is the full list of functions that trigger this sniff). This could lead to unexpected behavior as an array will be returned, but a string might be expected.
This PR adds the new sniff to WordPress-Extra as suggested by Juliette in this Slack thread.
It includes tests and sniff documentation in the XML format.
Related issue: #2459