WordPress.org

Make WordPress Core

Opened 4 years ago

Closed 4 years ago

#12320 closed defect (bug) (fixed)

Invalid call to wp_reset_query() inside Recent Posts widget

Reported by: michelwppi Owned by: azaozz
Milestone: 3.0 Priority: normal
Severity: normal Version: 2.9.2
Component: Widgets Keywords: has-patch commit
Focuses: Cc:

Description

Description:

When adding Recent posts widget in sidebar of a home page modified by a query_posts limiting to one cat. or other features, the loop result of posts is not the attended one but the original default home query.

The recent posts widget in defaut-widgets.php contains a curious line:

wp_reset_query();  // Restore global post data stomped by the_post().

The Query.php contains this function with his "obscure" comment:

/**
 * Destroy the previous query and setup a new query.
 *
 * This should be used after {@link query_posts()} and before another {@link
 * query_posts()}. This will remove obscure bugs that occur when the previous
 * wp_query object is not destroyed properly before another is setup.
 *
 * @since 2.3.0
 * @uses $wp_query
 */
function wp_reset_query() {

With this resetting approach and - not with temporay backup and restore - when, in loop in loop, sharing global vars as $wp_query or $wp_the_query, it introduces unwanted side effects.

Temporary workaround:
Around the sidebar top box, needs to add these temp vars:

global $wp_the_query, $wp_query;
$tmp_query = $wp_query;
if ( !function_exists('dynamic_sidebar') || !dynamic_sidebar('Sidebar top')) : ?>
<ul>
 	<li>?</li>
</ul>
<?php  endif; $wp_the_query = $tmp_query;  $wp_query = $tmp_query;

with the limit of only one recent posts widget in one sidebar box.

Don't hesitate to question me.

NOTE: In our http://dev.xiligroup.com/xili-language/ plugin, we decide to create a new like widget without this 'obscur' reset function wp_reset_query and added of other features for multilingual website.

Best regards, Michel of http://dev.xiligroup.com

Attachments (2)

12320.diff (609 bytes) - added by scribu 4 years ago.
12320.2.diff (1.1 KB) - added by scribu 4 years ago.
introduce wp_reset_postdata()

Download all attachments as: .zip

Change History (23)

comment:1 scribu4 years ago

  • Component changed from Query to Widgets
  • Keywords needs-patch added; default widget removed
  • Milestone changed from Unassigned to 3.0
  • Owner changed from ryan to azaozz
  • Priority changed from high to normal

It should probably create a separate query, instead of calling query_posts().

comment:2 scribu4 years ago

Oh, it seems it's already creating a separate query:

$r = new WP_Query(array('showposts' => $number, 'nopaging' => 0, 'post_status' => 'publish', 'caller_get_posts' => 1));

Please describe the unwanted side effects you're experiencing.

scribu4 years ago

comment:3 scribu4 years ago

  • Summary changed from Obscur wp_reset_query() function inside widget Recent Posts to Invalid call to wp_reset_query() inside Recent Posts widget

comment:4 scribu4 years ago

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

12320.diff restores the $post global without messing with $wp_query of $wp_the_query.

Try it out.

scribu4 years ago

introduce wp_reset_postdata()

comment:5 scribu4 years ago

12320.2.diff extracts the code into a new function - wp_reset_postdata() - which can be used in other contexts.

comment:6 filosofo4 years ago

Please help me to understand the nature of the problem.

  • Why would the sidebar be within a Loop?
  • Shouldn't a Loop within a Loop be using its own instantiation of WP_Query, instead of $wp_query or $wp_the_query?

comment:7 follow-ups: michelwppi4 years ago

  • Cc michelwppi added

Hi Scribu !
Thanks for your prompt answer.
I test your diffs inside a test website with some themes and the current Kubrick theme. Now it works. Your idea to separate reset of wp_query and reset of global post in two functions is a very very good idea.
By example, The new instantiation of WP_query in a WP default Recent Posts WIdget in sidebar don't interfere with a query_posts() inserted in the wp or wp_head 'action' filters for the home page or others.
Can you target the modifications (inside query.php and inside default-widgets.php) to milestone 2.9.3 ?

Hi filosofo, I agree, it is not really a loop in loop as described with template tags but remember that the results of the main query are available inside global $wp_query and $wp_the_query. And the instantation can use or share objects of these former globals.

Best regards

Michel of http://dev.xiligroup.com - Europe Time

comment:8 in reply to: ↑ 7 scribu4 years ago

Replying to michelwppi:

Can you target the modifications (inside query.php and inside default-widgets.php) to milestone 2.9.3 ?

It won't make it into 2.9.3, since that's reserved for security issues.

comment:9 scribu4 years ago

  • Keywords needs-testing removed

comment:10 in reply to: ↑ 7 filosofo4 years ago

Replying to michelwppi:

it is not really a loop in loop

  • What is "it"?
  • In other words, what is not really a loop in loop?
  • Please show sample code if available.

as described with template tags

  • What do you mean by a "loop as described with template tags"?
  • How do template tags "describe" a loop?

but remember that the results of the main query are available inside global $wp_query and $wp_the_query. And the instantation can use or share objects of these former globals.

  • Since the results of the main query are always inside $wp_the_query, why are still experiencing problems?
  • What are you unable to do with the current system?
  • What is the expected behavior, and how does the actual behavior differ?
  • Can you post some code that produces the problem?

Thank you.

comment:11 follow-up: michelwppi4 years ago

Replying to filosofo

I am not sure if it is here the right place to go deep about loops (main and others) and loop in loop (codex: http://codex.wordpress.org/The_Loop) and loops in sidebar or elsewhere. But I will try to be more explicit.

As described above and as well understood by scribu, the reset of global $wp_query is not justified here inside default - lastest posts widget -. Perhaps only the global $post is wished.
It is easy to confirm by tests and different themes. Depending of architecture of theme's files, the place of the function get_sidebar() - and the call of dynamic_sidebar() is crucial to see the bad effects of the unwished reset.
When the call of this function get_sidebar() building the widgets is after the main loop is accomplished, nothing occurs.

BUT is the call is before (by example just after the call of get_header()) and if the default query (home by example) is completed (via a filter) by a query_posts() call (by example to affine query according detected browser), the unwished big reset cancel this query addition and the main loop will begin with the very first query.

In more general terms, a widget must not interfere with his environment especially shared global vars. See above in original ticket the temporary workaround.

Best regards, Michel of http://dev.xiligroup.com

comment:12 in reply to: ↑ 11 filosofo4 years ago

Replying to michelwppi:

BUT is the call is before (by example just after the call of get_header()) and if the default query (home by example) is completed (via a filter) by a query_posts() call (by example to affine query according detected browser), the unwished big reset cancel this query addition and the main loop will begin with the very first query.

OK, so you're saying the problem occurs in a situation like this?

query_posts(/* some custom arguments */);

get_sidebar(); /* here the recent posts widget appears */

while (have_posts() ) : /* now the Loop starts */

If so, the main problem is that the theme is querying the wrong way. query_posts should be called immediately before its intended Loop, and certainly not have another template appear between it and the Loop.

A better way to handle the main request query is to hook into the $wp object, for example at the 'parse_request' action hook.

In more general terms, a widget must not interfere with his environment especially shared global vars.

That's probably a good idea for core, but many plugins and themes violate this.

comment:13 follow-up: scribu4 years ago

There might be some other issue with the reporter's code, but here is the problem this ticket is addressing:

  1. The Recent Posts widget creates its own query: $r = new WP_Query(...)
  1. Then, it calls wp_reset_query()
  1. wp_reset_query() unnecessarily changes the $wp_query global (see 1. above)

comment:14 in reply to: ↑ 13 filosofo4 years ago

Replying to scribu:

There might be some other issue with the reporter's code, but here is the problem this ticket is addressing:

To be clear, I wasn't objecting to your patch. I was just trying to understand what the OP was saying.

comment:15 scribu4 years ago

  • Keywords commit added

comment:16 follow-up: Denis-de-Bernardy4 years ago

  • Keywords commit removed

this is not an invalid call. it used to work when it got committed, and a separate ticket had us change the automatic setting up of the postdata.

the call is in there in case you use a recent posts widget in the loop, using the_widget().

comment:17 Denis-de-Bernardy4 years ago

I can't recall why the post data is no longer set up, though. scanning through the logs for that hook and for the likes of loop_start/loop_end would probably reveal it, though.

also, I continue to stand by my opinion on this. it wouldn't be such a huge mess if we weren't relying on globals, and references to globals (wp_the_query), and incorrect globals (wp_query in the various is_*() functions), and...

comment:18 in reply to: ↑ 16 scribu4 years ago

  • Keywords commit added; query loop removed

Replying to Denis-de-Bernardy:

this is not an invalid call. it used to work when it got committed, and a separate ticket had us change the automatic setting up of the postdata.

the call is in there in case you use a recent posts widget in the loop, using the_widget().

If you would read my comment again and also the docs for wp_reset_query() you will see that it is indeed misplaced.

comment:20 scribu4 years ago

And also look at the proposed patch, before commenting.

comment:21 westi4 years ago

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

(In [14607]) Introduce wp_reset_postdata(). Use it to reset the post global for the current query_posts() call after using a loop with a new WP_Query object. Fixes #12320 props scribu.

Note: See TracTickets for help on using tickets.