WordPress.org

Make WordPress Core

Opened 5 years ago

Closed 5 years ago

Last modified 5 years ago

#12754 closed enhancement (worksforme)

register_uninstall_hook() doesn't allow multiple callbacks

Reported by: scribu Owned by: westi
Milestone: Priority: normal
Severity: normal Version: 3.0
Component: Plugins Keywords: needs-patch
Focuses: Cc:

Description

Currently, if you call register_uninstall_hook() in the same plugin, with two different callbacks, only the second one will be called.

This is because the callbacks are stored in an associative array:

$plugin => $callback

The only information that actually needs to be stored is the list of uninstallable plugins. The callback(s) can be added at runtime, like it's done for register_deactivation_hook().

Attachments (2)

register_uninstall_hook.diff (1.7 KB) - added by scribu 5 years ago.
example-plugin.php (457 bytes) - added by scribu 5 years ago.

Download all attachments as: .zip

Change History (19)

comment:1 @scribu5 years ago

register_uninstall_hook.diff also includes the fix for #12752.

@scribu5 years ago

comment:2 @scribu5 years ago

In example-plugin.php, without the patch, only example_option_b is removed.

comment:3 follow-up: @nacin5 years ago

  • Keywords has-patch removed

This wouldn't work when the plugin is deactivated at the time of the uninstallation.

If we are to support multiple uninstall hooks, we would need to store multiple callbacks as an array, i.e. $uninstallable_plugins[plugin_basename($file)][] = $callback;, then loop through those to add the callbacks to the action.

comment:4 @nacin5 years ago

Or rather, it would require a specific situation to make it work. 1) register_uninstall_hook() would need to be run in global scope. 2) The callback would need to be in the same file as the call to register_uninstall_hook().

If you run register_uninstall_hook() only on activation (all that is needed, really), or keep the callback in another file, then this patch would not work.

comment:5 in reply to: ↑ 3 @scribu5 years ago

  • Keywords has-patch added

Replying to nacin:

This wouldn't work when the plugin is deactivated at the time of the uninstallation.

After applying the patch, please activate the example plugin, deactivate it and then uninstall it.

You will see that it does, in fact, work.

1) register_uninstall_hook() would need to be run in global scope.

I don't see what scope has to do with this.

2) The callback would need to be in the same file as the call to register_uninstall_hook().

Unlike with the uninstall.php method, the main plugin file is loaded before the hook is fired.

So, as long as the plugin includes the file with the callback(s), it doesn't matter where they are.

It's really not that different from register_deactivation_hook().

comment:6 @scribu5 years ago

If by scope, you mean it has to be called as soon as the plugin is loaded, please keep in mind that that's a requirement for register_activation_hook() too.

comment:7 follow-up: @nacin5 years ago

A deactivation hook is run when the plugin is deactivated. That means that the plugin is already activated and thus already included. The deactivation callback is then run and the active_plugins option is updated to remove the plugin for the next page load.

The uninstall hook may be run on a plugin that is not active, and thus not included. The way it currently works, is you can run register_uninstall_hook() on activation. By global scope, I meant on inclusion. It works both on a regular hook like init and on a one-off like activation.

If there's no conditional case involved, then there's little reason to keep updating the database option, so on activation is fine. But, if you were running it on the init hook, that hook is already fired by the time a deactivated plugin file is included for the uninstall callback to be run. That means no callback would be registered.

So, the only way the patch would work is if it is run in global scope. This may be a requirement for register_activation_hook(), but it is not a requirement for register_uninstall_hook(), which works perfectly fine if done only on activation. I'm not speaking for the original developer or committer, of course -- that could have been the idea.

I just searched briefly through the plugin directory and found a number of plugins using it in global scope, but I also found a few that did it on init, activation, or admin_init.

So, as long as the plugin includes the file with the callback(s), it doesn't matter where they are.

Sure, and that's a good practice, but if for whatever reason you wanted to keep the function in a separate file (and not use uninstall.php, for whatever reason) and use register_uninstall_hook(), that would no longer work under your patch, as register_uninstall_hook() would not be fired. Sure, that's a silly use case. I was just pointing out that you were making an assumption.

comment:8 in reply to: ↑ 7 ; follow-up: @scribu5 years ago

Replying to nacin:

So, the only way the patch would work is if it is run in global scope. This may be a requirement for register_activation_hook(), but it is not a requirement for register_uninstall_hook(), which works perfectly fine if done only on activation. I'm not speaking for the original developer or committer, of course -- that could have been the idea.

My opinion is that having the ability to register multiple uninstall hooks is more useful than being able to use it outside the global scope.

Also, it's consistent with the other functions. At the very least, it should be clearly stated in the docs that it doesn't behave like the others.

If there's no conditional case involved, then there's little reason to keep updating the database option, so on activation is fine.

Funny you should mention that. update_option() first does a $new_value === $old_value to see if it really needs to update the database.

I was calling register_activation_hook() with an array($obj, 'method') callback. Because the current object instance didn't match the unserialized instance, it _did_ force multiple UPDATE queries on each page load.

In contrast, storing just 'true' instead of the callback, the UPDATE query is only done the first time the plugin is run.

comment:9 in reply to: ↑ 8 ; follow-up: @nacin5 years ago

Replying to scribu:

My opinion is that having the ability to register multiple uninstall hooks is more useful than being able to use it outside the global scope.

I disagreee, but that's not the issue. The issue is compatibility. It currently doesn't work with multiple callbacks, but it currently *does* work outside of global scope. Two fixes: 1) use uninstall.php instead, or 2) we can make uninstall_plugins hold an array of callbacks. We should encourage the first (and do so in the inline docs and the Codex) and do the second.

Also, it's consistent with the other functions. At the very least, it should be clearly stated in the docs that it doesn't behave like the others.

Technically, only register_activation_hook() needs to be run in global scope. Neither deactivation nor uninstall need to be.

Funny you should mention that. update_option() first does a $new_value === $old_value to see if it really needs to update the database.

Fair enough. And it looks like it is autoloaded as well.

I was calling register_activation_hook() with an array($obj, 'method') callback. Because the current object instance didn't match the unserialized instance, it _did_ force multiple UPDATE queries on each page load.

The docs should demand it be a function callback. Maybe a deprecated arg call to not like arrays.

comment:10 in reply to: ↑ 9 @scribu5 years ago

I guess I'll have to keep using my trusty hack.

Two fixes: 1) use uninstall.php instead, or 2) we can make uninstall_plugins hold an array of callbacks. We should encourage the first (and do so in the inline docs and the Codex) and do the second.

Related to 1): Why don't we load the plugin before including uninstall.php ?

The docs should demand it be a function callback. Maybe a deprecated arg call to not like arrays.

Calling a static method like array('MyClass', 'method') shouldn't cause problems.

comment:11 @scribu5 years ago

  • Keywords needs-patch added; has-patch removed
  • Milestone changed from 3.0 to Future Release
  • Type changed from defect (bug) to enhancement

I think the conclusion would be either wontfix or store multiple "static" callbacks per plugin.

Moving to future release for now.

comment:12 follow-up: @jacobsantos5 years ago

For what it is worth, I think this should be closed as won't fix.

If you wish to run two functions at uninstall, then just call the second from the uninstall hook. There was never envisioned a need for running two hooks at once, because we wouldn't include two files at once either.

I mean, not be harsh, but really, the power of programming is to call functions within functions.

<?php
function example_install() {
	add_option('example_option_a', 'foo');
	add_option('example_option_b', 'bar');
	example_uninstall_a();
	example_uninstall_b();
}
register_activation_hook(__FILE__, 'example_install');
	
function example_uninstall_a() {
	delete_option('example_option_a');
}
	
function example_uninstall_b() {
	delete_option('example_option_b');
}
?>

comment:13 in reply to: ↑ 12 @scribu5 years ago

Replying to jacobsantos:

By that logic, you wouldn't need hooks at all, would you?

comment:14 @scribu5 years ago

  • Milestone Future Release deleted
  • Resolution set to worksforme
  • Status changed from new to closed

Anyhow, here's my workaround:

// Have more than one callback attached to the uninstall hook
function add_uninstall_hook($plugin, $callback) {
	// trigger $is_uninstallable_plugin flag
	register_uninstall_hook($plugin, '__return_false');

	add_action('uninstall_' . plugin_basename($plugin), $callback);
}

comment:15 @mikeschinkel5 years ago

  • Cc mikeschinkel@… added

@scribu - I'm just really curious; what's a use case where having multiple uninstall hooks would be really useful?

comment:16 @scribu5 years ago

It's needed if you want to take a "garbage-collected" approach. For example, I have an Options class and an AdminPage class that each add an uninstall hook to clean up after themselves.

comment:17 @nacin5 years ago

Related - #13786

Note: See TracTickets for help on using tickets.