WPTT / WPThemeReview

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

Exceptions to prefixing hook names #201

Open joyously opened 5 years ago

joyously commented 5 years ago

There is a core ticket #12563 open for creating an action that the theme invokes when the body tag is output.

The proposal is a function call, but since it is not in core yet, a theme could do this by calling do_action('wp_body_open') itself. But doing that results in the error Hook names invoked by a theme/plugin should start with the theme/plugin prefix.

Does this error for "Hook names" cover actions and filters, or just actions? It seems like there could be times when core filters could be used, as when using a menu walker class and on "image_size_names_choose".

jrfnl commented 5 years ago

The sniff covers both actions as well as filters.

Generally speaking, themes should hook into core filters and actions and shouldn't ever need to invoke the core hooks.

There are always exceptions, of course. In that case, the invocation can be whitelisted and the reason for calling the core hook documented, like so:

// phpcs:ignore WordPress.NamingConventions.PrefixAllGlobals.NonPrefixedHooknameFound -- Invoking  core hook for reasons.....
do_action( 'name_of_core_hook' );
timelsass commented 5 years ago

False positive in something like: I have a loop creating markup and inserting actions:

do_action( $col_data[ $type ] );

Results in:

WARNING Hook names invoked by a theme/plugin should start with the theme/plugin prefix. Found: "$col_data[ $type ]".

They vary depending on what plugins are installed.

I think hooks consisting of variables only should be excluded.

joyously commented 5 years ago

I don't think that's an exception. It should still start with the theme's prefix.

timelsass commented 5 years ago

Oh sorry, that example was unclear - that's being called from within a class and a function, so I wouldn't have prefixed the variable. I think the global variable sniff would be enough to alert about that though, there shouldn't be a reason why the hook name as only a variable should have to be checked further in that case.

joyously commented 5 years ago

We're not talking about prefixing variables here. We're talking about prefixing hook names. There are very few cases where themes should be calling do_action on unprefixed hook names.

timelsass commented 5 years ago

I know, the title of the ticket is "Exceptions to prefixing hook names," so I thought it made sense to place it here rather than a new ticket. Currently I'm getting the warnings above saying I should prefix the hook name, but it shouldn't be giving me one as it isn't contextually aware of what is stored inside of the variable, and I think that situation should be an exception to the existing sniff.

The note about the prefixing variables is saying that even if I were using a variable in the global namespace and calling do_action( $something ) directly in a template file, I would still get this error. Even in those situations it's not needed since variables already have checks for prefixing those names, and the sniff isn't aware of the value assigned to $something.

dingo-d commented 5 years ago

Should we have a sniff for that, or maybe some kind of whitelist added in the ruleset.xml (if this is possible ofc) for this?

Otherwise I'd like to close this as a part of the issue grooming process 🙂