Shiny Updates: Erroneous Plugin Deactivation

[voldemortensen]

I recently noticed with a plugin of mine, that if you change the name of the main file in a version update, i.e.:

plugin-name/plugin-name.php => plugin-name/different-name.php

the plugin becomes inactive after the update. This was not the case prior to Shiny Updates and is still an issue in trunk. I installed 4.1.x just to be sure.

Steps to reproduce:

1. Put plugin on .org repo
2. Install/activate plugin on a site.
3. Change the name of the main file in svn and commit it with a version bump.
4. Update the plugin with Shiny Updates.

[swissspidy]

Are you sure this wasn't reproducible prior to 4.2? Plus, how is it caused by Shiny Updates V1 and not by something else?

I was once playing with the idea of changing the main plugin file and IIRC @ipstenu and @otto42 said it's a bad idea. And it's understandable: The list of active plugins (e.g. woocommerce/woocommerce.php and so on) is stored in the options table. If an active plugin file is missing, the plugin will be deactivated.

That means if you're ever going to do such a change, you would need to keep the old plugin file for BC reasons. In it, you could add a migration script that changes the active_plugins option to include the new plugin file instead of the old one. Problem solved.

I've seen a few plugins changing the plugin name in an upgrade without such a migration part, which is a bad idea.

Perhaps we could change something about this in core, i.e. check if there's a new one in the same folder if the main plugin file is missing, but otherwise it's probably a documentation/education issue.

[Ipstenu]

This was not the case prior to Shiny Updates and is still an issue in trunk

Yes it was an issue before Shiny Updates. 100% positive :)

Changing the name of the main plugin file will cause your plugin to deactivate. So it's not a 'bad' idea, it's flat out terrible and will 'break' your plugin. So ... yeah. Don't do it.

[voldemortensen]

In my testing I was unable to reproduce it prior to 4.2, if that counts for anything.

Additionally, I changed the filename because a trademark bully was throwing his weight around. I don't have the time/funds to fight it. So I guess everyone will just need to reactivate it ¯\_(ツ)_/¯

[Otto42]

This is not unique to Shiny Updates, and has always been the case. Renaming a plugin and then updating to it will cause it to be deactivated.

See the validate_active_plugins() and validate_plugin() functions, which have been around since 2.5:
https://core.trac.wordpress.org/browser/trunk/src/wp-admin/includes/plugin.php#L908

Specifically, validate_plugin() checks for the plugin file to exist, and if it doesn't, then validate_active_plugins() will specifically deactivate it, removing it from the active_plugins list.

[voldemortensen]

It seems like no one really bothered to test for themselves, so here is a video. In this video:

1. I am on WordPress 4.1.5
2. I am installing a version of my plugin prior to renaming the main file.
3. I am updating the plugin.
4. The plugin stays active after the update, notice the "Plugin successfully reactivated" message shortly before I navigate to the "Installed Plugins" page.

If you see an issues with the way I updated the plugin, please let me know. I believe this would be a pretty normal update flow for people.

​https://cloudup.com/iF41800-XIJ

[Otto42]

Confirmed on a 4.1.5 install compared to trunk.

However, you can duplicate the results of Shiny Updates in trunk in that 4.1.5 install by using the "Bulk Actions" menu to perform the update. The checkbox->select Upgrade->click apply flow has the same result of deactivating your new plugin.

The reason for this is apparently the differences between the bulk_upgrade() function and the normal upgrade() functions found in the Plugin_Upgrader class.

The wp_ajax_update_plugin() function is calling bulk_upgrade(), so using the bulk-upgrade method of previous versions will act the same way:

https://core.trac.wordpress.org/browser/trunk/src/wp-admin/includes/ajax-actions.php#L3683

[Otto42]

Ugh. Okay, here's the differences between all of these.

1. Normal upgrade (old way). Uses the Plugin_Upgrader_Skin to display the relevant information about the upgrade process. Calls the Plugin_Upgrader::upgrade() function to do the upgrade. This upgrade() function deactivates the plugin silently (without calling deactivation functions) before upgrading. Afterwards, the Plugin_Upgrader_Skin code (yes, the skin) includes an iframe which calls the activate-plugin method to display the "Plugin reactivated successfully" message.

2. Bulk Upgrade: Does not deactivate plugins in the first place. If they change their main plugin file names in this process, they will thus not be loaded and thus will be deactivated, silently.

3. Shiny updates: Calls the bulk_upgrade() function via an ajax request, so essentially behaves identically to method 2.

Basically, the ajax action needs to check if the plugin in question was active before the upgrade, and reactivate it afterwards if so. Or to just behave more like method 1, since it's only upgrading a single plugin.

[Otto42]

Minimal patch to deactivate and reactivate the plugin during upgrades via ajax actions

[Otto42]

attachment:37470.2.diff​ patches the wp_ajax_update_plugin() function to deactivate the plugin prior ro upgrading it (just like the normal upgrade call does), and then it reactivates it afterward, even if the main plugin file name changed.

This makes the Shiny update method behave more like a normal update in this respect, but without all the iframe sadness.

[SergeyBiryukov]

Sounds like a duplicate of #20944 (or at least closely related).

[Otto42]

The iframe based re-activation of the plugin on an upgrade can be traced back to the original code 7 years ago:

https://core.trac.wordpress.org/browser/trunk/wp-admin/includes/class-wp-upgrader.php?rev=11005#L766

Dunno what all this code has been through since then. Probably in need of an overall redo and simplification, really.

[ocean90]

@pento, @obenland, @swissspidy: Do you remember why wp_ajax_update_plugin() uses Plugin_Upgrader::bulk_upgrade() and not Plugin_Upgrader::upgrade()? Introduced in [31333].

[swissspidy]

That was before my involvement with Shiny Updates, but I assume it was for practical reasons so wp_ajax_update_plugin() could handle both single and bulk plugin updates.

[Otto42]

No, the wp_ajax_update_plugin() function can only update a single plugin anyway, because of code earlier in the function. It has to be passed a plugin path and a slug through $_POST.

I suspect that the Automatic_Upgrader_Skin class is the main problem here, as it doesn't have any support for re-activating the plugin afterwards like the Plugin_Upgrader_Skin class does, so calling the normal ::upgrade() would always deactivate the plugin silently, while ::bulk_upgrade() would only deactivate the plugin in this special circumstance.

[dd32]

Yeah, "Bulk" upgrades don't perform the deactivate-reactivate dance as no-one could figure out a good way to do it without breaking everything.
Simply calling activate_plugins() in an AJAX request isn't enough, you need to handle the error conditionals from that (which is a forced redirect IIRC, at least in the case of fatals).

This isn't new to 4.6, is reproducible in older versions, and quite honestly, is low priority in the scheme of things. If there's a viable fix for 4.6, then lets do it, otherwise punting is fine.
Handling fatal errors during updates (also not a regression from 4.5, but a regression from a much older version) is a much higher priority issue than the filename changing, which this patch can't handle properly (The deactivate and activate calls *have* to be in separate processes, or at least the deactivate has to happen before plugin inclusion)

Do you remember why wp_ajax_update_plugin() uses Plugin_Upgrader::bulk_upgrade() and not Plugin_Upgrader::upgrade()?

IIRC Bulk upgrades were used simply to avoid the non-bulk upgrade methods which had the deactivate-reactivate functionalities, plus it has a better API. It was also the flow that most users were using at the time and IIRC not something that shiny updates pioneered.

[Otto42]

Agreed, this patch did not properly handle error cases. A more aggressive change would be needed to do that.

[ocean90]

Punting since it's not a recent regression.