Make WordPress Core

Opened 14 years ago

Closed 13 years ago

Last modified 13 years ago

#14915 closed defect (bug) (fixed)

Fix activation hook inconsistency

Reported by: nacin's profile nacin Owned by:
Milestone: 3.1 Priority: normal
Severity: normal Version: 2.9
Component: Plugins Keywords:
Focuses: Cc:

Description (last modified by 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.

Attachments (4)

class-wp-upgrader.15646.diff (2.5 KB) - added by joelhardi 13 years ago.
patch deleting deactivation/activation on plugin upgrade
class-wp-upgrader.15646.2.diff (3.4 KB) - added by joelhardi 13 years ago.
newer patch per my comment (but slightly more complicated). Doesn't set and then immediately unset $update_actionsactivate_plugin?.
test-plugin.tar.gz (1.5 KB) - added by joelhardi 13 years ago.
test plugin that always wants to be upgraded (see comment)
15671.diff (3.4 KB) - added by joelhardi 13 years ago.
New patch that changes single upgrade mode to do "silent" reactivation (de/activation hooks not run)

Download all attachments as: .zip

Change History (28)

#1 @scribu
14 years ago

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

#2 @nacin
14 years ago

  • Description modified (diff)

Corrected, thanks.

#3 follow-up: @sirzooro
13 years ago

  • Cc sirzooro added

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.

#4 in reply to: ↑ 3 @nacin
13 years ago

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
13 years ago

patch deleting deactivation/activation on plugin upgrade

#5 @joelhardi
13 years ago

  • Cc joel@… added
  • Keywords has-patch added

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.

#6 @scribu
13 years ago

  • Keywords needs-patch added; has-patch removed

I only see the deactivation hook removed there.

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

#7 @joelhardi
13 years ago

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
13 years ago

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

@joelhardi
13 years ago

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

#8 @joelhardi
13 years ago

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!

#9 @scribu
13 years ago

  • Keywords has-patch added; needs-patch removed

Pretty nifty plugin.

#10 follow-up: @dd32
13 years ago

  • Keywords needs-patch added; has-patch removed

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)

#11 in reply to: ↑ 10 @nacin
13 years ago

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.

#12 @dd32
13 years ago

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.

#13 @joelhardi
13 years ago

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.

#14 @dd32
13 years ago

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.

#15 @nacin
13 years ago

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.

#16 @dd32
13 years ago

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 :)

#17 @joelhardi
13 years ago

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.

#18 follow-up: @dd32
13 years ago

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.

#19 in reply to: ↑ 18 @joelhardi
13 years ago

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
13 years ago

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

#20 @joelhardi
13 years ago

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

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.

#21 @scribu
13 years ago

  • Keywords needs-refresh added; dev-feedback removed

I think 15671.diff is the way to go.

#22 @scribu
13 years ago

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

#23 @scribu
13 years ago

  • Keywords has-patch needs-refresh removed
  • Resolution set to fixed
  • Status changed from new to closed

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

#24 @azizur
13 years ago

  • Cc azizur added
Note: See TracTickets for help on using tickets.