Fix REST API auto-draft creation #26650
Conversation
|
Size Change: -629 B (0%) Total Size: 1.21 MB
ℹ️ View Unchanged
|
| ) | ||
| ); | ||
| } else { | ||
| // Potentially we could decide to update the content if different. |
There was a problem hiding this comment.
If we aren't comparing content as in the previous implementation, @david-szabo97 has been looking into re-writing the auto-drafts if the theme version changes - #26383
There was a problem hiding this comment.
yes, with this simpler syncing function, it's just a matter of comparing content here and updating the auto-draft if needed.
There was a problem hiding this comment.
There was a problem hiding this comment.
It's because gutenberg_resolve_template calls file_get_contents on every template in the theme while gutenberg_find_template_post_and_parts only called it on the one being requested. That means if you are loading 10 templates in the site editor then file_get_contents will get called 100 times.
There was a problem hiding this comment.
Where do you see the template part update was being done before? By my own reading of the code, it was not being done anywhere.
There was a problem hiding this comment.
I was talking about templates actually. They were being updated inside gutenberg_find_template_post_and_parts if the file contents had changed. I don't know about template parts.
There was a problem hiding this comment.
Where do you see the template part update was being done before?
From what I can tell, the following was the closest to this. It looks like it didn't so much update the auto-draft itself, but create a new one. The current code only gets to this point if a publish version isn't found, but if the contents of the auto-draft don't match the contents of the file then a new auto-draft is created:
gutenberg/lib/template-loader.php
Lines 174 to 178 in 422b21c
There was a problem hiding this comment.
Yes, I can see that difference and it's very problematic IMO. Creating a new auto-draft when the post_content changes is probably not the best approach to synchronization.
Let's leave this syncing behavior to its own dedicated PR since it's broken in master anyway (proliferation of auto-drafts).
There was a problem hiding this comment.
it's very problematic IMO.
Let's leave this syncing behavior to its own dedicated PR
Agreed, and that sounds like a good plan!
Yeah, those are some complex algorithms, and it's quite likely that we're now running auto-draft creation too much as a consequence of trying to capture all cases where they're needed. (We had this kind of problem before: #23662)
I definitely agree with the sentiment. I think part of the complexity results from the fact that we're really pushing the entities concept here, where a dedicated endpoint for template resolution might make more sense. We might want to revisit this design decision -- though it might mean an even bigger rewrite, so it's a decision that should not be taken lightly. More importantly however, I think these methods really need unit test coverage to make sure their desired behavior is documented and guarded against regressions, since from experience, these are quite fragile and very easy to break in some cases. (Unit tests could also be used to make sure that auto-drafts aren't created unnecessarily.) |
Definitely, I do think these flows are very important to test properly, thought, at this point, I'm not really sure whether the code I removed is necessary or not and what flows they are trying to achieve. We do have end 2 end tests for template parts loading, creating and saving and these tests are passing properly. The only thing on this PR that I'm certain that is broken now and untested is the site zip export. |
|
I personally wouldn't mind that this proposed code removal would break something that isn't covered with tests. It would be a good excuse to write more tests and better understand how we can further optimize existing flows. We should also keep in mind that at some point we will have to integrate all those hooks with the WordPress core so we better have the logic as much consolidated as possible. |
|
I agree with all of the sentiments about updating this code. I think it is complex partly because the initial concept was hacked together to get it working. We've been building off of that foundation ever since, so I doubt the code has ever gotten a thorough look-over (other than each of us trying to go through and understand it better.) |
youknowriad
requested review from
spacedmonkey and
TimothyBJacobs
as code owners
|
Ok, I pushed this a bit further. It now touches template resolution as well 😬 and fixes site exports. The main idea on which all this PR relies on is: Let's not mix templates from theme files and templates the CPT on our logic to render/edit templates. Let's just load the theme templates as CPT auto-drafts all the time and only rely on the CPT for the different flows. Noting that this was somehow how it used to work as well but it wasn't explicit, it was behind different function calls that are hard to follow. I think I'm personally satisfied with where this is heading but I'd love some testing and thoughts. |
|
@david-szabo97 yes, I'm going to leave it out, just make a place for it |
youknowriad
force-pushed
the
try/fix-template-part-loading-flows
branch
from
dbc60bd
to
942aac1
Compare
youknowriad
requested review from
ajitbohra,
chrisvanpatten and
mkaz
as code owners
|
I added a document to explain how template and template parts work. I think this is ready to land now. |
| if ( ! $theme ) { | ||
| update_post_meta( $post_id, 'theme', wp_get_theme()->get_stylesheet() ); | ||
| } | ||
| } |
There was a problem hiding this comment.
Im now realizing this means that custom template parts created through wp-admin will be given a theme value corresponding to the active theme. This will make it hard to differentiate TPs that were originally supplied by a theme vs. custom user created ones that should not be tied to a theme, which will in turn make it difficult to show a proper list of template parts in the site editor ( current theme + custom ). 🤔
I wonder how we can get around that.
There was a problem hiding this comment.
When I wrote this, template parts were theme-specific on my mind even user created one but if they are not, maybe we can just show all the template parts from all themes (non auto-drafts) + the auto-drafts of the current theme in the UI.
There was a problem hiding this comment.
This will be fixed by #27016, which has a new taxonomy term wp_file_based to indicate ones that originated from the theme. Ones that originated from the user won't have this taxonomy term.
There was a problem hiding this comment.
that taxonomy is weird, what if I have something that originates from the theme but then I completely edit it, at what time it stops to be something that originates from the theme and that becomes user based?
There was a problem hiding this comment.
I've replied about it in another PR, so let me just link that comment here: #27016 (comment)
The tl;dr is that _wp_file_based only indicate the origin of a template/part, which is useful for things like reverting it to the file version.
Once a template/part is not auto-draft anymore, it's safe to say it's "user based", regardless of the _wp_file_based tag.
(I'm totally open to change the tag name if y'all feel it's confusing!)
There was a problem hiding this comment.
When I wrote this, template parts were theme-specific on my mind even user created one but if they are not, maybe we can just show all the template parts from all themes (non auto-drafts) + the auto-drafts of the current theme in the UI.
I don't think there has been much discussion on whether they should or shouldn't be tied to the theme. However, in the past slug/theme combos were required to be unique and creating a template part required inputing a specific theme value. When this restriction was taken off, custom template parts were created with a '' theme name making them distinguishable from theme supplied ones. Now that they do have the same theme value, there are other conflicts that can come into play such as unedited theme supplied templates potentially referencing the wrong template parts as described in the second half of my comment here -#26950 (comment) - but using something like the wp_file_based could clear that up.
## Description Discovered while working on WordPress/wordpress-develop#1267. First introduced in `lib/template-loader.php` in #25739. No longer used per #26650 (where its callsites started using [`_gutenberg_get_template_paths`](https://github.com/WordPress/gutenberg/pull/26650/files#diff-f5b03c388f81fea69d0ababd289047e20deaad43084ad6e00ec14a5613e25136R60) in `lib/templates-sync.php` -- now in [`lib/full-site-editing/block-templates.php`](https://github.com/WordPress/gutenberg/blob/d7714aa8adf19277da1f0ea83b20be1cf234e50c/lib/full-site-editing/block-templates.php#L17)).
As you can see on the template-part e2e test failures that happen from time to time, creating and fetching drafts right now is broken and suffering a race condition. It's very hard to localize properly because it's very hard to reason about the flows to create those drafts right now. I think it's time to step back and think about how we want template parts flows to work.
This PR starts with the following assumption: The site editor always fetches template parts using REST API.
Based on this premise, why not just create these auto drafts on all
GETrequests to template parts? It's important to note that it's close to how it works currently but right now, it's hidden behind a number of function calls and is not consistent.This will probably break some minor flows like site export but I believe we can fix these differently.
I also looked at the code for initializing templates... there's a lot of unneeded code IMO that is just creating hidden bugs.
What do you think? What am I missing?