WordPress.org

Make WordPress Core

Opened 3 months ago

Closed 3 months ago

#47114 closed defect (bug) (fixed)

global $pages not available on the_post action

Reported by: david.binda Owned by: azaozz
Milestone: 5.2 Priority: highest omg bbq
Severity: normal Version: 5.2
Component: Posts, Post Types Keywords: has-patch commit
Focuses: Cc:

Description

After r44941 the $pages (and other global variables) are no longer available in the_post action.

As the the_post action is now being triggered from a new WP_Query::generate_postdata method, which is called from original WP_Query::setup_postdata before any of the global $id, $authordata, $currentday, $currentmonth, $page, $pages, $multipage, $more, $numpages; is populated, previously available global variables are no longer populated at the time the_post action is triggered.

I've whipped-up a quick unit tests to test this behaviour. The test passes prior r44941, but fails afterwards:

<?php
    function test_the_post_action() {
        $post = self::factory()->post->create_and_get();
        add_action( 'the_post', function() {
            $this->pages = $GLOBALS['pages'];
        } );
        setup_postdata( $post );
        $this->assertEquals( $GLOBALS['pages'], $this->pages );
    }

(In my tests, I added the test to https://core.trac.wordpress.org/browser/trunk/tests/phpunit/tests/query/setupPostdata.php )

Attachments (3)

47114.diff (1.8 KB) - added by david.binda 3 months ago.
move the the_post action from generate_postdata to setup_postdata
47114-2.diff (1.2 KB) - added by birgire 3 months ago.
Only the src/ part of 47114.diff
47114.2.diff (1.8 KB) - added by jorbin 3 months ago.

Download all attachments as: .zip

Change History (17)

#1 @david.binda
3 months ago

  • Version set to trunk

#2 @david.binda
3 months ago

  • Summary changed from global $pages not available on the_post to global $pages not available on the_post action

@david.binda
3 months ago

move the the_post action from generate_postdata to setup_postdata

#3 @azaozz
3 months ago

  • Milestone changed from Awaiting Review to 5.2
  • Priority changed from normal to highest omg bbq

This looks like a regression that may affect a lot of themes and plugins. The the_post action was explicitly fired (inside the loop) after setup_postdata() has set the globals. Even looking at the inline docs suggests that:

* Set up global post data. (from setup_postdata())
...
* Fires once the post data has been setup. (from do_action_ref_array( 'the_post', ... ))

Firing it before the globals are set would also affect reset_postdata() as some of the globals will be from the "other" $post, before they are overwritten with the "current" post's data.

I don't see problems with moving the action back to setup_postdata() so it fires after the expected globals are set as in 47114.diff. This doesn't seem to affect the fix from #42814 [44941], but may be missing something. @boonebgorges @spacedmonkey could you have a look?

Moving this for consideration before 5.2 and setting it as high priority as we are just few days from release.

#4 @azaozz
3 months ago

  • Keywords 2nd-opinion added

This ticket was mentioned in Slack in #core-committers by azaozz. View the logs.


3 months ago

#6 @spacedmonkey
3 months ago

  • Keywords has-patch added; 2nd-opinion removed

I looked a little bit at this last night. I don’t see any issue with moving the action to setup_postdata and don’t see how will break anything. I don’t have time to test this or commit access, but if you are happy @azaozz , I would go ahead and commit.

#7 @boonebgorges
3 months ago

  • Keywords commit added
  • Owner set to azaozz
  • Status changed from new to assigned

+1 on 47114.diff - thanks for the patch and diagnosis, @davidbinda. @azaozz OK to commit from me.

#8 @birgire
3 months ago

When 47114.diff is commited, just a small reminder to add the:

/**
 * @ticket 47114
 */

above the test_the_post_action() test method in 47114.diff.

The test in 47114.diff uses an anonymous function. I know the minimum PHP has been bumped to 5.6 so it should be supported, but I was wondering if there have been any discussion or guidelines regarding the general use of anonymous functions in core or tests (as there are both pros and cons using it) ?

Last edited 3 months ago by birgire (previous) (diff)

#9 @boonebgorges
3 months ago

Thanks, @birgire - I think the anonymous function should be taken out. We could probably live without the test altogether. The /src/ part of the patch is good to go in.

@birgire
3 months ago

Only the src/ part of 47114.diff

@jorbin
3 months ago

#10 @jorbin
3 months ago

I disagree with @boonebgorges in that I think the test is fine. We can always refactor the test later, but in general, I think this usage of an anonymous function is fine and having a test is better than no test.

Overall, I think this patch is good. Tested and not seeing any unexpected side effects.

#11 @azaozz
3 months ago

I think the anonymous function should be taken out.

...

I think this usage of an anonymous function is fine and having a test is better than no test.

The problem with using an anonymous function in an action or a filter is that it "breaks the API" and makes it impossible to remove. This is not a problem for tests as hooks there are generally non-removable by plugins, but the coding standards explicitly forbid that use, especially in core.

Think we should keep the test but change it to a non-lambda callback.

#12 @azaozz
3 months ago

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

In 45285:

Fix setup_postdata() to set the (inside the loop) globals before the_post action is fired. Follow-up from #42814 and [44941].

Props david.binda, spacedmonkey, boonebgorges, birgire, jorbin, azaozz.
Fixes #47114 for trunk.

#13 @azaozz
3 months ago

  • Resolution fixed deleted
  • Status changed from closed to reopened

Reopen for the 5.2 branch.

#14 @azaozz
3 months ago

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

In 45286:

Fix setup_postdata() to set the (inside the loop) globals before the_post action is fired. Follow-up from #42814 and [44941].

Props david.binda, spacedmonkey, boonebgorges, birgire, jorbin, azaozz.
Merges [45285] from trunk.
Fixes #47114 for 5.2.

Note: See TracTickets for help on using tickets.