global $pages not available on the_post action

[david.binda]

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 )

[david.binda]

move the the_post action from generate_postdata to setup_postdata

[azaozz]

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.

[spacedmonkey]

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.

[boonebgorges]

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

[birgire]

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) ?

[boonebgorges]

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]

Only the src/ part of 47114.diff

[jorbin]

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.

[azaozz]

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.

[azaozz]

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.

[azaozz]

Reopen for the 5.2 branch.

[azaozz]

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.