Template Parts: Fix loading issue #28088
Conversation
|
Size Change: -5 B (0%) Total Size: 1.28 MB
ℹ️ View Unchanged
|
|
Thanks for the proof of concept. As I explained in my previous comment, I believe there are better approaches and would like to see more discussion on potential alternatives. I always get a little bit worried when new patches are introduced before much discussion has taken place, especially when there is no linked issue and no patch description. (Sorry to be negative, but I'm in an opposite time zone to most people here and sometimes a lot can happen while I'm asleep.) |
|
@carlomanf Thanks for chiming in. I believe removing the "theme" attribute might be an option in the future but I don't see it as a realistic approach for the moment because:
I think the current approach in the PR is the best alternative we've tried/discussed so far (and we discussed a lot) but it doesn't mean we can't iterate on future proposals/alternatives. Please feel free to go ahead and try something else. |
jeyip
force-pushed
the
fix/template-parts-loading
branch
4 times, most recently
from
5e08ccd to
3b8e71b
Compare
@youknowriad If I'm reading correctly, you are open to my suggestion as part of a long-term solution, but you believe the I'm struggling to know what unwanted effects you're thinking of. The only scenario I can think of where removing the However, I would note the following:
If there is some other unwanted effect that I hadn't thought of, please let me know, but as far as I can see, it looks fairly safe to remove support for the |
jeyip
force-pushed
the
fix/template-parts-loading
branch
from
7bae766 to
469bfd7
Compare
jeyip
force-pushed
the
fix/template-parts-loading
branch
2 times, most recently
from
ba90f8b to
81abc9b
Compare
|
@carlomanf Thanks for the arguments, I think some of my earlier comments might have created some confusion. So this is my reasoning.
One of the big frustrations with classic themes is the fact that when you switch themes and come back, you can lose a lot of customizations, having a "theme" attribute solves that right away. Also, with FSE themes, it's actually very possible to use template parts from one theme to another and that they blend nicely with the design, the reason for this is "Global styles". Template parts inherits things from global styles, so they technically can adapt properly. It's I believe important that we explore these possibilities in FSE (cross-themes templates, or "no-theme" WP if you prefer). For example, I can see a world, where a user installs two themes and say, I'll use themeA as my primary theme but I prefer largely the "contact" page template from theme B and use it from there. OR I prefer the footer of theme B over theme A. I think this is definitely valid use-cases but I also think these need to be decisions made by the user and not something to be enabled inadvertently. That said, it's an unknown land, so it's better for us to focus on the smaller scope for now. I hope this clarifies my position. |
|
@youknowriad I agree with most of what you've said in your last comment. I think you might be misunderstanding what I'm talking about. You seem to be talking about the taxonomy term, I'm talking about the block attribute. I will probably submit a "competing" PR to this one and it might help you to better understand what I'm proposing. |
|
Just my 2c here: |
@aristath I agree with you, but the problem is not actually limited to themes in subfolders, the main issue for me is actually the fact that if you copy a theme, paste it into a renamed folder, it breaks because there's an "unwritten" assumption that the "theme" attribute in theme files should match the theme folder. |
Ah OK, gotcha. That makes sense then. Thank you for the clarification 👍 |
There was a problem hiding this comment.
It took me a while to get this working because I missed this:
Modify the source code of block templates and remove theme attributes from all
wp_template_parts.
I can verify that it works properly after doing that. Hooray! 🎉 As you mention, though:
This PR will require follow-up work (which has not been implemented yet) to properly address the template parts not loading issue. We're going to have to remove all theme attributes from theme provided template blocks.
I can see the purpose of having a theme attribute on saved-in-the-DB templates part references, but maybe we should just be stripping all theme attributes from file-based templates, all of the time? But that's outside the scope of just this PR.
Thats a good point. After this, what is really the use case for markup to ever supply a |
Stripping the theme attribute makes sense to me as well. I can't think of a valid use case for providing theme attributes in theme provided templates. 🤔 Stripping the attribute would also be quick to implement and easy to undo if we changed our minds.
💯 |
The same question crossed my mind and I agree with the above comments - I don't see much point in having them in file-based templates since that value is implicit based on the currently active theme. It also needs to be dynamic (depending on theme's location) and not hardcoded in html file.
@aristath there is a register_theme_directory in core, so in that sense I'd say it is officially supported and we have to consider cases where themes are outside of default theme directory (e.g. shipped within plugins or similar). I don't consider this a good practice but it is possible and supported atm. |
| if ( | ||
| 'core/template-part' === $block['blockName'] && | ||
| ! isset( $block['attrs']['theme'] ) && | ||
| wp_get_theme()->get_stylesheet() === $theme |
There was a problem hiding this comment.
Would it make sense to extract this as a bail condition before the for loop? It seems to be independent of any template blocks data being looped over.
Going a bit further, based on this description
@param string $theme the active theme's stylesheet.
Can we drop this parameter from the function, given that we can obtain this with wp_get_theme()->get_stylesheet()? 🤔 It looks to me that if the passed $theme parameter that is different from wp_get_theme()->get_stylesheet() this function will essentially be a no-op.
There was a problem hiding this comment.
Would it make sense to extract this as a bail condition before the for loop? It seems to be independent of any template blocks data being looped over.
If we keep $theme as a param, this makes sense.
Can we drop this parameter from the function, given that we can obtain this with wp_get_theme()->get_stylesheet()? 🤔 It looks to me that if the passed $theme parameter that is different from wp_get_theme()->get_stylesheet() this function will essentially be a no-op.
I mentioned something similar here #28088 (comment). It didn't generate a lot of discussion, but I prefer what you're describing.
| } | ||
|
|
||
| return $new_content; | ||
| } else { |
There was a problem hiding this comment.
Nitpick: we can drop the else block here since if ends with return, and just return $template_content at top level.
There was a problem hiding this comment.
My mistake if this is a convention! I thought run composer format would catch it. I'm on board.
There was a problem hiding this comment.
It might not be a written one but it's less keywords and nesting that achieves the same effect. 😄 Also that way it's immediately obvious that this function has a default return value while quickly scanning the code, at least to me.
| } | ||
|
|
||
| return $new_content; | ||
| } else { |
| @@ -5,6 +5,38 @@ | |||
| * @package gutenberg | |||
| */ | |||
|
|
|||
| /** | |||
| * Parses wp_template content and injects the current theme's | |||
There was a problem hiding this comment.
Would a more accurate description here be that it removes the theme attribute, instead of stating that it injects a new one?
| @@ -24,6 +56,9 @@ function gutenberg_edit_site_export() { | |||
| // Load templates into the zip file. | |||
| $templates = gutenberg_get_block_templates(); | |||
| foreach ( $templates as $template ) { | |||
| $updated_content = _remove_theme_attribute_from_content( $template['content'] ); | |||
There was a problem hiding this comment.
No need for declaring $updated_content variable here since it's not reused below.
$template->content = _remove_theme_attribute_from_content( $template['content'] );
|
@vindl I'll make a follow-up to refactor the code and comments. Thanks for chiming in 👍 |
…e HTML comment - Removes the "theme" key value pair from the HTML comment of block templates, in the documentation. As per the [recent changes in Gutenberg](WordPress#28088) (as of 9.9) the "theme" attribute should be removed from the template-part blocks.
Description
We currently query for template parts by theme in the client.
The canonical identifier for themes is the stylesheet property, which is the path of the theme directory relative to wp-content/themes. Unfortunately, the value of
themeprovided in the client side query above is, under some circumstances, not actually the theme stylesheet. In other words, sometimes it isn't the correct identifier.The mismatch happens because the
themevalue in our query is derived from block attributes in template blocks, which are defined by themes. Take, for example, the TT1 404 template in the TT1 blocks theme. A serialized template part in the template looks like this:<!-- wp:template-part {"slug":"header","theme":"tt1-blocks","align":"full", "tagName":"header","className":"site-header"} /-->The header template part in the 404 template has a "tt1-blocks" theme attribute. This theme attribute is what is eventually included in our client side query -- normally no big deal. If the WordPress theme exists in the root of the themes directory like so
wp-content/themes/tt1-blocks, then the theme stylesheet is "tt1-blocks", and it will match the theme attribute in the client side query.If, however, the theme exists in a subdirectory, like
wp-content/themes/subdirectory/tt1-blocks, the stylesheet then becomes "subdirectory/tt1-blocks". This no longer matches the theme attribute provided in the 404 template, and, as a result, the query is no longer correct.Note:
This PR will require follow-up work (which has not been implemented yet) to properly address the template parts not loading issue. We're going to have to remove all theme attributes from theme provided template blocks. Going back to the TT1 404 template as an example, when the follow-up PRs are completed, serialized template parts will look like this:
When loading theme provided template parts, the changes in this PR will then insert the correct theme stylesheet for the serialized theme attribute. This will allow template parts to load correctly again.
See this comment for more context.
How has this been tested?
Before applying this PR:
wp-content/themes..wp-env.override.json. This creates a copy of the locally installed TT1 theme in an "test" subdirectory in the WordPress volume.npx wp-env startApply this PR:
wp_template_partstt1-blocks/block-templates/front-page.htmlfor example<!-- wp:template-part {"slug":"header","theme":"tt1-blocks","align":"full", "tagName":"header","className":"site-header"} /--><!-- wp:template-part {"slug":"header", "align":"full", "tagName":"header","className":"site-header"} /-->npx wp-env start --updateScreenshots
Types of changes
Bug Fix for Automattic/wp-calypso#48145
Checklist: