Make WordPress Core

Opened 9 years ago

Closed 9 years ago

Last modified 9 years ago

#37583 closed defect (bug) (fixed)

Shiny Updates: Enhance error reporting when response is invalid

Reported by: ocean90's profile ocean90 Owned by: azaozz's profile azaozz
Milestone: 4.6 Priority: normal
Severity: normal Version: 4.6
Component: Upgrade/Install Keywords: has-patch has-screenshots commit
Focuses: ui, javascript, administration Cc:

Description

wp.updates.isValidResponse() checks the Ajax response and tries to provide a meaningful error message in case of an error. This doesn't work always:

  • Invalid nonce: "Update Failed: -1"
  • Connection lost: "Update Failed: error" (see #37550 for a screenshot)
  • 404 error: "Update Failed: $html" (https://cloudup.com/c-2MST8o_RE, unescaped in the admin notice)

Attachments (3)

37583.patch (2.9 KB) - added by ocean90 9 years ago.
invalid-nonce.png (68.3 KB) - added by ocean90 9 years ago.
connection-lost.png (62.1 KB) - added by ocean90 9 years ago.

Download all attachments as: .zip

Change History (9)

@ocean90
9 years ago

#1 @ocean90
9 years ago

37583.patch:

  • Escape messages for admin notices
  • Display "Update Failed: An error has occurred. Please reload the page and try again." for a nonce failure.
  • Display "Update Failed: Connection lost or the server is busy. Please try again later." for connection lost.
  • Both strings are already used by the editor/media components.

@ocean90
9 years ago

#2 @ocean90
9 years ago

  • Keywords has-screenshots commit added

A 404 seems to be unlikely, it would mean that admin-ajax.php is gone. So the raw output should be fine here.

#3 @ocean90
9 years ago

  • Owner set to azaozz
  • Status changed from new to reviewing

#4 @azaozz
9 years ago

37583.patch looks good but we may need to strip HTML from the passed error messages. Got this by adding sleep(31) to wp-config.php

Update Failed: <br /> <b>Fatal error</b>: Maximum execution time of 30 seconds exceeded in <b>\test\wp-config.php</b> on line <b>45</b><br />
Version 0, edited 9 years ago by azaozz (next)

#5 @ocean90
9 years ago

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

In 38205:

Update/Install: Replace "error" and "-1" failure messages with a more meaningful one.

  • "-1" is an invalid nonce error, show 'An error has occurred. Please reload the page and try again.'.
  • "error" means that the connection to the server was lost, show 'Connection lost or the server is busy. Please try again later.'.
  • Escape the message in wp-updates-admin-notice because the response may include HTML.
  • Remove HTML tags in wp.updates.isValidResponse() to make PHP's error messages more readable.

Props azaozz for review.
Fixes #37583.

#6 @ocean90
9 years ago

In 38206:

Update/Install: Replace "error" and "-1" failure messages with a more meaningful one.

  • "-1" is an invalid nonce error, show 'An error has occurred. Please reload the page and try again.'.
  • "error" means that the connection to the server was lost, show 'Connection lost or the server is busy. Please try again later.'.
  • Escape the message in wp-updates-admin-notice because the response may include HTML.
  • Remove HTML tags in wp.updates.isValidResponse() to make PHP's error messages more readable.

Merge of [38205] to the 4.6 branch.

Props azaozz for review.
See #37583.

Note: See TracTickets for help on using tickets.