WordPress / plugin-check

A repository for the new Plugin Check plugin from the WordPress Performance and Plugins Team.
https://wordpress.org/plugins/plugin-check/
GNU General Public License v2.0
269 stars 53 forks source link

Check: Generic function/class/define/option prefix names #523

Open davidperezgar opened 4 months ago

davidperezgar commented 4 months ago

This check aims to detect short or common prefixes that could cause fatal errors in WordPress installations. We consider as an error for this check.

How could develop this check?

We need to have a white list of common function starts. Actually we have in our internal scanner: __,_,-,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

And after, We check the list of named functions that are outside a Class, and a list o named Classes. Maybe we can go to Namespaces as well.

Our description to developers:

Generic function/class/define/namespace/option names

  All plugins must have unique function names, namespaces, defines, class and option names. This prevents your plugin from conflicting with other plugins or themes. We need you to update your plugin to use more unique and distinct names.

A good way to do this is with a prefix. For example, if your plugin is called "Easy Custom Post Types" then you could use names like these:   function ecpt_save_post() define( ‘ECPT_LICENSE’, true ); class ECPT_Admin{} namespace EasyCustomPostTypes; update_option( 'ecpt_settings', $settings );   Don't try to use two (2) or three (3) letter prefixes anymore. We host nearly 100-thousand plugins on WordPress.org alone. There are tens of thousands more outside our servers. Believe us, you’re going to run into conflicts.   You also need to avoid the use of _ (double underscores), wp , or _ (single underscore) as a prefix. Those are reserved for WordPress itself. You can use them inside your classes, but not as stand-alone function.

Please remember, if you're using _n() or __() for translation, that's fine. We're only talking about functions you've created for your plugin, not the core functions from WordPress. In fact, those core features are why you need to not use those prefixes in your own plugin! You don't want to break WordPress for your users.

Related to this, using if (!function_exists(‘NAME ‘)) { around all your functions and classes sounds like a great idea until you realize the fatal flaw. If something else has a function with the same name and their code loads first, your plugin will break. Using if-exists should be reserved for shared libraries only.

Remember: Good prefix names are unique and distinct to your plugin. This will help you and the next person in debugging, as well as prevent conflicts.   Example(s) from your plugin:

swissspidy commented 4 months ago

So this basically the WordPress.NamingConventions.PrefixAllGlobals PHPCS sniff in WPCS?

davidperezgar commented 4 months ago

Yes! You're right. We're asking 4 letters and WPCS has 3 letters. I'm going to make an issue for that.

davidperezgar commented 4 months ago

Ok, I've asked to update the 4 letters in the developer documentation and PrefixAllGlobals sniff.

ernilambar commented 3 months ago

I was digging more about this issue and I found out that PrefixAllGlobalsSniff has already a list of restricted prefixes in variable $prefix_blocklist. But it is not customizable as of now.

Ref: https://github.com/WordPress/WordPress-Coding-Standards/blob/develop/WordPress/Sniffs/NamingConventions/PrefixAllGlobalsSniff.php#L86-L91

protected $prefix_blocklist = array(
    'wordpress' => true,
    'wp'        => true,
    '_'         => true,
    'php'       => true, // See #1728, the 'php' prefix is reserved by PHP itself.
);

If we could make this array customizable then we can feed our own list of restricted prefixes. But also we need to find a way to run this sniff without passing list of prefixes and only check for those restricted prefixes. Currently this sniff runs only if we give a list of allowed prefixes.

swissspidy commented 3 months ago

Sounds like a good upstream contribution to WPCS :)

davidperezgar commented 3 months ago

Yeah!

ernilambar commented 3 months ago

Hope you don't mind me pinging here. CC @jrfnl for the the input and suggestion whether this customization is feasible in WPCS repo.

jrfnl commented 3 months ago

@ernilambar Please read the linked issues before pinging people: https://github.com/WordPress/plugin-check/issues/523#issuecomment-2241002831

As for:

If we could make this array customizable then we can feed our own list of restricted prefixes.

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

But also we need to find a way to run this sniff without passing list of prefixes and only check for those restricted prefixes. Currently this sniff runs only if we give a list of allowed prefixes.

I suggest opening separate new issues for each of these questions in the WPCS repo to discuss (also see the existing issue, though that's a different ask/decision point: WordPress/WordPress-Coding-Standards#2467).

ernilambar commented 3 months ago

Another approach could be modifying PrefixAllGlobalsSniff.php class as per PCP requirement and using composer-patches for the implementation.

Cons of this approach is addition of burden to maintain the patch.

This is PR which I was testing to see if we can implement the feature: https://github.com/ernilambar/WordPress-Coding-Standards/pull/1/files

In this PR:

swissspidy commented 3 months ago

Instead of patching I‘d just port this sniff over to this repo and modify it to our needs

jrfnl commented 3 months ago

Instead of patching I‘d just port this sniff over to this repo and modify it to our needs

That would be violating both the copyright as well as the license.

Instead of trying to reinvent the wheel, you may want to try to collaborate and contribute to WPCS instead.

swissspidy commented 3 months ago

That‘s what we suggested above :-) I was just thinking that a fork in the meantime allows for quicker iteration.

jrfnl commented 3 months ago

@swissspidy Quicker iteration ? How ? I suggested 10 days ago to open an issue in the WPCS repo and nobody has even bothered.

frantorres commented 3 months ago

Just for context on how this is right now being handled in the internal review script.

It's using a code parser to get all the relevant strings that need to be prefixed:

With all of this in hand, it removes some false positives, like common apply_filters that authors use because of that's an ok to use case integrating with some other plugin or even the core.

Then you have an array of all the names that we expect to be prefixed, processing it you get all possible prefixes included in them (by taking out all the possibilities of substrings separated by _ , - and ) and then you deduct which prefixes are common among them (by generating another array of coincidences prioritizing longer prefixes).

This has some kind of statistics involved as some plugins might have not-exactly the same but look-alike prefixes and that might be fine. It's considered a prefix as long it's (not considering namespaces):

Of course this has false positives.

Having this, we can check if that prefix is fine (checking the size and those cases of common not valid prefixes like set get ) or it is not, and in any case, anything else that is not under a considered common prefix is considered not prefixed.

Example of output:

# This plugin is using the prefix "klrmj" for 7 element(s).
# This plugin is using the prefix "kl" for 6 element(s).

# The prefix 'kl' is too short, we require prefixes to be over 4 characters.
plugin.php:61 function kl_init
plugin.php:71 class kl
plugin.php:81 global $kl_data;

# Cannot use "deactivate" as a prefix.
plugin.php:21 function deactivate_plugin
# Cannot use "enqueue" as a prefix.
plugin.php:279 function enqueue_styles

# Looks like there are elements not using common prefixes.
plugin.php:18 define('VERSION', '1.0.0');
plugin.php:86 update_option($option_name, $serialized_plugin_data);
              ↳ Detected name: 'plugin_data'
plugin.php:34 function standard_shortcode
jrfnl commented 3 months ago

Sounds similar to functionality already in the WPCS PrefixAllGlobals sniff. Has any of you ever tried running it with the --report=info option ?

davidperezgar commented 3 months ago

I couldn't manage to run PrefixAllGlobals with a simple php @jrfnl . Could you give me some guidance on how to run this sniff?

jrfnl commented 3 months ago

@davidperezgar phpcs -ps . --standard=WordPress --sniffs=WordPress.NamingConventions.PrefixAllGobals --report=full,source,info --runtime-set prefixes my_prefix,tgmpa

davidperezgar commented 3 months ago

Thanks! I'll check it out!!

davidperezgar commented 2 months ago

Hello @jrfnl Why is necessary to give prefixes? It isn't supposed to check the number of prefixes instead? If not, how could I check the number of prefixes? Thanks

jrfnl commented 2 months ago

Hello @jrfnl Why is necessary to give prefixes? It isn't supposed to check the number of prefixes instead? If not, how could I check the number of prefixes? Thanks

The sniff needs something to check against. Without a prefix/prefixes, the sniff can't flag anything.

davidperezgar commented 2 months ago

Yes, but we want to check all functions, classes and variables of a file. It should check any and tell if it's correct.

swissspidy commented 2 months ago

It does make sense that, if you don't pass prefixes, you don't know whether a function has the right prefix or not.

The only thing we have is a list of disallowed prefixes (__,_,-,set,get,is,save,show,update,add,...)

As per above suggestion I now created two distinct feature requests on the WPCS repo that would cover this use case:

frantorres commented 2 months ago

Right now in the internal script as described here it gathers all names and deduces statistically if there is something that could be considered a prefix, in that case, if it is not in the list of not allowed prefixes and it's long enough, it considers it a prefix of the plugin.

It also has some leeway, it may consider more than one prefix ok (some authors give different names to prefixes of options, functions, and/or namespaces). For example, it's quite common to use the Vendor name in the namespace and something else for prefixing option names.

This can also give false positives, it is more resource intensive, and needs to take into account all plugin files at once.

Also, I think it's quite opinionated, so I'm not sure if this approach is something ready for WPCS, but maybe for Plugin Check as a tool to flag and help authors identify possible issues?

swissspidy commented 2 months ago

Oh, I completely missed this.

That sounds like something that could work for Plugin Check indeed.

  1. Scan the code to find likely prefixes
  2. Pass those to the PrefixAllGlobals sniff
  3. Surface anything flagged by the sniff to the user as a warning, explaining which prefixes were used
davidperezgar commented 2 months ago

Yes, that could be the best approach.

ernilambar commented 1 month ago

That sounds like something that could work for Plugin Check indeed.

  1. Scan the code to find likely prefixes

@swissspidy What could be the best approach for scanning files and find potential prefixes? Any suggestions?

swissspidy commented 1 month ago

Well I'd start with the implementation you already have in your internal script, see https://github.com/WordPress/plugin-check/issues/523#issuecomment-2282814008

I don't know anything about that, so you'd have to ask within your team :)

ernilambar commented 1 month ago

In internal script, nikic/php-parser package is used.

swissspidy commented 1 month ago

Then you can start with that :)

davidperezgar commented 1 month ago

We have to save all prefixes and detect the ones that are less than 4 letters.

davidperezgar commented 1 month ago

This library provides a layer of abstraction to create checks more easily.

ernilambar commented 3 weeks ago

@davidperezgar I tried to split our only the Parser from the scanner but I found it is pretty much tied to other classes in the scanner. I am consulting with Fran about just developing lean and this PHP class just to find the potential prefixes. Will update here when there is some progress.

davidperezgar commented 3 weeks ago

Ok! I think this check is tough, but let's keep coding ;)

frantorres commented 3 weeks ago

Yes, all checks are tied up to some extent to the Parser class but I think this one can be easily separated, we just need to bring with us some functions like the ones that extract names.