Site Editor: Create auto-drafts for modified files only #26383
Conversation
david-szabo97
added
[Type] Performance
david-szabo97
added
the
[Status] In Progress
|
Size Change: +56 B (0%) Total Size: 1.19 MB
ℹ️ View Unchanged
|
david-szabo97
requested review from
vindl,
noahtallen,
mattwiebe,
Addison-Stavlo,
Copons and
jeyip
david-szabo97
requested review from
spacedmonkey and
TimothyBJacobs
as code owners
david-szabo97
changed the title
There was a problem hiding this comment.
Its nice to see more eyes in this template (and part) auto-drafting arena. It feels a bit all over the place atm (not this PR, just whats in place in general) and seems like it needs to be rethought a bit.
I see its still In Progress but a few Qs and thoughts:
david-szabo97
removed
the
[Status] In Progress
david-szabo97
changed the title
|
May also be interested in #26650 - @youknowriad is also looking into the template part auto drafting stuff currently. |
|
It seems we should hold on this one for a bit until #26650 lands and simplifies all this logic. |
@youknowriad that one appears to be about template parts, but this one is about templates, or do you anticipate extending that one to cover templates as well? |
Oh right, I missed that part. I'm thinking of taking a look at templates though but I don't know if there will be a lot of changes there. So actually, feel free to proceed here and I'll just check whatever state we'll have. |
|
Actually, I have now updated the PR to also introduce the same "sync" function for templates. |
|
Just curious.... Why not just store the |
Interesting! I wonder if this would work well with distributed databases/servers 🤔 But it does seem useful for development |
Is there any particular advantage to that over the current approach? It would be easily break if the db were manually modified and it would also complicate changing themes. The current approach by @david-szabo97 seems to work quite well. |
|
After thinking about this some more, using post-meta would probably be bad for performance... The suggested implementation with the separate option containing versions of all templates feels a bit weird (why save a post's attribute - like its version - in an option instead of the post itself), but at the same time it will perform better. |
It's not doing that. It's saving the version of the theme, not the version of the post. All theme-related data is stored in the options table. Doesn't seem weird to me.
True, but would anyone ever need to edit an FSE template directly? |
|
The new option is a single query, whereas checking the auto-drafts would require a single query for each template, plus we don't need the whole WP_Post object in this case. |
Fair point. The option will indeed be more performant. |
There was a problem hiding this comment.
This overall looks awesome, thanks for working on this!
Unfortunately, I am noticing that some template parts are not loading in the editor. They are loading on the front end of the site with the correct template, but they don't load in the site editor after a few loads.
Here are the details:
- twenty twentyone blocks does work. This is loaded as normal via wp-env.
- I have the Automattic/themes mapped to
wp-content/themes/themes. - Neither seedlet blocks or ibis (both from the above repo) will load template parts in the site editor.
I investigated a bit, and it is resolving the correct directory for the template parts:
$theme_slug = get_stylesheet();
// $theme_slug = "themes/seedlet-blocks"
$theme = wp_get_theme( $theme_slug );
$theme_dir = $theme->get_stylesheet_directory();
// $theme_dir = "/var/www/html/wp-content/themes/themes/seedlet-blocks"And that directory exists and contains the "block-template-parts" directory. So it seems like the code should be working. I really can't tell why twenty twenty one blocks is working but not the other two.
Thanks for testing with other themes, I totally forgot to do that 😄 This commit should fix the bug. Can you give it a check? |
There was a problem hiding this comment.
It's weird, for some reason it's still not working. Specifically, the index template won't load its template parts. The singular template does, and I can select the template parts from the sidebar. Edit: actually, the singular template won't either after cleaning the database.
|
how ready is this PR? It would be good to include in the RC release today. |
|
Fixed the errors reported by Noah above. Did a few tests and seems to work fine. If you could do some tests and report back that would be great. I'm trying to get E2E tests to pass. |
|
@david-szabo97 There's a lot of e2e test failures on master now which I'm working on. Most of the failures in the PR should be unrelated but I'll you know once I make progress so you can rebase. |
jorgefilipecosta
force-pushed
the
update/site-editor-template-reconcillation
branch
from
4b19a47 to
a522a6b
Compare
|
I've stumbled upon a potential issue, that might or might not be a big deal, depending on how "in depth" a site is used. Recently I've been working a lot with templates and template parts, and to keep things safe while switching branches, I got used to delete all As it turned out, I've tested this branch, then changed to work on something else, then deleted all templates and parts, and switched back on this branch. This is because the time check options were created during my original test. For new themes activations, this PR relies on the absence of time check options for the given theme, defaulting to 0, which is safely older than anything: $last_check = isset( $last_checks[ $theme_slug ] ) ? $last_checks[ $theme_slug ] : 0;This was also confirmed by the fact that once I deleted the time check options too, the template and parts were created successfully. In other words: if a user deletes all I fully recognize that this is not a normal flow, but I also can't help but wonder if the time check options might unintentionally hinder what is supposed to be a relatively safe operation, without possibility of recover (unless one is aware of the options). This will be aggravated if we end up displaying auto-draft in the templates and parts wp-admin lists (as proposed in #26636). (As a side note: the intention with showing auto-drafts there is to help with the development, rather than being a production feature) Am I overestimating this potential issue? |
|
I think it's an edge case... but perhaps we could delete the stored timestamps if the file doesn't exist? 🤔 |
@aristath Unfortunately it's the other way around 😅 The files are there, untouched. Basically we would need to:
Which... I'm pretty sure it would remove all benefits of this PR. 😄 |
|
With the 6e8bb9d change, the issue seems fixed. Just for additional context, I have found a definitely not edge case that would have triggered this issue (which, again, is now fixed 🙂):
This was caused by the same issue: we only compared the sync date with the file date, but didn't consider that a template might be deleted. 6e8bb9d should fix most use cases. I guess power users should be also able to delete a site option, if needs arise. 🤔 |
jorgefilipecosta
deleted the
update/site-editor-template-reconcillation
branch
| * @param WP_Post $post WP_Post instance of the deleted post. | ||
| */ | ||
| function gutenberg_clear_synchronize_last_checks_after_delete( $postid, $post ) { | ||
| if ( 'wp_template' !== $post->post_type || 'wp_template_part' !== $post->post_type ) { |
There was a problem hiding this comment.
Should this be 'wp_template' === $post->post_type?
There was a problem hiding this comment.
do we have a PR to fix the issues introduced here so I can backport them to 9.4?
Description
Only run synchronization logic for modified files rather than all file-based template and template part files.
Fixes #25868
How has this been tested?
wp_templateis created or updatedTypes of changes
Code quality & Performance
Checklist: