Skip to content
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

Merged
merged 11 commits into from
Aug 28, 2024

Conversation

rodrigoprimo
Copy link
Collaborator

@rodrigoprimo rodrigoprimo commented Jul 12, 2024

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

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.
@jrfnl
Copy link
Member

jrfnl commented Jul 26, 2024

We reviewed this in a pairing session, formal review will follow once the next iteration is up.

@rodrigoprimo rodrigoprimo force-pushed the new-get-meta-single-sniff branch 2 times, most recently from d60856e to ad4f238 Compare July 29, 2024 08:38
@rodrigoprimo
Copy link
Collaborator Author

@jrfnl, I implemented the changes that you suggested during our pairing session, and this PR is ready for a final review.

@rodrigoprimo
Copy link
Collaborator Author

When we checked this PR together, you suggested that I change the title of the XML documentation from Get Meta Single to Get Meta Function Single Parameter. I opted to follow the same pattern and rename the sniff as well from GetMetaSingle to GetMetaFunctionSingleParameter. Let me know if you prefer the new name or the old name.

Copy link
Member

@jrfnl jrfnl left a 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 to Get Meta Function Single Parameter. I opted to follow the same pattern and rename the sniff as well from GetMetaSingle to GetMetaFunctionSingleParameter. 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)

@jrfnl jrfnl removed this from the 3.x Next milestone Aug 20, 2024
@rodrigoprimo
Copy link
Collaborator Author

Thanks for your review, @jrfnl. I added new commits addressing all the points that you raised. Could you please take another look?

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.

That makes sense. I reverted to using GetMetaSingle as the name of the sniff.

Copy link
Member

@jrfnl jrfnl left a 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!

@jrfnl jrfnl added this to the 3.x Next milestone Aug 26, 2024
Copy link
Member

@dingo-d dingo-d left a 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

@dingo-d dingo-d merged commit 7f76630 into WordPress:develop Aug 28, 2024
37 checks passed
@rodrigoprimo rodrigoprimo deleted the new-get-meta-single-sniff branch August 28, 2024 14:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants