Make WordPress Core

Opened 10 years ago

Closed 10 years ago

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

Download all attachments as: .zip

Change History (9)

@ocean90
10 years ago

#1 @ocean90
10 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.

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

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

#4 @azaozz
10 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 10 years ago by azaozz (previous) (diff)

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