FSE: Verify if php template exists for a hybrid/universal theme with a block-based parent #31123
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
MaggieCabrera
requested review from
spacedmonkey and
TimothyBJacobs
as code owners
|
Size Change: -163 kB (-11%) 👏 Total Size: 1.31 MB
ℹ️ View Unchanged
|
aristath
reviewed
Apr 23, 2021
aristath
approved these changes
Apr 23, 2021
draganescu
reviewed
Apr 26, 2021
| // Use block template if there is no php template exists OR it's a child theme with no corresponding block template. | ||
| if ( ! file_exists( get_stylesheet_directory() . '/' . $template_type . '.php' ) || ( ! is_child_theme() && null !== _gutenberg_get_template_file( 'wp_template', $template_type ) ) ) { | ||
| add_filter( str_replace( '-', '', $template_type ) . '_template', 'gutenberg_override_query_template', 20, 3 ); | ||
| } |
There was a problem hiding this comment.
For better legibility, I would rewrite this as:
$has_php_template = file_exists( get_stylesheet_directory() . '/' . $template_type . '.php' );
$has_block_template = null !== _gutenberg_get_template_file( 'wp_template', $template_type );
if ( ! $has_php_template || ( ! is_child_theme() && $has_block_template ) ) {
add_filter( str_replace( '-', '', $template_type ) . '_template', 'gutenberg_override_query_template', 20, 3 );
}
|
It feels the change should be in the default filter function |
Yeah, I just thought it would be better to just not run the filter at all if it wasn't needed, but in trying to refactor inside the override function I found that the "has_block_template" check wasn't correct and it ended up being a bit more verbose, so I moved it inside |
scruffian
reviewed
Apr 28, 2021
|
|
||
| // if the theme is a child theme we want to check if a php template exists | ||
| // and that a corresponding block template from the theme and not the parent doesn't exist | ||
| $has_php_template = file_exists( get_stylesheet_directory() . '/' . $type . '.php' ); |
There was a problem hiding this comment.
is this a better option than using $template?
There was a problem hiding this comment.
$template will pull from the parent if the child doesn't have a template defined for a specific page since it uses locate_template(). With get_stylesheet_directory() we will be sure the path is not from the parent.
scruffian
reviewed
Apr 28, 2021
| $has_block_template = false; | ||
| $block_template = _gutenberg_get_template_file( 'wp_template', $type ); | ||
| if ( $block_template !== null && $block_template['theme'] == wp_get_theme()->get_stylesheet() ) { | ||
| $has_block_template = true; |
There was a problem hiding this comment.
I wonder if lines 77-81 would be better encapsulated in a separate function?
There was a problem hiding this comment.
I thought about but I didn't do it because I thought it was very unlikely that such a function would be reused since it would be very specific. And the rest of the gutenberg_override_query_template function is using similar checks so I thought this way was a bit more consistent. I'm happy to do it but I'm not sure what we gain from doing so.
scruffian
approved these changes
Apr 28, 2021
|
Ok let's bring this in then! |
7 tasks
This was referenced May 4, 2021
ockham
added a commit
that referenced
this pull request
May 12, 2021
The block template resolution algorithm is quite complex, as it has to cover quite a lot of different possible cases resulting from various combinations of - WP's "old" template hierarchy (including custom page templates, and child themes) - block templates. Things become especially complex when there are e.g. "oldschool" PHP templates with higher specificity than the available block templates; or when a child theme has a PHP template with equal specificity as the parent theme's corresponding block template. For more discussion on this, see #31399. Examples of previous iterations that sought to refine the algorithms' behavior include #29026, #30599, #31123, and #31336. This PR's goal is to eventually cover all of those cases.
|
We've now backported the block template resolution algorithm to Core. Unfortunately, when backporting the corresponding unit tests, we discovered that there's been a regression with the logic responsible for making sure that a child theme's PHP template takes precedence over its parent's block template, if both have equal specificity. (I likely introduced that regression myself as part of #31604 (or one of its follow-up PRs) -- it went unnoticed since I tried to cover it with a unit test (#31498, which passed -- but unbeknownst to me, only because of a faulty test setup.) Anyway, I've filed a Core ticket for the issue: https://core.trac.wordpress.org/ticket/54515 (since the relevant code now lives in Core). We can discuss potential solutions there. |
This was referenced Nov 27, 2021
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description
If a theme is a child of a block-based parent theme but has php templates for some of its pages, the php templates from the child won't override the html ones from the parent theme. This PR makes sure that if a php template exists on the child theme and no html template exists, the php one will be loaded over the parent's.
How has this been tested?
This has been tested with a child of BCB. BCB provides FSE templates for singular, 404, and search. I created a child with a 404.php template. Before my PR, the template from BCB would load. After my PR, the php one from the child will load.
I also tested creating a page + page-ID on the child theme, making sure that the page-ID one would always take priority over the page one.
Types of changes
Bug fix (non-breaking change which fixes an issue)
Checklist:
*.native.jsfiles for terms that need renaming or removal).