WordPress.org

Make WordPress Core

Opened 4 years ago

Last modified 7 months ago

#43502 new defect (bug)

`WP_REST_Posts_Controller::prepare_item_for_response()` doesn't reset postdata after calling setup_postdata()

Reported by: gerbenvandijk Owned by:
Milestone: Future Release Priority: normal
Severity: normal Version: 4.9.4
Component: REST API Keywords: needs-unit-tests good-first-bug has-patch needs-testing
Focuses: rest-api Cc:

Description

Hi all,

I seem to have stumbled upon a small oversight in WP_REST_Posts_Controller::prepare_item_for_response() , where it doesn't reset postdata after calling setup_postdata(), rendering this part of the API useless for developers who want to use this API in their custom code.

Ran into it while writing a custom function, that uses this part of the API to convert a regular post object into an object that matches the json scheme of the API.

I’ve fixed it temporarily by doing setting $GLOBALS[ 'post' ] = $original_post_id in my function (and by passing original_post_id as parameter to my function) - but it seems to me that this should be fixed in core.

function convert_post_object_to_rest_response($post)
{

    global $wp_rest_server;
    $post_type = get_post_type_object($post->post_type);
    $request = WP_REST_Request::from_url(rest_url(sprintf('wp/v2/%s/%d', $post_type->rest_base, $post->ID)));
    $request = rest_do_request($request);
    $data = $wp_rest_server->response_to_data($request, isset($_GET[ '_embed' ]));
    return $data;

}

It returns the right data, which is great - but when i in turn use this function to supply a field in one of my endpoints with this dat, all fields below it seem to inherit the ID i’ve passed.

i’ve traced this back to WP_REST_Posts_Controller::prepare_item_for_response, where it’s doing a setup_postdata call without resetting it later.

For now i’ve fixed it by setting $GLOBALS[ 'post' ] = $original_post_id in my function (and by passing original_post_id as parameter to my function), but I figured i'd report it here so it can be fixed from the core.

Attachments (4)

43502.diff (0 bytes) - added by manzoorwani.jk 3 years ago.
43502.2.diff (0 bytes) - added by manzoorwani.jk 3 years ago.
43502.3.diff (21.3 KB) - added by manzoorwani.jk 3 years ago.
43502.4.diff (1.8 KB) - added by manzoorwani.jk 3 years ago.

Download all attachments as: .zip

Change History (23)

#1 @SergeyBiryukov
4 years ago

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

Hi @gerbenvandijk, welcome to WordPress Trac! Thanks for the report.

WP_REST_Revisions_Controller::prepare_item_for_response() has the same issue.

#2 @soulseekah
4 years ago

The prepare_item_for_response method and the controller itself is not supposed to be called from within a loop, which is what you seem to be doing. You appear to be using a whole REST server for mere data transformation, overkill and unintended use ahead :)

However, it indeed does set the $post global and sets the global post data up. Should it? Probably not. Does it - yes. Why? get_the_content(), which doesn't accept an explicit post_ID argument, meaning that it will only work with a global context setup.

Should it play nice and reset the $post global to what it was? Maybe.

Should it also call wp_reset_postdata? Certainly not. For one it has no effect in the REST server context. For two, if it's called from your scenario it would actually mess up your loop, resetting too early (wp_reset_postdata should only be called after the loop has ended, right?)

In my opinion, this should be left alone. The context the REST server is being called from is highly atypical. The server is expected to live a very short life serving one response per request.

Thoughts?

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


4 years ago

#4 @audrasjb
4 years ago

  • Milestone changed from 4.9.5 to 4.9.6

Bumping to 4.9.6 due to 4.9.5 beta release.

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


4 years ago

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


3 years ago

#7 @desrosj
3 years ago

  • Milestone changed from 4.9.6 to 4.9.7

No progress here. Going to punt.

#8 @desrosj
3 years ago

  • Milestone changed from 4.9.7 to 4.9.8

Moving all tickets in 4.9.7 to 4.9.8.

#9 @pento
3 years ago

  • Milestone changed from 4.9.8 to Future Release

#10 @rachelbaker
3 years ago

  • Keywords good-first-bug added

We did used to set the $post global back to the previous post in v1, but I missed this when adding the setup_postdata() function back in v2. Setting the global was needed for filters like the_content however I should have been kind enough to restore the global when done. Be kind, rewind!

This is worth a patch and a unit test.

Some links for historical reference:

WP REST API v1 where the $post global was restored: https://github.com/WP-API/WP-API/blob/master/lib/class-wp-json-posts.php#L687

Bug Introduced in this PR: https://github.com/WP-API/WP-API/pull/753

Responding to this issue: https://github.com/WP-API/WP-API/issues/738

#11 @kakomap
3 years ago

Thanks @rachelbaker. Looking into this

@manzoorwani.jk
3 years ago

#12 @manzoorwani.jk
3 years ago

Hello guys,
Sorry for the mess up. Doing it for the first time :)
Please bear with me...

#13 @manzoorwani.jk
3 years ago

I hope the last one is OK :)

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


3 years ago

#15 @manzoorwani.jk
3 years ago

Any feedback on this? :)

#16 @antpb
2 years ago

  • Keywords has-patch needs-testing added; needs-patch removed

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


2 years ago

#18 @TimothyBlynJacobs
12 months ago

Hi @manzoorwanijk, sorry for such the long delay in getting back to you.

This should be able to be simplified by calling wp_reset_postdata at the end of prepare_item_for_response since the main query will take care of backing up the post global.

This will also need unit tests.

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


7 months ago

Call wp_reset_postdata at the end of prepare_item_for_response method

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

Note: See TracTickets for help on using tickets.