-
Notifications
You must be signed in to change notification settings - Fork 4.2k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Template Loader: Introduce get_template_hierarchy(), drop gutenberg_template_include_filter() #21980
Template Loader: Introduce get_template_hierarchy(), drop gutenberg_template_include_filter() #21980
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 <epiqueras@users.noreply.github.com>
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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/
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
$template_hierarchy = array_merge( get_template_hierachy( $template_type ), get_template_hierachy( 'index' ) ); |
I'll have to dig into why the index
fallback isn't working.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fix: #22954
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_template
filter was added to${type}_template
. It served two purposed:_wp_current_template_hierarchy
global.gutenberg_template_include_filter
filter 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.$settings
variable (which is passed to the client),edit-site-page.php
iterated over all template getters, calling each one of them, in order to trigger the${type}_template
filters (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_template
anymore (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_id
and_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_ids
global, 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: