WordPress / WordPress-Coding-Standards

PHP_CodeSniffer rules (sniffs) to enforce WordPress coding conventions
MIT License
2.56k stars 487 forks source link

Restrict the use of private functions or classes that are not meant to be extended #723

Open grappler opened 7 years ago

grappler commented 7 years ago

Once https://github.com/WordPress-Coding-Standards/WordPress-Coding-Standards/pull/633 has been merged I will work on a list of private WordPress functions that should not be used by themes and plugins.

https://codex.wordpress.org/Category:Private_Functions

and WP_Internal_Pointers which is not meant to be extended.

The following are exceptions that would still need to pass.

remove_action( 'admin_enqueue_scripts', array( 'WP_Internal_Pointers', 'enqueue_scripts' ) );
remove_action( 'admin_print_footer_scripts', array( 'WP_Internal_Pointers', 'pointer_wp390_widgets' ) );
GaryJones commented 6 years ago

633 has been merged ;-)

spacedmonkey commented 4 years ago

I was discuss this with @GaryJones and I agree we should try and stop developers using private functions. It is pretty simple to find private functions / classes in core, as they all marked as @access private.

I have generated a list of private functions and classes that can be found here.

jrfnl commented 4 years ago

If anyone wants to work on this, this wouldn't be too hard a sniff to create.

For the functions, it would be a case of combining the list created by @spacedmonkey with the AbstractFunctionRestrictionsSniff which already contains all the logic.

IMO that sniff should only look at global functions marked as private via the docblock. Class methods should not be addressed by the sniff. Moreover: those should be fixed in WP Core to have private set for visibility.

For the classes, it would be a case of combining the list created by @spacedmonkey with the AbstractClassRestrictionsSniff which, again, already contains all the logic.

For both sniffs, there are implementations available within the code base already which can be used as an example to guide you.

jrfnl commented 4 years ago

Note: regarding the remark by @grappler about certain callbacks which shouldn't be reported: at this time, the abstract sniffs do not look at callbacks, so that is not an issue which (at this time) would need special handling.

jrfnl commented 4 years ago

Open question: How should the "private classes" sniff handle it when a private class is being extended ? For some of the classes I see on the list - like WP_List_Table -, that would be reported via the abstract sniff, but should be allowed AFAICS. Or rather: that is the intended use.

I also think a critical look at the function list would be a good idea.

Until recently, functions like _deprecated_function() would have been on that list too, while it is common - and a good idea - for plugins to use those too (which is why for the _deprecated_....() functions this has been changed now).

I imagine there may be some more functions like that on the list, in which case I would encourage changing their designation in the docblock in WP core, rather than forbidding their use.

dingo-d commented 4 years ago

Are 'private classes' meant to be final? If that is the case, would there be any harm in just patching the core to actually add the final keyword which will prevent any extending of the class?

I know that we should just look at the standards but I kinda feel like core could do with some architectural changes...

jrfnl commented 1 year ago

Any sniff like this would greatly benefit from tooling to create the initial lists and allow for updating the lists with ease. See #1803