Make WordPress Core

Opened 10 years ago

Closed 10 years ago

Last modified 10 years ago

#39314 closed defect (bug) (fixed)

WP-API Backbone Client: buildModelGetter fails to reject deferred on fetch error

Reported by: westonruter Owned by: adamsilverstein
Priority: normal Milestone: 4.7.1
Component: REST API Version: 4.7
Severity: normal Keywords: has-patch has-unit-tests commit
Cc: Focuses:

Description

I found an issue where the promise returned by getAuthorUser never resolves in the case of a request error (such as a 404). I found that the client is only accounting for the success case in which the deferred is resolved, but it is not accounting for the error case in which the deferred is just left pending.

Attachments (3)

39314.0.diff​ (695 bytes ) - added by westonruter 10 years ago.
39314.diff​ (1.7 KB ) - added by adamsilverstein 10 years ago.
39314.2.diff​ (1.6 KB ) - added by rachelbaker 10 years ago.
pinking shears

Download all attachments as: .zip

Change History (15)

@westonruter
10 years ago

#1 @westonruter
10 years ago

  • Keywords has-patch added
  • Owner set to adamsilverstein
  • Status new → assigned
  • Version → 4.7

@adamsilverstein I made this 39314.0.diff​ patch off of wp-api.js, though I know it's a compiled file. I know you'll know how to apply it.

Also, any other cases of fetch being done in the Backbone client should also ensure that there is a rejection of the deferred in the case of an error as seen in this patch.

#3 @adamsilverstein
10 years ago

  • Keywords has-unit-tests commit added

Thanks @westonruter - good catch! I went ahead and added an error handler for the other get helper builder and a unit test to confirm the behavior for getAuthorUser in ​https://github.com/WP-API/client-js/pull/150 (​diff). Ultimately I would like to expand the unit test to cover all helpers. I confirmed the test I wrote fails before your patch and passes after it. I will add the test to my patch on #39264 when I get these tests into core.

#4 @adamsilverstein
10 years ago

In 39314.diff​ :

Ensure that when a helper method's underlying fetch call fails, reject gets called on the promise returned by the helper method. Enables better error handling.

#5 @adamsilverstein
10 years ago

@westonruter can you please give 39314.diff​ a review/commit?

#6 @rachelbaker
10 years ago

@adamsilverstein What do you want to do here? If @westonruter doesn't have the bandwidth to review, is there someone else? Or do you want to punt this to 4.7.2?

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


10 years ago

#8 @adamsilverstein
10 years ago

@rachelbaker I can commit to trunk, I think I need a review from a permanent committer to merge to 4.7.1

#9 @westonruter
10 years ago

@adamsilverstein I can confirm the patch works as expected.

@rachelbaker
10 years ago

pinking shears

#10 @rachelbaker
10 years ago

@adamsilverstein Patch looks good. I just trimmed some extra line breaks in 39314.2.diff​ for a cleaner diff.

#11 @adamsilverstein
10 years ago

  • Resolution → fixed
  • Status assigned → closed

In 39680:

REST API: Add error handling for fetch error in buildModelGetter of wp-api.js.

When a call to a model getter method fails, reject the returned deferred object. Enables better handling of fetch errors.

Props westonruter, adamsilverstein.
Fixes #39314.

#12 @dd32
10 years ago

In 39682:

REST API: Add error handling for fetch error in buildModelGetter of wp-api.js.

When a call to a model getter method fails, reject the returned deferred object. Enables better handling of fetch errors.

Props westonruter, adamsilverstein.
Merges [39680] to the 4.7 branch.
Fixes #39314.

Note: See TracTickets for help on using tickets.