Use theme mods for templates and template parts #31971
Conversation
|
If you want to learn more about WordPress development in general, check out the Core Handbook full of helpful information. |
carlomanf
changed the title
carlomanf
requested review from
ajitbohra,
spacedmonkey and
TimothyBJacobs
as
code owners
|
Hey there @carlomanf, thank you for working on this PR! It does simplify things a lot and from a quick look I took, the code makes sense... |
Yes, I was just looking at that myself. I think they're failing because they are expecting the templates to be classified using the taxonomy and post_name, rather than through theme mods. I can see if I can rewrite the tests to expect the changed format. |
I think this test class is failing because it switches theme to tt1-blocks before it creates the template that is not intended for tt1-blocks, so the template gets stored with tt1-blocks anyway. Suggestions? |
…ry/fse-theme-mods # Conflicts: # lib/full-site-editing/class-gutenberg-rest-templates-controller.php
|
No idea why |
|
Just copying in a comment from elsewhere for reference:
Also see this comment for further rationale. @youknowriad @ockham Given the feature freeze, are there any amendments that should be made to this patch to make it eligible for 5.8? (for example, temporarily disabling the additional API endpoint?) |
|
I've added this to the 5.8 board to get it properly reviewed and considered. I need to take some time to dive here and understand it properly and its impacts. Also @ockham for thoughts here. I'm also pinging @TimothyBJacobs here because he shared some concerns about using theme mods for the Site logo block, while it might not be the same thing here, it's always good to have more perspective. |
When a user creates their own template or part, it does not become "part of" the theme but is associated with it. I would argue that customized templates/parts and custom created ones could be considered content.
A user isn't creating a custom template or part with the goal of further developing the theme, they are doing so to help manage their content and how it is displayed. If they were to export their site and lose all their custom created posts (templates/parts) that helps manage and display their content, would it not feel like they are 'losing content'? Their site wouldn't look or work the same, custom posts they put effort into would disappear, and the whole process from this point of view would feel broken by forcing them to recreate all that (what is essentially) custom content. |
Switch to a different theme first: diff --git a/phpunit/class-block-templates-test.php b/phpunit/class-block-templates-test.php
index f0ec20b7bd..685d3dbd2a 100644
--- a/phpunit/class-block-templates-test.php
+++ b/phpunit/class-block-templates-test.php
@@ -14,7 +14,6 @@ class Block_Templates_Test extends WP_UnitTestCase {
private static $template_part_post;
public static function wpSetUpBeforeClass() {
- switch_theme( 'tt1-blocks' );
gutenberg_register_template_post_type();
gutenberg_register_template_part_post_type();
gutenberg_register_wp_theme_taxonomy();
@@ -23,6 +22,7 @@ class Block_Templates_Test extends WP_UnitTestCase {
// Set up a template post corresponding to a different theme.
// We do this to ensure resolution and slug creation works as expected,
// even with another post of that same name present for another theme.
+ switch_theme( 'test-theme' );
$args = array(
'post_type' => 'wp_template',
'post_name' => 'my_template',
@@ -39,6 +39,7 @@ class Block_Templates_Test extends WP_UnitTestCase {
wp_set_post_terms( self::$post->ID, 'this-theme-should-not-resolve', 'wp_theme' );
// Set up template post.
+ switch_theme( 'tt1-blocks' );
$args = array(
'post_type' => 'wp_template',
'post_name' => 'my_template',We can also drop the taxonomy handling in that class; a more complete diff would be:diff --git a/phpunit/class-block-templates-test.php b/phpunit/class-block-templates-test.php
index f0ec20b7bd..5125e80345 100644
--- a/phpunit/class-block-templates-test.php
+++ b/phpunit/class-block-templates-test.php
@@ -14,45 +14,34 @@ class Block_Templates_Test extends WP_UnitTestCase {
private static $template_part_post;
public static function wpSetUpBeforeClass() {
- switch_theme( 'tt1-blocks' );
gutenberg_register_template_post_type();
gutenberg_register_template_part_post_type();
- gutenberg_register_wp_theme_taxonomy();
+ // gutenberg_register_wp_theme_taxonomy();
gutenberg_register_wp_template_part_area_taxonomy();
// Set up a template post corresponding to a different theme.
// We do this to ensure resolution and slug creation works as expected,
// even with another post of that same name present for another theme.
+ switch_theme( 'test-theme' );
$args = array(
'post_type' => 'wp_template',
'post_name' => 'my_template',
'post_title' => 'My Template',
'post_content' => 'Content',
'post_excerpt' => 'Description of my template',
- 'tax_input' => array(
- 'wp_theme' => array(
- 'this-theme-should-not-resolve',
- ),
- ),
);
self::$post = self::factory()->post->create_and_get( $args );
- wp_set_post_terms( self::$post->ID, 'this-theme-should-not-resolve', 'wp_theme' );
// Set up template post.
+ switch_theme( 'tt1-blocks' );
$args = array(
'post_type' => 'wp_template',
'post_name' => 'my_template',
'post_title' => 'My Template',
'post_content' => 'Content',
'post_excerpt' => 'Description of my template',
- 'tax_input' => array(
- 'wp_theme' => array(
- get_stylesheet(),
- ),
- ),
);
self::$post = self::factory()->post->create_and_get( $args );
- wp_set_post_terms( self::$post->ID, get_stylesheet(), 'wp_theme' );
// Set up template part post.
$template_part_args = array(
@@ -62,9 +51,6 @@ class Block_Templates_Test extends WP_UnitTestCase {
'post_content' => 'Content',
'post_excerpt' => 'Description of my template part',
'tax_input' => array(
- 'wp_theme' => array(
- get_stylesheet(),
- ),
'wp_template_part_area' => array(
WP_TEMPLATE_PART_AREA_HEADER,
),
@@ -72,7 +58,6 @@ class Block_Templates_Test extends WP_UnitTestCase {
);
self::$template_part_post = self::factory()->post->create_and_get( $template_part_args );
wp_set_post_terms( self::$template_part_post->ID, WP_TEMPLATE_PART_AREA_HEADER, 'wp_template_part_area' );
- wp_set_post_terms( self::$template_part_post->ID, get_stylesheet(), 'wp_theme' );
}
public static function wpTearDownAfterClass() { |
|
lib/full-site-editing/class-gutenberg-rest-templates-controller.php
Outdated
Show resolved
Hide resolved
|
Layman question: What does "mod" mean here? |
|
| } elseif ( 'custom' !== $template->source ) { | ||
| $changes->post_name = $template->slug; |
ockham
Jun 8, 2021
Contributor
This removal caused the unit test failure in phpunit/class-gutenberg-rest-template-controller-test.php's test_update_item(). The reason is that upon creating the new wp_template post object (with its title set to My new Index Title), we're no longer using the template (file)'s slug (index), but creating a slug based on the post title -- in this case, my-new-index-title. That slug is then used in the theme mod as the key for the newly created post object's ID -- something like
array(
'my-new-index-title` => 6
)
This means that gutenberg_get_block_template() (which is e.g. used for the network response) cannot find a wp_template post object for the slug it was given (index, via the template ID tt1-blocks//index).
I think the right fix is to add this line (and L354) back -- I don't quite see why they were removed in the first place, so I hope that adding them back won't have any ripple effects. cc/ @carlomanf
This removal caused the unit test failure in phpunit/class-gutenberg-rest-template-controller-test.php's test_update_item(). The reason is that upon creating the new wp_template post object (with its title set to My new Index Title), we're no longer using the template (file)'s slug (index), but creating a slug based on the post title -- in this case, my-new-index-title. That slug is then used in the theme mod as the key for the newly created post object's ID -- something like
array(
'my-new-index-title` => 6
)This means that gutenberg_get_block_template() (which is e.g. used for the network response) cannot find a wp_template post object for the slug it was given (index, via the template ID tt1-blocks//index).
I think the right fix is to add this line (and L354) back -- I don't quite see why they were removed in the first place, so I hope that adding them back won't have any ripple effects. cc/ @carlomanf
carlomanf
Jun 8, 2021
Author
I think I removed that because I was only thinking about creating new templates, and not the possibility of overwriting a file-based one via PUT request. I can't think of any reason not to add back those lines for the existing endpoint, but they may have been needed for the new endpoint that is not being back-ported to 5.8?
I think I removed that because I was only thinking about creating new templates, and not the possibility of overwriting a file-based one via PUT request. I can't think of any reason not to add back those lines for the existing endpoint, but they may have been needed for the new endpoint that is not being back-ported to 5.8?
carlomanf
Jun 10, 2021
Author
What about adding back this line only if true === $template->has_theme_file?
What about adding back this line only if true === $template->has_theme_file?
ockham
Jun 10, 2021
Contributor
What about adding back this line only if true === $template->has_theme_file?
I think the elseif ( 'custom' !== $template->source ) check on R343 is pretty much synonymous to that, no? (Not entirely sure about the else branch, where we also had $changes->post_name = $template->slug; 🤔 )
What about adding back this line only if
true === $template->has_theme_file?
I think the elseif ( 'custom' !== $template->source ) check on R343 is pretty much synonymous to that, no? (Not entirely sure about the else branch, where we also had $changes->post_name = $template->slug;
carlomanf
Jun 10, 2021
Author
I think it can be removed from the else branch, because it would no longer be required to stop the front-end from resolving differently as a result of a slug change.
I think it can be removed from the else branch, because it would no longer be required to stop the front-end from resolving differently as a result of a slug change.
|
Can we try to get this ready quickly enough to be able to play with for some time in the Gutenberg plugin and make an informed decision for 5.8. Also, I believe we should consider a migration for the plugin if we do this, there's a number of FSE sites and custom templates out there now. |
I went ahead and implemented most of @ockham suggestions so it's now ready to test. We will still need a solution to the export/import problem though. |
|
Thanks a lot for those latest changes, @carlomanf! I had another look at the export/import problem and talked a bit with some FSE folks (@Addison-Stavlo @jeyip @creativecoder @Copons) about it over the past couple of days. Finally, I had a chat with @youknowriad and @mtias yesterday. We decided that we're not going to include this change in Core 5.8; I'll explain our reasoning below. We're still open to considering it for later versions of WordPress. The chief reason for this decision is the amount of work that’d still be needed in order to solve the export/import problem. Per discussion with FSE folks, we’d need to fix at least the following things:
Sneaking in this PR (or the backport) as-is would’ve already been a bit of a stretch of the notion of a bug fix, but these additional items inflate the scope significantly beyond the kind of change that’s supposed to be merged after Feature Freeze. At this point, given the choice between what seems it would be a rush to design, implement, and test these changes in the weeks that remain before the release, vs. including it in a later WordPress release, at the cost of requiring a migration from taxonomies to theme mods, I find the latter preferable. Thanks again for your work, Carlo! Let’s continue the conversation about this change — just outside the scope of WP 5.8. This will also give us more time to discuss the desired behavior of theme switches (and a possible UX) in a block-template theme world — Matías pointed me to this comment where he shared thoughts on a few possible scenarios. |
|
Thanks for the comprehensive reply @ockham. I think you covered most of the concerns for import / export, but I'll add a bit more context to one point.
The reasoning behind this might be unclear, so to elaborate further:
|
Sorry for the late reply here. I don't have any specific concerns about the REST API with this issue. |
|
Is there something blocking us here? |
Not necessarily, I was just waiting to see if anyone had any ideas on how to implement the required changes. Here are my thoughts:
Is this even necessary, since I notice that these screens are not included in core. If they stay out, then it could potentially simplify this other change:
If the admin screens are disabled, then this other change could then be implemented by moving the theme mod update from the
I don't actually agree with this change, because theme mods for widgets and menus don't get included in the export data and templates should mirror that precedent. I would just leave them un-associated upon import, and add a user interface for associating themes and templates on-demand. |
Description
Partially resolves #31954 and partially resolves #31397 (some background to this in comments there.)
To fully resolve these issues, a UI will need to be built. This branch does not attempt the UI, but implements a necessary change in the underlying infrastructure before any such UI will be possible.
The idea behind the approach is to use
theme_modsto store the "slugs" of templates and template parts, rather than thepost_nameof the posts themselves.This makes it trivial to assign any existing template or template part to any newly activated theme, using
set_theme_mod. It doesn't even need to have the same slug in both themes. For example, you could have the same template besinglein one theme andsingle-postin another theme.To do the same with the existing system is near impossible, because you quickly run into issues with slug conflicts.
The API endpoint for templates and template parts can now be used in two ways:(removed for now)active theme name // slug(existing, for fetching active templates and template parts)empty string // post ID(new, for fetching inactive templates and template parts)How has this been tested?
Tested out creating new templates in the template editor and the site editor. They correctly resolve on the front end and they are stored in the database as intended.
Screenshots
Types of changes
Nothing should break, just clears the way for new features to be added later.
Checklist:
*.native.jsfiles for terms that need renaming or removal).