Edit Site: Add multiple template loading #19141
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,3 @@ | ||
| <!-- wp:paragraph --> | ||
| <p>Header Template Part</p> | ||
| <!-- /wp:paragraph --> |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -28,7 +28,12 @@ class="edit-site" | |
| * @param string $hook Page. | ||
| */ | ||
| function gutenberg_edit_site_init( $hook ) { | ||
| global | ||
| $_wp_current_template_id, | ||
|
There was a problem hiding this comment. Curious why do we need the globals here, given that these are not coming from core WP, but are just something that's been introduced for site editor (in There was a problem hiding this comment. It's how we hijack template resolution to resolve block templates. |
||
| $_wp_current_template_name, | ||
| $_wp_current_template_content, | ||
| $_wp_current_template_hierarchy, | ||
| $_wp_current_template_part_ids; | ||
| if ( 'gutenberg_page_gutenberg-edit-site' !== $hook ) { | ||
| return; | ||
| } | ||
|
|
@@ -74,11 +79,53 @@ function gutenberg_edit_site_init( $hook ) { | |
| $settings['fontSizes'] = $font_sizes; | ||
| } | ||
|
|
||
| // Get all templates by triggering `./template-loader.php`'s logic. | ||
| $template_getters = array( | ||
|
There was a problem hiding this comment. Are we intentionally missing gutenberg/lib/template-loader.php Lines 23 to 41 in 56c043c In which case the call below becomes something like: call_user_func( 'get_' . $template_type . '_template' );There was a problem hiding this comment. This needs to match the code from Core template resolution; I think it would be premature to abstract it like that right now. |
||
| 'get_embed_template', | ||
| 'get_404_template', | ||
| 'get_search_template', | ||
| 'get_home_template', | ||
| 'get_privacy_policy_template', | ||
| 'get_post_type_archive_template', | ||
| 'get_taxonomy_template', | ||
| 'get_attachment_template', | ||
| 'get_single_template', | ||
| 'get_page_template', | ||
| 'get_singular_template', | ||
| 'get_category_template', | ||
| 'get_tag_template', | ||
| 'get_author_template', | ||
| 'get_date_template', | ||
| 'get_archive_template', | ||
| ); | ||
| $template_ids = array(); | ||
| $template_part_ids = array(); | ||
| foreach ( $template_getters as $template_getter ) { | ||
| call_user_func( $template_getter ); | ||
| apply_filters( 'template_include', null ); | ||
|
There was a problem hiding this comment. Should we document this filter according to WordPress documentation standards? It might make this logic easier to grok. There was a problem hiding this comment. But we are not following that convention in the rest of the plugin, are we? There was a problem hiding this comment. Now that you mention it, I actually don't remember seeing it around. |
||
| if ( isset( $_wp_current_template_id ) ) { | ||
| $template_ids[ $_wp_current_template_name ] = $_wp_current_template_id; | ||
| } | ||
| if ( isset( $_wp_current_template_part_ids ) ) { | ||
| $template_part_ids = $template_part_ids + $_wp_current_template_part_ids; | ||
| } | ||
| $_wp_current_template_id = null; | ||
| $_wp_current_template_name = null; | ||
| $_wp_current_template_content = null; | ||
| $_wp_current_template_hierarchy = null; | ||
| $_wp_current_template_part_ids = null; | ||
| } | ||
|
There was a problem hiding this comment. It would be cool to extract all the logic here into a utility function There was a problem hiding this comment. Good idea. It might be useful for plugin developers as well. There was a problem hiding this comment. Should we do this as a follow-up? |
||
| get_front_page_template(); | ||
| get_index_template(); | ||
| apply_filters( 'template_include', null ); | ||
| $template_ids[ $_wp_current_template_name ] = $_wp_current_template_id; | ||
| if ( isset( $_wp_current_template_part_ids ) ) { | ||
| $template_part_ids = $template_part_ids + $_wp_current_template_part_ids; | ||
| } | ||
| $settings['templateId'] = $_wp_current_template_id; | ||
| $settings['templateType'] = 'wp_template'; | ||
| $settings['templateIds'] = array_values( $template_ids ); | ||
| $settings['templatePartIds'] = array_values( $template_part_ids ); | ||
|
|
||
| // This is so other parts of the code can hook their own settings. | ||
| // Example: Global Styles. | ||
|
|
||
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.
Do we need the theme slug here? It'd be good if it could be implicit to the "current theme" as set in options.
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.
It's so that customized template parts can persist after switching themes. Different themes could both define headers.
Do you mean to make it default to the current theme if not set? We can do that, but we would need to save it on customized templates so that they don't load a different theme's template parts after a theme switch. It seems like a lot of additional implicit behaviors that might confuse people. I'm not sure saving one attribute in theme files is worth 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.
It's a weird requirement to have just for the sake of switching themes. It should be possible to handle it at the time of switching themes as well (also depending on what the UX looks like for what you want to preserve and what you want to replace) since we know both the previous and after contexts. Also not even sure that we want the concept of mixed parts between themes, I'd lean more towards a split of just
current themeanduser templates.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 is the implicit behavior I mentioned could be confusing.
It would be confusing if, after switching themes, those
user templatesstarted referencing different template parts, hence why we require the attribute after customization.We could default to the theme that originally created each
user template, but then how does the user add new template parts to existinguser templates.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 also didn't expect to see the theme slug here, since I interpreted these demo templates as a generic fallback that should work with any given theme.
Having said that, I do agree with this part in general. It just didn't seem necessary to me in this particular case. Would it make sense to persist it only once the demo template part has been modified?
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.
What if the user modifies the template, without touching the part, switches themes, and then ends up with a different template part reference. There are too many edge cases like this where we lose with nothing to win.