Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix Ticket - 57902 - REST API index does not respect fields #4483

Closed

Conversation

nirav7707
Copy link

@nirav7707 nirav7707 commented May 22, 2023

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 & links 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 & links both available
Case 5
  • URL - /wp-json
  • Entire response json object including _links

Copy link

@dlh01 dlh01 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi! Thanks for submitting the PR. I've left some notes about where I think this approach can be improved.


if ( ! empty( $links ) ) {
if ( ! empty( $_GET['_fields'] ) ) {
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

$_GET is not going to be reliable for checking _fields because REST API requests can be dispatched within PHP, not just with the browser.

Also, I think relying on $_GET here would mean that all responses from the server would use the same set of fields, ignoring the _fields passed to individual requests, which I don't think is the desired outcome.

I think we'll want to use the fields that are in the request object, not globals.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you, @dlh01 , for your feedback. I have made the necessary updates by replacing the use of $_GET with the $request object. Additionally, I have addressed the issue where the old logic failed to include the _links when the ?_fields query variable was not passed. The updated code resolves both of these issues. Please review it on your end and let me know if any further changes are required.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the updates! I'll try to review the PR again this weekend.

to validate the _links is requested or not in _fields query parameter, request
object is pass to function response_to_data as function parameter and assign
null as default value to the third parameter for the compatibility
@nirav7707 nirav7707 marked this pull request as ready for review May 24, 2023 04:04
@nirav7707 nirav7707 requested a review from dlh01 May 24, 2023 07:25
* @return array {
* Data with sub-requests embedded.
*
* @type array $_links Links.
* @type array $_embedded Embedded objects.
* }
*/
public function response_to_data( $response, $embed ) {
public function response_to_data( $response, $embed, $request = null ) {
$data = $response->get_data();
$links = self::get_compact_response_links( $response );
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is it better to check that links are needed in the first place before getting them?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agreed, it would be more efficient to skip getting the links at all if we know they are not requested. That would avoid wasted effort generating a links object we don't go on to use.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Right, it's respecting the _fields in function rest_filter_response_fields but

$links = self::get_compact_response_links( $response );
if ( ! empty( $links ) ) {
	$data['_links'] = $links;
}

This code will add the _links anyway, it's not validating the _fields. so I decide to add the $request object as parameter.

@spacedmonkey
Copy link
Member

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
5 participants