| CARVIEW |
Navigation Menu
-
Notifications
You must be signed in to change notification settings - Fork 4.6k
Allow template duplication + concept of active templates #67125
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
Conversation
|
The following accounts have interacted with this PR and/or linked issues. I will continue to update these lists as activity occurs. You can also manually ask me to refresh this list by adding the If you're merging code through a pull request on GitHub, copy and paste the following into the bottom of the merge commit message. To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook. |
ellatrix
force-pushed
the
try/active-templates-2
branch
from
9e9eafc to
1d75cf3
Compare
ellatrix
added
[Type] Enhancement
|
Size Change: +1.27 kB (+0.07%) Total Size: 1.95 MB
ℹ️ View Unchanged
|
packages/core-data/src/resolvers.js
Outdated
| name === 'wp_template' && | ||
| typeof key === 'string' | ||
| ) { | ||
| name = '_wp_static_template'; |
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.
Is the _ at the beginning necessary?
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.
We can change it to whatever we want. I was wondering though if we should make the endpoint private by making it a random path. But the post type can stay the same.
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.
static_template is fine for me, but maybe the _ is unnecessary.
| context: 'list', | ||
| } ); | ||
| const editAction = useEditPostAction(); | ||
| const setActiveTemplateAction = useSetActiveTemplateAction(); |
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.
Why is this action defined here and not in the "fields" package like all the other actions?
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.
Cause I didn't really know where they should be defined.
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 really see any fields or actions in there that are private right now. Let's maybe keep this private with the site editor until things stabilize over follow-ups. I'm not even sure if the active action is what we want considering the other explorations.
ellatrix
force-pushed
the
try/active-templates-2
branch
from
205296d to
0676add
Compare
|
Flaky tests detected in efa4291. 🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/17764382087
|
|
Pinging @WordPress/block-themers and @WordPress/outreach to get as many eyes on this as possible :) And help testing this would be much appreciated! |
youknowriad
added
Needs Dev Note
|
Using 2025, I removed all my custom templates and I see this in the theme templates: 
My understanding is that this one is marked "inactive" because it's not a "hierarchy" template, My feeling is that it doesn't mean entirely "inactive", it may be already assigned to some pages. So this raises two questions:
That said, It's not easy because "hierarchy" doesn't really mean anything to most users. |
|
One possible option would be to consider these "custom page templates" as always active (you can't really disable them). I noticed that right now, you can active/deactive them but it doesn't do anything really and they don't show up in "active templates" either. |
|
@youknowriad Yes, we should think about those.
Maybe some in-between state. "Active on a page" would be a bit long. We're seem to be calling these elsewhere "for a specific item", so maybe a gold badge with "Specific"? Not sure. It would be good if you can hover over the badge and see more info.
Yes, it's complex, because there's two scenarios: either you assign an existing template to be the page template. In which case the template in not active except for this specific page. Or you create a new specific template that has the slug I think we should do something here in this PR, but there's probably some details left to figure out later. |
|
For example, one option is to deprecate |
|
I found myself missing a way to name the templates that I create. I believe we need to:
I think this is the kind of details that can be addressed in a follow-up though, once the whole mechanic and UX is agreed upon. |
|
This is very cool. Some initial observations: After building this branch, a bunch of templates appeared in the 'Custom templates' section, many of which are duplicates of the same template. It's not clear why. Perhaps they're revisions? The point being that we should think about the upgrade process, expected results etc. Something feels wrong with the logic around the active state of the Front Page template. I have one (several actually, see previous point) in 'Custom templates', but none of them are active. It doesn't appear in 'Active templates' either. It seems to be ignoring the template hierarchy, maybe that's intended? :) I'm able to click into and 'edit' theme templates. Two issues around this; it's not clear what happens, I assume the template is duplicated, but is it also activated? It's inconsistent with theme patterns, which are entirely uneditable until duplicated. In general I think this part of the UX needs more thought, but for the initial implementation perhaps they should be consistent to help unify the concept? Potentially a big can of worms, but communicating the template hierarchy in the UI is quite challenging :) As a provocation, and to define the scope of design, do we need to do that with custom templates? Theoretically could any template be assigned to any page? I suppose this relates to the previous point about the 'Front Page' template. For The A couple of very small points on the design;
|
That is weird, it shouldn't be happening since you can only have one custom template per "hierarchy slug" in trunk. I wonder if you had some lingering broken templates (that were hidden in trunk) from very old branches or something.
I think it probably relates to the previous point. I think upon upgrade, all user templates should be "active" by default.
I agree, I think for me the ideal is to be explicit about what's happening (no edit) but there are some challenges with that:
|
| isBlockBasedTheme: | ||
| select( coreStore ).getCurrentTheme()?.is_block_theme, | ||
| canCreateTemplate: select( coreStore ).canUser( 'create', { | ||
| canCreateTemplate: select( coreStore ).canUser( 'read', { |
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.
Why did we change the perm? Seems natural to check for create in canCreate...
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.
Why? The commands are meant to navigate to templates, not create new 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.
But yes this variable should be renamed 😃
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 agree that using the create capability check here is odd but necessary. Users who can read templates might not have access to the Site Editor.
See #60317 (comment).
P.S. We should extract similar minor and unrelated fixes into separate PRs. There's already a lot going on here and that would make reviewing bit easier.
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.
Then this check should be called differently and we should use something else to check if the user has access to the site editor. This is not a good proxy for checking site editor access, it is more restrictive now. Also we should then use it as a condition to prevent all site editor commands from loading?
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.
Also we should then use it as a condition to prevent all site editor commands from loading?
It's hard to generalize it. The Site Editor needs more granular capability checks, but those should be mapped out separately - maybe even incorporated into the router.
As I mentioned, I would remove and extract similar fixes into separate PRs.
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.
Yes, here the change is necessary because you cannot create static templates through the endpoint, you can only create custom 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.
So I guess we need another additional check, not sure what
|
The following accounts have interacted with this PR and/or linked issues. I will continue to update these lists as activity occurs. You can also manually ask me to refresh this list by adding the Unlinked AccountsThe following contributors have not linked their GitHub and WordPress.org accounts: @erikjoling. Contributors, please read how to link your accounts to ensure your work is properly credited in WordPress releases. If you're merging code through a pull request on GitHub, copy and paste the following into the bottom of the merge commit message. To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook. |
|
|
||
| // How does this work? | ||
| // 1. For wp_template, we remove the custom templates controller, so it becomes | ||
| // a normal posts endpoint, modified slightly to allow auto-drafts. |
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 think this is the right thing to do but I'm a bit concerned about potential breaking changes.
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.
The part about changing it to a normal posts endpoint? That's valid. The alternative would be to create a different CPT or endpoint 🤔
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'm just wondering if the endpoint is going to continue working the exact same way or not? Do we have some fallbacks in place when previous "slug ids" are used?. I think a good summary of the endpoints changes would be good.
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 think a good summary of the endpoints changes would be good.
Came here to ask the same 🙇
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.
Let's add a fallback for a slug id in a follow-up as discussed. Right there should be a summary. The main change is that whatever imitation of the post type endpoint there was for wp_template is now replaced with a real post type endpoint. And another one is added to handle theme/plugin templates separately.
| } | ||
|
|
||
| add_filter( 'pre_get_block_templates', 'gutenberg_pre_get_block_templates', 10, 3 ); | ||
| function gutenberg_pre_get_block_templates( $output, $query, $template_type ) { |
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.
Can we have some comments just to clarify what these overrides do.
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 was removed btw, the filtering is way less hacky
| } | ||
| // Unless there is no persisted record, set the status to | ||
| // publish. | ||
| if ( name === 'wp_template' && persistedRecord ) { |
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 doesn't feel like the right place for this code. Should we have a similar behavior to pages/post with a publish flow :)
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.
Yeah the thing with templates is that they have to be published all the time, unless we change it to allow other statuses. I'm don't remember correctly if this was added to always force publish or if there was a UI reason.
| }, | ||
| ( state ) => [ | ||
| // Even though getDefaultTemplateId.shouldInvalidate returns true when root/site changes, | ||
| // it doesn't seem to invalidate this cache, I'm not sure why. |
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 weird, are you sure?
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'll test this again
| return state; | ||
| } | ||
|
|
||
| export function templateAutoDraftId( state = {}, action ) { |
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.
Why do we need this, we don't need it for any other entity?
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.
You mean auto-drafts? I don't think we create them client-side for any other entity, no. We do need auto-drafts here so we can show an editor when you click on the root canvas. Perhaps we can refactor this away if we change the UI a bit.
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.
In other entities, we do create auto-drafts before opening the editor... so there's no need for special handling like that.
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.
Before, in PHP no, before initializing the editor in JS?
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.
before loading the editor in JS. We call saveEntityRecord with auto-draft or something and the returned id is loaded in the editor.
| ); | ||
| } | ||
|
|
||
| function useTemplates( postType ) { |
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.
Where is this hook used? I think when we find ourselves merging these two list together, maybe there's a better path
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 only used twice within the same file. There will anyway need to be a follow-up ticket to look at separating calls to the APIs better everywhere including DataViews.
|
Another user feedback to be considered as a follow-up. We should probably make the "description" editable. I'm not sure if using the "default descriptions" is good or confusing. |
Yeah, we should. This is one of the things I was hoping to resolve with the slots/assignments UI approach. The default descriptions are used for slots rather than templates there. And then templates can have other excerpts/descriptions. |
Maybe it's as simple as using these descriptions as default values in the creation modal or something. |
|
I think it's going to be hard to nail everything here before merge. We'll definitely find edge cases... And we do want a lot of testing before WP release. So I suggest that we merge this PR and create an issue with all the follow-ups (user and technical) suggested in the comments above. |
|
Yes, sounds good, there's a lot of things to cover. The most important is that it goes in now with still 4 weeks to spare before Beta |
github-project-automation
bot
moved this from 🔎 Needs Review
to ✅ Done
in WordPress 6.9 Editor Tasks
|
So awesome to see this land 🚀 Nice work everyone! |
|
Thank you, this is exciting! Would love a written summary of what changes went in so far, for those of us who can't keep track of everything. |
|
Thanks to everyone who has been working on this PR. If you plan to submit any bugs or enhancements related to this PR as an issue, please feel free to add it to the 6.9 Editor tasks project board so that we won't miss important tasks. |
)" This reverts commit 32d3736.
| return unlock( select( coreStore ) ).getTemplateAutoDraftId( | ||
| templateId | ||
| ); |
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 appears to invoke a POST request every time an editor is opened, creating an auto-draft. Similar unguarded requests can be very problematic, as they can easily add a large number of unwanted records to the DB.
Here's WP CLI command to check it:
wp post list --post_type=wp_template --posts_per_page=-1 --post_status=auto-draft
Records from just last night
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 appears to invoke a POST request every time an editor is opened, creating an auto-draft. Similar unguarded requests can be very problematic, as they can easily add a large number of unwanted records to the DB.
Isn't this how the post editor works as well (forever). Maybe the auto-cleanup that the post editor has doesn't apply to templates for some reason?
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've not investigated further, but I guess a similar mechanism could resolve it. Additionally, these template auto-draft requests will fail for any non-admin user.
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 should work the same as post auto-draft, so I'll investigate. Also ideally we avoid the auto-draft entirely, I'll see if we can do that before the release.
Additionally, these template auto-draft requests will fail for any non-admin user.
That's fine no, templates can only de created by admins?
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.
IIRC, the general policy is to avoid displaying any error messages, even in the console. They'll be reported and will have to be resolved eventually. If the user can't create a template, we shouldn't dispatch a request.
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.
Oh you're saying we're making a request for non-admins. Sorry misread
Labels
18 participants
Creating a new PR to squash all commits and preserve the history of #66951.
What?
Fixes #66950.
Why?
See above.
How?
active_templatesthat contains a maps of slugs to active template IDs. To disable default templates, it can be set tofalse.auto-draftuser template is created behind the scenes in case edits are made. If no edits are made this disappears. The active template is also set to the draft. If the user makes edits and wants to save it, changing active templates can be unticked in the multi-entity saving panel. There's potential to improve the UX here with this new paradigm.One significant change that may cause breakage in plugins is the resulting output of
getEditedPostIdandgetCurrentPostIdfor templates changing from always being the static template ID (string) to sometimes, or always (due to auto-draft), being the numeric post ID of the edited template. IngetEditedPostId, which is always referring to a template, this is slightly mitigated by casting the ID to a string. However,getCurrentPostIdwill keep returning a number or string (which the docs mention even prior to this PR), and in both cases it might not return the original theme/plugin template ID, which can cause breakage when a plugin relies on the ID for checking the template slug. Plugins should really usegetEditedPostSluginstead.Testing Instructions
If you ever need to delete all the active templates, which will trigger migration, use
npm run env run cli wp option delete active_templates.Testing Instructions for Keyboard
Screenshots or screencast
Screen.Recording.2024-11-13.at.09.20.16.mov