Opened 3 years ago
Closed 3 years ago
#54552 closed task (blessed) (fixed)
Consider consolidating wp_is_block_template_theme() and WP_Theme::is_block_based()
Reported by: | noisysocks | Owned by: | hellofromTonya |
---|---|---|---|
Milestone: | 5.9 | Priority: | normal |
Severity: | normal | Version: | |
Component: | Themes | Keywords: | has-patch commit |
Focuses: | Cc: |
Description
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
towp_is_block_based_theme
so that the two methods are aligned?
Change History (13)
#2
@
3 years ago
- Keywords needs-patch added
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
- Remove
wp_is_block_template_theme()
- Rename the
WP_Theme::is_block_based()
toWP_Theme::is_block_theme()
- Replace all instances of
wp_is_block_template_theme()
withwp_get_theme()->is_block_theme()
#3
follow-up:
↓ 4
@
3 years ago
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:
- 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.
- If we decide to keep
wp_is_block_template_theme
we should rename it towp_is_block_theme
.
- If we decided to rename
WP_Theme::is_block_based()
method, it should not havetheme
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
#4
in reply to:
↑ 3
@
3 years ago
Replying to antonvlasenko:
When replacing
wp_is_block_template_theme
withwp_get_theme()→is_block_based()
, we must take into account thatget_stylesheet_directory
andget_template_directory
functions use filters (get_stylesheet_directory
andtemplate_directory
filters respectively).
These functions are used bywp_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 withinget_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 inget_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 inget_stylesheet_directory()
'theme_root'
which is invoked inget_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.
#5
follow-up:
↓ 6
@
3 years ago
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.
#6
in reply to:
↑ 5
@
3 years ago
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?
#7
@
3 years ago
Does that make sense?
Yes, it does.
Thank you for explaining, @hellofromTonya.
I'm working on a patch.
This ticket was mentioned in PR #2014 on WordPress/wordpress-develop by anton-vlasenko.
3 years ago
#8
- Keywords has-patch has-unit-tests added; needs-patch removed
Trac ticket: https://core.trac.wordpress.org/ticket/54552
#9
@
3 years ago
- Keywords has-unit-tests removed
Steps to test the PR:
- Since we have unit tests that cover these functions, I think it's enough to make sure that the tests pass.
- Additionally, we can activate Twenty Twenty-One and Twenty Twenty-Two themes and make sure they get loaded.
#10
@
3 years ago
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.
#11
@
3 years ago
- Keywords commit added
- Owner set to hellofromTonya
- Status changed from new to reviewing
PR 2014 is approved. Self-assigning for testing and commit.
hellofromtonya commented on PR #2014:
3 years ago
#12
Committed via https://core.trac.wordpress.org/changeset/52330.
#13
@
3 years ago
- Resolution set to fixed
- Status changed from reviewing to closed
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()
toWP_Theme::is_block_theme()
wp_is_block_template_theme()
towp_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:
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?
wp_is_block_template_theme()
, by design, will only evaluate the current theme.Is a 'helper' method needed?
wp_is_block_template_theme()
is a maintenance overhead - path updates would require changes inWP_Theme::is_block_based()
andwp_is_block-template_theme()
.WP_Theme::is_block_based()
is already apublic
method.Is a 'helper' method worth having?
wp_get_theme()
in Core, including 24 in Bundled Themes. Without additional 'helpers' for otherWP_Theme
methods, this would result usingwp_is_block_based_theme()
to check if it's block based, andwp_get_theme()->
for everything else.wp_get_theme->is_block_based()
directly and the ultimate deprecation ofwp_is_block_based_theme()
.