Allow template duplication + concept of active templates #67125
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.
Is the _ at the beginning necessary?
There was a problem hiding this comment.
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.
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.
Why is this action defined here and not in the "fields" package like all the other actions?
There was a problem hiding this comment.
Cause I didn't really know where they should be defined.
There was a problem hiding this comment.
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.
Why did we change the perm? Seems natural to check for create in canCreate...
There was a problem hiding this comment.
Why? The commands are meant to navigate to templates, not create new templates?
There was a problem hiding this comment.
But yes this variable should be renamed 😃
There was a problem hiding this comment.
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.
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.
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.
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.
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. |
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.
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.
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.
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.
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.
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.
Oh you're saying we're making a request for non-admins. Sorry misread
)" This reverts commit 32d3736.
|
I was reverted during 21.7 RC 2 but kept for 21.8, so changing milestones. |
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