WordPress.org

Make WordPress Core

Opened 3 years ago

Closed 2 years ago

#32086 closed defect (bug) (duplicate)

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

Reported by: Viper007Bond Owned by:
Milestone: Priority: normal
Severity: normal Version: 4.2
Component: Upgrade/Install Keywords: has-patch needs-testing
Focuses: administration Cc:

Description

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

Attachments (2)

32086.patch (2.0 KB) - added by mordauk 3 years ago.
32086-1.patch (439 bytes) - added by magicroundabout 3 years ago.
Use $this->result in Plugin_Upgrader->bulk_upgrade()

Download all attachments as: .zip

Change History (17)

#1 @mordauk
3 years ago

I've just replicated this.

@mordauk
3 years ago

#2 @mordauk
3 years ago

  • Keywords has-patch needs-testing added

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.

#3 @DrewAPicture
3 years ago

  • Milestone changed from Awaiting Review to 4.2.1

#4 @magicroundabout
3 years ago

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}}

#5 @magicroundabout
3 years ago

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

#6 @magicroundabout
3 years ago

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.

#7 @mordauk
3 years ago

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

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

#8 @magicroundabout
3 years ago

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?

#9 @magicroundabout
3 years ago

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

#10 @magicroundabout
3 years ago

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?

This ticket was mentioned in Slack in #core by magicroundabout. View the logs.


3 years ago

#12 @samuelsidler
3 years ago

  • Milestone changed from 4.2.2 to 4.2.3

#13 @rabmalin
3 years ago

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

#14 @magicroundabout
2 years ago

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

#15 @dd32
2 years ago

  • Component changed from Plugins to Upgrade/Install
  • Milestone 4.2.3 deleted
  • Resolution set to duplicate
  • Status changed from new to closed

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

Note: See TracTickets for help on using tickets.