Make WordPress Core

Opened 10 years ago

Closed 10 years ago

#32110 closed defect (bug) (fixed)

"Shiny" plugin updates not working when I try run mulitple updates at once and one fails

Reported by: magicroundabout's profile magicroundabout Owned by: dd32's profile dd32
Milestone: 4.2.3 Priority: normal
Severity: normal Version: 4.2
Component: Upgrade/Install Keywords: has-patch fixed-major
Focuses: Cc:

Description

This is in Chrome 42 on a Mac.

I'm having issues with shiny plugin updates getting stuck when an update fails.

If I click on a load of shiny updates to get them running, and one gets an "Update Failed", the others do not update. They just end up stuck in "Updating" mode.

Here's a screencast showing the issue: http://quick.as/LWOuLpmZ

Presumably the updates are queued in the browser as you click them. It looks like if admin-ajax returns success:false for an update, subsequent updates in the queue are not processed.

I don't get any JavaScript errors.

In the example in the screencast, I've no idea why Akismet returns successful. That's possibly a different bug.

I've looked at the requests in developer tools.

The second request returns:

{"success":false,"data":{"update":"plugin","plugin":"give\/give.php","slug":"give","oldVersion":"Version 0.7.5 beta","newVersion":""}}

The third request is not made.

Attachments (2)

updates.js.diff (327 bytes) - added by magicroundabout 10 years ago.
Added queueChecker() call to wp.updates.updateError method
updates2.js.diff (361 bytes) - added by magicroundabout 10 years ago.
Same as before, but unset the lock as well

Download all attachments as: .zip

Change History (19)

#1 @magicroundabout
10 years ago

Just dived in the code. Found wp-admin/js/updates.js

Is this as simple as adding a call to wp.updates.queueChecker(); to the end of wp.updates.updateError()

I'll added a patch. I don't know how you guys build the JS for testing so I don't know how to test it.

@magicroundabout
10 years ago

Added queueChecker() call to wp.updates.updateError method

#2 @magicroundabout
10 years ago

I was nearly there. Having been introduced to SCRIPT_DEBUG by @sergeybiryukov (thanks!) I ran a test and discovered that the lock needed unsetting to.

The following, therefore, is a tested patch!

@magicroundabout
10 years ago

Same as before, but unset the lock as well

#3 @SergeyBiryukov
10 years ago

  • Milestone changed from Awaiting Review to 4.2.1

#4 @DrewAPicture
10 years ago

  • Keywords has-patch needs-testing added

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


10 years ago

#6 @samuelsidler
10 years ago

  • Milestone changed from 4.2.2 to 4.2.3

#7 @stevenkword
10 years ago

I'm not able to reproduce the events shown in the video. Once disconnecting my connection and clicking update, I get all spinners with no failures. I am also not experiencing the issues seen with Akismet and wonder if the two are related. I'm going to investigate other ways to induce the failure so I can test this.

#8 follow-up: @magicroundabout
10 years ago

Hi @stevenkword

Thanks for dipping in to help with this.

Perhaps I wasn't quite clear enough. The connection disruption needs to be from the server running WordPress to the update server. So in my case I was doing this on a local install of WordPress, so pulling my network connection prevent WordPress contacting the update server.

I THINK, from what you've said, that you might have WordPress running remotely, and your network disconnection is preventing your browser contact WordPress.

I didn't actually explain this very well in the original explanation of the problem. Can you let me know if you were working on a local WordPress install or a remote one? Are you able to replicate on a local server (running on the same machine as your browser) with the network disconnected? Or somehow otherwise simulate a network failure on the server end?

The Akismet thing is raise elsewhere in #32086. It looks like Plugin_Upgrader->bulk_upgrade() is returning the wrong value, but I have bigger concerns over what the "result" of a bulk upgrade should be. Follow the thread over there - it's pretty complicated but hopefully someone can follow.

#9 in reply to: ↑ 8 @stevenkword
10 years ago

@magicroundabout -- I was testing this with a local WP instance running with VVV.

#10 follow-up: @magicroundabout
10 years ago

OK. Cool. So, just to double check, when you say "Once disconnecting my connection..." - how did you go about disconnecting your connection?

I ask because the behaviour you've described (all spinners, no updates (?), no errors) is what happens when your browser can't contact WordPress. So I'm wondering if you couldn't contact the web server on the Vagrant box?

(I'm not hugely familiar with Vagrant so I don't know what the details of its networking are)

#11 in reply to: ↑ 10 @stevenkword
10 years ago

VVV is more or less just a VM running on my local machine. OSX networking is passed through to the VM. So when I disable wifi in OSX, it also disconnects internet access to the VM. I believe I'm following the same steps you are, but am seeing different results.

#12 follow-up: @dd32
10 years ago

  • Keywords needs-testing removed

I'm not able to reproduce the events shown in the video. Once disconnecting my connection and clicking update, I get all spinners with no failures.

This is because wp.updates.updateError relies upon the server response to know which plugin it's actually operating upon. As a result, if the JSON return is malformed (ie. PHP notices) or the connection drops, the error handler is unable to report the failure to the UI.

The other issue at play here is that if an individual plugin update fails, the other updates are not canceled, nor continued. The patch here appears to fix this, and I see no harm in allowing other updates to continue (even if one has failed for some reason).

The cause for a plugin update failing, could be related to #32198

#13 @dd32
10 years ago

  • Owner set to dd32
  • Resolution set to fixed
  • Status changed from new to closed

In 32780:

Updates: When an update fails (and it's not during the credential form) allow further updates to continue, with the assumption that the error only affects a single plugin.
Props magicroundabout. Fixes #32110 for trunk.

#14 @dd32
10 years ago

  • Keywords fixed-major added
  • Resolution fixed deleted
  • Status changed from closed to reopened

Re-opening for 4.2.x consideration

#15 in reply to: ↑ 12 @stevenkword
10 years ago

Replying to dd32:

The other issue at play here is that if an individual plugin update fails, the other updates are not canceled, nor continued. The patch here appears to fix this, and I see no harm in allowing other updates to continue (even if one has failed for some reason).

I agree. The cause of the failure isn't as important here as the behavior upon failure. updates2.js.diff makes sense to me.

Last edited 10 years ago by stevenkword (previous) (diff)

#16 @dd32
10 years ago

#31892 was marked as a duplicate.

#17 @dd32
10 years ago

  • Resolution set to fixed
  • Status changed from reopened to closed

In 33306:

Updates: When an update fails (and it's not during the credential form) allow further updates to continue, with the assumption that the error only affects a single plugin.

Props magicroundabout.
Merges [32780] to the 4.2 branch
Fixes #32110 for 4.2

Note: See TracTickets for help on using tickets.