Make WordPress Core

Opened 13 years ago

Last modified 4 weeks ago

#18408 assigned defect (bug)

Can't wp_reset_postdata after custom WP_Query in an admin edit page

Reported by: ericlewis's profile ericlewis Owned by: audrasjb's profile audrasjb
Milestone: Future Release Priority: normal
Severity: normal Version: 4.7.2
Component: Query Keywords: needs-testing has-patch early needs-unit-tests
Focuses: Cc:

Description

While on an edit post page (or any post type), if you create a custom WP_Query object before the editor has been output, the post data from the custom WP_Query will fill out the edit post form, not the original content of the post that is actually trying to be edited.

I found this out when trying to create a custom metabox in the 'side' context. You can reproduce this by inserting this code into a plugin:

add_action( 'add_meta_boxes', 'myplugin_add_custom_box' );

function myplugin_add_custom_box() {
	add_meta_box( 
        'myplugin_sectionid',
        __( 'My Post Section Title', 'myplugin_textdomain' ),
        'myplugin_inner_custom_box',
        'post',
        'side'
    );
}

function myplugin_inner_custom_box() {
	global $post;
	$a = new WP_Query('post_type=page');
	while($a->have_posts() ) : $a->the_post();
	endwhile;
	wp_reset_postdata();
}

This happens because $wp_query->post is never defined in the admin load, which wp_reset_postdata relies on to reset the original post data.

I am attaching a patch that defines $wp_query->post after the $post global has been defined.

Attachments (6)

post.php (60.2 KB) - added by ericlewis 13 years ago.
wp-admin/includes/post.php
18408.patch (1.3 KB) - added by ericlewis 13 years ago.
.patch file version
18408.php (440 bytes) - added by ericlewis 9 years ago.
18408-wp-query-in-admin.php (585 bytes) - added by boonebgorges 9 years ago.
18408.5.diff (1.2 KB) - added by Howdy_McGee 13 months ago.
18408.6.diff (772 bytes) - added by Howdy_McGee 4 months ago.
A more direct, hookless reliant patch.

Download all attachments as: .zip

Change History (57)

@ericlewis
13 years ago

wp-admin/includes/post.php

@ericlewis
13 years ago

.patch file version

#1 @ericlewis
13 years ago

  • Keywords needs-testing dev-feedback added

#2 @ericlewis
13 years ago

  • Resolution set to invalid
  • Status changed from new to closed

#3 @SergeyBiryukov
13 years ago

  • Milestone Awaiting Review deleted

#4 @billerickson
12 years ago

Why was this set to invalid? I'm having the same issue.

My solution has been to save the original post in $original_post, then at the bottom of my metabox put setup_postdata( $original_post );

While this works, it would be much better to be able to use wp_reset_query or wp_reset_postdata.

If this ticket is invalid, what's the recommended approach for restoring the query in a metabox?

#5 @ericlewis
12 years ago

I set it to invalid because the proper thing to do in this situation is to use get_posts() anywhere in the back-end, and not use any template tags.

#6 @billerickson
12 years ago

Regardless of what method you use to generate the loop, if you set the post data to something else you'll need to reset it. Using get_posts:

$original_post = $post;
$myposts = get_posts( $args );
foreach( $myposts as $post ) :	setup_postdata($post);
...
endforeach: 
setup_postdata( $original_post);

Using WP_Query (how I'm doing it):

$original_post = $post;
$loop = new WP_Query( $args );
while( $loop->have_posts() ): $loop->the_post();
...
endwhile;
setup_postdata( $original_post );

I think it's reasonable to assume that most of the time you're doing a query in a metabox, you'll need access to the post data.

#7 @ericlewis
12 years ago

Bill - you can access all post data via the $myposts object in the foreach loop without using the setup_postdata() as class variables of each $post, I believe setup_postdata just makes it more simple to access via template tags.

I do agree with you that it would be nice to use template tags in meta boxes, just to keep things cleaner and one less thing to remember.

#8 @billerickson
12 years ago

Ah I see, I never use get_posts so was basing that simply on the examples of the codex page.

Nonetheless, I still think it's a valid complaint.

#9 @misternifty
10 years ago

  • Component changed from Editor to Options, Meta APIs
  • Resolution invalid deleted
  • Status changed from closed to reopened
  • Version changed from 3.2.1 to 3.8.1

This is not invalid. I was able to replicate this in 3.8.1. While using an explicit WP_Query object in a meta box, you cannot reset the postdata after the loop, no matter what you try. Any subsequent meta boxes identify global $post as the last post of the custom query.

@ericlewis assertion that WP_Query and template tags should not be used in wp-admin is not documented anywhere in the Codex that I can find nor in any posts by core contributors. Multiple loops using the an explicit creation of a WP_Query object in wp-admin post screens should work just like any other query method or function.

This is causing issues for other plugins that register meta boxes and generating support tickets like the following: http://wordpress.org/support/topic/plugin-wordpress-seo-by-yoast-after-update-it-wont-save-seo?replies=32
I identified the noted support example here after trying to save post meta for WP SEO but the meta that surfaced after update was based off the post ID of the last post in my custom query in the previous meta box. After a look under the hood, the meta was actually saved properly to the origin post ID as it was a form option in the DOM already.

Using get_posts instead of new WP_Query works, but that's just applying a bandaid fix when this should work out-of-the-box. It's my opinion that this needs to be fixed rather than say, 'use another query method'. Plugin authors will continue to use WP_Query in meta boxes and it will continue to break the main query for the subsequent page load.

#10 @nacin
10 years ago

  • Milestone set to Awaiting Review

#11 @boonebgorges
9 years ago

  • Component changed from Options, Meta APIs to Query

#12 @boonebgorges
9 years ago

Is this still an issue? I can't figure out how to reproduce it. When registering a metabox containing a WP_Query using the code provided by ericlewis, the edit post form on post.php is filling in the data from the expected post. Maybe I'm doing it wrong?

@ericlewis
9 years ago

#13 @ericlewis
9 years ago

In 2011, the right sidebar was output in markup before the main title/content. This has been switched sometime in the last three years (maybe during mp6?), so the bug doesn't express itself so clearly now.

See attachment:18408.php, which clearly shows the_title() after the wp_reset_postdata() is using data from the custom WP_Query, not the original post to be edited.

#14 @boonebgorges
9 years ago

  • Keywords 2nd-opinion added; dev-feedback removed

Thanks, ericlewis. Now I see what's going on.

The root issue is that wp_reset_postdata() is designed to reset globals to match the main query. But in the case of post.php, there is no main query - the $post global is populated manually, using get_post(), rather than through the normal $wp_query loop. So perhaps the most straightforward way to resolve this is by doing a "real" $wp_query loop on edit.php. See 18408-wp-query-in-admin.php. This is similar to your original suggestion to modify get_default_post_to_edit(), but far more targeted.

The good thing about this solution is that WP_Query instances on edit.php now work exactly like WP_Query instances anywhere else. The bad thing is that it's kinda arbitrary. On the front end, the WP bootstrap process (see especially WP::main()) fires up the main WP query based on the query vars parsed out of the URL. If we fake it on post.php as I've suggested in the patch, we're reproducing only part of this process, which could lead to other bits of confusion down the line. (For example, if you see a $wp_query global, do you also expect to find a $wp->matched_rule?)

It's possible that there would be odd side effects of the suggested $wp_query override - in particular, I guess, if a metabox is manipulating the global in some funny way. That being said, I'm having a hard time thinking of any realistic examples of possible breakage.

What do others think?

#15 @brettshumaker
9 years ago

I just ran into this inconsistency for a plugin I have that's doing some things in the admin bar. Originally it was only pulling a list of pages and adding their edit links to an admin bar item, but I had a feature request for the ability to add other post types to this item. I was doing so with WP_Query but found that it was breaking the edit screen in that the editable content was being pulled from the last post in my list of items in the admin bar. I thought that wp_reset_postdata() wasn't functioning properly at first but then realized what was going on.

I agree with @boonebgorges and @misternifty that this does need to be fixed so that the main query behavior is consistent across the admin area. It doesn't make sense for WP_Query to work on some but not all pages.

#16 @dezio1900
9 years ago

I experienced the same bug when I tried to change the Toolbar items inside the "wp_before_admin_bar_render" action hook. I wanted to add one custom type posts to the toolbar, and of course I used WP_Query, but that caused this bug to occur in posts edit screen.

I ended up using "get_posts" instead of "WP_Query" and that solved the problem. BUT this bug is not acceptable, and it should be fixed!

#17 follow-up: @boonebgorges
9 years ago

  • Milestone changed from Awaiting Review to Future Release

#18 in reply to: ↑ 17 @archon810
9 years ago

Replying to boonebgorges:

So FutureRelease - does it mean the fix is in place, waiting for the next release? It's a 4-year-old bug, and we just wasted several hours trying to figure out what's going on before finding this ticket.

#19 @barryceelen
9 years ago

This behaviour caused me some head scratching as well. Added a note to http://codex.wordpress.org/Class_Reference/WP_Query.

#20 @wonderboymusic
9 years ago

  • Keywords close added; 2nd-opinion removed

I can't begin to imagine what would break if we did:

	global $wp_query;

	if ( isset( $wp_query ) ) {
		$wp_query->reset_postdata();
	} else {
              // CODE that is certain to break untold # of plugins and themes
        }

Globals are super-fragile. I don't think we should start actively supporting theme functions and nested loops with proactive reset on this screen. You're better off toggling the global $post in your own code here.

#21 @wonderboymusic
8 years ago

  • Keywords ongoing added; close removed

This could be part of a larger discussion. I also want to immortalize EAL's first bug report.

#22 follow-up: @hungpham
8 years ago

  • Keywords needs-patch added; has-patch removed
  • Version 3.8.1 deleted

I am just facing to this trouble in version 4.4.1, and wondering how it is going? Had to change the $context in my meta_box to "normal" or keep default setting "advanced".

#23 in reply to: ↑ 22 @Mekku
8 years ago

Replying to hungpham:

I am just facing to this trouble in version 4.4.1, and wondering how it is going? Had to change the $context in my meta_box to "normal" or keep default setting "advanced".

My temporary solution is to add the following code in the same function/method that registers the metaboxes:

// the fix
global $post, $wp_query;
$wp_query->post = $post;

// add metaboxes
add_meta_box(...)
add_meta_box(...)

#24 in reply to: ↑ description ; follow-up: @quin452
7 years ago

  • Version set to 4.7.2

Any idea about this? I've encountered something similar.

I'm trying to populate a metabox dropdown with the titles of a custom post type.
The post which will hold this data is also a custom post type.

It works fine not using wp_query and hard coding the options, but when I use wp_query, there are issues in saving. It populates the dropdown, but the permalink is changed and the dropdown is reset/cannot be saved. It also affects the title sometimes.

#25 in reply to: ↑ 24 ; follow-up: @jpSimkins
7 years ago

To bypass this issue, do not use the_post as this is the issue.

I use this syntax and I have numerous metaboxes with no issue to the default post object nor the need to call wp_reset_postdata();.

<?php
$query = new \WP_Query($args);
if ($query->have_posts()) {
    foreach ($query->get_posts() as $p) {
        // nomal loop logic using $p as a normal WP_Post object
    }
}

It needs to be noted that you cannot use the loop methods like get_the_title() but you can easily call $p->post_title. I do not believe the filters are applied to these so you may need to use a different object for the backend than the frontend since the issue is only on the backend of WP.

Also, you could just use the same args with get_posts();

Last edited 7 years ago by jpSimkins (previous) (diff)

#26 in reply to: ↑ 25 ; follow-up: @nitesh.luharuka
7 years ago

This works perfectly. The problem persists with 4.7.4 as well.

Replying to jpSimkins:

To bypass this issue, do not use the_post as this is the issue.

I use this syntax and I have numerous metaboxes with no issue to the default post object nor the need to call wp_reset_postdata();.

<?php
$query = new \WP_Query($args);
if ($query->have_posts()) {
    foreach ($query->get_posts() as $p) {
        // nomal loop logic using $p as a normal WP_Post object
    }
}

It needs to be noted that you cannot use the loop methods like get_the_title() but you can easily call $p->post_title. I do not believe the filters are applied to these so you may need to use a different object for the backend than the frontend since the issue is only on the backend of WP.

Also, you could just use the same args with get_posts();

#27 in reply to: ↑ 26 @lagdonkey
7 years ago

Can confirm that in Wp 4.8.1, this issue is still present. What caused me to notice, was the fact that on save, the currently edited post/page's slug was changing to match the slug of the last post in the Wp_query I was running in the post editor. My meta box was hooked into low priority in the sidebar section of the editor.

What's particularly strange is, if you do something like

<?php
get_the_title($p->Id);
?>

Then you get the desired/expected result.

This solution saved my bacon, but the issue itself cost me about an hour of debugging, trying to find the culprit. Issue occurred on my local Apache hosted dev environment with no plugins and only a custom theme I was working on. If it at all helps, I've also noticed issues in other areas of wp's backend when dealing with custom wp_query's, most noticably in other plugins like Advanced custom fields.

Replying to nitesh.luharuka:

This works perfectly. The problem persists with 4.7.4 as well.

Replying to jpSimkins:

To bypass this issue, do not use the_post as this is the issue.

I use this syntax and I have numerous metaboxes with no issue to the default post object nor the need to call wp_reset_postdata();.

<?php
$query = new \WP_Query($args);
if ($query->have_posts()) {
    foreach ($query->get_posts() as $p) {
        // nomal loop logic using $p as a normal WP_Post object
    }
}

It needs to be noted that you cannot use the loop methods like get_the_title() but you can easily call $p->post_title. I do not believe the filters are applied to these so you may need to use a different object for the backend than the frontend since the issue is only on the backend of WP.

Also, you could just use the same args with get_posts();

Last edited 7 years ago by lagdonkey (previous) (diff)

#28 @ocean90
6 years ago

#41958 was marked as a duplicate.

#29 @billerickson
5 years ago

This issue still exists in WordPress 5.0, with both the Classic Editor and Block Editor.

You can reproduce the issue with this plugin: https://gist.github.com/billerickson/21ff280fb26c3b98120044d53f4e1b61

Classic Editor screenshot: https://cl.ly/e122cffed719
Block Editor screenshot: https://cl.ly/33bd55ca2869

This caused issues in Gutenberg blocks as well (see https://github.com/WordPress/gutenberg/issues/7468 ), and was temporarily fixed using the same approach outlined above - backup the global $post and restore it afterwards (see https://github.com/WordPress/gutenberg/pull/7889 ).

#30 @smerriman
4 years ago

This problem is very frustrating, and affects a lot of plugins. For example, Yoast SEO processes shortcodes on restoring a revision, which can result in being redirected to the wrong page after the revision is restored.

And they don't want to fix it, given it's a WordPress core issue: https://github.com/Yoast/wordpress-seo/issues/12017

The approach above was suggesting setting up an entire WP_Query loop on the admin page, which I can imagine could be tricky.

Rather than having to add hacky code throughout all plugins and a large number of other locations to backup and save the $post variable every time it might change, how about simply doing this once? When $post is first created in the admin, save it; then adjust the wp_reset_postdata function to restore it if you're in the admin.

That way plugins/shortcodes can continue to use wp_reset_postdata in the normal way.

#31 @Howdy_McGee
3 years ago

In the admin panel the global $wp_query is technically still available for use. We could simply store the global $post during the loop_start hook when specific flags are set ( is_admin()? ):

add_action( 'loop_start', function() {
	
	if( ! is_admin() ) {
		return;
	}
	
	global $post,
	$wp_query;
	
	$wp_query->post = $post;
	
} );

The above is a simple example but it seems like the global $wp_query is kept empty in most if not all admin panel requests. This would then allow the use of secondary WP_Querys and wp_reset_postdata(). Any foreseeable issues we could run into doing something like this?

Last edited 3 years ago by Howdy_McGee (previous) (diff)

#32 @Howdy_McGee
3 years ago

#30273 was marked as a duplicate.

#33 @smerriman
3 years ago

Just wanted to chime in with an extra thought here. I've long considered this a core WordPress bug - that wp_reset_postdata is the correct way for shortcodes, dynamic blocks, or admin code to run their own custom queries.

I've recently discovered, however, that it is not just the WordPress admin that is affected by the call. If completely ordinary theme code runs a custom loop and calls, for example, the_excerpt (which is empty, and thus processes the full content in order to generate an automatic one, and may run shortcodes etc), the $post variable is also broken as it is reset to the incorrect value.

Is the true situation here that there is no admin bug at all - and indeed it is *incorrect* for any shortcode, dynamic block, or custom code in the admin, to use wp_reset_postdata in the first place? (With the solution by eg Bill Erickson - save the $post variable and restore it afterwards - the correct coding method?)

Last edited 3 years ago by smerriman (previous) (diff)

#34 @smerriman
3 years ago

Sorry, ignore the above. Just realised excerpts strip blocks and shortcodes, so it doesn't affect the front-end after all.

#35 @Howdy_McGee
13 months ago

  • Keywords has-patch added; needs-patch removed

This patch allows developers to reset custom WP_Querys back to the original admin post using wp_rest_postdata().

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


12 months ago

#37 @SergeyBiryukov
12 months ago

  • Milestone changed from Future Release to 6.3

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


12 months ago

#39 @hellofromTonya
12 months ago

  • Keywords early added

As this ticket proposes changes to WP_Query global, a commit should land early in the dev cycle to allow for a longer amount of time for testing and feedback.

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


10 months ago

#41 @audrasjb
10 months ago

  • Owner changed from ericlewis to audrasjb
  • Status changed from reopened to assigned

As per today's bug scrub.
The proposed patch still needs testing. Reassign to myself.

#42 @audrasjb
9 months ago

  • Keywords needs-unit-tests added; ongoing removed

This would probably need some unit tests, too.

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


9 months ago

#44 @audrasjb
9 months ago

  • Milestone changed from 6.3 to 6.4

As per today's bug scrub, we're gonna move it to milestone 6.4, pending unit tests and manual testing.

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


7 months ago

#46 @oglekler
7 months ago

Hi @Howdy_McGee this ticket was reviewed during bug scrub. It needs to be done early due to impact it can possibly have, and also need to be covered with unit test, plus testers need an instruction how to check it. Can you please follow on this? Thank you 🙏

Add props too: @hellofromTonya @zunaid321

#47 @oglekler
6 months ago

  • Milestone changed from 6.4 to 6.5

Because this is an early ticket, I am moving it into the 6.5 milestone. If there will not be progress, next time it should be rescheduled to the Future release.

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


4 months ago

#49 @oglekler
4 months ago

This ticket was discussed during bug scrub.

It still needs unit tests and testing instructions. Because this is an early ticket, it was decided to highlight it in the Dev chat.

Add props to @webcommsat

@Howdy_McGee
4 months ago

A more direct, hookless reliant patch.

#50 @Howdy_McGee
4 months ago

The latest patch moves away from a hook-based approach and adds the association directly to post.php. This should account for the following cases:

  • Direct query overrides with the not-recommended query_posts() and wp_reset_query()
  • Secondary queries with WP_Query() and wp_reset_postdata()
  • Global post overrides with setup_postdata( $post ) and wp_reset_postdata().

I agree with @oglekler that this should be moved to Future release until Unit Tests can be added.

#51 @audrasjb
4 weeks ago

  • Milestone changed from 6.5 to Future Release

Moving to Future release, pending unit tests.

Note: See TracTickets for help on using tickets.