PrefixAllGlobalsSniff: always flag blocked prefixes

[swissspidy]

Is your feature request related to a problem?

This is related https://github.com/WordPress/plugin-check/issues/523. In the Plugin Check plugin we need to find a way to flag the lack of prefixes for global functions.

Describe the solution you'd like

PrefixAllGlobalsSniff has a hardcoded blocklist of disallowed prefixes:

https://github.com/WordPress/WordPress-Coding-Standards/blob/7f766304b0654cee7c1dfa8e548f65fce197e05c/WordPress/Sniffs/NamingConventions/PrefixAllGlobalsSniff.php#L78-L91

However, this blocklist is only used to compare it against the user-provided prefixes list.

So if I provide myplugin,wp as valid prefixes, the sniff will warn about wp as disallowed and remove it from the list. After that, all functions are checked for the myplugin prefix.

Now, when I don't pass any custom prefixes, the sniff doesn't do anything, as it doesn't know what valid prefix to look for.

However, since we already know that wp is not allowed, the sniff could always check for that regardless.

So I am proposing that, if no list of valid prefixes is passed, the sniff should always at least flag the blocked ones.

In other words, if I don't pass any configuration wp_awesomefunc() would get reported as having a disallowed prefix.

Additional context (optional)

• [x] I intend to create a pull request to implement this feature.

[davidperezgar]

That's a good one @swissspidy !

[jrfnl]

I'm not keen on this suggestion for the following reasons:

1. This sniff is about prefixing all globals. You're suggesting changing the complete premise of the sniff to one of "banning certain prefixes completely". To me that is not an acceptable change. It would also be very inhibitive for people using WPCS for in-company/private plugins/themes.
2. The ask is "flag the lack of prefixes for global functions", but if this would be handled via the PrefixAllGlobals sniff, it would not just affect global functions, but also namespace names, classes, variables, hook names, constants etc.

All in all, I don't think the proposed change is acceptable.

It could be considered to have a separate sniff which only checks global function declaration names against a banned prefix list, though I'm not fully convinced of the usefulness of such a sniff as what we want is that devs use a consistent prefix, while selectively flagging unwanted prefixes would undermine that advise.

As an alternative, what about if the PrefixAllGlobals sniff would get a new error for "no prefixes passed" ? with the recommendation that plugins/themes should always use a prefix/prefixes and pass the prefix(es) selected to WPCS ?

[swissspidy]

Thanks, I appreciate the quick feedback!

It could be considered to have a separate sniff which only checks global function declaration names against a banned prefix list, though I'm not fully convinced of the usefulness of such a sniff as what we want is that devs use a consistent prefix, while selectively flagging unwanted prefixes would undermine that advise.

From a WPCS point of view that definitely makes sense. For the plugins team it's different though, at least that's my understanding as an outsider. So I think we'll have to create such a new sniff for Plugin Check to accommodate our needs.

As an alternative, what about if the PrefixAllGlobals sniff would get a new error for "no prefixes passed" ? with the recommendation that plugins/themes should always use a prefix/prefixes and pass the prefix(es) selected to WPCS ?

This would help people getting started with WPCS, as they might otherwise never realize that the sniff is silently not running. So that makes sense to me 👍

[jrfnl]

It could be considered to have a separate sniff which only checks global function declaration names against a banned prefix list, though I'm not fully convinced of the usefulness of such a sniff as what we want is that devs use a consistent prefix, while selectively flagging unwanted prefixes would undermine that advise.

From a WPCS point of view that definitely makes sense. For the plugins team it's different though, at least that's my understanding as an outsider.

I'm not so sure it is different. After all, unless you require a consistent - and preferably unique - prefix(es) in all plugins/themes, that "ban" list would just keep expanding until it is a dictionary.

That's something for the plugin team to discuss further though and outside the scope of WPCS.

As an alternative, what about if the PrefixAllGlobals sniff would get a new error for "no prefixes passed" ? with the recommendation that plugins/themes should always use a prefix/prefixes and pass the prefix(es) selected to WPCS ?

This would help people getting started with WPCS, as they might otherwise never realize that the sniff is silently not running. So that makes sense to me 👍

Let's give other people some time to pitch in to see if anyone has a good reason why this shouldn't be done.

If there are no objections, this is a change for which a PR would be welcome.

Notes for anyone who wants to work on this:

• I think a warning would probably be the right error level.
• This will need a private property in the sniff to remember whether the warning has already been thrown or not, as otherwise it would be thrown for every single function/namespace/class/variable/constant/etc definition and would become silly noisy without conveying any new information. Throwing the warning once should suffice.