search

WordPress / WordPress-Coding-Standards

PHP_CodeSniffer rules (sniffs) to enforce WordPress coding conventions
MIT License
2.56k stars 487 forks source link

Warning if get_post_meta (and siblings) is used with exactly two parameters. #2459

Closed apermo closed 2 months ago

apermo commented 5 months ago

Preamble: Even i talk about get_post_meta for better readability, this also applies to get_user_meta, get_term_meta, get_comment_meta (list might not be complete).

Is your feature request related to a problem?

I frequently ran into forgetting the third parameter when using get_post_meta(), or found this issue when working with other developers. The third parameter is optional and defaults to false. If you forget to add the third parameter you'll receive an array of all entries with the given meta_key, which will be in most cases a single element, and in the majority of cases this is an oversight of the developer resulting in different kinds of unexpected behavior. Where a failing condition is the worst case, which actually could break logic in applications.

Example, developer wants to do something if a certain condition is met.

$meta = get_post_meta( $post_id, $meta_key );
if ( is_string( $meta ) && strlen( $meta ) > 5 ) {
    very_important_function();
}

In the given example the developer made some conditions which will always fail for the return of an array, empty or not, but even if his post_meta is as expected, the developer will not reach the inside of the condition, and it will take him a while to figure out why.

Since changing the default value of the third parameter a breaking change, this is not an option, and I just mention it, to underline that I'm not arguing to change it. But it could be an idea to add additional functions to core, which only allow to get a single meta.

Describe the solution you'd like

$all_meta = get_post_meta( $post_id ); // This is fine
$one_meta = get_post_meta( $post_id, $meta_key, true ); // Fine
$array_of_meta = get_post_meta( $post_id, $meta_key, false ); // Fine
$array_of_meta = get_post_meta( $post_id, $meta_key ); // This is where I would love to have the warning

I would like to have a sniff that encourages you to use the get_post_meta() with either 1 or 3 values, since in my experience, using it with 2 parameter, is most likely an oversight of the developer and rarely by design, see below for further context.

Additional context (optional)

I've checked multiple WordPress installations and a number of frequently used plugins, and the vast majority of uses were either using 1 parameter or 3, with the third mostly being true. So the sniff will in most cases just stay unnoticed for users of WPCS.

Old Slack Communication: https://wordpress.slack.com/archives/C5VCTJGH3/p1608538230036200

As discussed with Juliette, this is would be most suitable for Extra.

rodrigoprimo commented 4 months ago

As we discussed during WCEU, I will try to create a sniff to throw a warning when get_*_meta() is used with exactly two parameters.

jrfnl commented 4 months ago

@rodrigoprimo Heads-up: don't think in terms of parameter count. With named parameters available since PHP 8.0, parameter count has become irrelevant. You need to look for the specific parameters (and PHPCSUtils has helper functions for that).

rodrigoprimo commented 4 months ago

Thanks for the input, @jrfnl. I already have a working version of this sniff that I will polish a bit next week before creating a PR. I used the PregQuoteDelimiter sniff as a reference. Thanks to this sniff, I learned about PassedParameters::getParameterFromStack(), which I believe is one of the helper functions that you are referring to. The sniff I create extends AbstractFunctionParameterSniff, which does all the heavy lifting.

apermo commented 4 months ago

@rodrigoprimo How about GetMetaUnclearIntention which no longer uses a count for naming.

And following @jrfnl 's comment about PHP8, while it is true that the parameter count has become irrelevant, but since the function comes from an area before PHP8, the meta_key parameter is kind of mandatory, if you want to use the single which should not be part of this discussion or sniff, but this likely could need some attention.

rodrigoprimo commented 4 months ago

@apermo, I created a first version of this sniff in a branch in my fork: https://github.com/rodrigoprimo/WordPress-Coding-Standards/tree/new-get-meta-key-param-no-single-param-sniff. Let me know if you have time to check and test it. Any feedback that you have is welcome. If needed, I'm happy to provide detailed instructions on how to test the new sniff using my fork.

Next, per @jrfnl, I plan to run this sniff against the WP.org plugins (or a subset of those plugins) to see if we can get some data to confirm that it would actually help WP developers.

I haven't created the XML documentation for the sniff yet. I will do this after we check the data and confirm that we want to add this sniff to WPCS.

For now, I'm calling the sniff GetMetaKeyParamNoSingleParam. It is a bit long and I'm not super happy about it, but it is the best I could think so far.

apermo commented 4 months ago

@rodrigoprimo

As for the statistics, as mentioned before I've checked WordPress Core and a selection of Plugins at WordPress.org for the amount of parameters on the get_*_meta(). The outcome was roughtly 95-99% of all usages either using 1 or 3 parameters. The majority using 3 parameters, and I found a bunch using false als third parameter as well.

I would not be surprised to find some developers using a hardcoded [0] and no further use of the array, if one actually does omit the $single, like in this example.

$meta = get_*_meta( $id, $meta_key);
//...
echo $meta[0]; // Or some other use.

In best case you should expect the sniff to barely throw a warning for whatever you throw at it, if so, the plugins are well written. As mentioned, my need came from my days when I was less experienced and less used to the existance of the $single parameter. That is my intended use case, to warn developers of an unclear intention.

Have you considered using GetMetaUnclearIntention as name for the sniff?

I'll try to verify it tomorrow if I can.

rodrigoprimo commented 4 months ago

Have you considered using GetMetaUnclearIntention as name for the sniff?

I like it, thanks. Before changing the sniff name, I will wait to see if others have any input.

@jrfnl, today I ran the sniff against the 20 plugins listed on the first and the last page of the popular plugins section (https://wordpress.org/plugins/browse/popular/ and https://wordpress.org/plugins/browse/popular/page/49/): elementor, contact-form-7, wordpress-seo, classic-editor, woocommerce, akismet, wpforms-lite, all-in-one-wp-migration, litespeed-cache, wordfence, really-simple-ssl, jetpack, google-site-kit, duplicate-post, wp-mail-smtp, updraftplus, wordpress-importer, duplicate-page, all-in-one-seo-pack, google-analytics-for-wordpress, disable-feeds, sb-elementor-contact-form-db, simply-schedule-appointments, auto-upload-images, rara-one-click-demo-import, block-options, bnfw, woocommerce-payfast-gateway, password-protect-page, wc-hide-shipping-methods, fullwidth-templates, gallery-by-supsystic, whatshelp-chat-button, protect-uploads, zero-spam, persian-gravity-forms, wp-custom-admin-interface, persian-elementor, spicebox, and seraphinite-accelerator.

Here are the results ``` $ XDEBUG_MODE=off php ../../wpcs/vendor/bin/phpcs --report=summary,full -p --parallel=12 --standard=WordPress --sniffs=WordPress.WP.GetMetaKeyParamNoSingleParam elementor contact-form-7 wordpress-seo classic-editor woocommerce akismet wpforms-lite all-in-one-wp-migration litespeed-cache wordfence really-simple-ssl jetpack google-site-kit duplicate-post wp-mail-smtp updraftplus wordpress-importer duplicate-page all-in-one-seo-pack google-analytics-for-wordpress disable-feeds sb-elementor-contact-form-db simply-schedule-appointments auto-upload-images rara-one-click-demo-import block-options bnfw woocommerce-payfast-gateway password-protect-page wc-hide-shipping-methods fullwidth-templates gallery-by-supsystic whatshelp-chat-button protect-uploads zero-spam persian-gravity-forms wp-custom-admin-interface persian-elementor spicebox seraphinite-accelerator ...WWW..WWWW 12 / 12 (100%) PHP CODE SNIFFER REPORT SUMMARY ------------------------------------------------------------------------------------------------------------------------------ FILE ERRORS WARNINGS ------------------------------------------------------------------------------------------------------------------------------ akismet/class.akismet.php 0 1 all-in-one-seo-pack/app/Common/Admin/Dashboard.php 0 1 contact-form-7/includes/form-tag.php 0 1 gallery-by-supsystic/src/GridGallery/Galleries/Attachment.php 0 1 gallery-by-supsystic/src/GridGallery/Photos/Model/Photos.php 0 2 google-site-kit/includes/Core/Storage/User_Options.php 0 1 jetpack/class.jetpack-post-images.php 0 1 jetpack/class.jetpack.php 0 1 jetpack/_inc/lib/class.media.php 0 1 jetpack/jetpack_vendor/automattic/jetpack-publicize/src/class-publicize-base.php 0 1 jetpack/jetpack_vendor/automattic/jetpack-publicize/src/class-publicize.php 0 1 jetpack/jetpack_vendor/automattic/jetpack-stats/src/class-wpcom-stats.php 0 1 jetpack/jetpack_vendor/automattic/jetpack-videopress/src/class-attachment-handler.php 0 1 updraftplus/includes/updraftclone/temporary-clone-auto-login.php 0 1 woocommerce/includes/data-stores/class-wc-coupon-data-store-cpt.php 0 3 ------------------------------------------------------------------------------------------------------------------------------ A TOTAL OF 0 ERRORS AND 18 WARNINGS WERE FOUND IN 15 FILES ------------------------------------------------------------------------------------------------------------------------------ Time: 1 mins, 50.42 secs; Memory: 14MB FILE: akismet/class.akismet.php ------------------------------------------------------------------------------------------------------------------------------ FOUND 0 ERRORS AND 1 WARNING AFFECTING 1 LINE ------------------------------------------------------------------------------------------------------------------------------ 830 | WARNING | Passing the $key parameter to get_comment_meta() without the $single parameter is not recommended. An array | | will be returned, but a string might be expected. ------------------------------------------------------------------------------------------------------------------------------ FILE: google-site-kit/includes/Core/Storage/User_Options.php ------------------------------------------------------------------------------------------------------------------------------ FOUND 0 ERRORS AND 1 WARNING AFFECTING 1 LINE ------------------------------------------------------------------------------------------------------------------------------ 68 | WARNING | Passing the $key parameter to get_user_meta() without the $single parameter is not recommended. An array will | | be returned, but a string might be expected. ------------------------------------------------------------------------------------------------------------------------------ FILE: gallery-by-supsystic/src/GridGallery/Galleries/Attachment.php ------------------------------------------------------------------------------------------------------------------------------ FOUND 0 ERRORS AND 1 WARNING AFFECTING 1 LINE ------------------------------------------------------------------------------------------------------------------------------ 101 | WARNING | Passing the $key parameter to get_post_meta() without the $single parameter is not recommended. An array | | will be returned, but a string might be expected. ------------------------------------------------------------------------------------------------------------------------------ FILE: jetpack/class.jetpack-post-images.php ------------------------------------------------------------------------------------------------------------------------------ FOUND 0 ERRORS AND 1 WARNING AFFECTING 1 LINE ------------------------------------------------------------------------------------------------------------------------------ 365 | WARNING | Passing the $key parameter to get_post_meta() without the $single parameter is not recommended. An array | | will be returned, but a string might be expected. ------------------------------------------------------------------------------------------------------------------------------ FILE: gallery-by-supsystic/src/GridGallery/Photos/Model/Photos.php ------------------------------------------------------------------------------------------------------------------------------ FOUND 0 ERRORS AND 2 WARNINGS AFFECTING 2 LINES ------------------------------------------------------------------------------------------------------------------------------ 564 | WARNING | Passing the $key parameter to get_post_meta() without the $single parameter is not recommended. An array | | will be returned, but a string might be expected. 565 | WARNING | Passing the $key parameter to get_post_meta() without the $single parameter is not recommended. An array | | will be returned, but a string might be expected. ------------------------------------------------------------------------------------------------------------------------------ FILE: jetpack/jetpack_vendor/automattic/jetpack-stats/src/class-wpcom-stats.php ------------------------------------------------------------------------------------------------------------------------------ FOUND 0 ERRORS AND 1 WARNING AFFECTING 1 LINE ------------------------------------------------------------------------------------------------------------------------------ 435 | WARNING | Passing the $key parameter to get_post_meta() without the $single parameter is not recommended. An array | | will be returned, but a string might be expected. ------------------------------------------------------------------------------------------------------------------------------ FILE: jetpack/jetpack_vendor/automattic/jetpack-publicize/src/class-publicize-base.php ------------------------------------------------------------------------------------------------------------------------------ FOUND 0 ERRORS AND 1 WARNING AFFECTING 1 LINE ------------------------------------------------------------------------------------------------------------------------------ 896 | WARNING | Passing the $key parameter to get_post_meta() without the $single parameter is not recommended. An array | | will be returned, but a string might be expected. ------------------------------------------------------------------------------------------------------------------------------ FILE: jetpack/jetpack_vendor/automattic/jetpack-publicize/src/class-publicize.php ------------------------------------------------------------------------------------------------------------------------------ FOUND 0 ERRORS AND 1 WARNING AFFECTING 1 LINE ------------------------------------------------------------------------------------------------------------------------------ 711 | WARNING | Passing the $key parameter to get_post_meta() without the $single parameter is not recommended. An array | | will be returned, but a string might be expected. ------------------------------------------------------------------------------------------------------------------------------ FILE: jetpack/jetpack_vendor/automattic/jetpack-videopress/src/class-attachment-handler.php ------------------------------------------------------------------------------------------------------------------------------ FOUND 0 ERRORS AND 1 WARNING AFFECTING 1 LINE ------------------------------------------------------------------------------------------------------------------------------ 197 | WARNING | Passing the $key parameter to get_post_meta() without the $single parameter is not recommended. An array | | will be returned, but a string might be expected. ------------------------------------------------------------------------------------------------------------------------------ FILE: jetpack/class.jetpack.php ------------------------------------------------------------------------------------------------------------------------------ FOUND 0 ERRORS AND 1 WARNING AFFECTING 1 LINE ------------------------------------------------------------------------------------------------------------------------------ 6158 | WARNING | Passing the $key parameter to get_user_meta() without the $single parameter is not recommended. An array | | will be returned, but a string might be expected. ------------------------------------------------------------------------------------------------------------------------------ FILE: jetpack/_inc/lib/class.media.php ------------------------------------------------------------------------------------------------------------------------------ FOUND 0 ERRORS AND 1 WARNING AFFECTING 1 LINE ------------------------------------------------------------------------------------------------------------------------------ 244 | WARNING | Passing the $key parameter to get_post_meta() without the $single parameter is not recommended. An array | | will be returned, but a string might be expected. ------------------------------------------------------------------------------------------------------------------------------ FILE: woocommerce/includes/data-stores/class-wc-coupon-data-store-cpt.php ------------------------------------------------------------------------------------------------------------------------------ FOUND 0 ERRORS AND 3 WARNINGS AFFECTING 3 LINES ------------------------------------------------------------------------------------------------------------------------------ 146 | WARNING | Passing the $key parameter to get_post_meta() without the $single parameter is not recommended. An array | | will be returned, but a string might be expected. 315 | WARNING | Passing the $key parameter to get_post_meta() without the $single parameter is not recommended. An array | | will be returned, but a string might be expected. 380 | WARNING | Passing the $key parameter to get_post_meta() without the $single parameter is not recommended. An array | | will be returned, but a string might be expected. ------------------------------------------------------------------------------------------------------------------------------ FILE: updraftplus/includes/updraftclone/temporary-clone-auto-login.php ------------------------------------------------------------------------------------------------------------------------------ FOUND 0 ERRORS AND 1 WARNING AFFECTING 1 LINE ------------------------------------------------------------------------------------------------------------------------------ 72 | WARNING | Passing the $key parameter to get_user_meta() without the $single parameter is not recommended. An array will | | be returned, but a string might be expected. ------------------------------------------------------------------------------------------------------------------------------ FILE: all-in-one-seo-pack/app/Common/Admin/Dashboard.php ------------------------------------------------------------------------------------------------------------------------------ FOUND 0 ERRORS AND 1 WARNING AFFECTING 1 LINE ------------------------------------------------------------------------------------------------------------------------------ 170 | WARNING | Passing the $key parameter to get_user_meta() without the $single parameter is not recommended. An array | | will be returned, but a string might be expected. ------------------------------------------------------------------------------------------------------------------------------ FILE: contact-form-7/includes/form-tag.php ------------------------------------------------------------------------------------------------------------------------------ FOUND 0 ERRORS AND 1 WARNING AFFECTING 1 LINE ------------------------------------------------------------------------------------------------------------------------------ 371 | WARNING | Passing the $key parameter to get_post_meta() without the $single parameter is not recommended. An array | | will be returned, but a string might be expected. ------------------------------------------------------------------------------------------------------------------------------ ```

Is this more or less what you wanted to see when you mentioned running a draft of the sniff against a number of plugins to see if it would actually help people? Happy to run it against a bigger number of plugins.

jrfnl commented 4 months ago

But it could be an idea to add additional functions to core, which only allow to get a single meta.

@apermo I do think that would be a good idea and if this would be accepted in Core, the sniff can always be updated in the future.

If you do open a ticket for this, could you please link it in this thread ?

... comment about PHP8, while it is true that the parameter count has become irrelevant, but since the function comes from an area before PHP8, the meta_key parameter is kind of mandatory, if you want to use the single which should not be part of this discussion or sniff, but this likely could need some attention.

@apermo Agreed, but sniffs should function independently of PHP versions. That's the point of my remark. A custom company-specific plugin may use PHP 8.2+ specific code and be fine. An open source plugin in the plugin directory will need to support multiple PHP versions and generally cannot use named parameters (yet). Sniffs should handle both situations correctly and to do so, we need to take named parameters into account when writing the sniff ;-)

The outcome was roughtly 95-99% of all usages either using 1 or 3 parameters. The majority using 3 parameters, and I found a bunch using false als third parameter as well.

I agree with that expectation as those plugins will be published already and if they'd still contain a bug related to the use of these functions, I would have expected that to have been flagged/reported to the author by now.

This is mostly an error prevention sniff for a particularly confusing parameter default.

Other than that, @rodrigoprimo and me discussed the WIP for this sniff a little today in a call.

Findings:

  1. Let's make the name a little more generic. Maybe something along the lines of "FunctionNameParamName" and then have the error code be the more specific thing with an indication of what's "wrong" ? This will allow for potentially adding extra checks related to that function group/parameter to the sniff without the need for renaming the complete sniff.
  2. We also did a search of the WP Core source code and found a few more functions which will need to be examined by the sniff, including things like _wp_preview_meta_filter() and get_metadata_raw(). (I believe Rodrigo still has some feedback on that first one ?) I have shared my search results with Rodrigo so he has the list.
  3. We also had a brief look at the principle of the code abstraction, nothing much to share about that without the PR being public, but you'll see the result of that in the PR.
rodrigoprimo commented 4 months ago

I implemented the changes that I discussed with Juliette and opened a PR adding the new sniff: https://github.com/WordPress/WordPress-Coding-Standards/pull/2465.

We also did a search of the WP Core source code and found a few more functions which will need to be examined by the sniff, including things like _wp_preview_meta_filter() and get_metadata_raw(). (I believe Rodrigo still has some feedback on that first one ?)

I opened a WP core ticket suggesting an update to the documentation of the _wp_preview_meta_filter() function: https://core.trac.wordpress.org/ticket/61645.

GaryJones commented 2 months ago

Since #2465 is now merged, can this be closed?

jrfnl commented 2 months ago

@GaryJones Indeed, the PR description was missing the "fixes ..." comment to auto-close. I've added the ticket to the milestone and will close it now.