Make WordPress Core

Opened 5 years ago

Closed 5 years ago

#49147 closed defect (bug) (fixed)

Incorrect formatting of _links on OPTIONS request to embedded collections

Reported by: nsundberg's profile nsundberg Owned by:
Milestone: 5.4 Priority: normal
Severity: normal Version: 4.4
Component: REST API Keywords: good-first-bug has-patch has-unit-tests
Focuses: Cc:

Description

When sending an OPTIONS request to a non-variable route (eg. /wp/v2/posts) with ?_embed=true and WP_DEBUG enabled the following warning appears (example from 5.3.2):

Warning: Invalid argument supplied for foreach() in /wp-includes/rest-api/class-wp-rest-server.php on line 569

This is caused by the following lines which sets the link as a string instead of an associative array with a href key, like the embed_links method expects: class-wp-rest-server.php:L1216-L1218

$data['_links'] = array(
  'self' => rest_url( $route ),
);

Expected:

$data['_links'] = array(
  'self' => array(
    'href' => rest_url( $route ),
  ),
);

Was about to submit a patch but when looking into it I got a bit unsure if the _links property even should be included in these responses. From what I can tell no other endpoints include the _links property in the response for OPTIONS requests, but please advise.

Attachments (7)

49147.diff (555 bytes) - added by nsundberg 5 years ago.
49147.2.diff (581 bytes) - added by nsundberg 5 years ago.
49147.3.diff (1.6 KB) - added by johnwatkins0 5 years ago.
Adds unit test for the change attached earlier
49147.4.diff (1.9 KB) - added by johnwatkins0 5 years ago.
49147.5.diff (1.7 KB) - added by johnwatkins0 5 years ago.
Removed debug code left in previous atttachment
49147.6.diff (1.7 KB) - added by johnwatkins0 5 years ago.
Use rest_get_server instead of new WP_REST_Server
49147.7.diff (1.5 KB) - added by TimothyBlynJacobs 5 years ago.

Download all attachments as: .zip

Change History (22)

#1 @TimothyBlynJacobs
5 years ago

  • Keywords needs-refresh good-first-bug added
  • Milestone changed from Awaiting Review to 5.4
  • Version changed from 4.5 to 4.4

Thanks for the ticket @nsundberg!

I think it'd be best to fix the link to be an associate array as you suggested. I think it'd be a good separate enhancement to add those self links to the OPTIONS response.

@nsundberg
5 years ago

#2 @nsundberg
5 years ago

  • Keywords has-patch added; needs-refresh removed

#3 follow-up: @TimothyBlynJacobs
5 years ago

Thanks for the patch @nsundberg. There should be another layer of a wrapping array around the wrapping you added. In other words, each relation should be an array of links, where each link is an array with an href key.

For example the following JSON is expected.

{
  "_links": {
    "self": [
      {
        "href": "https://"
      }
    ]
  }
}

@nsundberg
5 years ago

#4 @nsundberg
5 years ago

Ah, sorry, didn't look close enough!

This ticket was mentioned in Slack in #core-restapi by timothybjacobs. View the logs.


5 years ago

#6 @TimothyBlynJacobs
5 years ago

  • Keywords needs-unit-tests added

Thanks for the patch @nsundberg! Do you want to try tackling a unit test for this?

This ticket was mentioned in Slack in #core-restapi by kadamwhite. View the logs.


5 years ago

@johnwatkins0
5 years ago

Adds unit test for the change attached earlier

#8 follow-up: @johnwatkins0
5 years ago

  • Keywords has-unit-tests added; needs-unit-tests removed

#9 in reply to: ↑ 3 @birgire
5 years ago

Replying to TimothyBlynJacobs:

Thanks for the patch @nsundberg. There should be another layer of a wrapping array around the wrapping you added. In other words, each relation should be an array of links, where each link is an array with an href key.

For example the following JSON is expected.

{
  "_links": {
    "self": [
      {
        "href": "https://"
      }
    ]
  }
}

Hi, I was just wondering about the array wrappers for the self key.

I skimmed through:

https://developer.wordpress.org/rest-api/using-the-rest-api/linking-and-embedding/

and

http://stateless.co/hal_specification.html
https://tools.ietf.org/html/draft-kelly-json-hal-08

but I didn't see a similar self array HAL examples there, just "self": { "href" :"https://..." }

So out of curiosity, is this self array wrapping a special WordPress implementation? Thanks.

#10 in reply to: ↑ 8 @birgire
5 years ago

Replying to johnwatkins0:

Thanks for the unit test in 49147.3.diff.

I was just wondering about the GET method test there:

Shouldn't the OPTIONS method be targetted specifically here, according to the ticket's title?

ps: the @ticket 49147 annotation is missing in 49147.3.diff and array() should be used instead of the array alias [] according to the code standard.

@johnwatkins0
5 years ago

@johnwatkins0
5 years ago

Removed debug code left in previous atttachment

#11 @johnwatkins0
5 years ago

Thanks for looking, @birgire . I've added 49147.5.diff with updates based on your feedback.

Last edited 5 years ago by johnwatkins0 (previous) (diff)

#12 @TimothyBlynJacobs
5 years ago

@birgire We don't treat self special, the REST API allows for any link relation to have multiple links for it so we enforce that each link relation is an array, even if it would only ever have one link.

Thanks for adding the tests @johnwatkins0! Instead of creating a new WP_REST_Server can we use rest_get_server() here?

@johnwatkins0
5 years ago

Use rest_get_server instead of new WP_REST_Server

#13 @johnwatkins0
5 years ago

Thanks, @TimothyBlynJacobs. Updated in 49147.6.diff.

#14 @TimothyBlynJacobs
5 years ago

Uploaded a refreshed patch to use rest_url to build the link we compare against and some minor consistency changes.

#15 @TimothyBlynJacobs
5 years ago

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

Closed by r47326.

Note: See TracTickets for help on using tickets.