WordPress.org

Make WordPress Core

Opened 3 years ago

Closed 3 years ago

#24758 closed defect (bug) (fixed)

Revisions needs to gracefully handle AJAX failures.

Reported by: markjaquith Owned by: markjaquith
Milestone: 3.6 Priority: normal
Severity: normal Version: 3.6
Component: Revisions Keywords: has-patch commit
Focuses: Cc:

Description

Currently we just leave the spinner going.

Attachments (10)

24758.diff (3.6 KB) - added by markjaquith 3 years ago.
24758.2.diff (4.2 KB) - added by markjaquith 3 years ago.
24758.3.diff (4.4 KB) - added by markjaquith 3 years ago.
24758.4.diff (4.5 KB) - added by markjaquith 3 years ago.
Stops after 1
24758.5.diff (5.0 KB) - added by markjaquith 3 years ago.
24758.6.diff (5.0 KB) - added by markjaquith 3 years ago.
New error message.
24758.7.diff (5.8 KB) - added by markjaquith 3 years ago.
Smaller spinner. Tweaked error message.
24758.8.diff (5.6 KB) - added by markjaquith 3 years ago.
24758.9.diff (5.6 KB) - added by markjaquith 3 years ago.
24758.10.diff (5.6 KB) - added by markjaquith 3 years ago.

Download all attachments as: .zip

Change History (22)

@markjaquith
3 years ago

#1 @markjaquith
3 years ago

Simple error message.

#2 @nacin
3 years ago

Related, #24757. Me: "Perhaps if the request times out, errors out, or takes more than X seconds, we need to cut in half the number of revisions we just tried to request."

@markjaquith
3 years ago

#3 @markjaquith
3 years ago

24758.2.diff implements Nacin's suggestion. on .fail(), it cuts the number in half. I tested this by setting the AJAX callback to fail if the number of requested comparisons was greater than 5. It cut down from 40 to 20, then 20 to 10, then 10 to 5, which worked, so it did the rest of them by 5. Nice.

@markjaquith
3 years ago

#4 @markjaquith
3 years ago

24758.3.diff makes sane the promise attempt.

#5 @ocean90
3 years ago

To test this, I simply produced a fatal error in wp_ajax_get_revision_diffs. On a post with 200 revisions the number goes down: 20, 10, 5, 3, 2, 1. Works so far. But I didn't get the error message and it still hammers the server with a request. Shouldn't we stop after X failed request?

#6 @markjaquith
3 years ago

Yeah, it should stop after 1.

@markjaquith
3 years ago

Stops after 1

@markjaquith
3 years ago

#7 @markjaquith
3 years ago

24758.5.diff stops after one, and properly displays the error message. Tested with a fatal error in wp_ajax_get_revision_diffs().

@markjaquith
3 years ago

New error message.

#8 follow-up: @ocean90
3 years ago

The error message is not displayed on first load. But requests stop.

#9 in reply to: ↑ 8 @adamsilverstein
3 years ago

Replying to ocean90:

The error message is not displayed on first load. But requests stop.

the first diff is now preloaded in php so you will never see an error on first load. i set up a random error in wp_ajax_get_revision_diffs() and tested, with 24758.6.diff i do see the error message briefly, until the ajax re-fires and loads correctly. i also observed the number of diffs bundled per request reducing to 1 and then giving up on further requests.

looks good!

Last edited 3 years ago by adamsilverstein (previous) (diff)

@markjaquith
3 years ago

Smaller spinner. Tweaked error message.

#10 @nacin
3 years ago

  • Keywords has-patch added
  • Owner set to markjaquith
  • Status changed from new to assigned

@markjaquith
3 years ago

@markjaquith
3 years ago

@markjaquith
3 years ago

#11 @nacin
3 years ago

  • Keywords commit added

Good to go here on .10.diff.

#12 @markjaquith
3 years ago

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

In 24732:

Revisions: Better error handling.

  • Shows an error message if the current diff can't be loaded.
  • For bulk pre-loading, catches errors, and cuts subsequent requests in half, until eventually giving up.
  • Some CSS fixes related to this, and the loading spinner.
  • wp.revisions.loadAll() now returns a promise representing whether or not all revisions could be loaded.

Fixes #24758.

Note: See TracTickets for help on using tickets.