Fix admin styles

[spacedmonkey]

Trac ticket: https://core.trac.wordpress.org/ticket/57629

This Pull Request is for code review only. Please keep all other discussion in the Trac ticket. Do not merge this Pull Request. See GitHub Pull Requests for Code Review in the Core Handbook for more details.

[spacedmonkey]

The function get_theme_file_path does not exist in script-loader.php

[joemcgill]

I've applied this patch to trunk and tried to test the admin with SCRIPT_DEBUG set to false and this change doesn't have any effect, i.e., the styles are still broken. Reverting to the original change does work:

	$theme_has_support = is_readable( get_stylesheet_directory() . '/theme.json' );

	// Look up the parent if the child does not have a theme.json.
 	if ( ! $theme_has_support ) {
 	    $theme_has_support = is_readable( get_template_directory() . '/theme.json' );
 	}

[SergeyBiryukov]

Thanks! Doesn't seem to fix the issue for me, it then fails with a similar but different error:

Fatal error: Uncaught Error: Class 'WP_Theme' not found in wp-includes/theme.php:131

Stack trace:
#0 wp-includes/global-styles-and-settings.php(411): wp_get_theme()
#1 wp-includes/script-loader.php(1647): wp_theme_has_theme_json()
#2 wp-admin/load-styles.php(49): wp_default_styles(Object(WP_Styles))
#3 {main} thrown in wp-includes/theme.php on line 131

class-wp-theme.php is not loaded in wp-admin/load-styles.php either, not sure if it should be 🤔

Pinging @azaozz for advice 🙂

[spacedmonkey]

@SergeyBiryukov @joemcgill Moving this function seems to work for me. I am not sure about moving functions like this. I don't see the harm personally, but want to check.

[joemcgill]

This is a clever workaround. This is loaded early enough that it should be fine, but would be good to do some extra testing.

[SergeyBiryukov]

This function is a part of a larger set:

• get_theme_file_uri()
• get_parent_theme_file_uri()
• get_theme_file_path()
• get_parent_theme_file_path()

Moving them to wp-includes/theme.php makes sense at glance. We already have a few others there:

• get_stylesheet_directory_uri()
• get_template_directory_uri()
• get_theme_root_uri()
• etc.

But in that case, all four should be moved, not just one 🙂

[spacedmonkey]

But in that case, all four should be moved, not just one 🙂

I agree, I these should live theme.php. Should I move them @SergeyBiryukov ?

[joemcgill]

The main risk in moving functions is that someone is doing a check to see if those functions are loaded, and if not, are manually requiring the original file, ex:

if ( ! function_exists( 'get_theme_file_path' ) {
    require require ABSPATH . WPINC . '/link-template.php';
}

$theme_path = get_theme_file_path( $path );

By moving these functions, code doing something like ☝🏻 will now fatal. I don't see examples of this in the wild, but the safest way to handle this is to wrap the places where we're moving functions with a similar function_exists check and load the new file if needed. This is probably very edge case, so not a blocker, but something to keep in mind if we get bug reports after this change.

[joemcgill]

IMO, moving additional functions can be a follow-up task, since this is needed to unblock the 6.3-beta1 release.

[spacedmonkey]

By moving these functions, code doing something like ☝🏻 will now fatal. I don't see examples of this in the wild, but the safest way to handle this is to wrap the places where we're moving functions with a similar function_exists check and load the new file if needed. This is probably very edge case, so not a blocker, but something to keep in mind if we get bug reports after this change.

I was just searching that now. https://www.wpdirectory.net/search/01H3ZBHTNVSS5ZWW91ZXXGEE7J. I can't see any examples.

[spacedmonkey]

After looking at this again. I am not sure about moving all these functions.

I think the path based function make sense.
get_theme_file_path()
get_parent_theme_file_path()

But link based function should remain in link-template.php
get_theme_file_uri()
get_parent_theme_file_uri()

[SergeyBiryukov]

I agree, I these should live theme.php. Should I move them @SergeyBiryukov ?

I think so 🙂

But link based function should remain in link-template.php
get_theme_file_uri()
get_parent_theme_file_uri()

There are a few other theme-related *_uri() functions in theme.php already:

• get_stylesheet_directory_uri()
• get_stylesheet_uri()
• get_locale_stylesheet_uri()
• get_template_directory_uri()
• get_theme_root_uri()

So I would say get_theme_file_uri() and get_parent_theme_file_uri() belong there too, it seems like a more appropriate and consistent location for them. There are no other *_uri() functions in link-template.php.

I think I would prefer moving them all in one go, though moving the additional functions in a follow-up commit might also be an option.

On a related note, phpunit/tests/link/themeFile.php should also be moved to phpunit/tests/theme/themeFile.php.

[spacedmonkey]

@SergeyBiryukov @joemcgill I have another solution that doesn't involve moving the functions.

#4730