WordPress.org

Make WordPress Core

Opened 2 months ago

Closed 4 weeks ago

#41056 closed defect (bug) (fixed)

WP-API JS Client: Settings is incorrectly registered as a collection

Reported by: adamsilverstein Owned by: adamsilverstein
Milestone: 4.8.1 Priority: normal
Severity: normal Version: 4.7
Component: General Keywords: has-patch has-unit-tests commit fixed-major
Focuses: javascript, rest-api Cc:

Description

Similar to the me endpoint, /wp/v2/settings is a model, not a collection. This is incorrectly detected due to the JS code assuming anything not ending with a regex is a collection, which is not true.

Potentially, we need to change the API to better expose what is and isn't a collection to the JS. The schema in the returned OPTIONS currently indicates that the response is an object, whereas it's actually a list of objects. If we change this, we need to look carefully at how we retrofit this, as changing it may break compatibility.

We should discuss how best to expose collections vs models via the API. Changing the schema is the best way, I think. See WP-API/proposals#2 for that discussion.

In the meantime, we should special-case /settings as well.

Attachments (2)

41056.diff (2.9 KB) - added by adamsilverstein 2 months ago.
41056.2.diff (3.3 KB) - added by adamsilverstein 5 weeks ago.

Download all attachments as: .zip

Change History (17)

#1 @adamsilverstein
2 months ago

In 41056.diff:

  • Correct handling for the settings endpoint as a model
  • Enable passing the 'words' for the regex test that differentiate between models and collections
  • Add me/settings to the unit tests.

#2 @adamsilverstein
2 months ago

  • Keywords has-patch has-unit-tests added; needs-patch removed
  • Milestone changed from Awaiting Review to 4.8.1
  • Owner set to adamsilverstein
  • Status changed from new to assigned
  • Version set to 4.7

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


6 weeks ago

This ticket was mentioned in Slack in #core-js by adamsilverstein. View the logs.


6 weeks ago

#5 @kadamwhite
6 weeks ago

@adamsilverstein Reviewed. In general it looks good; in lieu of a better way to clearly explain how we want an endpoint to be treated, this seems like a straightforward solution. One naming nitpick, I'd suggest modelEndpoints instead of singleModels as I feel it's a little more descriptive. If I see singleModels I would want to know what other kinds of models there are, vs understanding that this is a model and not a collection.

#6 @adamsilverstein
6 weeks ago

@kadamwhite thanks for the feedback, I like your naming suggestion. I will work on an updated patch.

#7 @adamsilverstein
6 weeks ago

  • Keywords needs-refresh added

#8 @adamsilverstein
5 weeks ago

in 41056.2.diff

  • use modelEndpoints as the variable name for the endpoints that are models.

#9 @jbpaul17
5 weeks ago

  • Keywords needs-testing added; needs-refresh removed

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


5 weeks ago

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


5 weeks ago

#12 @kadamwhite
5 weeks ago

  • Keywords commit added; needs-testing removed

Refreshed patch looks good to me

#13 @kadamwhite
5 weeks ago

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

In 41112:

WP-API JS Client: Interpret Settings resource as a model.

The REST API does not provide a mechanism to distinguish between endpoints representing models and those representing collections, so the Backbone client must make that distinction internally. Previously wp-api.js accounted for /users/me, but not for /settings. This patch updates the logic so that /settings is properly registered as a Backbone model.

When calling wp.api.init, additional endpoints can be specified to be models using the modelEndpoints argument.

Props @adamsilverstein.
Fixes #41056.

#14 @westonruter
4 weeks ago

  • Keywords fixed-major added
  • Resolution fixed deleted
  • Status changed from closed to reopened

Re-opening for 4.8.1

#15 @westonruter
4 weeks ago

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

In 41126:

WP-API JS Client: Interpret Settings resource as a model.

The REST API does not provide a mechanism to distinguish between endpoints representing models and those representing collections, so the Backbone client must make that distinction internally. Previously wp-api.js accounted for /users/me, but not for /settings. This patch updates the logic so that /settings is properly registered as a Backbone model.

When calling wp.api.init, additional endpoints can be specified to be models using the modelEndpoints argument.

Merges [41112] onto 4.8 branch.
Props adamsilverstein, kadamwhite.
Fixes #41056 for 4.8.1.

Note: See TracTickets for help on using tickets.