Make WordPress Core

Opened 15 months ago

Closed 10 months ago

Last modified 10 months ago

#57629 closed enhancement (fixed)

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

Reported by: flixos90's profile flixos90 Owned by: spacedmonkey's profile spacedmonkey
Milestone: 6.3 Priority: normal
Severity: normal Version:
Component: Themes Keywords: has-patch dev-feedback
Focuses: Cc:

Description

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.

Change History (48)

This ticket was mentioned in PR #3876 on WordPress/wordpress-develop by @spacedmonkey.


15 months ago
#1

  • Keywords has-patch added

Use get_theme_file_path function to stop replicated code.

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

#2 @spacedmonkey
14 months ago

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.

#3 @costdev
14 months ago

  • Keywords dev-feedback added

@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

@spacedmonkey commented on PR #3876:


14 months ago
#4

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

@hellofromTonya commented on PR #3876:


14 months ago
#5

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.

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


14 months ago

#7 @costdev
14 months ago

  • Milestone changed from 6.2 to 6.3

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

@spacedmonkey commented on PR #3876:


11 months ago
#8

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

@oandregal commented on PR #3876:


11 months ago
#9

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?

@oandregal commented on PR #3876:


11 months ago
#10

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.

@spacedmonkey commented on PR #3876:


11 months ago
#11

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

@oandregal commented on PR #3876:


11 months ago
#12

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.

#13 @spacedmonkey
10 months ago

@flixos90 Is this good to commit?

@johnbillion commented on PR #3876:


10 months ago
#14

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.

#15 @johnbillion
10 months ago

@flixos90 Can you give this the final nod?

#16 @spacedmonkey
10 months ago

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

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


10 months ago

#18 @oglekler
10 months ago

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

#19 @spacedmonkey
10 months ago

@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

#20 @oglekler
10 months ago

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

#21 @spacedmonkey
10 months ago

  • Resolution set to fixed
  • Status changed from assigned to closed

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.

#22 @SergeyBiryukov
10 months ago

  • Resolution fixed deleted
  • Status changed from closed to reopened

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

#23 follow-up: @oglekler
10 months ago

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?

#24 in reply to: ↑ 23 ; follow-up: @SergeyBiryukov
10 months ago

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

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

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


10 months ago

#27 in reply to: ↑ 24 @spacedmonkey
10 months ago

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.

joemcgill commented on PR #4727:


10 months ago
#28

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 commented on PR #4727:


10 months ago
#29

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 commented on PR #4727:


10 months ago
#30

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

@SergeyBiryukov commented on PR #4727:


10 months ago
#31

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 commented on PR #4727:


10 months ago
#32

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 commented on PR #4727:


10 months ago
#33

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 commented on PR #4727:


10 months ago
#34

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

@spacedmonkey commented on PR #4727:


10 months ago
#35

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 commented on PR #4727:


10 months ago
#36

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 commented on PR #4727:


10 months ago
#37

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 commented on PR #4727:


10 months ago
#39

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

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

#40 @peterwilsoncc
10 months ago

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

#41 @peterwilsoncc
10 months ago

  • Resolution set to fixed
  • Status changed from reopened to closed

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.

#43 @spacedmonkey
10 months ago

@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 );
Last edited 10 months ago by spacedmonkey (previous) (diff)

#44 @spacedmonkey
10 months ago

  • Resolution fixed deleted
  • Status changed from closed to reopened

#47 @spacedmonkey
10 months ago

  • Resolution set to fixed
  • Status changed from reopened to closed

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.

@peterwilsoncc commented on PR #4737:


10 months ago
#48

Committed in r56094

Note: See TracTickets for help on using tickets.