AJAX plugin update says it updated when in reality it didn't

[Viper007Bond]

The AJAX plugin update functionality is reporting a success and that it updated the plugin when in reality it did not. A refresh of the page shows it needs updating.

Attempting to update using the traditional method by opening the AJAX link in a new tab shows it's failing due to file permissions (I don't have it writable):

Downloading update from ​https://downloads.wordpress.org/plugin/list-pages-shortcode.1.7.2.zip…

Unpacking the update…

Installing the latest version…

Removing the old version of the plugin…

Could not remove the old plugin.

Plugin update failed.

Return to Plugins page

[mordauk]

I've just replicated this.

[mordauk]

This is happening because $upgrader->bulk_upgrade( array( $plugin ) ); is still returning an array when a plugin fails to update. The WP_Error is returned as the plugin, so $results[ $plugin ] has to be checked for a WP_Error.

32086.patch fixes this by adding in another WP_Error check inside of if ( is_array( $result ) ) { check.

[magicroundabout]

I've replicated this too. See the video I made for #32110 : ​http://quick.as/LWOuLpmZ

I've applied @mordauk's patch and tested and this behaviour still exists. Any other ideas?

I definitely get a success response from admin-ajax:

{"success":true,"data":{"update":"plugin","plugin":"akismet\/akismet.php","slug":"akismet","oldVersion":"Version 3.0.1","newVersion":"Version 3.0.1","test":null}}

[magicroundabout]

Note: this always happens on the FIRST plugin I update only. If I get the plugins page up on a local install with three updates available, unplug my network, and attempt the updates, the first one reports success even though nothing has updated.

Hopefully this is the same as @viper007bond's issue.

I'm pretty new to core contrib, so please be nice! :)

[magicroundabout]

I used xdebug to dig around quickly. After the

$result = $upgrader->bulk_upgrade( array( $plugin ) );

call.

$result[<some plugin>] at this point is null, not a WP_Error.

Investigating further.

[mordauk]

Hmmm, it was definitely a WP_Error for me, though that could be because I manually changed the permissions on the plugin folder to 555 to force an error.

[magicroundabout]

Use $this->result in Plugin_Upgrader->bulk_upgrade()

[magicroundabout]

OK. After a good xdebug session, I think there's a tiny error in the Plugin_Upgrader class. I've added 32086-1.patch (I can't get that to link to the patch - sorry!)

This fixes my issue: plugins report success despite not upgrading due to no network.

I don't know if this breaks anything else. Are there regression tests on plugin upgrades?

[magicroundabout]

My concern with my patch is that $this->result will end up being the result of the LAST update performed, which in the case of a shiny-update is OK because there's only one of them, but may not be right for bulk upgrades in general.

The alternative patch would be to use just $result on both lines 896 and 907.

BUT...the bigger question here is what should Plugin_Upgrader->result be for a bulk update? (It seems to be used again in Plugin_Upgrader->plugin_info() which isn't used internally in the class, but is called by the skins. - at which point I'm out of my depth - HELP!!)

[magicroundabout]

One more thing...sorry.

It was bugging me, as a travelled home from the office, that in my case (with a disconnected network) only the FIRST update incorrectly reported success. I've not xdebugged it to be sure, but I THINK this is because Plugin_Upgrader->bulk_upgrade() calls wp_clean_plugins_cache() at the end, regardless.

My guess is that this wipes out any stored data about plugin updates. Successive attempts to run updates, with the network offline, then fail because the plugins_cache is empty (it won't be refreshed because the network is offline) and implies that there is nothing to do.

So I can explain the behaviour, I think. But I wonder, more generally, if clearing the cache here is the right thing to do. Let me explain:

It's unlikely but I can imagine a situation where (with the network online):

• User visits the plugins screen and sees some updates for plugins X, Y, and Z.
• The update for plugin Z is a minor update, say, v1.0.0 to 1.0.1
• User clicks update for plugins X and Y
• AT THIS EXACT MOMENT the author of plugin Z releases a new major version with breaking changes (we'll ignore time from commit to actually being available to download from the repo)

The user's screen now does not reflect reality. They could click the update for plugin Z thinking it's a non-breaking minor update, only to find that they get a major update that breaks their site.

So, my other question: should wp_ajax_update_plugin() in ajax-actions.php pass clear_update_cache => false to $upgrader->bulk_upgrade() ? This is possibly a question for another ticket?

[rabmalin]

Could this ticket https://core.trac.wordpress.org/ticket/32219 be Duplicate to this issue?

[magicroundabout]

Hi @rabmalin

I see you're kinda new to trac, so thanks for the comprehensive bug report with screenshots over on #32219.

I'm pretty new to trac myself so hope we can work through this together.

I think this IS the same issue. But I'm going to leave someone more knowledgable to close off the other ticket in case they think there's something different there.

I'm not sure how much of this discussion here that you've followed, but my general feeling is that there's a whole load of issues in the detail of how the upgrader routines work, and particularly with how they are used by the new "shiny" AJAX updates on the plugins screen. I think it needs a bit of a review and some bigger decisions made. But we'll see.

I'm hoping to work with someone a bit more experienced on it in the near future. Watch this space!

I'll update your other ticket too.

Thanks for contributing!

Ross

[dd32]

I'm fairly certain this is now resolved in #32473. Re-open if this is a distinct and still reproducable error.