Site Editor: Template parts not loading when block theme has parent directory #27633
Conversation
jeyip
added
[Type] Bug
|
Size Change: +14 B (0%) Total Size: 1.28 MB
ℹ️ View Unchanged
|
packages/block-library/src/template-part/edit/use-template-part-post.js
Outdated
Show resolved
Hide resolved
|
Yeah, we will definitely need to not use pub before merging — that’s just a platform choice, and a theme could be placed under any arbitrary subdirectory. Are we sure that in gutenberg, the theme slug excludes a subdirectory? (You could test by creating a custom mapping from “wp-content/themes/xyz” to a local block based theme in wp-env.) |
| @@ -23,14 +23,17 @@ export default function useTemplatePartPost( postId, slug, theme ) { | |||
| // load the auto-draft created from the | |||
| // relevant file. | |||
| if ( slug && theme ) { | |||
There was a problem hiding this comment.
The theme value on line 25 seems misleading. We no longer use it in the template parts query for reasons explained in the PR description (for auto-drafted template parts, it doesn't account for themes that have a parent directory).
At this point, it's really only used to determine if the current template part block is a placeholder or if its auto-drafted. Maybe we should have a follow-up PR to refactor it?
There was a problem hiding this comment.
A follow up makes sense to me, this does seem a bit outdated with how things work now.
Checking the theme field that was added to the html markup was an early way to resolve things (and as you have noted a bit problematic when theme directories may be nested elsewhere). And now that we are only concerned with auto-drafts from currently activated theme using getCurrentTheme instead seems to make sense.
That makes total sense 👍 I'll make a quick follow-up after I get this merged. |
| const cleanedSlug = cleanForSlug( slug ); | ||
| const themeSlug = currentTheme.replace( '/', '-' ); |
There was a problem hiding this comment.
Will this work for arbitrary paths registered with register_theme_directory?
jeyip
force-pushed
the
fix/template-parts-not-loading
branch
from
81a800f to
da9f50e
Compare
|
It does fix the problem. But I don't think this is the best way to fix this. Also, this change causes another bug where you can't use template parts from a different theme other than your current theme. After some digging, I realized the So to my understanding, the root cause is:
|
|
For more clarification, here is an example with If we put the theme into a folder called I don't know how we should handle this though, a few ideas:
|
I think we are leaning towards option 5: omit the directory path from the |
This makes total sense to me, especially given that, as mentioned by @Copons, if we use the full path and there's a migration where theme directories are switched around, users might “lose” their templates. Ex. If |
Side note: @david-szabo97 I had considered this, and I thought I remembered reading somewhere that this was already a self-imposed limitation of full site editing. That, however, might have changed, and either way, stripping any theme directory pathing seems like a better overall solution. Thanks for reviewing 👍 |
This would only be true for auto-drafts referenced by a template. In which case assuming current theme might make sense? 🤔 |
jeyip
force-pushed
the
fix/template-parts-not-loading
branch
from
68c04c5 to
0123a5c
Compare
There was a problem hiding this comment.
Wondering about an edge case if we assume current theme for auto-drafts in the template part block:
- User edits a theme supplied template and makes it a
publish/ custom version. But never edits the template-part auto-draft it references, so it is still saved by slug/theme and not Id. - User then switches themes and uses their custom published template. Now the template part block assumes the auto draft is from the new theme, and the custom template loads an unexpected version of the template part?
| 'field' => 'slug', | ||
| 'field' => 'name', |
There was a problem hiding this comment.
Im curious about the switch from slug to name here? It looks like the templates-sync still queries and creates based on slug, so could this potentially cause issues?
There was a problem hiding this comment.
Im curious about the switch from slug to name here?
When themes are added to taxonomies, a slug is generated that looks like parent-directory-some-theme while the name looks like parent-directory/some-theme
Using name instead of slug allows us to query for the theme on the client without doing any string transformations. We can simply pass through the value of getCurrentTheme().stylesheet (which looks like parent-directory/some-theme, not parent-directory-some-theme), whereas before, we had to do a .replace to remove the / and add a -
There was a problem hiding this comment.
It looks like the templates-sync still queries and creates based on slug, so could this potentially cause issues?
Ahhh I must've missed a few values. I'll take a closer look at the php files to find other instances. But yeah. Ideally all queries with slug are switched to name if we go down this route.
I know that, in previous discussions, one of our concerns was other developers having to "know" implicit rules about how to query template parts by theme. For example, as mentioned above, they might have to be aware that, when they query, they need to use a .replace method on getCurrentTheme().stylesheet. With this change, it's no longer the case.
I'd love your thoughts though
There was a problem hiding this comment.
That all makes sense. Im not really sure if there are any other implications to using name over slug. But if we are consistent with it and it works, it sounds great to have the tax term and getCurrentTheme().stylesheet in sync.
Good questions! I'm going to think these over |
Very keen observation. The most natural way to handle that scenario seems like what we already use, which is theme block attributes 🤔 There's definitely a path forward, but in that case, the solution here would depend on other changes as well:
|
🤔 Even with that I still see the same scenario happening. Customizing a template from one theme without customizing the template part, switching to another theme, using that first customized template... the template part will still correspond to the new theme instead of the one the template originally referenced. Its an edge case but Im not sure how concerning it should be. |
|
Oh, I think I get what you were saying earlier then.
Im catching on, 😆 - so we would assign that theme attribute so it would be the appropriate value considering the directories. The problem with this approach means whenever we load the editor with a new theme, the editor will initially load in the dirty state since we have changed attributes of the template part block. We previously had some behavior that caused this sort of thing and it was definitely unfavorable. 🤔 To avoid this we would have to parse and change the markup at the time we are creating the auto-draft of the template itself to assign that correct theme attribute on the template part blocks appropriately. Im not sure if thats the right way to go either. |
Oh yikes. Thanks for the context 👍 I wasn't aware that this was an issue. In that case, I wonder if we can move forward with this PR at all without tradeoffs. 🤔 |
|
Now being handled by other PRs like #28088 |
Description
After latest Gutenberg plugin updates (v9.5.1), template parts are no longer loading properly when using themes that have a parent directory
Cause
themeblock attribute (see twentytwentyone-blocks) which resolves to something liketwentytwentyone-blocksthemeattribute is used in a template parts query on loadthemeprovided value is, however, incorrect. It doesn't account for themes, and their slugs, with a parent directory. As a result, we query bytwentytwentyone-blocks, when in reality, we need to query by[PARENT_DIRECTORY]-twentytwentyone-blocksHow has this been tested?
Screenshots
Before
Types of changes
Bug fix for #48145
Checklist: