PrefixAllGlobalsSniff: extend blocklist

[swissspidy]

Is your feature request related to a problem?

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

Related:

• PrefixAllGlobalsSniff: always flag blocked prefixes #2480

Describe the solution you'd like

PrefixAllGlobalsSniff has a hardcoded blocklist of disallowed prefixes:

WordPress-Coding-Standards/WordPress/Sniffs/NamingConventions/PrefixAllGlobalsSniff.php

Lines 78 to 91 in 7f76630

	/**
	* Prefix blocklist.
	*
	* @since 0.12.0
	* @since 3.0.0 Renamed from `$prefix_blacklist` to `$prefix_blocklist`.
	*
	* @var array<string, true> Key is prefix, value irrelevant.
	*/
	protected $prefix_blocklist = array(
	'wordpress' => true,
	'wp' => true,
	'_' => true,
	'php' => true, // See #1728, the 'php' prefix is reserved by PHP itself.
	);

In our use case, it would be helpful if we could customize the blocklist.

The counter argument was:

Making it customizable would allow for people to clear out the list, resulting in the opposite effect.

So what we could do instead is only allow extending the list, not removing items from it.

This way, in our project we can easily prevent functions starting with some words known not be unique, like e.g. admin or plugin.

So something like phpcs -ps . --standard=WordPress --sniffs=WordPress.NamingConventions.PrefixAllGobals --report=full,source,info --runtime-set prefix_blocklist admin,plugin would flag the following functions:

• wordpress_myfunc()
• wp_myfunc()
• admin_myfunc()
• plugin_myfunc()

Additional context (optional)

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

[jrfnl]

@swissspidy In my opinion, there are two aspects to this "ask":

1. Should the list of prefixes which are being blocked be expanded and if so, with which additional prefixes ?
   I'm open to expanding the list with some additional prefixes for which it makes sense to block them for all plugins (i.e. including private/in-company plugins).
   Proposals for expansions to the list can be discussed in this ticket.
2. Should the list of prefixes being blocked be made customizable ?
   Other than for the plugin team, I don't see a use-case for this and I don't think the maintenance burden of a feature which has only one user should be put on WPCS .
   This can still be handled by any competent dev in the plugin team itself without putting the maintenance burden on us AND without violating license/copyright.
   Some example/pseudo-code for how this could be handled:

namespace Whatever\You\Use\Sniffs;

use PHP_CodeSniffer\Sniffs\Sniff;
use PHPCSUtils\BackCompat\Helper;
use WordPressCS\WordPress\Helpers\RulesetPropertyHelper;
use WordPressCS\WordPress\Sniffs\NamingConventions\PrefixAllGlobalsSniff;

class PluginTeamPrefixAllGlobals implements Sniff {
    private $wpcs_sniff;
    public function __construct() {
        $wpcs_sniff = new PrefixAllGlobals();
    }
    public function register() {
        return $wpcs_sniff->register();
    }
    public function process( File $phpcsFile, $stackPtr ) {
        $prefixes = [];

	// Get the prefixes in the same way as WPCS does.

        // Allow overruling the prefixes set in a ruleset via the command line.
        $cl_prefixes = Helper::getConfigData( 'prefixes' );
        if ( ! empty( $cl_prefixes ) ) {
            $cl_prefixes = trim( $cl_prefixes );
            if ( '' !== $cl_prefixes ) {
                $prefixes = array_filter( array_map( 'trim', explode( ',', $cl_prefixes ) ) );
            }
        }

        $prefixes = RulesetPropertyHelper::merge_custom_array( $prefixes, array(), false );
        if ( empty( $prefixes ) ) {
            // No prefixes passed, nothing to do.
            return;
        }

	// Do your own pre-validation and pass the prefixes to WPCS.
        $wpcs_sniff->prefixes = $this->prevalidate_prefixes($prefixes);

        return $wpcs_sniff->process_token( $phpcsFile, $stackPtr );
    }

    private function prevalidate_prefixes( $prefixes ) {
        // Do whatever else you want to prevalidate prefixes, like flag additional prefixes which shouldn't be allowed for plugins in the plugin directory.
        return $prefixes;
    }
}

[swissspidy]

1. The following blocklist was mentioned over at Check: Generic function/class/define/option prefix names plugin-check#523: __,_,-,set,get,is,save,show,update,add,wordpress,wp,woocommerce,wc,table,html,css,js,input,output,plugin,plugins,my_plugin,myplugin,prefix,my_custom,custom,as,widget,oauth2,handle,generate,post,site,remove,filter,display,init,start,check,sync,cache,phpmailer,declare,register,enable,include,search,upgrade,update,setup,create,admin,load,theme,fetch,apply,clear,verify,test,insert,acme,app,render,rest. A non-exhaustive list of some commonly seen prefixes in submitted plugins. I don't think all of them are worth adding in WPCS, so not really worth pursuing from our end.
2. Thanks for the suggestion, very helpful! My understanding is that we would need PrefixAllGlobalsSniff: always flag blocked prefixes #2480 for this to work as we do not have a list of valid prefixes, only a blocklist of disallowed ones. But without the allowlist, the sniff currently won't process any tokens.

[jrfnl]

@swissspidy I'm going to answer in reverse order

2. Thanks for the suggestion, very helpful! My understanding is that we would need PrefixAllGlobalsSniff: always flag blocked prefixes #2480 for this to work as we do not have a list of valid prefixes, only a blocklist of disallowed ones. But without the allowlist, the sniff currently won't process any tokens.

Not necessarily. This issue and #2480 address different problems.

• PrefixAllGlobalsSniff: always flag blocked prefixes #2480 is about a complete ban on symbols from starting with certain prefixes.
• This issue is about preventing plugin/theme owners from choosing an ill-advised prefix for their plugin/theme.

These are both aspects of the same larger problem - preventing name collisions -, but they are distinctly different aspects of the problem.

1. The following blocklist was mentioned over at Check: Generic function/class/define/option prefix names plugin-check#523:

That's a large list, so I think we need to look at each individual prefix and discuss them individually.

Keep in mind: the way the blocklist in WPCS works, is that something which is on the blocklist is not allowed to be the prefix, it doesn't block a prefix starting with something on the blocklist.
So: wp is not allowed as a prefix, but wpcs would be allowed, _ is not allowed, but ______ would be allowed (though still not a good idea and would be flagged by the ValidFunctionName sniff as such as well).

I'll group the prefixes now based on my opinion.
Keep in mind, this is only my opinion and opinions of other interested parties may differ (and I'd like to hear additional opinions!).

I've also not done a check against existing plugins. If a popular existing plugin would be blocked from using the sniff if a certain prefix would be added to the list, that would be a good reason not to accept the suggestion, so the "Acceptable suggestions" list will still need additional verification.

Already on the list

• _
• wordpress
• wp

Acceptable suggestions

• __ - Mind: this would be blocked by the 3 char minimum anyway, but as this is a reserved prefix for PHP I think adding it to the list is a valid suggestion.
• save
• show
• update
• table
• html
• input
• output
• plugin
• plugins
• my_plugin
• myplugin
• prefix
• my_custom
• custom
• widget
• oauth2
• handle
• generate
• post
• site
• remove
• filter
• display
• init
• start
• check
• sync
• cache
• phpmailer - If prefixes used by external dependencies of WP are being added, what about also adding id3, getid3, simplepie, requests, ixr, sodium (and possibly more) for consistency and completeness ?
• declare
• register
• enable
• include
• search
• upgrade
• update
• setup
• create
• admin
• load
• theme
• fetch
• apply
• clear
• verify
• test
• insert
• render
• rest

Invalid

• - - Would be blocked by the character minimum anyway, would also be blocked by the "valid symbol" check as this is not a valid character to start symbol names with in PHP, so I don't see any point in adding it.

Would already be blocked by the 3 character minimum for prefixes anyway, so no point in adding this

• is
• wc
• js
• as

Would be blocked by the upcoming 4 character minimum for prefixes anyway, so no point in adding this

• set
• get
• add
• css
• app

Other unacceptable suggestions

• woocommerce - This would prevent WooCommerce from using the sniff.
• acme - This would prevent ACME from using the sniff.

[swissspidy]

This issue is about preventing plugin/theme owners from choosing an ill-advised prefix for their plugin/theme.

While I did not open this issue with this intention, I do now understand the sniff's purpose better thanks to your elaboration.

These are both aspects of the same larger problem - preventing name collisions -, but they are distinctly different aspects of the problem.

Acknowledged. As per #2480 (comment) I think we'll create a new sniff to address this for plugin reviews.

That's a large list, so I think we need to look at each individual prefix and discuss them individually.

Thanks a lot for going through these & grouping them!

At first glance the "Acceptable suggestions" looks good for further analysis.

The plugins team could probably provide further input regarding commonly seen names potentially causing collisions.

If prefixes used by external dependencies of WP are being added, what about also adding id3, getid3, simplepie, requests, ixr, sodium (and possibly more) for consistency and completeness ?

Makes sense to me.

[jrfnl]

Thanks a lot for going through these & grouping them!

At first glance the "Acceptable suggestions" looks good for further analysis.

The plugins team could probably provide further input regarding commonly seen names potentially causing collisions.

Let's leave this ticket open for a few weeks to allow for other people to pitch in and give their opinion and for the plugin team to provide further input.

After that, I'd welcome a PR to address this ticket.