Problems with the current register_uninstall_hook implementation

[x3r0ss]

During my recent plugin development I have discovered some issues with the plugin register_uninstall_hook.

What it does is it saves the callback into the database, one of the problems is that it seems to re-save it every time the register_uninstall_hook function is called.

And second is that it stores the entire callback in the database even if you're using a class callback.

Now it seems that this entire uninstall hook is weird in some sense, because well the plugin is already deactivated so the hook would be inaccessible when deleting the plugin.

I think that either the plugin hook should be removed (And just leave uninstall.php for uninstalling) or that we find a way to fix it to work properly.

So to wrap it up.

• Somehow saving the hook resaves it every time (Possibly due to trying to save a callback to an object)
• uninstall hook is crooked the way it works now, need a reimplementation.

Hope we can work something out that would work best, I don't mind writing the implementation.

Regards, Xeross

[sivel]

It's too late in the development cycle to target this for 3.0, we will need to look at getting it into 3.1.

[jacobsantos]

Yeah, there has been one other ticket on this already. The hook implementation is really for simple uninstall processes. Meant to remove an option or two. If you want to keep everything in one file for a really simple plugin. The uninstall.php really separates the complexity and allow for more advanced uninstall.

[x3r0ss]

Possible Solution 1

[x3r0ss]

Sorry for the mess-up witht the second diff, tried to replace the first one but didn't check the checkbox.

Okay, so I understand you're late in the development cycle but as far as I can see this is only an internal changes and seems it won't bring in any bugs (Except for people that should be using uninstall.php).

And next to that nothing will change for plugin authors.

If I can not convince you, then let it be implemented for 3.1.

[x3r0ss]

Replying to jacobsantos:

With my implementation the logic however complex it is can be put in the plugin file itself (I just don't like yet another file in my plugin folder), and it can use parts of the plugin during the removal which might prevent some redundancy in the code.

Only bad thing might be that it actually needs to include the plugin file to do this.

[nacin]

Very simply, the plugin might not be active, which is why we are storing callbacks in the database in the first place. That means we need to re-load the entire plugin on a require_once just to check if it has added a callback to an uninstall_$plugin hook.

The patch (I've deleted the duplicate one) would force any plugin -- one that may or may not be active, and more importantly may or may not be safe to run in the middle of a script during the uninstall hook detection/firing process -- to be included.

You are also assuming that the uninstall hook is being registered in global scope, which is not only an unfair assumption but improper use of the hook. It should instead be run on a hook, preferably activation. Most plugins that use uninstall hooks do indeed do this, that way you're only writing to the database once. There's no reason to run it again after activation. This thus addresses your "re-save" concern.

The other ticket is #12754. That ticket describes that a class method using a static calling method will work.

Including a script and running one small component out of it can cause problems, more so if we decide to change its implementation. There are ideas in #12754 best left to a plugin that will work, and beyond that, uninstall.php is the preferred method anyway.

Suggesting wontfix.

[nacin]

In the first paragraph, "That means we need to re-load the entire plugin on a require_once just to check if it has added a callback to an uninstall_$plugin hook," I was referring to the suggested patch, not current behavior, in case that is not obvious.

[x3r0ss]

Okay I understand that you don't want that entire file to be loaded which I understand, but as it is only once to remove the plugin I don't see how this would be a big problem.

And if the plugin is running unsafe things in the file it should use uninstall.php anyways.

I guess an alternative to this that doesn't require the core to be changed is to include the class in my uninstall.php.

And finally I think an alternative suggestion would be that if you detect the callback to contain an instantiated class you don't save it to the DB and issue a warning or something like that.

And from what I understand the hook should only be ran once (On plugin activation I guess).

If that is the case there's still a problem that if you register the hook twice (Deactivate/reactivate ?) it will add the hook a second time in the database (Now I don't know if this only happens with instantiated classes though) which might be a problem worth looking into.

Replying to nacin:

In the first paragraph, "That means we need to re-load the entire plugin on a require_once just to check if it has added a callback to an uninstall_$plugin hook," I was referring to the suggested patch, not current behavior, in case that is not obvious.

Yes I understood that

[nacin]

The option is keyed with the file name, with the value being the callback, so while it will fetch from the DB and attempt to update the option on deactivate-reactivate, it won't actually do anything in terms of duplicating the hook. In fact update_option() should return false since the arrays would match, so it wouldn't even run the update query.

It's not the issue of that I don't want the entire file loaded. We already do that if the plugin is uninstallable to actually trigger the hook. It's also not the plugins that use uninstall.php specifically to prevent issues. (If the plugin is running unsafe things in global scope, then chances are the plugin author isn't at the point where he is using an uninstall hook to clean up his data. The venn diagram circles probably don't overlap.)

The issue would be plugins that aren't uninstall-aware, and are now suddenly getting included to check if it can be uninstalled, instead of checking for registered callbacks. That's a change in behavior and I'm honestly not sure how many plugins it would break, but when in doubt, I usually assume issues would arise.

[x3r0ss]

Replying to nacin:

The option is keyed with the file name, with the value being the callback, so while it will fetch from the DB and attempt to update the option on deactivate-reactivate, it won't actually do anything in terms of duplicating the hook. In fact update_option() should return false since the arrays would match, so it wouldn't even run the update query.

It's not the issue of that I don't want the entire file loaded. We already do that if the plugin is uninstallable to actually trigger the hook. It's also not the plugins that use uninstall.php specifically to prevent issues. (If the plugin is running unsafe things in global scope, then chances are the plugin author isn't at the point where he is using an uninstall hook to clean up his data. The venn diagram circles probably don't overlap.)

The issue would be plugins that aren't uninstall-aware, and are now suddenly getting included to check if it can be uninstalled, instead of checking for registered callbacks. That's a change in behavior and I'm honestly not sure how many plugins it would break, but when in doubt, I usually assume issues would arise.

Ah yes I forgot that, so how about just registering that there is a uninstall hook and only if it is registered there is then we include the file, would that be a viable solution ?

[nacin]

That's basically what's going on now. We also just happen to store the callback info.

Again, if we change behavior, then we break any plugin that registers an uninstall hook on an activation hook, which is currently proper behavior. We'd then be reversing that, and forcing plugins to register uninstall hooks in global scope.

scribu has a workaround in #12754 that, while I don't think needs to go into core, seems like a way to do exactly what you're trying to do here.

[x3r0ss]

Yeh that seems about what I suggested here, guess I'll go with the uninstall.php approach and just include the plugin file itself.

Now the only thing I think should be added is a big warning people shouldn't register a callback on an instantiated class in the uninstall hook, because that will create a huge wp_options field (10MB for 1 row), and eventually prevent the wordpress install from working.

[nacin]

It's keyed with the plugin file name, so I'm not sure how you're getting that large of a row.

[x3r0ss]

Replying to nacin:

It's keyed with the plugin file name, so I'm not sure how you're getting that large of a row.

I don't think that the set_option function handles large objects too well then, think it somehow corrupted the field then.

What I did was

 register_uninstall_hook(__FILE__, array($this, "uninstall"));

Perhaps it's a bug that has appeared in WP3.0, recently switched to the nightlies (And now the SVN)

[jacobsantos]

Replying to x3r0ss:

What I did was

 register_uninstall_hook(__FILE__, array($this, "uninstall"));

This is because the callback is serialized when it is put into WordPress and therefore will also include the class defined in $this. Again, as it was stated in the last ticket, the object callback can be used, but must be a static method or a function. The only unsupported callback is that which you are using above.

[x3r0ss]

Replying to jacobsantos:

Replying to x3r0ss:

What I did was

 register_uninstall_hook(__FILE__, array($this, "uninstall"));

This is because the callback is serialized when it is put into WordPress and therefore will also include the class defined in $this. Again, as it was stated in the last ticket, the object callback can be used, but must be a static method or a function. The only unsupported callback is that which you are using above.

Which I understand now but there's no documentation saying it's unsupported as far as I know, and perhaps explicitly checking if they're passing an object and throwing an exception if they do would be an improvement.

[nacin]

Replying to x3r0ss:

Which I understand now but there's no documentation saying it's unsupported as far as I know, and perhaps explicitly checking if they're passing an object and throwing an exception if they do would be an improvement.

Agreed. Moving to 3.1 pending patch. We can probably get away with using _deprecated_argument() here.

[scribu]

don't allow object methods as callbacks

[nacin]

(In [16339]) Only a static class method or function can be used in an uninstall hook. props scribu, fixes #13786.