Template Loader: Introduce get_template_hierarchy(), drop gutenberg_template_include_filter() #21980
Conversation
ockham
requested review from
vindl,
felixarntz,
noahtallen,
epiqueras and
Addison-Stavlo
There was a problem hiding this comment.
This is looking great.
We should add tests for these new functions if possible now that we are formalizing the behavior.
I think the next step should be extracting get_template_ids and get_template_part_ids from gutenberg_edit_site_init so that we can access them through a REST endpoint and for unblocking things like #21958.
lib/edit-site-page.php
Outdated
| } | ||
| $settings['templateId'] = $current_template_post->ID; | ||
|
|
||
| $current_template_id = $template_ids['index'] ?? $template_ids['front-page']; |
There was a problem hiding this comment.
I don't think we should load both the index template and the front-page template under any circumstances.
I mirrored Core's template-loader here:
|
Size Change: +135 B (0%) Total Size: 821 kB
ℹ️ View Unchanged
|
Thanks @epiqueras! I tried to get PHP unit tests to run locally but unfortunately wasn't able to. I looked a bit into the PR that brings them into I can either cycle back to this next week, or you can merge on my behalf, and I'll add unit tests later. |
|
We can follow-up with the tests, but I am still concerned about this, #21980 (comment). My original idea was to resolve every template that can be resolved through all the Core post types. This PR loads |
Ah, my bad, I forgot to address that comment!
Will look into this! |
|
I think I need a bit more clarification on this 😅
The previous behavior reset the To restore this behavior, the following patch would probably be enough: diff --git a/lib/edit-site-page.php b/lib/edit-site-page.php
index acc8f1f4cf..5d53ce770e 100644
--- a/lib/edit-site-page.php
+++ b/lib/edit-site-page.php
@@ -146,7 +146,6 @@ function gutenberg_edit_site_init( $hook ) {
}
$template_types = array(
- 'index',
'404',
'archive',
'author',
@@ -156,7 +155,6 @@ function gutenberg_edit_site_init( $hook ) {
'date',
// Skip 'embed' for now because it is not a regular template type.
'home',
- 'front-page',
'privacy-policy',
'page',
'search',
@@ -180,9 +178,10 @@ function gutenberg_edit_site_init( $hook ) {
$_wp_current_template_part_ids = null;
}
- $current_template_id = $template_ids['index'] ?? $template_ids['front-page'];
+ $front_page_template_hierarchy = array_merge( get_template_hierachy( 'front-page' ), get_template_hierachy( 'index' ) );
+ $front_page_template_post = gutenberg_find_template_post( $template_hierarchy );
- $settings['templateId'] = $current_template_id;
+ $settings['templateId'] = $front_page_template_post->ID;
$settings['templateType'] = 'wp_template';
$settings['templateIds'] = array_values( $template_ids );
$settings['templatePartIds'] = array_values( $template_part_ids );
@@ -200,7 +199,7 @@ function gutenberg_edit_site_init( $hook ) {
'/wp/v2/types?context=edit',
'/wp/v2/taxonomies?per_page=-1&context=edit',
'/wp/v2/themes?status=active',
- sprintf( '/wp/v2/templates/%s?context=edit', $current_template_id ),
+ sprintf( '/wp/v2/templates/%s?context=edit', $front_page_template_post->ID ),
array( '/wp/v2/media', 'OPTIONS' ),
);
$preload_data = array_reduce(However, it seems like you might be saying that we should actually return the first template post in the list (ordered as the original getters list was) for which |
|
Basically, Because in our use case, we are kind of simulating visits to every template type, we should only load array_merge( get_template_hierachy( $template_type ), get_template_hierachy( 'index' ) );That would be the template hierarchy in every iteration of the loop. |
…emplate, drop gutenberg_template_include_filter
Co-Authored-By: Enrique Piqueras <[email protected]>
ockham
force-pushed
the
update/template-loader-no-template-hierarchy-global
branch
from
aeb7a04
to
3f199c6
Compare
|
Addressed feeback -- this should be ready for another look 🙂 |
| $settings['templateId'] = $current_template_post->ID; | ||
| $settings['homeTemplateId'] = $current_template_post->ID; | ||
|
|
||
| $current_template_id = $template_ids['front-page']; |
There was a problem hiding this comment.
Right now, if the block-based theme doesn't have a template file called front-page.html it throws a 404 when trying to load the page. Could it be related to this somehow?
There was a problem hiding this comment.
Found my way back here as well 😄 It's pretty evident in the default twentytwenty theme, which doesn't have a front-page template.
https://github.com/WordPress/twentytwenty
This causes the Site Editor to not load at all.
Seems it should be resilient to the absence of this template, possibly falling back to another applicable template?
e.g. front-page, home, index
https://developer.wordpress.org/files/2014/10/Screenshot-2019-01-23-00.20.04.png
https://developer.wordpress.org/themes/basics/template-hierarchy/
There was a problem hiding this comment.
That's possible I'm afraid 😬
I'm still touching this file pretty regularly, will look into fixing it 👍
There was a problem hiding this comment.
Seems it should be resilient to the absence of this template, possibly falling back to another applicable template?
e.g.
front-page,home,indexhttps://developer.wordpress.org/files/2014/10/Screenshot-2019-01-23-00.20.04.png
https://developer.wordpress.org/themes/basics/template-hierarchy/
Yeah, I'm painfully aware of the template hierarchy 😬 The function that's used to populate $template_ids actually falls back to index, see
gutenberg/lib/template-loader.php
Line 226 in 19fb4ae
I'll have to dig into why the index fallback isn't working.
Description
Previously, the template loader made gratuitous use of filters and globals in order to communicate data from one function to another. Specifically:
gutenberg_override_query_templatefilter was added to${type}_template. It served two purposed:_wp_current_template_hierarchyglobal.gutenberg_template_include_filterfilter was added totemplate_include. That filter determined the relevant template post from_wp_current_template_hierarchy, and used it to render the template via the template canvas.$settingsvariable (which is passed to the client),edit-site-page.phpiterated over all template getters, calling each one of them, in order to trigger the${type}_templatefilters (and thus,gutenberg_override_query_template), to obtain the template hierarchies, and subsequently, the relevant template IDs. During each iteration, care had to be taken to evaluate and then reset the relevant globals.Due to the implicit nature of globals and filters, this is harder to follow and reason about than a call stack of functions with explicit arguments and return values.
Hence, this PR makes the following changes:
get_template_hierachy()function that determines the template hierarchy for a given template type. This encapsulates adding and removing a filter (which is still required) so it doesn't have to leak a global, as the previous approach did.edit-site-page.php(see item 3. above), getting rid of most globals involved in that file.gutenberg_override_query_templateanymore (see item 1.1 above), re-purpose it to actually render the template canvas, cutting out the now-superfluousgutenberg_template_include_filter(item 2.), which is thus removed. This still requires two globals (_wp_current_template_idand_wp_current_template_content), to communicate the template that needs to be rendered to the canvas. However, their use is much more limited.This PR still does not touch the
_wp_current_template_part_idsglobal, in order to ensure that the auto-draft saving behavior isn't changed. I'm planning to work on this in another follow-up.This is a follow-up to #21959, and another preparatory step for https://github.com/WordPress/gutenberg/pull/21877/files#r416075231.
How has this been tested?
Verify that Full Site Editing works as before.
Types of changes
Refactor for encapsulation, readability, and extensibility.
Checklist: