Make WordPress Core

Opened 2 years ago

Last modified 8 months ago

#55917 new defect (bug)

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

Reported by: lumpysimon's profile lumpysimon Owned by:
Milestone: Future Release Priority: normal
Severity: normal Version: 6.0
Component: Query Keywords: has-patch needs-testing
Focuses: Cc:

Description

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.

Attachments (2)

55917.diff (563 bytes) - added by SergeyBiryukov 23 months ago.
55917.2.diff (876 bytes) - added by SergeyBiryukov 23 months ago.

Download all attachments as: .zip

Change History (24)

#1 @mukesh27
2 years ago

  • Milestone changed from Awaiting Review to 6.1

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.

#2 follow-up: @johnbillion
2 years ago

  • Keywords needs-testing added
  • Milestone changed from 6.1 to 6.0.1

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.

#3 in reply to: ↑ 2 @hasanuzzamanshamim
2 years ago

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.

#4 @mukesh27
2 years ago

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.

#5 follow-up: @azouamauriac
2 years ago

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.

This ticket was mentioned in Slack in #core-test by ironprogrammer. View the logs.


23 months ago

#7 @ironprogrammer
23 months ago

  • Component changed from Administration to Query
  • Focuses administration removed
  • Keywords dev-feedback added

Updating component and keywords per test triage.

#8 @SergeyBiryukov
23 months ago

  • Milestone changed from 6.0.1 to 6.0.2

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

#9 in reply to: ↑ 5 @SergeyBiryukov
23 months ago

  • Keywords has-patch added; dev-feedback removed
  • Milestone changed from 6.0.2 to 6.0.1

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.

#10 follow-up: @aristath
23 months ago

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

#11 in reply to: ↑ 10 @SergeyBiryukov
23 months ago

  • Milestone changed from 6.0.1 to 6.0.2

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.

#12 @SergeyBiryukov
22 months ago

  • Milestone changed from 6.0.2 to 6.1

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.

Last edited 22 months ago by SergeyBiryukov (previous) (diff)

#13 @lumpysimon
22 months ago

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;

#14 @hellofromTonya
22 months ago

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.

Last edited 22 months ago by hellofromTonya (previous) (diff)

#15 @hellofromTonya
22 months ago

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

This ticket was mentioned in Slack in #core by chaion07. View the logs.


20 months ago

#17 @desrosj
20 months ago

  • Milestone changed from 6.1 to 6.2

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

This ticket was mentioned in Slack in #core by costdev. View the logs.


16 months ago

#19 @costdev
16 months ago

  • Milestone changed from 6.2 to Future Release

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.

#20 @antonvlasenko
12 months ago

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.

Last edited 11 months ago by antonvlasenko (previous) (diff)

#21 @antonvlasenko
11 months ago

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):
https://cldup.com/ff0pdZwTeL.png https://cldup.com/fLOtvYIUaL.png

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.
Last edited 11 months ago by antonvlasenko (previous) (diff)

#22 @oglekler
8 months ago

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.

Last edited 8 months ago by oglekler (previous) (diff)
Note: See TracTickets for help on using tickets.