Use `get_theme_file_path()` in `wp_theme_has_theme_json()`

[flixos90]

Follow up to #56975: Potentially the new wp_theme_has_theme_json() function should rely on get_theme_file_path() instead of manually checking stylesheet directory and template directory itself.

Use get_theme_file_path function to stop replicated code.

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

[spacedmonkey]

I think we should consider also merge ​this at the same time.

The use case here, is if I am developer, I want to use filter the location of the theme.json to load from my plugin or somewhere, I would use the theme_file_path filter.

[costdev]

@flixos90 @spacedmonkey @hellofromTonya Myself and Mukesh reviewed this ticket today.

With 6.2 Beta 1 being released later today, is this ready for commit, or is there still more discussion needed on the PR and it should be moved from the milestone?

Additional props: @mukesh27

@mukeshpanchal27 @felixarntz

I have include another commit, to also use the theme_file_path filter in theme json resolver. If we are going to implement using this filter in one place, it needs to be done in all.

think of this use case.

• Developer, has a filter that filters the location of the theme.json file.
• Calls wp_theme_has_theme_json, get return true.
• But then, get_theme_data in theme json resolver, use get_file_path_from_theme that does not respect that.

Why is_readable() vs file_exists()?

is_readable — Tells whether a file exists and is readable
Returns true if the file or directory specified by filename exists and is readable, false otherwise.

​https://www.php.net/manual/en/function.is-readable.php

file_exists — Checks whether a file or directory exists
Returns true if the file or directory specified by filename exists; false otherwise.

​https://www.php.net/manual/en/function.file-exists.php

is_readable() then is an extra safety measure to ensure the file is usable for reading it into memory.

[costdev]

This ticket was discussed in the pre-6.2 Beta 1 bug scrub. Moving this to the 6.3 milestone.

is_readable() then is an extra safety measure to ensure the file is usable for reading it into memory.

I recommend using is_readable, as this what is correctly in core.

I think using the theme_file_path is extremely useful for developers.

I would like to unblock this, can I get a review of this.

@hellofromtonya @oandregal @felixarntz

My main comment is that these changes should happen in Gutenberg first. This is something I have recurrently mentioned, and it takes time and energy for everyone. I feel I sound like a broken record. I'm sorry about that. Is there something I can do to clarify this, so nobody needs to waste energy/time on this again?

For full transparency: other than I already commented ​back in February, I don't fully understand how this is impactful (performance, users, etc.). I feel others would be better equipped than I am to help figure this out.

I feel I sound like a broken record. I'm sorry about that.

Can you point me in the direction of the documented process for this? @oandregal

Can you point me in the direction of the documented process for this?

As far as I know, it has been good etiquette among people that work together, and it has been transmitted PR to PR in comments. I have been personally doing this for months, if not a year. I know @azaozz and @hellofromtonya have been sharing this as well in other channels, they will have more context as to where.

I don't know that there is a handbook page anywhere that states this. I'm happy to help writing one down if that's useful to help us focus on solving user's problems.

[spacedmonkey]

@flixos90 Is this good to commit?

Following on from the above, is this process documented in a handbook yet? If there are files in core that need to be modified first in Gutenberg then ideally there should be a test that fails when they're modified in a core PR.

[johnbillion]

@flixos90 Can you give this the final nod?

[spacedmonkey]

There is a gutenberg PR that can be found here - ​https://github.com/WordPress/gutenberg/pull/45831

[oglekler]

@spacedmonkey it is very vague from my point of view what the Gutenberg PR has to do with yours, can you please, explain it a bit. Thank you!

[spacedmonkey]

@oglekler Basically wp_theme_has_theme_json and related code, do not use get_theme_file_path. This means that they do not run through the filter theme_file_path. All other theme files use this function. For consistency and filterable, get_theme_file_path should be used everywhere.n

[oglekler]

Great, thank you, @spacedmonkey 🙏 The PR looks very straight forward. @flixos90 please take a look 🙏 🙏 🙏 😅

[spacedmonkey]

In 56073:

Themes: Use get_theme_file_path() in wp_theme_has_theme_json().

Ensure that all places where theme.json is included, use get_theme_file_path or WP_Theme->get_file_path, so that the path is run through theme_file_path filter. This change also means that the method get_file_path_from_theme can be deprecated, as it is no longer used in core.

Props flixos90, spacedmonkey, costdev, johnbillion, oglekler, hellofromtonya, mukesh27, audrasjb, oandregal.
Fixes #57629.

[SergeyBiryukov]

This appears to cause a fatal error in wp-admin/load-styles.php if SCRIPT_DEBUG is disabled:

Fatal error:  Uncaught Error: Call to undefined function get_theme_file_path() in wp-includes/global-styles-and-settings.php:411

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

[oglekler]

Loaded functions should not depend on whether global debug constants are defined or not 🤔 If this is the case, we need to solve this too. Am I missing something architectural?

[SergeyBiryukov]

Replying to oglekler:

Loaded functions should not depend on whether global debug constants are defined or not 🤔 If this is the case, we need to solve this too. Am I missing something architectural?

wp-admin/load-styles.php was introduced in [10357] / #8628 and is specifically designed to be as lightweight as possible, so it skips loading most of WordPress core files, except for script-loader.php and very few dependencies.

It may be retired in the future as per #57548, but we're not quite there yet :)

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

[spacedmonkey]

Replying to SergeyBiryukov:

Replying to oglekler:

Loaded functions should not depend on whether global debug constants are defined or not 🤔 If this is the case, we need to solve this too. Am I missing something architectural?

wp-admin/load-styles.php was introduced in [10357] / #8628 and is specifically designed to be as lightweight as possible, so it skips loading most of WordPress core files, except for script-loader.php and very few dependencies.

It may be retired in the future as per #57548, but we're not quite there yet :)

Good spot @SergeyBiryukov . Here is a fix ​https://github.com/WordPress/wordpress-develop/pull/4727 . It is simple.

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' );
        }

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 🙂

@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.

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 🙂

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 ?

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.

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

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.

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

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.

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

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

​https://github.com/WordPress/wordpress-develop/pull/4730

[peterwilsoncc]

I think ​solution two looks to be the safest and lightest touch approach.

[peterwilsoncc]

In 56085:

Script Loader: Prevent fatal error in load-styles.php.

Duplicates the code of get_theme_file_path() within wp_theme_has_theme_json(). This is to account for the load-styles.php context in which the former function is unavailable.

Follow up to [56073].

Props SergeyBiryukov, oglekler, spacedmonkey, joemcgill, dd32.
Fixes #57629.

Committed r56085 / ​https://github.com/WordPress/wordpress-develop/actions/runs/5397736741

[spacedmonkey]

@peterwilsoncc @joemcgill @SergeyBiryukov

The current commit is a little wasteful. One a normal theme ( without a parent ), it will do two file_exists. Imagine this.

A theme without a theme.json.
Calls file_exists( get_stylesheet_directory() . '/theme.json' ) , return false,
Then set $path = get_template_directory() . '/theme.json'; and does another file exists.
Calls if ( file_exists( $path ) ) {.

This is two file_exists, which can be expensive.

In my PR.

        $stylesheet_directory = get_stylesheet_directory();
        $template_directory   = get_template_directory();
        $directories          = array( $stylesheet_directory );
        if ( $stylesheet_directory !== $template_directory ) {
                $directories[] = $template_directory;
        }

        $theme_has_support = false;
        foreach ( $directories as $directory ) {
                /** This filter is documented in wp-includes/link-template.php */
                $file_path = apply_filters( 'theme_file_path', $directory . '/theme.json', 'theme.json' );
                // Does the theme have its own theme.json?
                $theme_has_support = is_readable( $file_path );
                if ( $theme_has_support ) {
                        break;
                }
        }

This only loops around options if then is a child then, otherwise do one loop.

Another solution would be doing this.

        $stylesheet_directory = get_stylesheet_directory();
        $template_directory   = get_template_directory();
        // This is the same as get_theme_file_path(), which isn't available in load-styles.php context
        if ( $stylesheet_directory !== $template_directory && file_exists( $stylesheet_directory . '/theme.json' ) ) {
                $path = $stylesheet_directory . '/theme.json';
        } else {
                $path = $template_directory . '/theme.json';
        }

Also a nitpic.

This

if ( file_exists( $path ) ) {
    $theme_has_support = true;
} else {
    $theme_has_support = false;
}

Should be this.

$theme_has_support = file_exists( $path );

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

[spacedmonkey]

I have a PR ready for review - ​https://github.com/WordPress/wordpress-develop/pull/4737.

[spacedmonkey]

In 56094:

Themes: Use improved support for child themes in wp_theme_has_theme_json().

Improve logic in wp_theme_has_theme_json(), to only check to see if theme.json exists in child theme, if the current theme is a child theme. Without this change, file_exists would be called twice on every request, which is wasteful.

Follow up to [56085].

Props spacedmonkey, SergeyBiryukov.
Fixes #57629.

Committed in r56094