Make WordPress Core

Opened 13 months ago

Closed 7 months ago

Last modified 5 months ago

#57902 closed defect (bug) (fixed)

REST API index does not respect fields

Reported by: spacedmonkey's profile spacedmonkey Owned by: spacedmonkey's profile spacedmonkey
Milestone: 6.4 Priority: normal
Severity: normal Version: 6.1
Component: REST API Keywords: has-patch has-unit-tests needs-dev-note
Focuses: rest-api, performance Cc:

Description

Follow on from [53760].

The REST API index, that can be found at /wp-json, does not respect fields param.
Take this API request for example.

/wp-json?_fields=name

Links are still prepared and returned.

{
  "name": "Test site",
  "_links": {
    "help": [
      {
        "href": "https://developer.wordpress.org/rest-api/"
      }
    ]
  }
}

Change History (30)

#1 @spacedmonkey
13 months ago

Pinging @dlh who worked on the original patch in [53760].

#2 @dlh
13 months ago

  • Keywords needs-patch added
  • Milestone changed from Awaiting Review to 6.3
  • Owner set to dlh
  • Status changed from new to assigned

Thanks for the ping! I agree, this seems like an oversight in [53760]. I'm going to assign it to myself to try to address in 6.3.

This ticket was mentioned in PR #4483 on WordPress/wordpress-develop by nirav7707.


10 months ago
#3

  • Keywords has-patch added; needs-patch removed

Trac ticket: https://core.trac.wordpress.org/ticket/57902

## Description
This PR introduces validation for the _links field in the _fields query parameter. If the _links field is present, it will be appended to the response object. Otherwise, it will not be included.

#### Test Case

###### Case 1

  • URL - /wp-json?_fields[]=name&_fields[]=_links
  • Name & link both available

###### Case 2

  • URL - /wp-json?_fields[]=name
  • Name available

###### Case 3

  • URL - /wp-json?_fields=name
  • Name available

###### Case 4

  • URL - /wp-json?_fields=name,_links
  • Name & link both available

#4 @niravsherasiya7707
10 months ago

Hello @dlh , I have verified and applied the patch. Kindly review it on your end and let me know if any further changes are required. Thank you.

#5 @spacedmonkey
10 months ago

@dlh Where are we with this ticket? Do you think we can get this in 6.3?

#6 @dlh
9 months ago

@spacedmonkey I've reviewed the latest PR, and it still doesn't seem like the right direction to me, but I haven't taken a closer look yet. I'm hoping to do that after wrapping up #41692. Since this is a bugfix, do we have a bit more time before it has to be punted?

#7 @oglekler
9 months ago

I have doubts that it can land into 6.3. From my point of view, we need to cover this with a unit test, and also I left a comment for PR.

@spacedmonkey please consider what needs, possible and realistic this moment.

This ticket was mentioned in PR #4784 on WordPress/wordpress-develop by @spacedmonkey.


9 months ago
#8

  • Keywords has-unit-tests added

#9 @spacedmonkey
9 months ago

@oglekler @dlh I put together a quick PR for this. Took a little longer than I thought.

https://github.com/WordPress/wordpress-develop/pull/4784

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


9 months ago

@spacedmonkey commented on PR #4784:


9 months ago
#11

CC @dlh01

kadamwhite commented on PR #4784:


9 months ago
#12

Tests fail.

I'm interested in the fact that this patch seeks to prevent the links being added (which is convenient, since we already have access to the current $request in this code), whereas https://github.com/WordPress/wordpress-develop/pull/4483 comes at it by trying to avoid generating the links. Do we need some code on both sides?

If we can get the tests passing on this patch, I'm 👍 on at minimum not _adding_ them when we can avoid it, regardless of whether we also pursue something more like the other patch later on.

@spacedmonkey commented on PR #4784:


9 months ago
#13

@kadamwhite Sorry for pushing up unit test fails. Should be fixed now.

The idea of this ticket is two fold.

  1. Respect _fields=links and not return that in the response.
  2. No prepare the links in the first place, saving database / cache lookups.

See https://github.com/WordPress/wordpress-develop/commit/b7bae6936a9ad54f85bad7e5a73a9d110190d927 for referennce, where we did this for other endpoints.

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


9 months ago

#15 @mukesh27
9 months ago

  • Keywords changes-requested added

Thanks @spacedmonkey for PR, it looks good at first glance, but upon further review, there are some changes and feedback that need to be addressed.

#16 @spacedmonkey
9 months ago

  • Owner changed from dlh to kadamwhite
  • Status changed from assigned to reviewing

Assigning to @kadamwhite for final review.

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


9 months ago

#18 @spacedmonkey
9 months ago

  • Keywords changes-requested removed

@swissspidy commented on PR #4784:


9 months ago
#19

Isn't this a breaking change? If someone used ?_fields=xyz before, they got the links for free. Now they have to use ?_fields=xyz,links to get the same behavior.

This was already a problem in 6.1 when this was added to all the other REST controllers.

I'd be wary of making such a breaking change that late in the cycle.

@spacedmonkey commented on PR #4784:


9 months ago
#20

Isn't this a breaking change? If someone used ?_fields=xyz before, they got the links for free. Now they have to use ?_fields=xyz,links to get the same behavior.

This was already a problem in 6.1 when this was added to all the other REST controllers.

I'd be wary of making such a breaking change that late in the cycle.

@swissspidy
The fact that links are returned even when they are not requested, is a bug. You do not request this data and is generated and returned anyway. If you don't do anything, just hit the index, you still will get links. The only change is behaviour here, is if you request /wp-json?_fields=name, links will not be returned. By why would expect the links to be there, you didn't request them.

@swissspidy commented on PR #4784:


9 months ago
#21

The only change is behaviour here, is if you request /wp-json?_fields=name, links will not be returned. By why would expect the links to be there, you didn't request them.

Because that was the existing behavior ever since WordPress 4.7.0. It just worked. This bug fix will break this behavior, simple as that. Even if it's not documented and not intended.
If you remember, we had the exact same problem with 6.1.0 when this change broke REST API usage in the Web Stories plugin. But that ship has sailed now.

Don't get me wrong, I don't have a problem with making this change, as it was already changed for all the other controllers and the index controller isn't used as much. I just like to point out that now is very late to do this change. I suggest doing this early in the 6.4.0 cycle instead.

#22 @spacedmonkey
9 months ago

  • Keywords early added
  • Milestone changed from 6.3 to 6.4

After a slack conversion with @swissspidy @TimothyBlynJacobs, it seems like this is too late to commit in WP 6.3. I am punting to WP 6.4 and adding early.

#23 @hellofromTonya
7 months ago

I'm doing an audit of all tickets marked 6.4 early. What is early The handbook explains the keyword as:

This keyword should only be applied by committers. The keyword signals that the ticket is a priority, and should be handled early in the next release cycle.

early means the commits need to happen during the early part of the Alpha cycle; else, the ticket gets bumped.

A change here could have impacts. A longer period of time is needed for extenders awareness and to test and give feedback. The longer period helps to raise confidence while giving time for further adjustments if additional changes are needed.

Thus, yes, I do think this ticket is an early candidate.

#24 @spacedmonkey
7 months ago

Now we are in 6.4, can we look to review and commit this ? CC @swissspidy @TimothyBlynJacobs

#25 @spacedmonkey
7 months ago

  • Focuses performance added
  • Keywords commit added
  • Owner changed from kadamwhite to spacedmonkey

#26 @spacedmonkey
7 months ago

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

In 56566:

REST API: Avoid unnecessarily preparing item links REST API index.

Building upon the changes introduced in [53760], this commit refines the behavior of the REST API index. Specifically, it addresses performance concerns related to the unnecessary preparation of item links, such as site icon and logo links.

Prior to this update, the index controller was invoking the prepare_links method regardless of whether the _links or _embedded fields were requested in the response. This led to unnecessary database lookups and decreased overall performance.

In this commit, we implement a more efficient approach. Now, the prepare_links method will only be called when the _links or _embedded fields are explicitly requested in the response. This optimization ensures that we prepare links only when they are intended for inclusion in the API response, reducing unnecessary overhead.

By implementing this improvement, we enhance the overall efficiency and performance of the WordPress core REST API index controller.

Props spacedmonkey, niravsherasiya7707, dlh, mukesh27, costdev, swissspidy.
Fixes #57902.

#29 @spacedmonkey
7 months ago

  • Keywords needs-dev-note added; early commit removed

This change should be called out in dev notes / field guide.

This ticket was mentioned in Slack in #core-performance by 611shabnam. View the logs.


5 months ago

Note: See TracTickets for help on using tickets.