Fatal error in WP6.0 when calling get_current_screen on the pre_get_posts action

[lumpysimon]

In 5.9 and earlier versions, it's possible to call get_current_screen from within a function that runs on the pre_get_posts hook. But in version 6.0 it causes a fatal error Call to undefined function get_current_screen().

add_action( 'pre_get_posts', 'abcdef_sort_posts' );

function abcdef_sort_posts( $query ) {

	if ( wp_doing_ajax() ) {
		return;
	}

	if ( ! is_admin() ) {
		return;
	}

	$screen = get_current_screen();

	if ( 'edit-page' == $screen->id ) {

		$query->set( 'orderby', 'post_title' );
		$query->set( 'order', 'DESC' );

	}

}

I came across the issue on two sites, I can reproduce it by installing 5.9 with no plugins enabled, adding the above code in a plugin or functions.php, everything works fine, then upgrading to 6.0 and visiting any admin screen causes the fatal error. Front-end is fine.

[mukesh27]

Hi there!

Thanks for the ticket.

I reproduce the issue in 6.0 and nightly version 6.1-alpha-53461.

Set 6.1 milestone for visibility. Fill free to update it.

[johnbillion]

Git bisect is telling me that r53282 is the culprit, although I can't see anything in that changeset that looks troublesome. Moving to 6.0.1 for investigation.

[hasanuzzamanshamim]

Replying to johnbillion:

Git bisect is telling me that r53282 is the culprit, although I can't see anything in that changeset that looks troublesome. Moving to 6.0.1 for investigation.

I tested this issue reverting r53282 commit and got the src/wp-includes/script-loader.php
file changes create this issue.

add_action( 'wp_loaded', $fn_register_webfonts );

I found this hook makes the issue.

[mukesh27]

I try to track the issue and this function call $settings = WP_Theme_JSON_Resolver::get_merged_data()->get_settings(); generate issue for 6.0 version and function dependency called wp_get_recent_posts() function to get recent posts for wp_global_styles post type.

Ping @hellofromTonya for more help on this issue.

[azouamauriac]

In addition of the snippet in this ticket description, we can add the following(in function.php) to reproduce the issue

add_action( 'wp_loaded', 'test' );
function test(){
	var_dump(wp_get_recent_posts());
}

Therefore the bug is occurring because the hook wp_loaded triggers pre_get_posts earlier, before get_current_screen() is available... I'm wondering if we can change wp_loaded by another without breaking anything.

[ironprogrammer]

Updating component and keywords per ​test triage.

[SergeyBiryukov]

Looks like this still needs more investigation and a patch. With 6.0.1 RC1 coming soon, moving to 6.0.2 for now.

[SergeyBiryukov]

Replying to azouamauriac:

Therefore the bug is occurring because the hook wp_loaded triggers pre_get_posts earlier, before get_current_screen() is available... I'm wondering if we can change wp_loaded by another without breaking anything.

I remembered that we did explore moving $fn_register_webfonts to wp_enqueue_scripts in #55632, but it was not required for that particular issue. See the discussion in comment:7:ticket:55632 and on ​PR #2638.

55917.diff​ resolves the issue in my testing. Moving back for 6.0.1 consideration.

[aristath]

The webfonts API is a scenario where we need to take lots of things into account... Webfonts need to be properly loaded in the block editor, both in the normal edit screen and the site-editor, they also need to be available in REST-API calls for the editor because some styles get added via REST calls, and they also need to be available in the frontend.
That is why the wp_loaded hook was selected... If I remember correctly it was one of the very few that worked for all cases.
We can try using wp (which runs after pre_get_posts), but this will need to be thoroughly tested to ensure that webfonts are still properly loaded both in the editors & the frontend. 👍

cc @hellofromTonya, I remember we were discussing this hook a few months ago

[SergeyBiryukov]

Replying to aristath:

We can try using wp (which runs after pre_get_posts), but this will need to be thoroughly tested to ensure that webfonts are still properly loaded both in the editors & the frontend. 👍

Thanks for the insights! It looks like this is not as straightforward as I thought :)

55917.2.diff​ changes the action to wp and adds a comment summarizing your reply to explain why this specific action was chosen.

Since this still needs thorough testing and WP 6.0.1 is about to be released tomorrow, moving to 6.0.2 for now.

[SergeyBiryukov]

I was told by @aristath that @hellofromTonya is working on a refactoring of core's Webfonts API implementation, and this patch may not be needed in near future.

Moving this to 6.1 for now for further tracking. In the meantime, I think a workaround would be to check for get_current_screen() existence before calling it.

[lumpysimon]

I've worked around it by removing the call to get_current_screen() and replacing it with my own function which is exactly the same:

global $current_screen;

if ( ! isset( $current_screen ) ) {
    return null;
}

return $current_screen;

[hellofromTonya]

I'm so sorry I'm late in responding to this issue. Somehow I missed the pings. My apologies.

FYI: The WP 6.0 [53282] stopgap is temporary and will be replaced in the future when a proper API is introduced (currently planned for WP 6.1).

From @SergeyBiryukov:

I was told by @aristath that @hellofromTonya is working on a refactoring of core's Webfonts API implementation, and this patch may not be needed in near future.

Yes, a new implementation is being explored to use Core's Dependencies API (​see the PR here). In this new implementation, the API does not hook into 'wp_loaded'.

As noted by @mukesh27:

I try to track the issue and this function call $settings = WP_Theme_JSON_Resolver::get_merged_data()->get_settings(); generate issue for 6.0 version and function dependency called wp_get_recent_posts() function to get recent posts for wp_global_styles post type.

While this specific fatal is pointing at the stopgap being hooked into wp_loaded, is that the true root cause? Or is the root cause deeper within the code, meaning a hidden problem exists that could cause other problems in other scenarios?

Thinking out loud, I wonder:

• Is there are other instances within Core where this same code might also be called before get_current_screen() is available?
• What is the impact of not having get_current_screen() function in memory when this happens? (ie beyond the fatal noted in this specific use case)

The answers may reveal if there is a deeper issue within Core, if there are some scenarios where theme.json resolver may not work as intended, and/or if our assumptions (such as when to fetch the user's selected global styles) are incorrect.

[hellofromTonya]

I added this Trac ticket to the Webfonts project board for tracking within the scope of the API's ongoing roadmap. ​https://github.com/WordPress/gutenberg/projects/66

[desrosj]

WordPress 6.1 RC1 is due out any moment. Punting this one to 6.2.

[costdev]

This ticket was discussed in the recent bug scrub. This still needs deeper investigation and is being tracked in ​the Fonts API project board. Moving this to Future Release until the root cause is found.

[antonvlasenko]

I'm investigating this issue to see if I can move it forward.

Update, May 31st: I ran out of time today. I will have the opportunity to work on this issue in about 12 days.
Otherwise, please feel free to work on it if you wish to do so.

June 12th: I'm back and working on this issue.

[antonvlasenko]

TLDR: I've spent some time investigating, and I don't see any issues with the current implementation of the Fonts API.

A more detailed explanation:

The patch proposed by @SergeyBiryukov resolves the "issue" in one place, but since then, another section of the code has emerged that creates a similar issue: ​https://github.com/WordPress/gutenberg/pull/42454/files#diff-28f36bb64fbbc8ce728db401b1e4cf30c9f44db7f5dcce1b7b0cc9ce60b1a209R191.
That PR introduces a new function called build_template_part_block_instance_variations(), which also causes a fatal error when using the code snippet above (and it has been ​backported to Core).

For reference, here are the call stacks for both fatal errors (i.e., the "old" and the "new" one):

It can be observed that in both cases, the fatal error occurs when the WP_Query::get_posts() method is called and the pre_get_posts action is fired within that method.

It is logical to assume that the get_current_screen() function may be undefined when the pre_get_posts action is fired, ​based on the description of the `pre_get_posts` filter: the action fires after the query variable object is created but before the actual query is run.

From my perspective, pre_get_posts is 100% backend logic unrelated to the frontend.
During the Core initialization, the list of posts may be needed long before the current page and corresponding "screen" are known.

The fact that the code provided in the example above worked in earlier WordPress versions is more of a coincidence than a rule.
I see two ways to solve this issue:

1. Do not rely on get_current_screen() being defined and use some low-level PHP function to determine the current page.
2. Use another action to sort posts, which is guaranteed to be fired after the get_current_screen() function is defined.

[oglekler]

I am suggesting to move this ticket into 6.5 for more active discussion. I'm also expecting/want to use the get_current_screen() function much earlier than is possible right now - this it problem with getting information about the current page when it is needed. I think this is the problem to solve, and it is a bit difficult to figure out from the conversation why fonts are related to this ticket.