WordPress.org

Make WordPress Core

Opened 21 months ago

Last modified 3 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:
PR Number:

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 14 months ago.
43502.2.diff (0 bytes) - added by manzoorwani.jk 14 months ago.
43502.3.diff (21.3 KB) - added by manzoorwani.jk 14 months ago.
43502.4.diff (1.8 KB) - added by manzoorwani.jk 14 months ago.

Download all attachments as: .zip

Change History (21)

#1 @SergeyBiryukov
21 months 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
21 months 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.


21 months ago

#4 @audrasjb
21 months 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.


20 months ago

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


20 months ago

#7 @desrosj
20 months ago

  • Milestone changed from 4.9.6 to 4.9.7

No progress here. Going to punt.

#8 @desrosj
19 months ago

  • Milestone changed from 4.9.7 to 4.9.8

Moving all tickets in 4.9.7 to 4.9.8.

#9 @pento
17 months ago

  • Milestone changed from 4.9.8 to Future Release

#10 @rachelbaker
16 months 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
14 months ago

Thanks @rachelbaker. Looking into this

#12 @manzoorwani.jk
14 months ago

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

#13 @manzoorwani.jk
14 months ago

I hope the last one is OK :)

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


14 months ago

#15 @manzoorwani.jk
9 months ago

Any feedback on this? :)

#16 @antpb
6 months ago

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

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


3 months ago

Note: See TracTickets for help on using tickets.