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

Site Editor: Templates Reconciliation Performance #25868

Closed
Copons opened this issue Oct 6, 2020 · 7 comments · Fixed by #26383
Closed

Site Editor: Templates Reconciliation Performance #25868

Copons opened this issue Oct 6, 2020 · 7 comments · Fixed by #26383
Assignees

Comments

@Copons
Copy link
Contributor

Copons commented Oct 6, 2020

In #25739 we merged an update to the templates handling that could affect the Site Editor performance on sites with many templates and template parts.

(cc @epiqueras who worked on this and might have a deeper knowledge of the whole logic.)

Template Reconciliation

Templates (and template parts) can be both provided by the theme as HTML files, or by the site as wp_template (and wp_template_part) posts.
Since there isn't a single source of truth, they need to be reconciled every now and then.

When the reconciliation happens:

  • If the template is a post with status publish (meaning: it has been modified and saved by the user), we don't need to reconcile.
  • If the template is a post with status auto-draft (meaning: it is a direct copy from the original template file), we compare it with the template file, check if the content has been changed, and update the template post if needed.
  • If the template file hasn't been stored as post yet: insert a new post with status auto-draft using the template file as content.

This reconciliation happens by filtering rest_wp_template_query (and rest_wp_template_part_query), which runs on any REST request on wp_template (and wp_template_part).

Before #25739, we only reconciled the requested template or template part.
Now, we reconcile all templates and template parts.

Audit

If the site has a lot of templates, this might become very performance intensive.

Let's check what kind of performance impact this have, and address it as much as we can.
Chances are that we don't really need to reconcile all templates so often.

@david-szabo97
Copy link
Member

david-szabo97 commented Oct 21, 2020

I'm happy to take this since I introduced this feature 😄

I think the most straightforward solution is to only reconcile the template and template parts when the

  • theme installed theme activated
  • theme updated

I'd also add one more to the basket: reconcile when the site editor is opened. This is required to make sure that whenever the user makes changes to the HTML files or create new ones, they are reconciled. Since the child theme where the user add/update these files aren't going to be "updated". This request can run in the background without affecting anything on the site editor.

What do you think?

@Copons
Copy link
Contributor Author

Copons commented Oct 21, 2020

I'm happy to take this since I introduced this feature 😄

I think the most straightforward solution is to only reconcile the template and template parts when the

  • theme installed
  • theme updated

I'd also add one more to the basket: reconcile when the site editor is opened. This is required to make sure that whenever the user makes changes to the HTML files or create new ones, they are reconciled. Since the child theme where the user add/update these files aren't going to be "updated". This request can run in the background without affecting anything on the site editor.

What do you think?

There is another possible case, probably not very common: users can manually modify the theme files without releasing a full theme update.
And they can do it either inside wp-admin (Appearance -> Theme Editor; we might have access to a hook here), or directly via FTP (etc.).

So: the three reconciliation cases you mentioned should be plenty enough to cover wp-admin.
I wonder if we need to do anything for the front end.

E.g.
User creates a new template file directly on the site's server, without triggering any reconciliations. What happens?

  • The site already has a wp_template for the same content. Which one will be used? The wp_template or the new file template?
  • The site doesn't have any templates for that content. Will the new file template be used, or will the site fall back, for example, to the index template?

@david-szabo97
Copy link
Member

  • The site already has a wp_template for the same content. Which one will be used? The wp_template or the new file template?

If they are the same priority (eg. you created a front-page entity of wp_template then created a front-page.html file) then the wp_template will be used and the file will be ignored.

  • The site doesn't have any templates for that content. Will the new file template be used, or will the site fall back, for example, to the index template?

The one with the higher priority. If you had a wp_template with index slug but you created front-page.html in the theme, then the front-page.html will be visible when you visit the front page. In this case wp_template entity isn't created, it's only in memory.

@Copons
Copy link
Contributor Author

Copons commented Oct 21, 2020

Got it.
So as it seems we don't need to do anything for the front end! Nice!

@carlomanf
Copy link

carlomanf commented Nov 2, 2020

I'm happy to take this since I introduced this feature 😄

I think the most straightforward solution is to only reconcile the template and template parts when the

  • theme installed
  • theme updated

I'd also add one more to the basket: reconcile when the site editor is opened. This is required to make sure that whenever the user makes changes to the HTML files or create new ones, they are reconciled. Since the child theme where the user add/update these files aren't going to be "updated". This request can run in the background without affecting anything on the site editor.

What do you think?

Just to clarify, when you say "theme installed" do you mean "theme activated?"

Furthermore, if you do mean "theme activated," should the reconciliation take place on the same auto-draft from the previously active theme, or should a new auto-draft be created for the new theme?

I'm hoping the former, as it would avoid polluting the database, as long as there are no side effects.

@aristath
Copy link
Member

aristath commented Nov 3, 2020

Semi-related: Part of the performance-degradation issue when there are many templates is the use of post-metas. We are tracking that on #24377 and the related PR on #24720

@david-szabo97
Copy link
Member

Just to clarify, when you say "theme installed" do you mean "theme activated?"

@carlomanf Yes, I mean activated. Corrected it in my comment.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants