WordPress.org

Make WordPress Core

Opened 9 years ago

Closed 4 years ago

Last modified 4 years ago

#9256 closed enhancement (fixed)

clean up the global variables when moving out of the loop

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

Description

On a theme, that I am developing, the sidebar has a new WP_Query to get only one post from a specific category. All works well (index, single, archive, pages) displaying correctly the result of the_content() of the main column (the main query) and the result of the_content() of my custom query in the sidebar.

The problem is that only on pages and articles that have pagination (using the <!--nextpage--> construct), the function "the_content()" of the sidebar displays the same content of the main column on pages 2, 3, 4 (and so on) of those pages/articles, while on page 1 the various contents are displayed properly.

Is it a bug?

As MichaelH reported, using "echo $post->post_content;" fixes the issue temporarily.

Attachments (4)

9256.diff (557 bytes) - added by Denis-de-Bernardy 9 years ago.
9256.2.diff (974 bytes) - added by wonderboymusic 5 years ago.
9256.3.diff (961 bytes) - added by wonderboymusic 5 years ago.
9256.4.diff (18.4 KB) - added by boonebgorges 4 years ago.

Download all attachments as: .zip

Change History (25)

#1 @scribu
9 years ago

  • Milestone changed from Unassigned to 2.8
  • Version set to 2.7.1

#2 @Viper007Bond
9 years ago

  • Keywords reporter-feedback added; sidebar the_content removed
  • Milestone changed from 2.8 to Unassigned

Are you making sure to use $query_string in your new query? Otherwise the URL parameters saying page 2 has been requested won't be passed.

#3 @Denis-de-Bernardy
9 years ago

  • Keywords needs-patch added
  • Milestone changed from Unassigned to Future Release
  • Type changed from defect (bug) to enhancement

nah, it's related to the use of globals. even if you actually use the reset query functionality, WP will leave a boat load of globals around that then get used elsewhere.

#4 @Denis-de-Bernardy
9 years ago

  • Keywords reporter-feedback removed
  • Summary changed from Use of the_content() in sidebar to clean up the global variables when moving out of the loop

#5 @Denis-de-Bernardy
9 years ago

isn't there a wp_reset_query_vars() or something like that already?

#6 @Denis-de-Bernardy
9 years ago

  • Component changed from General to Query
  • Keywords has-patch needs-testing added; needs-patch removed

in case it's still useful.

#7 @ryan
9 years ago

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

(In [11400]) Reset post-related globals in wp_reset_query(). Props Denis-de-Bernardy. fixes #9256

#8 @ryan
9 years ago

  • Milestone changed from Future Release to 2.8

#9 @robbono
9 years ago

  • Resolution fixed deleted
  • Status changed from closed to reopened

I've applied and tested the attached patch to query.php, but using the modified wp_reset_query() function at the end of the single.php loop, for example, still does not prevent the post's content from being output when the_content() is used in the sidebar on the 2nd page of a paginated post.

#10 @nacin
9 years ago

  • Milestone changed from 2.8 to Future Release

#11 @mdgl
8 years ago

I think the problem here is in "setup_postdata()" (query.php) and its handling of global variables.

The function "setup_postdata()" sets the global variable "$page" based on the instance variables of the global query object, i.e. "$page = get_query_var('page')".

Of course if you have done a separate query (e.g. with "get_posts()", "get_children()" or a new "WP_Query" object) and are trying to make use of the template functions by calling "setup_postdata()", this is incorrect as the value of "$page" should be related to your separate query, not the enclosing loop.

The problem is further compounded because "setup_postdata()" only overwrites the first entry in the global "$pages[]" array if your separate query has only a single page. Hence, the other array entries still contain data from the main page and can be inadvertently accessed by function "get_the_content()" using the incorrect value of "$page".

I believe a workaround is to code as follows:

$myposts = &get_posts(...) /* Or new WP_Query or whatever. */
global $post, $page;
$mysavedpost = $post;
foreach ($myposts as $post) {
   setup_postdata($post); $page = 1;

   the_title(); the_content(); /* And so on. */

}
$post = $mysavedpost; setup_postdata($post);

With the increased use of widgets and complex page templates, such "nested loops" can only become more and more common. Although we're still using global variables in the loop, and "setup_postdata()" still needs to be fixed, a better solution here would be to offer full support for a "save/restore" paradigm for handling nested loops, for example:

$myposts = new WP_Query(...);
$myposts->save_loop_state(); /* Copies all required globals into new query object. */
while ($myposts->have_posts()) :
   $myposts->the_post(); /* Stomps on global variables. */
   ... /* Use global variables through template tags. */
endwhile;
$myposts->restore_loop_state(); /* Restores all required globals from new query object. */

With a bit of thought, this could perhaps even be made automatic and transparent within the query "have_posts()" and "the_post()" methods, although you would still have to have a method for coping with early exit from the loop.

Presently, we seem to have too much forcing of one loop to run after another, use of "wp_reset_query()" and so on, which is all a bit messy (IMHO).

#12 @gvenk
8 years ago

  • Cc gvenk added

#13 @wonderboymusic
5 years ago

  • Milestone changed from Future Release to 3.7

This is very interesting one - to not break anything, I added an argument to setup_postdata() which will set the local var $page to 1 if there is no pagination requested:

function setup_postdata( $post, $paginate = true )

The gist of this ticket:

  1. You have a paginated post and you are on page 2
  2. You make your own query for other posts anywhere else.
  3. You call setup_postdata() on a post with 2 pages
  4. You call the_content()
  5. Page 2's content shows up for that post

9256.2.diff​ fixes that.

#14 @wonderboymusic
5 years ago

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

#15 follow-up: @helen
5 years ago

  • Milestone changed from 3.7 to Future Release

nacin: This whole globals mess needs a major cleanup. Way bigger than that. I suggest punt.

#16 @wonderboymusic
5 years ago

  • Milestone changed from Future Release to 3.9

9256.3.diff is a refresh

#17 in reply to: ↑ 15 @helen
4 years ago

  • Milestone changed from 3.9 to Future Release

Replying to helen:

nacin: This whole globals mess needs a major cleanup. Way bigger than that. I suggest punt.

Seems like this still stands.

@boonebgorges
4 years ago

#18 @boonebgorges
4 years ago

  • Keywords needs-unit-tests removed
  • Milestone changed from Future Release to 4.1

This ticket is essentially the same as #20904 and #25349. The root issue is that setup_postdata() is not always sensitive to its current environment (ie, the WP_Query class from which it's initiated). 9256.4.patch addresses this root issue by doing the following:

  • Convert setup_postdata() to a method of WP_Query, and make setup_postdata() a wrapper for $wp_query->setup_postdata for backward compatibility. (see #20904)
  • In WP_Query::setup_postdata(), get the value of $page out of the current object, rather than using get_query_var(), which references the $wp_query global. This means that <!--nextpage--> is respected in secondary loops.
  • In WP_Query::setup_postdata(), only force $more to 1 when (a) you're looking at an RSS feed, or (b) you're looking at the permalink of the $post passed into the method. This means that <!--more--> is respected in secondary loops. This change, along with the previous one, mean that (a) get_the_excerpt() and (b) wp_link_pages() work as expected in secondary loops.
  • Lots of unit tests, mainly to cover the more mundane parts of setup_postdata(). The critical ones for this ticket are the ones marked with @ticket annotations.

I'm not sure if this counts as the "major cleanup" that nacin discusses earlier, but it's a substantial enough improvement that I think we should consider it for 4.1. Would like feedback from a lead on this one.

This ticket was mentioned in IRC in #wordpress-dev by boonebgorges. View the logs.


4 years ago

#20 @boonebgorges
4 years ago

  • Owner set to boonebgorges
  • Resolution set to fixed
  • Status changed from reopened 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.

#21 @boonebgorges
4 years ago

In 30367:

Add unit test files mistakenly excluded from [30085].

See #9256, #25349.

Note: See TracTickets for help on using tickets.