Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add check for register_uninstall_hook #716

Open
terriann opened this issue Jun 24, 2022 · 1 comment
Open

Add check for register_uninstall_hook #716

terriann opened this issue Jun 24, 2022 · 1 comment

Comments

@terriann
Copy link
Contributor

What problem would the enhancement address for VIP?

Because the contents of /wp-content/plugins and /wp-content/themes are setup as read-only directories on the VIP Go platform, admin users cannot uninstall plugins from within the wp-admin. This results in the function register_uninstall_hook never being able to run.

A VIP customer can deactivate a plugin through the admin, but uninstallation can only be done by removing the plugin from the GitHub repo and deploying the changes. If a customer needs to run any clean-up steps associated with a plugin or theme's use of register_uninstall_hook, their development team has to reverse engineer the code to execute any functionality that needs to run.

This is not entirely unique to VIP, but only impacts WordPress environments where the /wp-content/plugins and /wp-content/themes are read-only, so it may not be a good fit for the WordPress Coding Standards.

Describe the solution you'd like

A sniff that will flag places where the register_uninstall_hook function is used. Preferably as an error because it is functionality that will not work.

What code should be reported as a violation?

register_uninstall_hook( __FILE__, 'your_prefix_uninstall' );

What code should not be reported as a violation?

I'm unaware of cases where this might be detected as a false positive.

Additional context

None applicable

@jrfnl
Copy link
Collaborator

jrfnl commented Jun 25, 2022

@terriann I'm wondering if this is not turning the world upside down as - as a general practice -, having an uninstall script in place is a good thing (and more plugins/themes should have this!).

With that in mind, I agree this is not a good fit for WPCS as you are basically asking to flag a good practice as bad.
I also think this should probably be a warning, not an error and needs a highly customized notification message explaining why it is discouraged in the VIP environment + what the alternative is.

More than anything, I wonder if it should be flagged at all.

The typical use of an uninstall script is to clean up after a plugin:

  • Remove any options created for the plugin from the options database table.
  • Remove any custom table columns created for the plugin from the database.
  • Remove any custom tables created for the plugin from the database.

It is rare for a plugin/themes to touch the file system and if they do, generally speaking, files should be placed in the wp-content/uploads directory, which I don't expect to be read-only on VIP and if that's not read-only, then cleaning up a custom directory created within wp-content/uploads from within an uninstall script should work fine.

All of these, seem like good things to do when uninstalling a plugin/theme, the fact that actual uninstall won't work and needs to be done via the git repo, is a whole other matter, as that's not something the plugin/theme uninstall script - which IIRC is called before the files are removed - does in the first place.

Also, flagging the use of register_uninstall_hook() will not catch the existence of a uninstall.php script, which AFAIK is the recommended way of performing uninstall steps and does not need to be registered.

I wonder if instead this should be solved by the VIP platform short-circuiting the uninstall process between running the plugin/theme install script & doing the actual removal of a plugin/theme and showing an informative message saying something along the lines of "the uninstall script has been run, now remove the plugin/theme from the git repo" ?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants