Make WordPress Core

Opened 4 years ago

Last modified 4 months ago

#49985 assigned defect (bug)

REST API: Using _embed and _fields query parameters in the same query

Reported by: anouarbenothman's profile anouarbenothman Owned by: timothyblynjacobs's profile TimothyBlynJacobs
Milestone: Future Release Priority: normal
Severity: normal Version: 5.4
Component: REST API Keywords: early has-patch has-unit-tests
Focuses: rest-api Cc:

Description

When performing a REST API request and adding the _embed and _fields query parameters. the API attempts to skip unneeded fields but does not embed resources that are linked with embeddable set to true.

In some cases we may need _embed and _fields in the same query.
I am requesting the feature to be added to allow the filtering of resources and embedding them (the embeddable ones) at the same time when specifying the _embed and _fields.

Example:
http://<site>/wp-json/wp/v2/posts?_fields=title,author&_embed=author

If implemented, the above request should only return title and author, and embed the author.

Change History (35)

#1 @ocean90
4 years ago

Hello @anouarbenothman, welcome to Trac!

Thanks for the report. I noticed this a while ago too but seems like I never created a ticket for it. The workaround is to include _links in the fields too so you get at least all embed data.

No embed: https://wordpress.org/news/wp-json/wp/v2/posts?_embed=true&_fields=id,title
With embed: https://wordpress.org/news/wp-json/wp/v2/posts?_embed=true&_fields=id,title,_links

#49538 is about getting only a subset of the embed data.

#2 @TimothyBlynJacobs
4 years ago

  • Keywords needs-patch needs-unit-tests added
  • Milestone changed from Awaiting Review to Future Release

Yeah this is a bit of an annoying issue. The problem is that rest_filter_response_fields will remove the _links field before link embedding is processed. So the server doesn't know what resources to embed.

We could perhaps fix this by not excluding _links in rest_filter_response_fields if _embed is set.

Another step would be to reduce the _links to only contain entries specified by _embed if _links is not included in _fields at all.

#3 @anouarbenothman
4 years ago

  • Keywords needs-patch needs-unit-tests removed

Thank you very much for the reply.

#4 @anouarbenothman
4 years ago

  • Keywords needs-patch needs-unit-tests added

#5 @TimothyBlynJacobs
3 years ago

  • Keywords early added
  • Milestone changed from Future Release to 5.7
  • Owner set to TimothyBlynJacobs
  • Status changed from new to assigned

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


3 years ago

This ticket was mentioned in PR #867 on WordPress/wordpress-develop by engahmeds3ed.


3 years ago
#7

  • Keywords has-patch added; needs-patch removed

https://core.trac.wordpress.org/ticket/49985

  • [x] Adjust the code to check if request has embed and _links are not inside fields then add specific embeds fields to be fetched only.
  • [ ] Adjust tests

TimothyBJacobs commented on PR #867:


3 years ago
#8

Thanks for working on this @engahmeds3ed!

The patch looks good to me! It'd be great to get test coverage on this as well.

The test failures are a bit odd, it looks like the rest_filter_response_fields tests are passing an array instead of a proper WP_REST_Request object. I think changing the tests to properly pass a request object would be the fix here. I don't think we need to adjust rest_filter_response_fields to accept an array.

engahmeds3ed commented on PR #867:


3 years ago
#9

Thanks @TimothyBJacobs for your help here ( as usual ), this is my first time writing WP Core test here and interact with all of these coding standards, plz when u have a minute check this one and tell me if this test is enough or we need to add some other test cases?!

TimothyBJacobs commented on PR #867:


3 years ago
#10

Looks great to me @engahmeds3ed, thanks for working on it! Just a minor feedback item.

TimothyBJacobs commented on PR #867:


3 years ago
#11

Looks great to me @engahmeds3ed, thanks for working on it! Just a minor feedback item.

TimothyBJacobs commented on PR #867:


3 years ago
#12

@engahmeds3ed Sorry for not noticing the other day, but this actually isn't quite right yet. We need to handle when _embed=1 or just ?_embed, in other words when all embeddable links should be embedded. See rest_parse_embed_param.

We could probably do this by looping over $response→get_links() and looking for links that are set as embeddable.

engahmeds3ed commented on PR #867:


3 years ago
#13

@TimothyBJacobs good catch, I'm adjusting that now and add also new test case for it

TimothyBJacobs commented on PR #867:


3 years ago
#14

A couple of things we need to handle. When requesting a collection with _embed=1, for instance http://localhost:8889/wp-json/wp/v2/posts?_embed=1&_fields=id,author, I get the following response.

[
  {
    "id": 1,
    "author": 1
  }
]

That's because in this case the links aren't a part of WP_REST_Response::get_links() but in each individual item's _links. So we need to do the same kind of wp_is_numeric_array check we see at the bottom of rest_filter_response_fields.

Also, for a single item, the _links aren't automatically reduced. For instance, http://localhost:8889/wp-json/wp/v2/posts/1?_embed=replies,author&_fields=id,author.

{
  "id": 1,
  "author": 1,
  "_links": {
    "self": [
      {
        "href": "http:\/\/localhost:8889\/wp-json\/wp\/v2\/posts\/1"
      }
    ],
    "collection": [
      {
        "href": "http:\/\/localhost:8889\/wp-json\/wp\/v2\/posts"
      }
    ],
    "about": [
      {
        "href": "http:\/\/localhost:8889\/wp-json\/wp\/v2\/types\/post"
      }
    ],
    "author": [
      {
        "embeddable": true,
        "href": "http:\/\/localhost:8889\/wp-json\/wp\/v2\/users\/1"
      }
    ],
    "replies": [
      {
        "embeddable": true,
        "href": "http:\/\/localhost:8889\/wp-json\/wp\/v2\/comments?post=1"
      }
    ],
    "version-history": [
      {
        "count": 0,
        "href": "http:\/\/localhost:8889\/wp-json\/wp\/v2\/posts\/1\/revisions"
      }
    ],
    "wp:attachment": [
      {
        "href": "http:\/\/localhost:8889\/wp-json\/wp\/v2\/media?parent=1"
      }
    ],
    "wp:term": [
      {
        "taxonomy": "category",
        "embeddable": true,
        "href": "http:\/\/localhost:8889\/wp-json\/wp\/v2\/categories?post=1"
      },
      {
        "taxonomy": "post_tag",
        "embeddable": true,
        "href": "http:\/\/localhost:8889\/wp-json\/wp\/v2\/tags?post=1"
      }
    ],
    "curies": [
      {
        "name": "wp",
        "href": "https:\/\/api.w.org\/{rel}",
        "templated": true
      }
    ]
  },
  "_embedded": {
    "author": [
      {
        "id": 1,
        "name": "admin",
        "url": "http:\/\/localhost:8889",
        "description": "",
        "link": "http:\/\/localhost:8889\/author\/admin\/",
        "slug": "admin",
        "avatar_urls": {
          "24": "http:\/\/2.gravatar.com\/avatar\/b642b4217b34b1e8d3bd915fc65c4452?s=24&d=mm&r=g",
          "48": "http:\/\/2.gravatar.com\/avatar\/b642b4217b34b1e8d3bd915fc65c4452?s=48&d=mm&r=g",
          "96": "http:\/\/2.gravatar.com\/avatar\/b642b4217b34b1e8d3bd915fc65c4452?s=96&d=mm&r=g"
        },
        "_links": {
          "self": [
            {
              "href": "http:\/\/localhost:8889\/wp-json\/wp\/v2\/users\/1"
            }
          ],
          "collection": [
            {
              "href": "http:\/\/localhost:8889\/wp-json\/wp\/v2\/users"
            }
          ]
        }
      }
    ],
    "replies": [
      [
        {
          "id": 1,
          "parent": 0,
          "author": 0,
          "author_name": "A WordPress Commenter",
          "author_url": "https:\/\/wordpress.org\/",
          "date": "2021-01-17T02:03:45",
          "content": {
            "rendered": "<p>Hi, this is a comment.<br \/>\nTo get started with moderating, editing, and deleting comments, please visit the Comments screen in the dashboard.<br \/>\nCommenter avatars come from <a href=\"https:\/\/gravatar.com\">Gravatar<\/a>.<\/p>\n"
          },
          "link": "http:\/\/localhost:8889\/hello-world\/#comment-1",
          "type": "comment",
          "author_avatar_urls": {
            "24": "http:\/\/1.gravatar.com\/avatar\/d7a973c7dab26985da5f961be7b74480?s=24&d=mm&r=g",
            "48": "http:\/\/1.gravatar.com\/avatar\/d7a973c7dab26985da5f961be7b74480?s=48&d=mm&r=g",
            "96": "http:\/\/1.gravatar.com\/avatar\/d7a973c7dab26985da5f961be7b74480?s=96&d=mm&r=g"
          },
          "_links": {
            "self": [
              {
                "href": "http:\/\/localhost:8889\/wp-json\/wp\/v2\/comments\/1"
              }
            ],
            "collection": [
              {
                "href": "http:\/\/localhost:8889\/wp-json\/wp\/v2\/comments"
              }
            ],
            "up": [
              {
                "embeddable": true,
                "post_type": "post",
                "href": "http:\/\/localhost:8889\/wp-json\/wp\/v2\/posts\/1"
              }
            ]
          }
        }
      ]
    ]
  }
}

To fix this, we'll probably need to manually call WP_REST_Response::remove_link for the links that aren't necessary.

#15 @TimothyBlynJacobs
3 years ago

  • Type changed from enhancement to defect (bug)

Reclassifying this as a bug with the aim of landing the fix this week.

#16 follow-up: @TimothyBlynJacobs
3 years ago

  • Milestone changed from 5.7 to 5.8

Going to go ahead and punt this to 5.8. The changes needed are a bit complex and we're coming up on RC1.

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


3 years ago

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


3 years ago

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


3 years ago

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


3 years ago

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


3 years ago

#22 in reply to: ↑ 16 @chaion07
3 years ago

Replying to TimothyBlynJacobs:
Hi @TimothyBlynJacobs do you have this planned and ready for execution anytime soon for 5.8?

Going to go ahead and punt this to 5.8. The changes needed are a bit complex and we're coming up on RC1.

Thank you in advance!

#23 @desrosj
3 years ago

  • Milestone changed from 5.8 to 5.9

Punting to 5.9 as the 5.8 deadline is today and there has been no progress this cycle.

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


3 years ago

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


3 years ago

This ticket was mentioned in Slack in #core-test by hellofromtonya. View the logs.


3 years ago

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


3 years ago

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


2 years ago

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


2 years ago

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


2 years ago

#31 @hellofromTonya
2 years ago

  • Keywords has-unit-tests added; needs-unit-tests removed
  • Milestone changed from 5.9 to 6.0

With 5.9 Beta 1 tomorrow, the cycle is way past early. Moving this to 6.0.

#32 @costdev
2 years ago

@TimothyBlynJacobs PR 867 was updated with some changes after your feedback. Are you available to do another review on the PR and we'll try to land this one in 6.0?

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


2 years ago

#34 @peterwilsoncc
2 years ago

  • Milestone changed from 6.0 to Future Release

This was discussed during a bug scrub today, due to the lack of activity on the ticket and PR recently it was decided to move this off the 6.0 milestone.

#35 @spacedmonkey
4 months ago

@anouarbenothman @TimothyBlynJacobs Is this ticket still valid. I feel like a lot has changed since this was created.

Note: See TracTickets for help on using tickets.