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

Exceptions to prefixing hook names #201

Open
joyously opened this issue Feb 12, 2019 · 7 comments
Open

Exceptions to prefixing hook names #201

joyously opened this issue Feb 12, 2019 · 7 comments

Comments

@joyously
Copy link

joyously commented Feb 12, 2019

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
Copy link
Contributor

jrfnl commented Feb 13, 2019

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
Copy link
Member

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
Copy link
Author

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

@timelsass
Copy link
Member

timelsass commented Mar 26, 2019

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
Copy link
Author

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
Copy link
Member

timelsass commented Mar 26, 2019

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
Copy link
Member

dingo-d commented May 18, 2019

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 🙂

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

4 participants