Skip to content
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

Full Site Editing: Avoid using the templates and template parts CPTs for theme provided templates #27321

Closed
youknowriad opened this issue Nov 27, 2020 · 27 comments · Fixed by #27910
Assignees
Labels
Framework Issues related to broader framework topics, especially as it relates to javascript [Priority] High Used to indicate top priority items that need quick attention [Status] In Progress Tracking issues with work in progress

Comments

@youknowriad
Copy link
Contributor

youknowriad commented Nov 27, 2020

Right now, full site editing UI and frontend rendering fetch templates from the wp_template and wp_template_part CPT, whether it's for the templates that are edited by the user or the ones that are provided by the theme. This means a "synchronization" step is needed between the theme files and the CPT.

This synchronization simplifies the fetching by allowing us to only use the CPT endpoints and WPQuery to retrieve the desired templates but it has its drawbacks:

  • Performance: on each wp_load or QPQuery for the templates, we compare the file modification data to ensure whether we need to update the templates or not. This might become costly depending on the number of templates and templates parts we need to deal with.
  • Stale templates: Each time we switch to another theme, a new set of templates and template parts are created and added to the database, we could consider removing the unmodified templates on theme switch but it comes at a cost.
  • Dynamic templates: we're considering introducing PHP templates for translations, the syncing would need to be performed on each local switch. (and this scales to any dynamic behavior we might want to introduce and might not be easy to "detect")

Proposal

We should consider checking what would it take to update the framework to rely on the files directly for unmodified templates. Some potential challenges here will be:

  • Make the editor work with entities created on the client.
  • Potential changes to core-data saveEntity actions to map temporary ids with real ones.
  • Resolving a template would require fetching both the saved templates and the available files in order to find the right template for a given page.

cc @vindl @Copons @david-szabo97 @Addison-Stavlo @noahtallen @jeyip @mattwiebe @mtias @aristath @ockham @carlomanf

@youknowriad youknowriad added Framework Issues related to broader framework topics, especially as it relates to javascript [Feature] Full Site Editing labels Nov 27, 2020
@Copons
Copy link
Contributor

Copons commented Nov 27, 2020

This sounds like a good solution, actually.

I was pondering endlessly on this yesterday, and I think the two key parts (aside from the resolution, which is an obvious need) for this to work would be:

  • Only create a post copy for a template/part when it's inserted into the site editor, customized, and saved.
  • Add the template/part files to their corresponding get/list requests, to use them in the site editor.

By converting files into "fake posts" (e.g. by constructing new WP_Post objects with the file's slug and content) and returning them in the list we might be good, for as far as the Gutenberg entities are concerned.

I might give this a try to see if my hypothesis actually make sense, but given how finicky this whole thing is, I strongly encourage others to try as well.

@aristath
Copy link
Member

Sounds like the way to go.
I'll start working on an exploratory PR to see how it goes 👍

@ockham
Copy link
Contributor

ockham commented Nov 27, 2020

Love to see this coming together, and to get rid of those pesky auto-generated CPTs ❤️

Carrying over some conceptual ideas from #27284 (comment) that might be useful for implementing this:

  • Template resolution should be a completely separate concern from template file/CPT lookup. (All that template resolution needs to know is the answer to the question Is there a template for ...?. Template file/CPT lookup can answer that, but encapsulate and hide which of either it finds.)
  • The frontend should work if there are no CPTs at all, by simply falling back to the template file. It might make sense to start by implementing this part, as it should be pretty straight-forward. (Might also be useful to add unit test coverage early.)
  • In the editor, maybe we can think of the fallback to the template file as conceptually similar to the blank state a user sees when starting a new post in the post editor (which also doesn't require a post object in the DB, nor an ID etc). Basically a customizable initial state that can be passed as a prop to the editor.

@youknowriad
Copy link
Contributor Author

starting a new post in the post editor (which also doesn't require a post object in the DB, nor an ID etc)

Right now! this does require a post object which is I believe the main reason FSE started with auto-drafts.

@mtias
Copy link
Member

mtias commented Nov 27, 2020

Thanks for describing the main aspects. This is an important infrastructure step that should simplify the system quite a bit. The initial conveniences of relying on the CPT for all loading was useful during prototyping but is now starting to create many reconciliation issues and increasing the code complexity (sync logic, discriminating customized and not customized work, excluding auto-drafts on navigator, theme updates, etc).

@mtias mtias added the [Priority] High Used to indicate top priority items that need quick attention label Nov 27, 2020
@bobbingwide
Copy link
Contributor

I'm all for this. With regards to:

Dynamic templates: we're considering introducing PHP templates for translations...

Just recently I've been investigating a solution for internationalization & localization that will work directly against the template and template parts without any need for special mark up in the HTML.

The basic premise is that the system should be able to automatically generate locale specific html files.
There are 3 stages

  1. Parse the html and blocks to extract the translatable text.
  2. Translate offline.
  3. Reparse and apply the translations to generate the locale specific template files.

At run time, the locale specific templates and template parts will be loaded instead of the US English defaults. There's no further need for translation. No gettext filtering.

But I have some nagging questions about FSE

  • What can be done to stop users from accessing these .html files directly? Or doesn't it matter?
  • How will WordPress Multi Site cope with multiple users attempting to customize multiple themes?
  • How do plugins deliver templates and template parts in FSE?

@noahtallen
Copy link
Member

noahtallen commented Nov 27, 2020

Continuing some of my thought from #27284:

Performance: on each wp_load or QPQuery for the templates, we compare the file modification data to ensure whether we need to update the templates or not. This might become costly depending on the number of templates and templates parts we need to deal with.

I do wonder how performance compares if we are reading from the filesystem on every request. For example, is rendering a template by reading from the database significantly faster than rendering a template by reading from the filesystem? If it is much faster, maybe we would prefer to keep the CPT approach for rendering? Currently, we just compare file stats, but if we stopped using CPTs, we would also be reading the whole file directly which is more expensive. I also wonder if this prevents us from using caching mechanisms as easily.

I also think we can iterate the sync experience so that it does less work. It doesn't need to sync everything on every load. We could make it smarter and therefore more performant :)

Also, regarding developer experience:

  • Are we gaining a lot of built-in stuff by relying on posts? E.g. queries, entity resolvers, block content parsing, etc. Would we have to re-build that for an alternative approach?
  • Would we be making everything less consistent by having two different storage methods for templates? Like, we would no longer be able to ask WordPress to give us all entities with postType: 'wp_template'. We would have to do a query for template CPTs, and then a filesystem query. I wonder if the developer experience of just treating the database wp_template CPT as the source of truth would be better.

Perhaps I misunderstand, but in my opinion, I think the sync mechanism we have right now (which just happens on wp_load) is really not that complex. (Though it certainly used to be!) I worry that creating some alternative method dealing with some non-post entity would be more complicated just because it is different from how everything else works 🤔

@mtias
Copy link
Member

mtias commented Nov 27, 2020

Loading templates from files is how WordPress has worked since themes were introduced, so there's a strong precedent in how things work. The database should be generally relied upon for user content and configuration; storing every theme file on theme switch as post types just seems convoluted, increasing the weight of the database with no mechanisms for freeing it later on (say, if you "deactivate" or "remove" a theme). These are things that could be accounted for in code, for sure, but it raises the question: what drives the need for copying files into posts in the first place?

@carlomanf
Copy link

  • Resolving a template would require fetching both the saved templates and the available files in order to find the right template for a given page.
  • The frontend should work if there are no CPTs at all, by simply falling back to the template file. It might make sense to start by implementing this part, as it should be pretty straight-forward. (Might also be useful to add unit test coverage early.)

I think it should be pointed out that these behaviours were already implemented for templates until they were changed in version 9.4, and I believe they still are happening into 9.4 for template parts.

@noahtallen
Copy link
Member

Those are great points matias, thanks for the comment :)

what drives the need for copying files into posts in the first place?

In my mind, beyond the need for editing, an answer might be to have a single source of truth for templates. That makes it easier to do things like "get a list of all the templates" (perhaps for displaying in wp-admin). With an alternate approach, we don't have a source of truth -- it could be the filesystem, or it could be the database. We'd need to create new API endpoints or new functions for compiling the data, without relying on the post query stuff. And that's not just for Gutenberg, but also for plugin/theme developers.

sThat said, if we do need dynamic templates, we probably can't use the database for them, so we might need to build all of that out anyways. 🤔

@youknowriad
Copy link
Contributor Author

That makes it easier to do things like "get a list of all the templates"

In my mind, for the client, this can remain by potentially having a dedicated endpoint for templates (and not the normal CPT one) and potentially similar APIs in the backend.

@mtias
Copy link
Member

mtias commented Nov 30, 2020

I don't think source of truth is a very compelling argument in this case given interaction with it happens through abstracted APIs anyways. The single source of truth is also a bit of a mirage since theme files already exist separately, requiring a synchronization and reconciliation step to feed the custom post type, plus extra logic to discriminate user content, and likely some involved setup to handle translations.

@aristath
Copy link
Member

Today I started just spitballing ideas in a branch, mostly experimenting with ideas and checking which areas of the code will be affected.

  • we'll be able to remove the templates-sync implementation
  • we'll be able to remove the wp_theme taxonomy
  • a lot of code will be cleaned up (like really a lot of it)

I didn't get things to work in my branch, but from my brief exploration, it's definitely possible.
Ideally, we'd introduce a new REST endpoint and not use the one for posts. If we use the one we currently have for posts we can still do it, but we'll need to fake the file templates inside the response (doable, but ugly).

@ockham
Copy link
Contributor

ockham commented Nov 30, 2020

@noahtallen I think we can mitigate a lot of those developer experience/single-source-of-truth concerns by providing abstractions at the right level -- such as a REST API endpoint, as suggested by @youknowriad.

At an even lower level, we can have PHP functions that encapsulate the CPT/file lookup that can be used for frontend rendering, in combination with template resolution.

An approach like that would allow us to encapsulate the 'hybrid' CPT/file lookup into a more abstract template lookup function where possible, and to expose the different mechanisms to the developer where needed (e.g. for the Site Editor). I believe that an approach like this would represent the underlying constraints quite well.

@aristath
Copy link
Member

we can have PHP functions that encapsulate the CPT/file lookup that can be used for frontend rendering, in combination with template resolution.

Yeah, getting the frontend to work was relatively easy in my brief experiment (see this code for an ugly - but working - example).
The frontend worked fine, the complicated implementation will be for the site-editor 'cause that's where we'll need the new REST endpoint and lots of refactoring. It's definitely worth it though, the overall structure and data path becomes a lot cleaner if there are no auto-drafts.

@aristath
Copy link
Member

aristath commented Dec 2, 2020

As an experiment, I tested using a REST endpoint like this: https://github.com/WordPress/gutenberg/blob/11fd10cb4ef97021c914a035b5a79ad494b7c125/lib/class-wp-rest-theme-templates.php
The endpoint works fine... The issue is that in all our JS we assume a post object from the REST response and that's what makes things a little harder. file-based templates will not have an ID, and that makes things a bit weird.

Before we attempt removing drafts, I think the first step would be to go through all instances of findTemplate and think about what we'll do if there is no actual post

@youknowriad
Copy link
Contributor Author

The issue is that in all our JS we assume a post object from the REST response and that's what makes things a little harder. file-based templates will not have an ID, and that makes things a bit weird.

I think maybe we need a way to create "new entity records" in "core-data" with "temporary ids" and replace these on save.

@bobbingwide
Copy link
Contributor

Just recently I've been investigating a solution for internationalization & localization that will work directly against the template and template parts without any need for special mark up in the HTML.

I published a Feature request yesterday. See #27402

@aristath I'd like to be able to understand how you intend to achieve your solution and how this might fit in with i18n/l10n.

It would be nice to have an outline of your proposed changes.

I assume the primary requirement is to improve server response when the theme is not customised in any way.

But, assuming some form of customisation is necessary - menus and widgets being the first things to be changed - then how will the solution work?

  • How will user customised templates / template parts be stored on the server?
  • How will this work for WordPress Multi Site?
  • Will there be autosaves and revisions?
  • When a theme is updated how will a user 'reconcile' their customized parts with the theme's changes?
  • Will there be support for child themes?
  • What about grandchild themes - which currently require a plugin based solution?

@aristath
Copy link
Member

aristath commented Dec 2, 2020

@aristath I'd like to be able to understand how you intend to achieve your solution and how this might fit in with i18n/l10n.

It doesn't affect i18n in any way. Whether the strings exist in the db or a file should not affect i18n.

It would be nice to have an outline of your proposed changes.

There is no outline because there are no proposed changes. At this stage I'm simply experimenting, trying to identify the pain points and getting an idea of what we'll need to do so that a plan can be formulated and implemented.

The idea here is that only customized templates will be saved in the db. If nothing was customized in the index file for example, then no index post will be created in the db (with the wp_template post-type).
Since only modified templates/template-parts will be saved in the db, there will not be an auto-draft post-status for the post-types, and a lot of things will internally be significantly simplified.

How will user customised templates / template parts be stored on the server?

Same as now, in posts with the wp_template post-type.

How will this work for WordPress Multi Site?

Multisite or single site should be the same, there is no need to have anything different for that.

Will there be autosaves and revisions?

autosaves no because only modified templates will be stored in the db. For revisions I don't know, I don't think that has anything to do with this issue 🤔

When a theme is updated how will a user 'reconcile' their customized parts with the theme's changes?

Customized parts cannot - and should not - be automatically reconciled. If the user wants to update their customized templates they can do it manually. Non-customized templates won't have an issue and there won't be a need to reconcile anything (so we can probably trash the lib/templates-sync.php file once this lands).

Will there be support for child themes?

Of course. It's just a matter of checking both the parent and child themes when retrieving the list of template files.

What about grandchild themes - which currently require a plugin based solution?

Grand-child themes were never officially supported in WordPress. They work without a plugin in general, but that is mostly a happy accident and not a conscious decision or implementation. I don't see why we couldn't do it, but I don't believe it will be a priority.

@bobbingwide
Copy link
Contributor

Thanks. I'd very much appreciate it if you could read my proposal in which locale specific templates and template parts are built statically. With this solution the Site Editor would need to know which locale the user is working in and load the templates and template parts from the appropriate folder. If the user's locale is not available, then the default ( en_US ) would be used.

I'm also wondering what the process might be for a theme developer.
The current process of exporting to a zip file is not a good experience.
At present it's much easier to work in an IDE. copying and pasting any changes you make from the Code editor for wp_template or wp_template_part or any other post.

Customized parts cannot - and should not - be automatically reconciled. If the user wants to update their customized templates they can do it manually.

How would the end user access the changed templates?
What if there was a security fix?

@aristath
Copy link
Member

aristath commented Dec 2, 2020

How would the end user access the changed templates?

If they delete their customized template the default will be used. Maybe there will be a button to revert a template to its default state. Whatever the solution, it wouldn't be related to this issue which is about db-vs-file templates when they are not customized.

I'm also wondering what the process might be for a theme developer.
The current process of exporting to a zip file is not a good experience.
At present it's much easier to work in an IDE. copying and pasting any changes you make from the Code editor for wp_template or wp_template_part or any other post.

The current process is built having in mind that the user will be able to export their theme, get a file, and then they'll be able to simply upload that file to another site.
You can still use the editor and copy-paste the results from it, it's under Appearance > Templates.
But again, this is not something that is related to this ticket which is about db-vs-file templates when they are not customized.

What if there was a security fix?

I don't see how a security issue would even be possible in what will basically be just blocks content saved in the editor. Even if PHP was allowed in the theme template files, the moment the user customizes a template it gets saved in the db as a custom post, so any logic is lost and the only thing saved is the actual content. So there is no security concern.

@aristath
Copy link
Member

aristath commented Dec 3, 2020

I think maybe we need a way to create "new entity records" in "core-data" with "temporary ids" and replace these on save.

I'm afraid I don't know how to do that... Would someone else be able to work on this so it can move forward faster and not wait for me to familiarize myself with that part of the codebase? I feel this issue is too important to delay as a lot of other tickets can move forward once this is done.

@Copons
Copy link
Contributor

Copons commented Dec 11, 2020

So, I've been experimenting on this issue for a while, and I've finally managed to come up with something that could be workable. 🤔

You can check out the draft PR: #27624
There are no docs, no comments, no cleanup (the current sync is just commented out), etc.

The core concepts are:

  • Overload the REST controllers of templates and template parts to return both publish posts and theme files (only if there is no corresponding custom post already).
  • To be included in the response, theme files are converted into "fake" WP_Post objects, with a temporary ID (file-template-${ slug }). These fake posts are not saved in the database.
  • When the site editor request template/parts, it simply adds them to the entity store as usual. Fake posts are inserted with that file-template- ID, but they work nonetheless.
  • In the site editor, I've added a TemplateProvider wrapper for the normal EntityProvider. It observes the entity ID and, if it's file-template-, it saves the fake post into an actual auto draft, and updates the entity ID to the saved one (which is now a real post).
  • Just to clarify: auto drafts are only created when a template/part becomes the main editor's entity. This happens automatically for the front page template on site editor load, or whenever we change template from the sidebar.
    This will create more auto drafts than strictly needed, but to be honest that's not that much different than what happens when creating a new post.
  • Now that we have a real ID, we can modify the template and publish it. Next requests will use the publish template, and won't include its corresponding theme file.
  • I've also updated the Template Part block to use the correct fake template part, although I haven't tested template parts thouroughly yet.
  • In the PHP side, I've updated the template resolution to also consider the theme files if no relevant publish post is found.

There are quirks and kinks to iron out, some are visible (titles are a mess right now), some will be trickier to find.

I'd appreciate if someone would give it a spin. 🙇‍♂️
I've included some easy enough test steps in the PR!

@youknowriad
Copy link
Contributor Author

Good work here

In the site editor, I've added a TemplateProvider wrapper for the normal EntityProvider. It observes the entity ID and, if it's file-template-, it saves the fake post into an actual auto draft, and updates the entity ID to the saved one (which is now a real post).

I didn't take a look at the PR, the only thing that stands out to me in your explanation is this item. For me, ideally, this should be transparent and the "core-data" level. Meaning when we "save" an entity and the returned ID is different from the saved one, I imagine we could do a "mapping" to replace the fake id in state. Wouldn't something like that work? It seems also a good thing to have regardless, for instance, we could decide that we don't create auto-drafts in the post editor when loading the page, but we just create a local post object with a temporary id.

@carlomanf
Copy link

carlomanf commented Dec 12, 2020

Most of the React code goes over my head, but why not use 0 as the fake post ID? ID's are not meant to be strings. Also, why 'draft' as the post status?

@Copons
Copy link
Contributor

Copons commented Dec 14, 2020

Most of the React code goes over my head, but why not use 0 as the fake post ID? ID's are not meant to be strings. Also, why 'draft' as the post status?

@carlomanf The ID is definitely debateable.
We can't give all fake template posts 0 because they will be added as entities to the Redux state which is an ID-keyed object. I haven't actually tried, but I think they would replace each other.
On top of this, we also need a way to know which posts are fake. In my initial tinkering I gave them a negative ID, or tried with a (negative) timestamp. The idea was that if ID < 0 the post is fake.
That way though, we have less control over the fake posts, with the only added value of keeping the ID numeric — which is not a big deal imho: those are temporary IDs, and we won't use them when saving the post.
I think I had also some technical challenges with that approach, but I can't seem to remember it right now.

As for the draft status: again it's not a big deal, since they are not real posts. They could be publish for that matter.
But I guess your question is more like: "Since we are saving them as auto-draft, why not generating the fake posts as auto-draft as well?"
That's because auto-draft is an unwieldy beast.
Just to give you an example, I've had to filter the wp_template REST schema to allow us to save auto-draft via API.
But either way, the auto-draft status is handled in a very specific way by both WordPress core, and Gutenberg as well.
I was more interested in working on my proof of concept than exploring all the possible edge cases caused by using auto-draft on the fake post. 😄
And finally, exactly as for the string ID, I reasoned that what we do on the fake posts is just not a big deal.
They are only created by the Site Editor to fill a temporary gap. We actually only really need slug and content. Everything else is "dressing" to make sure they are handled appropriately by both new WP_Post() and the REST controller.

@mtias
Copy link
Member

mtias commented Jan 18, 2021

Thanks for the work on this one!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Framework Issues related to broader framework topics, especially as it relates to javascript [Priority] High Used to indicate top priority items that need quick attention [Status] In Progress Tracking issues with work in progress
Projects
None yet
Development

Successfully merging a pull request may close this issue.

8 participants