FSE: Simplify template loader logic #31604
Merged
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
|
Size Change: 0 B Total Size: 1.32 MB ℹ️ View Unchanged
|
ockham
commented
May 7, 2021
Comment on lines
-109
to
-115
| if ( | ||
| ! $is_custom_template && | ||
| $current_template_slug !== $current_block_template_slug && | ||
| $current_template_slug === $template_item_slug | ||
| ) { | ||
| return $template; | ||
| } |
There was a problem hiding this comment.
We're iterating over candidate $templates.
- The slug for the fallback PHP
$templateis in$current_template_slug. - The slug for the best block template match for candidate
$templatesis in$current_block_template_slug. - The slug for the current item in the
$templatesiteration loop (without.phpsuffix) is in$template_item_slug.
We return the PHP $template (1.) rather than the block template (2.) if:
- The block template part of the regular template hierarchy (and not a custom page template), and
- the block and PHP templates have different slugs -- meaning they apply to different items in the candidate
$templateslist, and - the PHP template is for the item in the candidate
$templateslist that we're currently iterating over.
Room for optimization mostly comes from the fact that we only want to consider block templates with higher specificity than the candidate PHP $template (instead of the inequality check found here; the higher specificity is implicitly handled here by the loop-breaking logic in L81-L100).
ockham
requested review from
vdwijngaert,
MaggieCabrera,
aristath,
youknowriad and
a team
aristath
reviewed
May 10, 2021
youknowriad
reviewed
May 10, 2021
I'm not sure it would work, I'd need to try it out. OTOH, I'm not sure the previous code would work that way either. We might have to add an extra clause, to maybe bail early if TBH, moving this logic into Core sounds like it could end up a bit messy (especially if we want to move it e.g. into The way we're doing this now is by querying the DB relatively late in the process (in the We could of course move everything pretty much as-is over into Core, and make sure that the I'll give this some more thought; maybe I can think of a way to make it somewhat nice 🤔 |
|
We need to move this code to Core one way or the other for 5.8 https://core.trac.wordpress.org/ticket/53176 and the sooner the better :)
It seems the best way to do this in Core is to actually add the html templates to candidate list in the correct order in Core? |
Yeah, that's what I tried to say (maybe too verbosely?) But what about |
|
Also: Would an iterative approach be okay?
I'm happy to work on the migration. |
|
@ockham Yes, that definitely works for me, these PR do look good. I was just trying for us to be mindful of that :) |
|
👋 Could I maybe get approval for this one? I'd like to get it in for WP 5.8 (i.e. GB 5.7 RC1) 🙏 |
ockham
force-pushed
the
update/simplify-template-loader-logic
branch
from
381cfcc to
98a5051
Compare
aristath
approved these changes
May 13, 2021
45 tasks
Merged
7 tasks
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
Based on #31498 (which adds unit test coverage for
lib/full-site-editing/template-loader.php).Don't merge this PR before #31498!
The template loader logic has grown a bit convoluted due to the recent addition of the concept of hybrid themes (i.e. both PHP and block templates allowed in one theme) and subsequent bug fixes (some of them related to child themes, others to custom page templates). #31498 had added unit test coverage to (hopefully!) cover exemplary cases from all those categories.
This allows us to more confidently simplify (and later, if needed, further extend) the template loader algorithm.
The core idea for simplifcation in this PR is as follows:
gutenberg_override_query_template( $template, $type, $templates)is fed the results of Core's own template resolution algorithm. It provides a list of candidate$templates(e.g.array( 'page-home.php', 'page-123.php', 'page.php' )), and, if a matching template PHP file could be found in the current theme (including child and parent themes), that file's path in$template(e.g./var/www/html/wp-content/themes/test-theme/page-123.php).This means when attempting to find a block template, we iterate over
$templates, but we cut off at the entry corresponding to our fallback PHP template (in the example case, we only passarray( 'page-home.php', 'page-123.php' )togutenberg_resolve_template(the function that retrieves block templates)), since we're only interested to find block template counterparts for those candidate slugs (which have higher or equal specificity as our fallback PHP template), but not in any matches with lower specificity (e.g./var/www/html/wp-content/themes/test-theme/block-templates/page.html).The new logic is further explained in code comments. I'll add some GH comments to the old logic for more clarification.
How has this been tested?
Verify that unit tests go green in CI.
Locally: