Make WordPress Core

Opened 9 years ago

Last modified 2 weeks 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:
Priority: normal Milestone: Future Release
Component: REST API Version: 4.9.4
Severity: normal Keywords: good-first-bug has-patch has-unit-tests
Cc: Focuses: rest-api

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 (8)

43502.diff (0 bytes ) - added by manzoorwani.jk 8 years ago.
43502.2.diff (0 bytes ) - added by manzoorwani.jk 8 years ago.
43502.3.diff (21.3 KB ) - added by manzoorwani.jk 8 years ago.
43502.4.diff (1.8 KB ) - added by manzoorwani.jk 8 years ago.
43502.5.diff (646 bytes ) - added by sachinrajcp123 13 months ago.
Fixes issue where setup_postdata() is not followed by wp_reset_postdata() in prepare_item_for_response().
43502.6.diff (3.3 KB ) - added by motylanogha 5 months ago.
Refreshed patch for current trunk. Saves and restores $GLOBALSpost in prepare_item_for_response(), including the HEAD early return path. Includes 2 unit tests. All 283 posts controller tests pass.
ticket-43502.png (7.7 KB ) - added by liaison 5 months ago.
github CI actions 238 check pass.
43502.7.diff (661 bytes ) - added by dhavalkapadane 2 weeks ago.
Refreshed patch for #43502: adds wp_reset_postdata() before the final return in WP_REST_Posts_Controller::prepare_item_for_response(). Covers the standard response path only (not the HEAD-request early return).

Download all attachments as: .zip

Change History (43)

#1 @SergeyBiryukov
9 years ago

  • Keywords needs-patch needs-unit-tests added
  • Milestone Awaiting Review4.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
8 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.


8 years ago

#4 @audrasjb
8 years ago

  • Milestone 4.9.54.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.


8 years ago

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


8 years ago

#7 @desrosj
8 years ago

  • Milestone 4.9.64.9.7

No progress here. Going to punt.

#8 @desrosj
8 years ago

  • Milestone 4.9.74.9.8

Moving all tickets in 4.9.7 to 4.9.8.

#9 @pento
8 years ago

  • Milestone 4.9.8Future Release

#10 @rachelbaker
8 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
8 years ago

Thanks @rachelbaker. Looking into this

@manzoorwani.jk
8 years ago

#12 @manzoorwani.jk
8 years ago

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

#13 @manzoorwani.jk
8 years ago

I hope the last one is OK :)

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


8 years ago

#15 @manzoorwani.jk
7 years ago

Any feedback on this? :)

#16 @antpb
7 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.


7 years ago

#18 @TimothyBlynJacobs
6 years 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.


5 years ago
#19

Call wp_reset_postdata at the end of prepare_item_for_response method

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

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


3 years ago
#20

  • Keywords has-unit-tests added; needs-unit-tests removed

Hi, I am including some tests for the changes from this PR:

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

However I want to mention that the post data is still not being reverted even with the wp_reset_postdata() action added to the methods.

#21 @elenachavdarova
3 years ago

Hi,

In https://github.com/WordPress/wordpress-develop/pull/4591, I have included tests for the changes from this PR: https://github.com/WordPress/wordpress-develop/pull/1165

However I want to mention that the post data is still not being reverted even with the wp_reset_postdata() action added to the methods.

#22 @SirLouen
14 months ago

  • Keywords needs-unit-tests added; has-unit-tests removed

Combined Bug Reproduction and Patch Test Report

Description

✅ This report validates that the indicated patch works as expected.

Patches tested:

Environment

  • WordPress: 6.9-alpha-60093-src
  • PHP: 8.2.28
  • Server: nginx/1.29.0
  • Database: mysqli (Server: 8.4.5 / Client: mysqlnd 8.2.28)
  • Browser: Chrome 137.0.0.0
  • OS: Windows 10/11
  • Theme: Twenty Twenty 2.9
  • MU Plugins: None activated
  • Plugins:
    • Postdata Test 1.0.0
    • Test Reports 1.2.0

Bug Reproduction

  1. Create a post. Take note of the ID for such post
  2. For testing this, we need to add the code in Supp Artifacts somewhere where it could be executed, like a plugin, functions.php, etc… This code takes the code provided in the OP and adds a shortcodes to display the results of using that function during the loop processing.
  3. Modify the code here: $test_post = get_post(1); and instead of 1 use the ID you got in the first step
  4. Create a second post
  5. Add the shortcode [test_postdata]
  6. Check the post results
  7. 🐞 Post ID after API call shows the first one used instead of the current post, as expected. This proves that REST Request is modifying the global $post data.

Expected Results

  • The Post ID after API is the same before and after

Actual Results

  1. ✅ Issue resolved with first patch.
  2. ⚠️ Unit Tests are inadequate

Additional Notes

  • The solution is pretty straightforward, as commented by @TimothyBlynJacobs long time ago. I can't understand why i thas not been merged yet. Maybe the absence of valid Unit Tests has been the problem. I will work on something.
  • I'm trying to understand the Unit Tests provided in PR 4591, but they don't seem to be correct. For the first Unit Test test_postdata_reset_by_prepare_item_for_response, it's using a variable $post_id for the posts endpoint that has not even been declared previously.
  • For this, I'm restoring the needs-unit-tests workflow tag, as the ones provided are not valid.

Supplemental Artifacts

Test Code:

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;
}

function test_template() {
    global $post;
    echo 'Original Post ID: ' . $post->ID . '<br>';
    
    $test_post = get_post(1);
    $response = convert_post_object_to_rest_response($test_post);
    echo 'Returned API ID: ' . $response['id'] . '<br>';

    echo 'Post ID After API Call: ' . $post->ID . '<br>';
    echo 'Post Title: ' . get_the_title() . '<br>';
}

add_shortcode('test_postdata', 'test_template');

#23 @SirLouen
14 months ago

  • Keywords needs-testing removed

@sachinrajcp123
13 months ago

Fixes issue where setup_postdata() is not followed by wp_reset_postdata() in prepare_item_for_response().

#25 @sachinrajcp123
13 months ago

I’ve submitted a patch (43502.5.diff) that adds wp_reset_postdata() after setup_postdata() in the prepare_item_for_response() method of the WP_REST_Posts_Controller class.

This ensures global post data is restored after rendering post data for the REST API. Kindly review the patch.

Last edited 5 months ago by sachinrajcp123 (previous) (diff)

@motylanogha
5 months ago

Refreshed patch for current trunk. Saves and restores $GLOBALSpost in prepare_item_for_response(), including the HEAD early return path. Includes 2 unit tests. All 283 posts controller tests pass.

#26 @motylanogha
5 months ago

  • Keywords has-unit-tests added; needs-unit-tests removed

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


5 months ago
#27

This PR addresses a long-standing issue in WP_REST_Posts_Controller::prepare_item_for_response() where the global $post object was not restored after calling setup_postdata().

When the REST API processes post data, it temporarily modifies the global $post state to ensure core functions (like get_the_content()) work correctly. However, failing to reset this state leads to "global context pollution," especially when the REST API is called internally during a custom WP_Query loop or other custom development contexts.

Changes included in this PR:

Manual State Backup: Saves the original $GLOBALSpost before modification.

Robust Restoration: Ensures the global $post is restored to its original state (including null) in both the main execution path and the early-return path for HEAD requests.

Comprehensive Unit Tests: 1. Verified that the global $post is restored to its previous object after the API call.

  1. Verified that the global $post is restored to null if it wasn't set before the call.

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

Use of AI Tools
AI assistance: Yes
Tool(s): Gemini
Model(s): Gemini 3 Flash
Used for: Identifying the core architectural issue (global state pollution vs. wp_reset_postdata limitations) and drafting the technical explanation for the PR description. The final implementation was verified against the latest Trac patch (#43502.6.diff) and local PHPUnit tests.

@liaison
5 months ago

github CI actions 238 check pass.

#28 @liaison
5 months ago

I've refreshed the patch and opened PR #11455.

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

Tests conducted:

  • Verified all 238 CI checks passed (PHP 7.2 - 8.4).
  • Included 2 new unit tests specifically for global $post restoration (one for existing post, one for null).
  • Confirmed fix for 'pollution' during internal REST API calls.

github CI actions 238 check pass.

Last edited 5 months ago by liaison (previous) (diff)

@liaison commented on PR #11455:


5 months ago
#29

@apermo, I've fixed the indentation. Thanks a lot.

@liaison commented on PR #11455:


5 months ago
#30

@apermo, I've fixed the indentation. Thanks a lot.

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


4 months ago
#31

prepare_item_for_response() overwrote $GLOBALS['post'] and called setup_postdata() without restoring the previous state. Anything running after a REST request that re-used the controller (block rendering, the_content filters, etc.) saw the wrong global post.

This PR captures the previous global before mutation and restores it on every return path, including the early HEAD-request return.

Tests cover:

  • Restoration when a global post is already set
  • Restoration to null when no global post was set
  • Restoration on the HEAD-method early-return branch

Complements GH-1165 and GH-11455.

#32 @westonruter
5 weeks ago

In 62952:

REST API: Restore the global post after preparing a revision.

WP_REST_Revisions_Controller::prepare_item_for_response() set the global $post to the revision being prepared and called setup_postdata(), but never restored the previous value, so the change persisted for the remainder of the request. WP_REST_Autosaves_Controller delegates to this method and the block editor preloads the autosaves endpoint on every load, so a post with a pending autosave could leave the global $post pointing at the autosave revision while edit-form-blocks.php built the editor bootstrap, initializing the editor with the wrong post and rewriting the URL to it.

The previous global $post is now captured before setup_postdata() and restored on every return path, including the early return for HEAD requests.

Developed in https://github.com/WordPress/wordpress-develop/pull/12248.
Follow-up to r40601, r59899.

Props micahele, gusgomezpg, westonruter, wildworks, dhrupo.
See #40626, #43502.
Fixes #65495.

@motylanogha commented on PR #11695:


5 weeks ago
#33

[62952] just fixed the identical setup_postdata() / unrestored global $post bug in WP_REST_Revisions_Controller (committed under #65495, with a "See #43502" reference).

This PR applies the same fix to the sibling case: WP_REST_Posts_Controller::prepare_item_for_response() still does $GLOBALS['post'] = $post; setup_postdata( $post ); (around line 1854) without restoring the previous $post on any return path, including the early HEAD return. Same fix shape as [62952], different controller — a natural follow-up while this area is being tidied up.

Rebased status: MERGEABLE, full CI green, and has unit tests covering the restore on every return path. CC @westonruter since you just handled the revisions controller.

@dhavalkapadane
2 weeks ago

Refreshed patch for #43502: adds wp_reset_postdata() before the final return in WP_REST_Posts_Controller::prepare_item_for_response(). Covers the standard response path only (not the HEAD-request early return).

#34 @dhavalkapadane
2 weeks ago

Tested this using a reproduction script that sets up postdata for one post via setup_postdata(), then makes an internal REST API request (rest_do_request()) for a *different* post mid-loop, then checks global $post afterward.

Without any fix:

BEFORE REST call: global $post ID = 9
AFTER REST call:  global $post ID = 6  (leaked to the REST-requested post)
Expected:         global $post ID = 9

This confirms the bug: the REST response process leaves global $post pointing at the REST-requested post instead of restoring the original loop's post.

Note on the attached patch: its context no longer applies cleanly to current trunk. prepare_item_for_response() in class-wp-rest-posts-controller.php now calls setup_postdata( $post ) unconditionally near the top of the method (not conditionally inside a post_type_supports( ..., 'revisions' ) check as the original patch assumes), so it doesn't apply as-is and needs to be refreshed.

Attached 43502.7.diff, which adds wp_reset_postdata() before the final return apply_filters(...) statement in prepare_item_for_response(). With this in place, the bug is fixed:

BEFORE REST call: global $post ID = 9
AFTER REST call:  global $post ID = 9
Expected:         global $post ID = 9

Note: this patch covers the standard response path. There's also an early return for HEAD requests (if ( $request->is_method( 'HEAD' ) )) earlier in the method that isn't addressed here and may need the same treatment for full coverage.

Environment: WordPress trunk (wordpress-develop local env), PHP 8.3.33, Windows.

#35 @motylanogha
2 weeks ago

Thanks for the runnable reproduction, that is the part these old tickets usually lack.

One correction on the direction of 43502.7.diff, and it is also the reason your script cannot see the problem: wp_reset_postdata() on its own does not restore the caller's state, it restores the *main* query's state.

function wp_reset_postdata() {
        global $wp_query;

        if ( isset( $wp_query ) ) {
                $wp_query->reset_postdata();
        }
}

public function reset_postdata() {
        if ( ! empty( $this->post ) ) {
                $GLOBALS['post'] = $this->post;
                $this->setup_postdata( $this->post );
        }
}

Two consequences follow.

1. A secondary loop gets a different wrong post back, not the right one.

The shape in the original report is a template looping something other than the main query:

$q = new WP_Query( array( 'post_type' => 'book' ) );

while ( $q->have_posts() ) {
        $q->the_post();          // sets $GLOBALS['post'] from $q
        rest_do_request( ... );  // the leak happens here
        // wp_reset_postdata() inside the controller restores $wp_query->post,
        // which is the MAIN query's post, not $q's.
}

WP_Query::the_post() sets the global from $this, while wp_reset_postdata() reads $GLOBALS['wp_query']. In a secondary loop those are different objects, so the patch swaps one wrong post for another. Your script restores correctly only because the loop it sets up is the main query, so the two coincide there.

2. It cannot restore "there was no post".

reset_postdata() is a no-op when $wp_query->post is empty, and it has no way to express unsetting the global. If $GLOBALS['post'] was not set before the call, the patch either leaves the leaked value in place or invents a main-query post.

What holds in both cases is capturing the previous value and putting it back:

$previous_post   = isset( $GLOBALS['post'] ) ? $GLOBALS['post'] : null;
$GLOBALS['post'] = $post;

setup_postdata( $post );

// ...

$GLOBALS['post'] = $previous_post;
wp_reset_postdata();

return apply_filters( "rest_prepare_{$this->post_type}", $response, $post, $request );

You are right that the HEAD early return needs the same treatment, and it is easy to miss because it returns before the main exit point.

Both points are already covered in https://github.com/WordPress/wordpress-develop/pull/11695, which takes the save-and-restore approach above, handles the HEAD path, and adds three regression tests: the normal path, the HEAD path, and the case where the global was unset beforehand. It applies to current trunk and CI is green on it.

There is committed precedent for the shape as well. [62952] fixed the identical unrestored $post global in WP_REST_Revisions_Controller under #65495, carrying a "See #43502" reference. This ticket is the sibling case in WP_REST_Posts_Controller, so landing it the same way keeps the two controllers consistent.

Note: See TracTickets for help on using tickets.