Introduce register_update_hook()

[scribu]

Currently, plugin authors overload the activation hook with routines for upgrading from older versions.

We should have a dedicated hook for when plugins are upgraded.

The activation hook should only be fired when the plugin is activated by the user.

[scribu]

Related discussion: ​http://lists.automattic.com/pipermail/wp-hackers/2010-September/035030.html

[nacin]

As said over wp-hackers by someone else: I don't think it is a good idea to assume that upgrades to plugins/themes are made through the WP admin interface.

Take, for example, SVN. Introducing a new hook here is only going to cause inconsistencies. At least activation and deactivation we can largely control. (Deactivation less so, as the files can vanish.) Schema upgrades are probably best handled on a case by case basis.

[ericmann]

WordPress could automatically verify plug-in versions on init. We already store a list of active plug-ins in the DB ... it would be fairly trivial to pass an expected plug-in version with the upgrade hook and check it against whatever version was "active" on the site. If the two version numbers are different, we can assume an update has happened (either though the UI, via SVN, or via FTP) and call the appropriate function.

[scribu]

Eric, that's a great idea. It combines the best-practice suggested on wp-hackers with a consistent approach.

[Denis-de-Bernardy]

+1 to nacin's comment. An upgrade hook is not going to work for users who upgrade through SVN or FTP.

Eric's idea is impractical: fetching the version of plugins on all page loads involves parsing their headers. That remained quite slow last I checked.

[nacin]

Suggesting wontfix here, and address the root problem: #14915

[scribu]

Eric's idea is impractical: fetching the version of plugins on all page loads involves parsing their headers. That remained quite slow last I checked.

Not at all. Plugins that would call register_upgrade_hook() would have their current version stored in the DB, and then, only on each admin page load check that option, which would get autoloaded anyway.

It would just be a standardization of what plugin authors would have to do anyway.

[scribu]

#14915 is not the root problem.

[ericmann]

This patch does a couple of things. Firstly, it adds an option called active_plugin_versions that maps plugin file names to their current version. The key/value pair is added to the database when the plugin is first activated.

WordPress will check the plugin's version against its stored database value whenever register_update_hook is called. It's getting the expected (current) version number from the function call itself and the stored version number from get_option ... no need to re-parse plugin headers. If the expected version is higher than the value stored in the database, WordPress assumes an update and fires the update-plugin_{pluginname} action.

The function register_update_hook accepts three parameters - the plugin filename, a callback function, and the plugin version the system is expecting - this version should be the same as the current version of the plugin and is explicitly set by the plugin.

I've done some preliminary testing by tweaking the version number of Hello Dolly to see how things work. It seems to be working just fine so far.

Unfortunately, it only works for single sites at the moment ... I'm not up to speed with MS development yet, so I didn't even touch the active_sitewide_plugins settings, though I'm open to advice and guidance here.

[scribu]

The function register_update_hook accepts three parameters - the plugin filename, a callback function, and the plugin version the system is expecting - this version should be the same as the current version of the plugin and is explicitly set by the plugin.

Is the third parameter really necessary?

[mikeschinkel]

Replying to scribu:

Not at all. Plugins that would call register_upgrade_hook() would have their current version stored in the DB, and then, only on each admin page load check that option, which would get autoloaded anyway.

Rather than just store versions, why not store more plugin metadata?

See: ​http://wordpress.stackexchange.com/questions/361/

[ericmann]

Replying to scribu:

Is the third parameter really necessary?

It's not a deal-breaker ... it was a preemptive compromise based on Denis' statement about the speed of parsing plugin header data. If we want, we could just scrape the plugin meta instead rather than passing the version number.

[scribu]

I see. It's probably better to specify an explicit version anyway, since not every update will have a schema change.

More observations on the patch:

• the callback should receive the previous version as a parameter
• there's no point in using add_action() / do_action() in the same function

[ericmann]

Replying to scribu:

More observations on the patch:

• the callback should receive the previous version as a parameter
• there's no point in using add_action() / do_action() in the same function

Great suggestions; I've incorporated them both. Though I'm still wondering how best to apply this patch to multisite/network-wide plugins. I want to keep things as consistent as possible, but without direct experience in site-wide options, I'm not quite sure where to begin ...

[scribu]

For network plugins, you can use add_site_option() etc. to store a separate array of version and then you would have to run the upgrade procedure for each site. Similar to #14170

A more pressing problem is that in your current patch, you tie the version update to the activation procedure, so we're back where we started. Upgrades shouldn't be tied to activation at all.

The way I see it, the active version should be added and updated from register_update_hook().

[ericmann]

Moves all update handling to register_update_hook. Extends for multisite.

[ericmann]

Replying to scribu:

Upgrades shouldn't be tied to activation at all.

The way I see it, the active version should be added and updated from register_update_hook().

I agree ... and I've changed the newest patch to do just that. register_update_hook will do all the work, checking the plugin's current version against its stored value, running the callback function if necessary, and updating the stored plugin version afterwards. I've also changed it so we're passing both the current version and the stored version to the callback function. I can think of a few times where that would have helped a few of my clients when updating plugins across multiple versions.

I also took a stab at extending this for multisite. If we're working with a multisite installation, I use get_site_option and update_site_option instead of their standard single-installation counterparts. This makes sense to me, so correct me if the implementation seems to be in error.

[scribu]

Use is_plugin_active_for_network()

[scribu]

plugin-versions.2.diff:

I replaced the is_multisite() check with is_plugin_active_for_network() as that's what we're interested in.

I think passing only the stored version to the callback is enough, since it will already know what the latest version is.

I'm also passing wether the current plugin is network activated, so that the callback can handle that on it's own.

[ericmann]

Uses is_multisite() so the hook works for themes.

[ericmann]

I like the changes. Had I realized is_multisite() was used in is_plugin_active_for_network() I might have done it this way. Though the is_plugin_active checks seem a bit redundant to me. The code in the plugin calling register_update_hook will only get called if the plugin is active, correct?

Does keying on is_plugin_active mean we can't use this hook for themes as well? More theme developers are wrapping custom plugin functionality into their functions.php files, so I could see several situations where a theme author would want to use this hook as well. If I'm right that the code will only be executed if the plugin is active, could we assume is_plugin_active will return true and replace that check with is_multisite() instead? (ala plugin-versions.3.diff)

[scribu]

It's not the same thing. is_plugin_active_for_network() checks if a plugin is network-activated.

Themes can't be network activated and plugin_basename() probably doesn't work either.

So, I think we should make a separate function for themes (in a separate ticket).

[ericmann]

Good to know ... let's go with plugin-versions.2.diff then. Can we remove the other two patches to eliminate confusion?

[scribu]

Uploaded files can't be removed. Anyway, in plugin-versions.4.diff I've added an is_admin() at the beginning, to keep the front-end light and free of potential weirdness.

[ericmann]

Replying to scribu:

keep the front-end light and free of potential weirdness.

+1 To that.

[scribu]

Only fire if is_admin(); updated docblock

[batmoo]

This is awesome guys!

Replying to scribu:

I've added an is_admin() at the beginning, to keep the front-end light and free of potential weirdness.

The one potential problem with only firing the update hook on is_admin is that the front-end is going to have updated plugin code which likely expects updated data, and that's more likely to cause weirdness.

For example, if I go from 0.1 to 0.2 and my update hook changes the name of an option from 'my_option' to 'myplugin_my_option', my code is going to reference the option as 'myplugin_my_option'. If a user visits my site before the update hook fires, my plugin is probably not going to work. But then again, this might just be a coding practice issue.

And from the looks of it, the current patch fires update_option/update_site_option on every page load for all update hooks registered. It might be better be to fire those only if the version is different.

[Denis-de-Bernardy]

Expanding on that last remark, there should probably be two procedures. One that tells the plugin to run the upgrade code on all page loads, regardless of where we are on the site. The other that tells the plugin to make these changes persistent, which runs in the admin area only, preferably only when the admin is logged in, and which updates the stored plugin version. This, to avoid hammering the DB on large sites.

The two functions need to be well documented, too, so they're not misused. Most plugin devs don't realize that non-atomic upgrade code can wreck their data when it's run concurrently on multiple page loads.

[scribu]

Replying to batmoo:

Replying to scribu:

I've added an is_admin() at the beginning, to keep the front-end light and free of potential weirdness.

The one potential problem with only firing the update hook on is_admin is that the front-end is going to have updated plugin code which likely expects updated data, and that's more likely to cause weirdness.

Considering that 99.9% of plugin updates will be done through the admin, I don't think this is a problem.

And from the looks of it, the current patch fires update_option/update_site_option on every page load for all update hooks registered. It might be better be to fire those only if the version is different.

update_option() and update_site_option() do an UPDATE only if the new value is different from the old value, so it's the same thing.

[StephenCronin]

Replying to scribu:

Considering that 99.9% of plugin updates will be done through the admin, I don't think this is a problem.

Isn't that one of the arguments for this in the first place? That the existing activation hook shouldn't be used, because it only works when people upgrade plugins through admin. Ie it won't work when plugins are update via FTP, etc

Now, we're replacing that with something that won't work in the same cases (ie when a plugin is updated via FTP)?

Am I missing something here?

[nacin]

Replying to StephenCronin:

Now, we're replacing that with something that won't work in the same cases (ie when a plugin is updated via FTP)?

It will work when a plugin is updated through FTP. It just won't fire until the admin area is accessed. I consider that an extreme edge case at that point. And FWIW, WordPress doesn't run the database upgrade for a new version until the admin area is accessed, either, so it's consistent with that.

[StephenCronin]

Replying to nacin:

It will work when a plugin is updated through FTP. It just won't fire until the admin area is accessed. I consider that an extreme edge case at that point. And FWIW, WordPress doesn't run the database upgrade for a new version until the admin area is accessed, either, so it's consistent with that.

hmm can't argue with it being consistent with the core db upgrade, and for most plugin updates it won't be an issue, but there will occasionally be plugin updates where something needs to be updated immediately. In those cases, the plugin may behave inconsistently until the user accesses the admin area (typically in my case that could be up to a week!).

It seems that the only bulletproof solution is for the plugins to handle this themselves.

If that's the case, perhaps we should scrap this new function and just educate plugin authors about handling it themselves.

[scribu]

hmm can't argue with it being consistent with the core db upgrade, and for most plugin updates it won't be an issue, but there will occasionally be plugin updates where something needs to be updated immediately. In those cases, the plugin may behave inconsistently until the user accesses the admin area (typically in my case that could be up to a week!).

No matter how the plugin does it, it won't be able to sense how often you login to the admin. So you would have to modify it either way.

We can educate plugin authors that the update hook will only run in the admin and they can decide for themselves if they should use register_update_hook() or a custom solution.

[scribu]

I'm going to commit this for 3.1 and leave the automated network activation handling for a future release.

[Viper007Bond]

I still think this should be wontfix. Let plugin authors take care of this themselves.

[westi]

1 We are past the freeze for 3.1 and shouldn't be adding any more new features.
2 This is more code than a plugin needs to implement the solution itself and will run more often than it would need to in a plugin - yes you have an is_admin() check but plugins doing this themselves would be able to hook into an admin focused action hook thereby ensuring it was an admin page load.
3 If plugins behave inconsistently until the upgrade code has run then they are poorly coded :-)

I'm in the WONTFIX camp but this is definetly too late for 3.1

[scribu]

Oh well, I guess this is what plugin frameworks are for :)