Fix activation hook inconsistency

[nacin]

The activation hook fires whenever a plugin is updated through the admin, but not during bulk updates.

We need to resolve this inconsistency, and either A) fire activation hooks for bulk updates, or B) stop activation hooks from firing for single updates.

I think I suggest (B), despite the backwards compatibility issue. It's unintuitive for an activation hook to be fired on update, and additionally it is not reliable due to manual updates (including SVN). As it is, we silence deactivation hooks on update.

Related, a register_upgrade_hook() ticket #14912, that I believe should be wontfix'd. Schema updates require special handling at the plugin level IMO for now.

[scribu]

I think you mean you suggest B) - stop activation hooks from firing for single updates, which I agree with.

[nacin]

Corrected, thanks.

[sirzooro]

There is one more case when activation hook does not fire - single update on multisite install. In this case hook fires on one blog instance only (the one where admin is logged), but not for all remaining ones in the network.

[nacin]

Replying to sirzooro:

There is one more case when activation hook does not fire - single update on multisite install. In this case hook fires on one blog instance only (the one where admin is logged), but not for all remaining ones in the network.

A very complex problem, mostly unrelated: #14170.

[joelhardi]

patch deleting deactivation/activation on plugin upgrade

[joelhardi]

Added a patch to do (B). Fun, nothing but deleting code it turned out to be.

I figure you have "real" tests, but let me know if you want a test plugin that always asks to be upgraded, and logs its activation/deactivation calls to a stack stored in an options variable.

[scribu]

I only see the deactivation hook removed there.

Also, I'm curious about your test plugin. :)

[joelhardi]

Lines 1033-1036 that I deleted are the reactivation code.

Immediately after, $update_actions gets set, but you'll notice that right afterward it's unset if the plugin is already active (which it is). So, I *think* I got everything. Definitely can verify that the activation hook wasn't being called.

On second look, I guess setting and then immediately unsetting that var based on an if statement is kinda lame, and it would be better to just do:

    if ( ! $this->plugin_active )
      $update_actions =  array(
        'activate_plugin' => ...
        'plugins_page' => ...
      );

I'll redo the patch. And, will upload this test plugin.

[joelhardi]

newer patch per my comment (but slightly more complicated). Doesn't set and then immediately unset $update_actionsactivate_plugin?.

[joelhardi]

test plugin that always wants to be upgraded (see comment)

[joelhardi]

OK, to use my test plugin, untar it and you'll see there's a patch to apply to the tree. Basically, the patch just inserts one line of code into two different files to call a function that injects data into the http response WordPress gets when it does the remote query for available plugin versions.

(NB that one of the files it's patching is class-wp-upgrader.php so if you wind up doing something with my code be sure not to commit this line!)

Then, to use my permanently out of date plugin, open it in your editor and change the $zipfile line to whatever URL or filesystem location you're going to put the .zip in. Then save, zip the folder and move the zip file into place.

Now activate the plugin. You'll see it adds an admin page under the Settings menu where it dumps its log to screen.

Then, to use it, just go to the plugin page, there should be the yellow "new version available" strip, so you can click the "upgrade automatically" link.

So, if you run it without my patch, you'll see it pretty obviously run the deactivation hook and then the reactivation in the iframe. You can check the log dump page and see that deactivate() and activate() were called. With the patch, no activation/reactivation!

[scribu]

Pretty nifty plugin.

[dd32]

The Deactivation code which is run is run "silently" that is, no deactivation hooks are called. The removed lines in the patch result in the plugin being active on the site & maintainence mode not being activated for non-bulk upgrades.

The idea there is that Singular plugin upgrades would not affect the functionality of the website.

There are 2 inconsistencies here which may want to be looked at:

Singular:

• Plugin is deactivated for entirety of upgrade process (Deactivation hook NOT run)
• Re-activation occurs, This step reactivates the plugin on the site, and does a sanity check of the php file (ie. Plugin doesnt re-activate if the new version causes a fatal error) (Activaton hook RUN)

Bulk:

• Website is put into Maintainence Mode
• Plugin upgrade is done (Deactivation hook is NOT run, Activation hook is NOT run)
• Website is removed from Maintainence Mode.

It doesn't appear that a fatal-error check is ever done on the bulk upgrade process however to me.

It might be worth getting rid of the singular upgrade mode and implement everything fully in the bulk upgrade functionality, That would include adding a no-fatal-error check to the bulk upgrade process for active plugins (And if one is hit deactivate it)

[nacin]

It doesn't appear that a fatal-error check is ever done on the bulk upgrade process however to me..

:(

It might be worth getting rid of the singular upgrade mode and implement everything fully in the bulk upgrade functionality, That would include adding a no-fatal-error check to the bulk upgrade process for active plugins (And if one is hit deactivate it)

Sounds reasonable. Will be tough to sandbox these, however, as we can't keep redirecting. Might need to hit wp-load with an Http request a bunch of times if it is more than one? And only kick on maintenance mode then also.

[dd32]

as we can't keep redirecting. Might need to hit wp-load with an Http request a bunch of times if it is more than one?

I'm thinking that a HTTP request to loop back (with a nonce or something to bypass maintainence mode) for each active plugin as the final step of upgrading

And only kick on maintenance mode then also.

I think it only kicks in right now if any of the plugin's you're upgrading are active. Maintainence mode is used so that when the plugin is only half-upgraded, a hit to the front end won't cause a "PHP Unexpected end of file" type error.

Another thought just hit me. It's been "common" to utilise the activation handler to die-out to prevent the plugin being activated on blogs that do not meet the requirements, I cant think of a plugin off the top of my head which does this, But the old upgrade method had the ability that plugins could deactivate themselves upon upgrades if the new version wasn't compatible for some reason.

[joelhardi]

Thanks everybody, I understand the issues better now. FYI I came across a minor issue but put it in a new ticket #14963 as it's basically unrelated.

With respect to the single-plugin upgrade process, since the existing code (including the silent deactivation) is what we want -- with the exception of the activation hooks being re-run when the plugin is reactivated -- it would be very simple to just modify the existing re-activation so that it also operates in a "silent" mode that doesn't call activation hooks.

Currently, deactivate_plugins() accepts an optional $silent parameter, but activate_plugins() and activate_plugin() do not. I'd suggest adding one so that these activate and deactive functions are more consistent. Then, class-wp-uploader would just set $silent to true when reactivating. To me, having both activate and deactivate functions accept $silent parameters would be nicely consistent.

So, the single upgrade method would be what you want: activate/deactivate hooks no, but error check yes.

Then, even if you go the other way (eliminate the single upgrade code and run everything through bulk upgrade), the effort wouldn't be wasted since probably will want to add this "activate in silent mode" function to the bulk upgrade process to add error checking anyway.

Anyway, let me know if you have any feedback on that idea, I could easily code this up.

re: chaining http redirects, this starts to look icky to me. It seems like a perfect use case for AJAX, but I don't know what the rules are for JS dependency in the WP backend.

It's been "common" to utilise the activation handler to die-out to prevent the plugin being activated on blogs that do not meet the requirements, I cant think of a plugin off the top of my head which does this, But the old upgrade method had the ability that plugins could deactivate themselves upon upgrades if the new version wasn't compatible for some reason.

That might be something to relate to #14912. Alternately, if there were an "install" hook, plugin authors would stop caring that the activation hook is run on both installs and upgrades.

To turn this hack into more of a sanctioned API, could just define an exception class like PluginValidException or something that plugin authors could choose to throw to disable their plugins. Put the try/catch in do_action() and then plugin authors could say "disable me" whenever they discover a problem, not just activation.

Another idea would be to have plugins be able to provide a list of requirements (i.e. things that would be easy for WordPress to check, like PHP version and required PHP modules) that WordPress would check, or else optionally register a callback that would be called on install/activation/upgrade to do whatever checks it wants and return a TRUE or raise an informative exception whether it should be enabled or not. With something like that you wouldn't even have to install the plugin first, just unzip in a temp location and do the check, which would make it easy to just leave the old version of a plugin installed if the new one isn't compatible.

[dd32]

re: chaining http redirects, this starts to look icky to me. It seems like a perfect use case for AJAX, but I don't know what the rules are for JS dependency in the WP backend.

No redirects are going to be used. I'll make damn sure of that. :P

However, In order to activate a plugin AND have it sandboxed during the activation to check for errors, a seperate HTTP request MUST be made, It can be in the form of a Iframe (as current for the single upgrader), A request which redirects afterwards (in the case of the "Activate Plugin" function), or it can be WordPress making an external HTTP Request to itself.

[nacin]

Activate all plugins, run HTTP request, and it it fails, deactivate by one in reverse alphabetical order until the site comes back up (or deactivate all of them). I've proposed something similar to that in #14096.

[dd32]

Activate all plugins, run HTTP request, and it it fails, deactivate by one in reverse alphabetical order until the site comes back up

Sounds sane to me, Reduces the ammount of requests by first checking if any of them would cause the failure to start with :)

[joelhardi]

Activate all plugins, run HTTP request, and it it fails, deactivate by one in reverse alphabetical order until the site comes back up (or deactivate all of them).

Agree. Also still works if there are any cross-dependency issues with plugins.

Only problem with 1-by-1 deactivation is that it might take a while if they have a lot of plugins installed. Could branch and only try this if they have fewer than a certain number of plugins, otherwise leave everything deactivated. Or, could improve it by using a binary tree algorithm. i.e. 1st test for errors with all plugins active; if fails, split plugin list in half and try on each half, etc.

[dd32]

Only problem with 1-by-1 deactivation is that it might take a while if they have a lot of plugins installed.

You'd only be testing the plugins which were just upgraded.

The 100ms/plugin (which is a guess) delay for checking them would be insanely faster than say, the user upgrading n+1 plugins.

[joelhardi]

You'd only be testing the plugins which were just upgraded.

Right, I'm worried about the case where a user comes along to upgrade their long-dormant blog and they have 10 plugins or more to upgrade, that the upgrade process might become more fragile or take a long time and be prone to user interruption. Playing devil's advocate -- i.e., do we need to think about the UI when adding (silent) reactivation and error checking to the bulk upgrade process? Just a simple code branch (i.e. do not try to auto-reactivate if n > 10) at minimum might be a good idea, even if it's at n > 100 which is extremely unlikely to be encountered.

The 100ms/plugin (which is a guess) delay for checking them would be insanely faster than say, the user upgrading n+1 plugins.

I don't disagree with the enhancement (add reactivation to bulk upgrade) or think users should have to single-upgrade.

Just thinking through the reactivation process and suggesting possible pitfalls in the case where there's a en error with one of the plugins reactivated and the code has to step back and isolate the plugin causing the error. A binary search algorithm in this case would be much better than a linear search.

100 ms is pretty optimistic, I just took a look at an 8-core 2.5Ghz Xeon box and just the init (require_once('wp-admin.php');) on the admin side is mean 195ms. So, for instance, suppose you are upgrading 10 plugins and there is one that causes a fatal error. If you use a binary search you will have to do 7-9 page loads to isolate the bad plugin. If each page load is half a second, that's ~4 seconds total.

Is that reasonable? Well ... I'm not the boss, but 4 seconds seems reasonable to me.

[joelhardi]

New patch that changes single upgrade mode to do "silent" reactivation (de/activation hooks not run)

[joelhardi]

OK, bulk update process aside, I just added a new patch​ per dd32's feedback that I believe addresses the original issue with the single plugin update.

It adds a "silent mode" to activate_plugins() identical to the one that already exists for deactivate_plugins() and uses it when a single plugin is updated. So, the plugin is loaded in its iframe sandbox to check for errors, but the activation hook is not run.

[scribu]

I think ​15671.diff is the way to go.

[scribu]

(In [16012]) Don't call activation hooks when upgrading. Props joelhardi for initial patch. See #14915

[scribu]

I'm calling this fixed. Please open new tickets for other issues, such as bulk handling large numbers of plugins.