WordPress.org

Make WordPress Core

Opened 7 years ago

Closed 5 years ago

Last modified 5 years ago

#20904 closed defect (bug) (fixed)

WP_Query instances getting entangled with $wp_query global

Reported by: boonebgorges Owned by: boonebgorges
Milestone: 4.1 Priority: normal
Severity: normal Version:
Component: Query Keywords: has-patch
Focuses: Cc:
PR Number:

Description

WP_Query::the_post() fires setup_postdata(), which then uses get_query_var(). get_query_var(), in turn, calls the get() method on the $wp_query global object. In this way, WP_Query instances are not truly insulated from each other - they all are tied up with $wp_query in a way that seems unnecessary, and causes fatal errors in some cases.

In normal use, you would never really notice the issue, because after the 'init' action, the following offending line in setup_postdata() will return the 'page' query var from $wp_query:

$page = get_query_var('page');

And, since secondary WP_Query instances don't paginate in the same way as the primary query, the 'page' property on those instances doesn't matter much. So, while the value of $page is not strictly speaking correct in these cases (it comes from the wrong object), it generally fails silently.

However, if you instantiate a WP_Query object before 'init', you get a fatal error, because $wp_query->get() is an undefined method. Here's a simple mu-plugin that will demonstrate the problem:

function bbg_create_early_wp_query_object() {
	$q = new WP_Query( array(
		'post_type' => 'post'
	) );

	if ( $q->have_posts() ) {
		while ( $q->have_posts() ) {
			$q->the_post();
		}
	}
}
add_action( 'plugins_loaded', 'bbg_create_early_wp_query_object' );

My suggested solution is to move the setup_postdata() logic into a method of the WP_Query class, and then use $this->get( 'page' ) rather than get_query_var( 'page' ), to ensure that you're referring to the query vars of the correct query object. The old function setup_postdata() then becomes a wrapper for $wp_query->setup_postdata().

In addition, I modified the way that the $more global is set, so that it references the current object as well. This prevents _doing_it_wrong() errors when firing up a WP_Query instance before 'init'.

Attachments (3)

20904.patch (3.0 KB) - added by boonebgorges 7 years ago.
20904.diff (3.2 KB) - added by wonderboymusic 6 years ago.
20904.2.diff (3.4 KB) - added by wonderboymusic 6 years ago.

Download all attachments as: .zip

Change History (15)

@boonebgorges
7 years ago

#1 @scribu
7 years ago

  • Keywords 3.5-early added
  • Milestone changed from Awaiting Review to Future Release

We're still tied to all those globals, but this is definitely an improvement.

#2 @wonderboymusic
7 years ago

  • Milestone changed from Future Release to 3.6

#3 @ryan
6 years ago

  • Milestone changed from 3.6 to Future Release

@wonderboymusic
6 years ago

#4 @wonderboymusic
6 years ago

  • Milestone changed from Future Release to 3.7

Boone, you are an attractive and intelligent man - I refreshed the patch so it doesn't blow up against trunk

#5 @wonderboymusic
6 years ago

  • Keywords needs-unit-tests added

#6 @johnbillion
6 years ago

  • Keywords 3.5-early removed
  • Milestone changed from 3.7 to 3.8

This won't make 3.7 without unit tests. Punting.

#7 @wonderboymusic
6 years ago

20904.2.diff passes Unit Tests now, it was failing 3 of them

#8 @nacin
6 years ago

  • Keywords 3.9-early added; needs-unit-tests removed
  • Milestone changed from 3.8 to Future Release

And, since secondary WP_Query instances don't paginate in the same way as the primary query, the 'page' property on those instances doesn't matter much.

Re-reading this, they don't matter much, but they do matter in some cases, right? Like if you're doing your own crazy handling? I am all for this change, though it seems minor, given it more or less works fine as-is. While it allows WP_Query to be used before init, I'm not sure allowing this is actually desirable. Post types aren't registered yet and a lot still needs to be initialized.

I'm wondering if we should tackle this in conjunction with #25349 and #9256. I'd be game for a 3.9 overhaul of post data setup, ideally untangling these globals in the process.

#9 @boonebgorges
6 years ago

While it allows WP_Query to be used before init, I'm not sure allowing this is actually desirable.

Fair enough, but the point about pre-init is just to give a particularly striking instance of the problem. After init, the leakiness is still there, but doesn't throw any notices, and so goes unnoticed.

given it more or less works fine as-is.

In WP, yes, but there are probably others that are attempting things that ought to be legit (however "crazy") but run into problems because the objects aren't encapsulated. 20904.2.diff is a very modest way of fixing it. If it could be rolled into a larger overhaul of the way post globals work, that'd be excellent, but if that doesn't end up happening in 3.9, I think this more conservative patch is worth considering.

#11 @boonebgorges
5 years ago

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

In 30085:

Improve global variable setting in setup_postdata().

setup_postdata() is responsible for setting a number of global variables
that are used for post pagination ($pages, $page, $nextpage) and the
generation of post excerpts ($more). These variables should be sensitive to
the currently running instance of WP_Query - rather than the main query -
so that these features work properly inside of secondary WP_Query loops.

This changeset moves the logic of setup_postdata() into a method on WP_Query,
and converts setup_postdata() to a wrapper.

Props boonebgorges, wonderboymusic.
See #25349.
Fixes #9256, #20904.

#12 @boonebgorges
5 years ago

  • Milestone changed from Future Release to 4.1
Note: See TracTickets for help on using tickets.