Skip to:
Content

Opened 6 years ago

Last modified 4 months ago

#2755 new enhancement

Uninstall button and routine

Reported by: boonebgorges Owned by:
Milestone: Future Release Priority: major
Severity: normal Version:
Component: Core Keywords: needs-patch
Cc: sbressler

Description

BP's deactivation hook bp_loader_deactivate() (bp-loader.php) deletes a bunch of live site options. This is a Bad Idea, especially because plugins often have to be deactivated for testing and maintenance (and upgrades!).

Three options:
1) Remove the deactivation hook altogether
2) Leave it, but modify it from removing site options that are still valid
3) Leave it, but just for the do_action() hook.

My vote is for option (3). There might still be reasons why some other plugin would want to hook to BP activation, so it's worth it to keep the hook. The removal of stale settings should be done by proper upgrade routines like the one in bp-core-upgrade.php (in case someone wants to deactivate/activate without updating).

Attachments (1)

bp_uninstaller.patch (1.2 KB) - added by ddean 5 years ago.
uninstall.php proposal

Download all attachments as: .zip

Change History (20)

#1 follow-up: @nacin
6 years ago

4) uninstall.php plz :-)

#2 @jeffsayre
6 years ago

Since we've basically been debating the idea behind this ticket on Twitter, I thought it best to bring the discussion to were it belongs--this ticket.

Here's a recap of some of my comments and thoughts on this issue:

  1. Plugin deactivation is different than plugin uninstalling. They are different events in WP's plugin API.
  1. There is value in deactivation routines. They can be used to allow Site Admin to reset the plugin's environment back to initial defaults without having to uninstall and then reinstall a plugin. But the resetting (deleting of) plugin meta data (the stored serialized options data) shouldn't be done automatically on deactivation. Site Admins should have control over if and when a plugin's meta data are reset.
  1. Whereas I have a function that hooks to register_deactivation_hook(), just as most (all?) BP plugins do, I disable all of the delete_site_option() and delete_option() calls and instruct admins to uncomment these lines if they need to reset the plugin's environment to default settings. This is bad and not user friendly.
  1. So, I've considered a more useful approach: create an Admin setting that allows the Site Admin to indicate whether meta data should be purged upon plugin deactivation. The function hooked to the deactivation event will use a constant to determine whether or not any delete_site_option() and delete_option() calls should be fired.
  1. Andrew Nacin suggested another approach which I believe is better. WordPress should have a "Defaults" button in the plugins menu that allows a Site Admin to easily restore a plugin back to its original factory settings with a simple push (and I assume with various warning messages and a confirmation button). This button is a nice idea, offers very clear meaning, and would obviate the need for a special function hooked to register_deactivation_hook(). Instead, there would need to be a function hooked to a new Plugin Defaults hook.
  1. The Plugin Defaults hook is something that would have to be added to WP's core, to the Plugin API (see: /wp-includes/plugin.php).

So, if we go this route, this becomes a WP ticket and not a BP ticket.

#3 @sbressler
6 years ago

  • Cc sbressler added

FYI: If you want the "Defaults" button on the plugin page along with "Deactivate | Edit", while it could be added to core, you could do it yourself, too, without anything being added to core. Just use something like this:

function bp_action_links( $links, $file ) {
	$plugin_file = basename( __FILE__ );
	if ( basename( $file ) == $plugin_file ) {
		$settings_link = '<a href="[link or JavaScript to reset BP to default settings">Reset to Default</a>';
		array_unshift( $links, $settings_link );
	}
	return $links;
}
add_filter( 'plugin_action_links', 'bp_action_links', 10, 2 );

Though forgive me if I'm misunderstanding the approach you suggest in 5.

#4 @boonebgorges
6 years ago

I am going to remove all of the delete_site_options() calls from the deactivation hook (but leave it in place for the 'bp_loader_deactivate' action). That will solve the immediate problem at hand. Then I'm going to flip this ticket into an enhancement request for a proper uninstall procedure.

#5 @boonebgorges
6 years ago

(In [3549]) Don't delete site options when BP is deactivated. See #2755

#6 @boonebgorges
6 years ago

  • Milestone changed from 1.3 to 1.4
  • Summary changed from Remove deactivation hook to Uninstall button and routine
  • Type changed from defect to enhancement

#7 @DJPaul
6 years ago

  • Keywords needs-patch added
  • Severity set to normal

This would be great to get into 1.6 if someone can provide a patch. Otherwise, it'll get punted.

@ddean
5 years ago

uninstall.php proposal

#8 in reply to: ↑ 1 @ddean
5 years ago

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

Replying to nacin:

4) uninstall.php plz :-)

This seems like the easiest way to do an uninstall routine.

#9 @boonebgorges
5 years ago

  • Keywords dev-feedback added

Patch looks sensible to me. I feel very uneasy about auto-deleting data upon deletion (ddean's patch does not do this). If we're going to have a BP table-deletion routine, we really must have a UI for it, which gives multiple levels of warning about the irretrievability of data. In the meantime, the proposed patch seems like a reasonable addition. Thoughts from others?

#10 @DJPaul
5 years ago

The proposed patch *does* have a placeholder function to drop tables. I think if we just dropped the settings, and the user re-activated the plugin, I think the wizard might get stuck. uninstall.php is only triggered when the "Delete" plugin option is hit.

#11 @boonebgorges
5 years ago

  • Keywords 1.7-early added
  • Milestone changed from 1.6 to Future Release

I think if we just dropped the settings, and the user re-activated the plugin, I think the wizard might get stuck.

That would need testing, which we don't have time for this cycle :) We should do this very early in the next cycle, though, as not having anything in place makes us Bad Citizens.

#12 @boonebgorges
5 years ago

  • Keywords 1.7-early removed
  • Milestone changed from Future Release to 1.7

Let's look at this for 1.7.

#13 @DJPaul
5 years ago

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

#14 @johnjamesjacoby
4 years ago

  • Milestone changed from 1.7 to 1.8

Patch is a good start, but no time for this in 1.7. Moving to 1.8.

#15 @boonebgorges
4 years ago

  • Milestone changed from 1.8 to Future Release

#16 @slaFFik
6 months ago

  • Milestone changed from Future Release to 2.8

I see the value in uninstall.php and BuddyPress cleanup, so I would like to bring this for 2.8 release.

This ticket was mentioned in Slack in #buddypress by slaffik. View the logs.


6 months ago

This ticket was mentioned in Slack in #buddypress by slaffik. View the logs.


4 months ago

#19 @slaFFik
4 months ago

  • Milestone changed from 2.8 to Future Release

We definitely need to do this, but this is a too big task for the remaining time in 2.8 release cycle.

Note: See TracTickets for help on using tickets.