WPTT / WPThemeReview

PHP_CodeSniffer rules (sniffs) to enforce WordPress theme review coding conventions
MIT License
207 stars 38 forks source link

Whitelist wp_body_open function from PrefixAllGlobals sniff #229

Closed dingo-d closed 5 years ago

dingo-d commented 5 years ago

From WordPress 5.2 a new function is added – wp_body_open(), as noted in the make post: https://make.wordpress.org/themes/2019/03/29/addition-of-new-wp_body_open-hook/

For backwards compatibility sake a proposal was added in the post to add (in functions.php for instance)

<?php
if ( ! function_exists( 'wp_body_open' ) ) {
        function wp_body_open() {
                do_action( 'wp_body_open' );
        }
}

This will, however, trigger the PrefixAllGlobals native sniff even though it's a valid way to ensure backwards compatibility.

Functions declared in the global namespace by a theme/plugin should start with the theme/plugin prefix. Found: "wp_body_open". Hook names invoked by a theme/plugin should start with the theme/plugin prefix. Found: "wp_body_open".

The existing sniff should probably be extended to include a whitelist for this function.

jrfnl commented 5 years ago

Based on the premise used elsewhere that themes should support the latest WP version and up to three versions before it, this shim should only be needed until WP 5.6 comes out.

With that in mind, I'm not sure whether it's a good idea to make an exception in the sniff (which would need to be removed again later) or whether theme reviewers should be educated to just ignore that particular one.

You wouldn't want to accidentally whitelist a function with the same name which does something completely different (and would conflict with WP core).

dingo-d commented 5 years ago

With that in mind, I'm not sure whether it's a good idea to make an exception in the sniff (which would need to be removed again later) or whether theme reviewers should be educated to just ignore that particular one.

Ideally it would be best if the reviewers are educated to ignore that, but as most of them never read on the basic requirements, I see this popping up a lot as a question on the TRT channel.

I think I could just ignore this in the Theme Sniffer plugin (just add a whitelist array which is scanned against once the sniffing is done, and remove it from the report).

I do see your point, so I'll close this issue here an move it to the sniffer repo :+1:

dingo-d commented 5 years ago

On second thought, could we maybe throw a warning about this kind of functions, so that reviewers do take a look (just in case something bad is hooked), but can be safely ignored?

jrfnl commented 5 years ago

@dingo-d Don't know whether you've looked at the logic of the sniff, but that would effectively mean you would need to overload the whole sniff and maintain a duplicate. I cannot imagine that's what you want.

dingo-d commented 5 years ago

Yeah, that would be a big overhead for a small change. I'll try to see how to implement this in a theme sniffer, as that is the tool that most reviewers are using.