WordPress.org

Make WordPress Core

Opened 7 years ago

Closed 7 years ago

Last modified 7 years ago

#5625 closed enhancement (fixed)

Include a separate uninstall for plugins

Reported by: arickmann Owned by: jacobsantos
Milestone: 2.7 Priority: normal
Severity: normal Version: 2.6
Component: Administration Keywords: plugins has-patch
Focuses: Cc:

Description

There is a clear difference between deactivate and uninstall. Deactivating a plugin should not remove all of its settings from the database but currently there is no central way for plugins to uninstall, and therefore no reminder to plugin developers to do so.

The plugins.php file can accomodate a separate uninstaller if one is included with the plugin.

I have attached a plugin that shows how it can be achieved.

Attachments (13)

fun_with_uninstallation.php (3.5 KB) - added by arickmann 7 years ago.
Plugin that adds uninstallation capability.
fun_with_uninstallation0.2.php (5.6 KB) - added by arickmann 7 years ago.
Version 0.2 that uses a hook.
sample_uninstallable_plugin.php (3.1 KB) - added by arickmann 7 years ago.
A sample plugin that implements the uninstall process.
wp-admin_includes_plugin.php.diff (3.5 KB) - added by arickmann 7 years ago.
Plugin functions from wp-admin/includes/plugin.php
wp-includes_plugin.php.diff (2.3 KB) - added by arickmann 7 years ago.
Plugin functions from wp-includes/plugin.php
wp-admin_plugins.php.diff (2.9 KB) - added by arickmann 7 years ago.
Plugins page from wp-admin/plugins.php
5625.r6603.diff (11.1 KB) - added by darkdragon 7 years ago.
Patch that allows for dynamically adding of options and tables. Full patch with all of other three patches
5625.r6603.2.diff (11.1 KB) - added by darkdragon 7 years ago.
Switches $tables and $options parameters, based off of arickmann patches
7372.r8529.patch (13.2 KB) - added by jacobsantos 7 years ago.
Initial uninstall hook support
5625.r8539.diff (15.9 KB) - added by santosj 7 years ago.
Implements Strider's suggestions with support for uninstall.php and only display uninstall when deactivated
5625.r8555.diff (12.7 KB) - added by santosj 7 years ago.
Calls uninstall hook on delete plugin
5625.r8557.diff (12.6 KB) - added by santosj 7 years ago.
Adds uninstall hook to delete process, with support for hook and uninstall file.
5625.8586.diff (1.0 KB) - added by santosj 7 years ago.
Set @since phpdoc tags. Based off of r8586

Download all attachments as: .zip

Change History (67)

@arickmann7 years ago

Plugin that adds uninstallation capability.

comment:1 @DD327 years ago

In my mind it shouldnt be a seperate file which is checked for, Some plugins use a folder, Some are simply a single file plonked into the plugins folder.

I think it should probably be something similar to this:

* Click uninstall
* Page loads and includes active plugins, if specified plugin is not activated, include it anyway
* plugin uses register_uninstall_hook() to register an uninstall proceedure for itself
* If hook has been registered for current plugin:
   * plugins.php calls 'uninstall_plugin-{$plugin_name}'
   * Redirect to main page "Plugin uninstalled"(Possibly at this stage the files could be deleted if requested, That fits in with the auto-install of plugins in another ticket)
  Else:
   * Redirect to main page "Plugin has not registered an uninstall method"

I'd suggest something similar to Gallery 2's plugin install/uninstall method, I was pretty impressed by that.

comment:2 @darkdragon7 years ago

I agree with DD32, it should be a hook.

comment:3 @darkdragon7 years ago

  • Keywords plugin-management added

comment:4 follow-up: @arickmann7 years ago

I agree that it should be a hook, but I was wary about including a plugin file that is not active; there could be a number of reasons why it isn't active.

If it can be included in a way that activates the hook, doesn't break any class references, but doesn't run any other code then that would be ideal.

comment:5 in reply to: ↑ 4 ; follow-up: @DD327 years ago

Replying to arickmann:

I agree that it should be a hook, but I was wary about including a plugin file that is not active; there could be a number of reasons why it isn't active.

Thats true, I guess if WP was to only load what it needs to (ie. no other plugins, and limited functions) when uninstalling a deactivated plugin it might work.. But if the user wants to uninstall the plugin because its corrupt, or malicious, then idealy the user will not want WP to load the plugin.

comment:6 in reply to: ↑ 5 @arickmann7 years ago

Replying to DD32:

Thats true, I guess if WP was to only load what it needs to (ie. no other plugins, and limited functions) when uninstalling a deactivated plugin it might work.. But if the user wants to uninstall the plugin because its corrupt, or malicious, then idealy the user will not want WP to load the plugin.

Ok. So the way I see it there are two problems here:

  1. How do we ring-fence the undesirable code;
  2. How do we choose to run the uninstall code;

This is what I now have in mind. If the solution seems reasonable I will consider the actual code necessary:

  • The plugin calls register_uninstaller( $plugin_path ); when it is activated which adds it to a list of plugins with uninstallers;
  • The developer wraps the active plugin content in an if statement that evaluates whether the plugin is active.
  • If the plugin is not active, and has an uninstaller, the option is available to uninstall it and if called it runs wrapped in an output buffer in the same way as when it is initially installed.
  • If successful it removes itself from the list of uninstallers.

@arickmann7 years ago

Version 0.2 that uses a hook.

@arickmann7 years ago

A sample plugin that implements the uninstall process.

comment:7 @arickmann7 years ago

I've added a new version, plus an example plugin that implements the process.

This now does use a hook, registered when the plugin is activated.

The uninstall option is only offered if that hook has been registered. If the plugin is in the process of being uninstalled then the plugin is loaded, with the code being ringfenced by a function that checks to see if it is active (is_plugin_active).

The ringfencing code is placed at the top of the plugin so only the uninstall functions are exposed.

comment:8 @darkdragon7 years ago

Thanks man for taking ownership of this ticket. The work you have done so far is totally awesome.

However, I would very much rather it was part of the core and not a plugin. Dude, a plugin... that was included with WordPress! Awesome!

comment:9 @arickmann7 years ago

I think I have boiled it down to the essential processes now so have included a patch for three files within the core.

The processes work like this:

The plugin author can include the following function as part of their activation process:

register_plugin_assets( FILE , callback or NULL , array of database table names, array of WordPress option names );

When a plugin is deactivated, the plugins page will check to see if any assets have been registered and if so will provide the option to uninstall the plugin.

If the user chooses to uninstall it, the plugin will be included, the callback function will be registered as an action (uninstall_pluginname) and the action will be called, allowing other plugins to hook into this uninstall process.

After the callback has run the delete_plugin_assets function will be called and will delete the database tables and options mentioned.

It will then redirect to the plugins page and display the success or failure message.

The uninstall option will not be available while the plugin is activated.

I reconsidered the consequences of just installing the plugin, vs requiring the plugin author to ringfence their uninstall callback and I think they are acceptable. No actions or filters will be called and the user will be redirected back to the plugins page after the uninstall process.

@arickmann7 years ago

Plugin functions from wp-admin/includes/plugin.php

@arickmann7 years ago

Plugin functions from wp-includes/plugin.php

@arickmann7 years ago

Plugins page from wp-admin/plugins.php

comment:10 @arickmann7 years ago

  • Keywords has-patch added

comment:11 @darkdragon7 years ago

Looks good. So the register_plugin_assets() would go in the activation hook. I'm going to see about testing it.

comment:12 @darkdragon7 years ago

  • Keywords tested added

Wow! Just tested the system. I really do think you could split up the callback, options, and tables into three functions. You can also check to see if one the tables removed are part of WordPress protected tables to prevent accidental or maliciously deleting WordPress tables.

I hope your latest patch is committed.

For your information, for the phpdoc, there is a dash between the function name and description.

comment:13 @DD327 years ago

The only thing i can see wrong, is that some plugins create dynamic options while running ie cache_<md5>, It'd be nice to be able to append these options to the asset list.

As for the admin page, It'd be nice that if activated plugins had a link to the Configuration page for itself, I'd nearly suggest another column for Configure/Uninstall(when activated/deactivated), but there may allready be too many columns :)

(Apologies if you allready know this next stuff)
When creating patches, Its best to create it from the base WordPress directory, That way all teh changes files are included into one diff.

If you use windows, then TortoiseSVN is the recomended software by most, it'll allow you to pick which files in the directories you want to include in the diff.

comment:14 follow-up: @darkdragon7 years ago

Well, the configure I think is in another ticket or the discussion was part of a different ticket. That would be nice too. Using a similar system like arickmann's, it might as well be possible.

Also, it is a little bit difficult to figure out if the system appears well with the update message (but that might just be a mute point).

If the functionality was split up, that it would allow for dynamically adding options.

comment:15 in reply to: ↑ 14 @DD327 years ago

Replying to darkdragon:

Well, the configure I think is in another ticket or the discussion was part of a different ticket. That would be nice too. Using a similar system like arickmann's, it might as well be possible.

I was just throwing it out into the discussion for alternate ways for the uninstall link to be shown.

#4498 (Add configuration page links to Plugins page)

@darkdragon7 years ago

Patch that allows for dynamically adding of options and tables. Full patch with all of other three patches

comment:16 @darkdragon7 years ago

New patch implements my suggestions and allows for dynamically adding of options and tables to the already defined list of tables and options.

comment:17 @darkdragon7 years ago

I think the link at the side would be good for those of us who have higher resolutions (like me) who have the extra space to give. Having what could be four options, might be a little bit excessive.

comment:18 follow-ups: @DD327 years ago

Looks good to me :)

One final suggestion, Perhaps the tables and options parametres of register_plugin_assets() should be switched, Its more likely a plugin would register options than tables(If they choose to use that function rather than the individual ones)

I think the link at the side would be good for those of us who have higher resolutions (like me) who have the extra space to give. Having what could be four options, might be a little bit excessive.

I got to thinking about that too, I'm not sure if theres any redesign happening for the plugins table, but IMO, Maybe it could be changed so that there's a single "Actions" column, with pipe(|) seperated commands which can be applied to that plugin(activate/deactivate, edit, uninstall, configure, etc) - If anyone thinks thats a good idea, i'll open a ticket for it later, But once again, I'm not sure on what the redesign has got in mind for it(the version could also be shown under the plugins name too.. that'd give a bit more space).

comment:19 in reply to: ↑ 18 @darkdragon7 years ago

Replying to DD32:

Looks good to me :)

One final suggestion, Perhaps the tables and options parametres of register_plugin_assets() should be switched, Its more likely a plugin would register options than tables(If they choose to use that function rather than the individual ones)

I agree, so I'm going to do it.

I got to thinking about that too, I'm not sure if theres any redesign happening for the plugins table, but IMO, Maybe it could be changed so that there's a single "Actions" column, with pipe(|) seperated commands which can be applied to that plugin(activate/deactivate, edit, uninstall, configure, etc) - If anyone thinks thats a good idea, i'll open a ticket for it later, But once again, I'm not sure on what the redesign has got in mind for it(the version could also be shown under the plugins name too.. that'd give a bit more space).


That would be sweet! Depending on what the final pending design is. If this ticket and yours get in, I might just rewrite my plugin for 2.5 with these features, since well the plugin isn't exactly compatible with 2.3 and I need to upgrade WordPress before it is eventually hacked, which would be bad for me.

@darkdragon7 years ago

Switches $tables and $options parameters, based off of arickmann patches

comment:20 @darkdragon7 years ago

Just need dev support.

comment:21 in reply to: ↑ 18 @DD327 years ago

Replying to DD32:

I'm not sure if theres any redesign happening for the plugins table, but IMO, Maybe it could be changed so that there's a single "Actions" column, with pipe(|) seperated commands which can be applied to that plugin(activate/deactivate, edit, uninstall, configure, etc)

See #5660

comment:22 @arickmann7 years ago

Thanks for the guidance on patches ( I am using Tortoise but I'm new to it) and PHPDoc, I didn't know either of those things.

I wonder if there is a danger of putting uninstall too close to activate or edit in the table?

comment:23 follow-up: @codealsatian7 years ago

This is an awesome addition to WordPress! I do want to suggest an addition to the functionality.

I have a plugin that writes Post and Page meta options. It would be great if those options can also be dynamically added as plugin assets to be deleted during the uninstall.

comment:24 in reply to: ↑ 23 @arickmann7 years ago

Replying to codealsatian:

This is an awesome addition to WordPress! I do want to suggest an addition to the functionality.

I have a plugin that writes Post and Page meta options. It would be great if those options can also be dynamically added as plugin assets to be deleted during the uninstall.

I think this may be more suited to the callback function. I'm not sure it is common enough to need specific code to handle it.

comment:25 @strider727 years ago

Just my 2 cents RE user interface:

1) Please DO NOT add an "are you sure?" of any kind to deactivation. If a plugin author wants to put an "are you sure" or whatever on deactivate, he can already do that with the existing deactivation hook.

2) Please _do_ include an "are you sure?" with uninstall

3) Probably best if "uninstall" only appears on plugins page when the plugin is _not_ active -- i.e. if plugin is active, "uninstall" is simply not there -- this will avoid accidental clicks.

Sorry if I repeat things already done -- just saw this ticket and don't have chance to test now.

comment:26 @DD327 years ago

See Also: #7091 (Bulk managing of plugins & Plugin deletion)

comment:27 @DD327 years ago

  • Keywords plugins added
  • Version set to 2.6

See Also: #7091 (Bulk managing of plugins & Plugin deletion)

comment:28 @santosj7 years ago

  • Owner changed from anonymous to jacobsantos

comment:29 @santosj7 years ago

  • Keywords needs-patch added; has-patch tested removed

comment:30 @jacobsantos7 years ago

Andrew,

Do you mind if the first patch is simple, with focus on implementing a simple check for whether the user hooks into the uninstall action for that plugin? I would like the ability to register assets and allow WordPress to automatically uninstall it for me.

However, I think that only advanced users will do that. If a user is going to install the plugin, then I think they'll know enough to put all of the uninstall stuff in the uninstall hook.

@jacobsantos7 years ago

Initial uninstall hook support

comment:31 @jacobsantos7 years ago

  • Keywords has-patch needs-testing added; needs-patch removed

comment:32 @jacobsantos7 years ago

Initial uninstall hook support adds uninstall link and uninstall button to active plugins. The issue with the uninstall is that it needs to see if the hook exists and can't see it unless the plugin is active. It is then assumed that if the user just wants to deactivate the plugin then the uninstall won't exist.

It is possible to create another option for plugins who can be uninstalled, when they are activated. This will allow for the plugins to be uninstalled. It will require that the plugin be activated (without the activate hook being called) and then have the hook ran.

comment:33 @strider727 years ago

I agree with Jacob that it should be there when plugin is _not_ activated. In fact, I think the "uninstall" link should _ONLY_ be there when the plugin is not activated.

In order for that to work, the uninstall function would have to be a separate file in the plugin directory -- in essence a separate mini plugin of its own.

This is because we _can't_ load the main plugin file to check for an uninstall, because many plugins run code simply when the file is loaded, and we don't want that if it's not activated.

So... I suggest that we standardize a file inside the plugin folder named "uninstall.php". The uninstall link is added to the Plugin "Action" column if three criteria have been met:

1) The plugin is not currently activated.
2) The uninstall.php file exists.
3) At some point while it was activated, the plugin registered itself as having an uninstall function (this prevents us for having uninstall functions for plugins that haven't added anything to the DB).

Other notes:

  • The registration noted in criteria 3 would have to be persistent even after the plugin is deactivated.

comment:34 @santosj7 years ago

Yeah, the original patch did that, it allowed you to register "assets" which I think I is like what you propose. I think checking for the file could be a good idea for whether the plugin can be uninstalled instead of checking for a function.

comment:35 @santosj7 years ago

So basically register_uninstall_hook() really just updates an WordPress option that allows for the function to be called later. It is possible (through a major regex jujitsu) to pull the function and eval() it from the file.

comment:36 @strider727 years ago

I wouldn't regex it from the file. Only register it if the plugin actively registers it -- not just because uninstall.php exists.

Why?

1) We don't need an uninstall for a plugin that was never activated.
2) A plugin may or may not do something to the database, depending on how it's used. The author may only register it after it does X to the database.

So... register_uninstall_hook() should set WP to show the "uninstall" link if <a href="#comment:33">criteria 1 and 2</a> are met (i.e. uninstall.php exists and the plugin is not activated). Clicking the uninstall link should simply load that plugin's uninstall.php.

Or do we want to do a lot of abstracting and only have authors pass option names and (maybe) table names into new core WP functions? If we want to do the abstraction, we need to account for a lot of possibilities (records added to tables, columns added to tables, tables added to DB....)

comment:37 @santosj7 years ago

No on the abstracting. If that is the case, then Andrew's patch would be fine and should be committed. I think it would be easier to get a simple patch which supports the uninstall hook than Andrew's implementation.

I'll create a patch which implement your suggestions, Strider72, because I think they are good.

@santosj7 years ago

Implements Strider's suggestions with support for uninstall.php and only display uninstall when deactivated

comment:38 @santosj7 years ago

Latest patch needs testing. It supports both the uninstall.php and the uninstall hook.

comment:39 @santosj7 years ago

  • Keywords dev-feedback added

I'm not sure if this is going to get in 2.7, it would be nice if it could, but it needs testing.

comment:40 @santosj7 years ago

Going to upload a test plugin later.

comment:41 @ryan7 years ago

Why not do uninstall as part of delete? Who wants to "uninstall" a plugin but leave it laying around. Just delete it. Uninstall is just a cleanup step when deleting.

comment:42 follow-up: @santosj7 years ago

I figure that the user might want to keep the plugin, but it makes sense to do it that way. I suppose that would be even easier. However, I might want to uninstall a plugin but keep the plugin files in case I want to activate the plugin again.

How about I add the uninstall as part of the delete as well as the uninstall button? Would that satisfy the requirements enough to have this be committed? Or is only on delete or nothing?

comment:43 in reply to: ↑ 42 ; follow-up: @ryan7 years ago

How about I add the uninstall as part of the delete as well as the uninstall button? Would that satisfy the requirements enough to have this be committed? Or is only on delete or nothing?

I'm imagining the support questions about what the heck the difference between Delete and Uninstall are. I think it's such an edge case that it's not worth having an uninstall button.

@santosj7 years ago

Calls uninstall hook on delete plugin

comment:44 in reply to: ↑ 43 @strider727 years ago

Replying to ryan:

I'm imagining the support questions about what the heck the difference between Delete and Uninstall are. I think it's such an edge case that it's not worth having an uninstall button.

Maybe the two should be combined. Have a link that says "Uninstall", and when you click it WP says "This will clear all settings and data for this plugin. Would you also like to delete the plugin files? Yes/No"

I'm not familiar with the Delete function. How exactly is that going to work? (Link please?)

comment:45 @santosj7 years ago

Well, it is a mute point since the hook doesn't work.

comment:46 @santosj7 years ago

Now it works, uploading new patch soon.

@santosj7 years ago

Adds uninstall hook to delete process, with support for hook and uninstall file.

comment:47 @santosj7 years ago

Patch correctly implements uninstall hook for delete plugin process. Allows for both the uninstall hook and for an uninstall file.

comment:48 @santosj7 years ago

Can this get into 2.7? It does need more testers.

comment:49 @ryan7 years ago

  • Resolution set to fixed
  • Status changed from new to closed

(In [8585]) Plugin uninstall hooks from santosj. fixes #5625

comment:50 @ryan7 years ago

  • Milestone changed from 2.9 to 2.7

@santosj7 years ago

Set @since phpdoc tags. Based off of r8586

comment:51 @santosj7 years ago

  • Resolution fixed deleted
  • Status changed from closed to reopened

Still need to set the @since tags for the new functions.

Thanks for committing!

comment:52 @santosj7 years ago

  • Keywords plugin-management needs-testing dev-feedback removed

comment:53 @ryan7 years ago

  • Resolution set to fixed
  • Status changed from reopened to closed

(In [8587]) Update since tags. Props santosj. fixes #5625

comment:54 @strider727 years ago

The "We're going to delete all your files" page should also warn that all options, etc. are going to be deleted. Otherwise this might be unexpected behavior. (I can imagine somebody deleting a plugin that is corrupted, with the intent of reinstalling it. They may not want to delete the settings/data.)

Personally, I think we should allow, on that page, an option to delete but _not_ run the uninstall.

Otherwise, nice addition. (If I do say so myself ;-) )

Note: See TracTickets for help on using tickets.