Make WordPress Core

Opened 8 years ago

Closed 8 years ago

Last modified 8 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 8 years ago.
invalid-nonce.png (68.3 KB) - added by ocean90 8 years ago.
connection-lost.png (62.1 KB) - added by ocean90 8 years ago.

Download all attachments as: .zip

Change History (9)

@ocean90
8 years ago

#1 @ocean90
8 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
8 years ago

#2 @ocean90
8 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
8 years ago

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

#4 @azaozz
8 years ago

37583.patch looks good but we may need to strip HTML from the passed error messages as they are escaped and would show. 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 />
Last edited 8 years ago by azaozz (previous) (diff)

#5 @ocean90
8 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
8 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.