Open swissspidy opened 2 weeks ago
@swissspidy In my opinion, there are two aspects to this "ask":
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;
}
}
__,_,-,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.@swissspidy I'm going to answer in reverse order
- 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.
These are both aspects of the same larger problem - preventing name collisions -, but they are distinctly different aspects of the problem.
- 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.
_
wordpress
wp
__
- 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
-
- 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.is
wc
js
as
set
get
add
css
app
woocommerce
- This would prevent WooCommerce from using the sniff.acme
- This would prevent ACME from using the sniff.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 https://github.com/WordPress/WordPress-Coding-Standards/issues/2480#issuecomment-2323033634 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.
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.
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.
Related:
2480
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
In our use case, it would be helpful if we could customize the blocklist.
The counter argument was:
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
orplugin
.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)