Consider consolidating wp_is_block_template_theme() and WP_Theme::is_block_based()

[noisysocks]

We have two pretty similar methods for determining if the active theme is a block theme:

wp_get_theme()→is_block_based()

wp_is_block_template_theme()

Both were added in WP 5.9 so it's now or never if we want to simplify this API.

• Should we have one and not the other?
• Should we rename wp_is_block_template_theme to wp_is_block_based_theme so that the two methods are aligned?

[costdev]

What's the difference?

wp_is_block_template_theme()
Evaluates the current theme.

WP_Theme::is_block_based()
Evaluates whichever theme is returned from, for example:

• new WP_Theme( $stylesheet, $theme_root )
• wp_get_theme( $stylesheet, $theme_root )

Is a class method needed?

• If we are going to have this functionality in only one place, the class itself makes the most sense to me.
• wp_is_block_template_theme(), by design, will only evaluate the current theme.

Is a 'helper' method needed?

• The existing wp_is_block_template_theme() is a maintenance overhead - path updates would require changes in both WP_Theme::is_block_based() and wp_is_block_template_theme().
• WP_Theme::is_block_based() is already a public method.
• If we do want a 'helper' method for the current theme, then I'd agree that we should rename the 'helper' method so that they are aligned.
• I don't think a 'helper' method needs to be anything more than this:

<?php

/**
 * Returns whether the current theme is a block-based theme or not.
 *
 * @since 5.9.0
 *
 * @return boolean Whether the current theme is a block-based theme or not.
 */
function wp_is_block_based_theme() {
        return wp_get_theme()->is_block_based();
}

Is a 'helper' method worth having?

• This is a minimal and arguable improvement in readability.
• There are currently 122 instances of wp_get_theme() in Core, including 24 in Bundled Themes. Without additional 'helpers' for other WP_Theme methods, this would result using wp_is_block_based_theme() to check if it's block based, and wp_get_theme()-> for everything else.
• IMO, this is likely going to result in using wp_get_theme->is_block_based() directly and the ultimate deprecation of wp_is_block_based_theme().

[hellofromTonya]

Thanks @noisysocks for opening this ticket to discuss the consolidation and naming for what's needed not only for 5.9 but for the future.

This is a follow-up to [52279].

Consolidate?

What's used in Core?

Let's explore how Core uses functionality to get information about a theme. @costdev gave a good inside look. I'd like to add to his comment.

get_theme_file_path() is used by themes to get its file path. Core itself uses it in only 3 places: twice in wp_is_block_template_theme() and once in get_block_editor_theme_styles().

How does Core get information about the current theme?

It uses helper functions (such as get_stylesheet() and get_stylesheet_directory()) and/or wp_get_theme() which uses get_stylesheet() to default to the current theme. Which is primarily used? wp_get_theme() is primarily used with a fallback to the helper functions.

My thoughts

I think the helper global function wp_is_block_template_theme() is not needed and is redundant. It's usages can be replaced with wp_get_theme()->is_block_based(). By not passing a stylesheet name, wp_get_theme() will default to the current theme. Therefore, it is doing the same thing as wp_is_block_template_theme().

Is the method name the right name?

Is "block-based" themes the right now? Isn't the new terminology more aligned with block theme whereas "block-based" themes term was before FSE. Is yes, then I'd suggest changing the method name to is_block_theme() which would change invoking it to

wp_get_theme()->is_block_theme()

@noisysocks @antonvlasenko @costdev what do you think about renaming the method?

My vote

1. Remove wp_is_block_template_theme()
2. Rename the WP_Theme::is_block_based() to WP_Theme::is_block_theme()
3. Replace all instances of wp_is_block_template_theme() with wp_get_theme()->is_block_theme()

[antonvlasenko]

When replacing wp_is_block_template_theme with wp_get_theme()→is_block_based(), we must take into account that get_stylesheet_directory and get_template_directory functions use filters (stylesheet_directory and template_directory filters respectively).
These functions are used by wp_is_block_template_theme function internally.
While WP_Theme::get_stylesheet_directory and WP_Theme::get_template_directory methods don't use any filters (used by WP_Theme::is_block_based.
So, there could be some inconsistency in how these methods behave.
Should we add these filters to WP_Theme::get_stylesheet_directory and WP_Theme::get_template_directory methods as well?

My vote:

1. I don't mind removing wp_is_block_template_theme.

On the other hand, It's handy to call just 1 function (wp_is_block_template_theme) instead of using wp_get_theme()→is_block_based(). It's kind of syntactic sugar.

2. If we decide to keep wp_is_block_template_theme we should rename it towp_is_block_theme .

3. If we decided to rename WP_Theme::is_block_based() method, it should not have theme in it.

Check this code snippet out:

$theme = new WP_Theme();
$theme->is_block_theme(); // <- this

We are repeating the name of the class (theme) in the method's name. It looks redundant to me.

IMO the current name is better (no tautology).

$theme = new WP_Theme();
$theme->is_block_based(); // <- this

[hellofromTonya]

Replying to antonvlasenko:

When replacing wp_is_block_template_theme with wp_get_theme()→is_block_based(), we must take into account that get_stylesheet_directory and get_template_directory functions use filters (get_stylesheet_directory and template_directory filters respectively).
These functions are used by wp_is_block_template_theme function internally.

Yes, but wp_get_theme() is also filterable as:

• it invokes get_stylesheet() which is filtered through 'stylesheet' which is also invoked within get_stylesheet_directory()
• and it invokes get_option() which is also filterable
• and WP_Theme::get_file_path() invokes the same 'theme_file_path' filter as in get_theme_file_path().

This is why using wp_get_theme() instead of new WP_Theme() ensures the filters are available.

Filters that are not invoked with the wp_get_theme()->is_block_based() approach are:

• 'stylesheet_directory' which is in get_stylesheet_directory()
• 'theme_root' which is invoked in get_theme_root()

However, the combination of these 2 filters is available through the 'theme_file_path' filter.

The question then is this: Does any of the Gutenberg code dependent upon the 'stylesheet_directory' and/or 'theme_root' filters? Searching the codebase, I did not find any code in Core that is hooked into either of these filters.

So I think it's safe to remove the wrapper function. It don't think it adds value. And once it's in a final release, it has to be maintained for backwards-compatibility. Unless there's a compelling reason to add it, removing it reduces maintenance and documentation costs/effort.

[antonvlasenko]

Thank you for taking the time to explain this, @hellofromTonya ! I should have checked the wp_get_theme function.

The question then is this: Does any of the Gutenberg code dependent upon the 'stylesheet_directory' and/or 'theme_root' filters? Searching the codebase, I did not find any code in Core that is hooked into either of these filters.

I cannot find any of the Gutenberg code that uses these filters. There is only one PHPUnit test that uses theme_root filter (I guess we don't care about that).
But there could be some 3rd party plugins that depend on these 2 filters.

However, the combination of these 2 filters is available through the 'theme_file_path' filter.

I'm sorry, I don't understand what you mean by available through the 'theme_file_path' filter.
Probably I'm missing something.
I thought that filters are independent of each other.

[hellofromTonya]

Replying to antonvlasenko:

However, the combination of these 2 filters is available through the 'theme_file_path' filter.

I'm sorry, I don't understand what you mean by available through the 'theme_file_path' filter.
Probably I'm missing something.
I thought that filters are independent of each other.

I'm sorry for the confusion. I'll try to explain. Please excuse me if I over-explain something that you already know.

'theme_file_path' filter event is fired 2 separate places: get_theme_file_path() and the new WP_Theme::get_file_path() method. Either of those filter events will call all callbacks registered/hooked to it.

What I meant by the "combination of these 2 filters" is this:

'theme_file_path' filter exposes the file's path ("combination") whereas the other 2 filters expose the pieces of the file's path, i.e. the theme's directory name and the root path to that directory. Using 'theme_file_path' gives the ability for plugins and themes to gain access.

Does that make sense?

[antonvlasenko]

Does that make sense?

Yes, it does.
Thank you for explaining, @hellofromTonya.
I'm working on a patch.

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

[antonvlasenko]

Steps to test ​the PR:

1. Since we have unit tests that cover these functions, I think it's enough to make sure that the tests pass.
2. Additionally, we can activate Twenty Twenty-One and Twenty Twenty-Two themes and make sure they get loaded.

[hellofromTonya]

Sharing conversations I had with the Gutenberg team:

The wrapper helper function will remain as it's needed for WordPress cross-version support in the Gutenberg plugin and to make it easier to backport changes from Gutenberg to Core. Extenders may also find this function helpful.

[hellofromTonya]

​PR 2014 is approved. Self-assigning for testing and commit.

Committed via https://core.trac.wordpress.org/changeset/52330.

[hellofromTonya]

In 52330:
Themes: Better names for WP_Theme::is_block_theme() and wp_is_block_theme() and make wp_is_block_theme() a wrapper.

This commit renames the following method and function to better represent block theme terminology:

• WP_Theme::is_block_based() to WP_Theme::is_block_theme()
• wp_is_block_template_theme() to wp_is_block_theme()

It also changes wp_is_block_theme() to be a helper wrapper (sugar syntax) for wp_get_theme()->is_block_theme();. Why? To ensure both the method and function behave the same, to help Gutenberg maintain WordPress cross-version compatibility, and to make it less cumbersome to port changes from Gutenberg to Core.

Follow-up to [52069], [52247], [52279].

Props antonvlasenko, costdev, hellofromTonya, noisysocks.
Fixes #54550.

Note: I made an error and the Fixes should have been this ticket. :facepalm: