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: Add theme taxonomy to templates and template parts #27016

Merged
merged 40 commits into from
Nov 18, 2020

Conversation

david-szabo97
Copy link
Member

@david-szabo97 david-szabo97 commented Nov 16, 2020

Description

Fixes #24377

Replace theme meta with theme taxonomy. Register a taxonomy called wp_theme and use it to store the theme of the template and template parts.

A special value is used to mark templates that were created from a theme template file. This value is: _wp_file_based.

How has this been tested?

Everything should work the same way as before. You will need to start clean. (Delete all templates and template parts)

Types of changes

Breaking change

Checklist:

  • My code is tested.
  • My code follows the WordPress code style.
  • My code follows the accessibility standards.
  • My code has proper inline documentation.
  • I've included developer documentation if appropriate.
  • I've updated all React Native files affected by any refactorings/renamings in this PR.

@github-actions
Copy link

github-actions bot commented Nov 16, 2020

Size Change: -39 B (0%)

Total Size: 1.19 MB

Filename Size Change
build/block-library/index.js 147 kB -39 B (0%)
ℹ️ View Unchanged
Filename Size Change
build/a11y/index.js 1.14 kB 0 B
build/annotations/index.js 3.8 kB 0 B
build/api-fetch/index.js 3.42 kB 0 B
build/autop/index.js 2.84 kB 0 B
build/blob/index.js 664 B 0 B
build/block-directory/index.js 8.72 kB 0 B
build/block-directory/style-rtl.css 943 B 0 B
build/block-directory/style.css 942 B 0 B
build/block-editor/index.js 133 kB 0 B
build/block-editor/style-rtl.css 11.3 kB 0 B
build/block-editor/style.css 11.3 kB 0 B
build/block-library/editor-rtl.css 9.04 kB 0 B
build/block-library/editor.css 9.04 kB 0 B
build/block-library/style-rtl.css 8.1 kB 0 B
build/block-library/style.css 8.1 kB 0 B
build/block-library/theme-rtl.css 792 B 0 B
build/block-library/theme.css 793 B 0 B
build/block-serialization-default-parser/index.js 1.87 kB 0 B
build/block-serialization-spec-parser/index.js 3.06 kB 0 B
build/blocks/index.js 48 kB 0 B
build/components/index.js 171 kB 0 B
build/components/style-rtl.css 15.3 kB 0 B
build/components/style.css 15.3 kB 0 B
build/compose/index.js 9.95 kB 0 B
build/core-data/index.js 14.8 kB 0 B
build/data-controls/index.js 827 B 0 B
build/data/index.js 8.8 kB 0 B
build/date/index.js 11.2 kB 0 B
build/deprecated/index.js 768 B 0 B
build/dom-ready/index.js 571 B 0 B
build/dom/index.js 4.92 kB 0 B
build/edit-navigation/index.js 11.2 kB 0 B
build/edit-navigation/style-rtl.css 881 B 0 B
build/edit-navigation/style.css 885 B 0 B
build/edit-post/index.js 306 kB 0 B
build/edit-post/style-rtl.css 6.48 kB 0 B
build/edit-post/style.css 6.47 kB 0 B
build/edit-site/index.js 23.2 kB 0 B
build/edit-site/style-rtl.css 3.89 kB 0 B
build/edit-site/style.css 3.89 kB 0 B
build/edit-widgets/index.js 26.4 kB 0 B
build/edit-widgets/style-rtl.css 3.16 kB 0 B
build/edit-widgets/style.css 3.16 kB 0 B
build/editor/editor-styles-rtl.css 476 B 0 B
build/editor/editor-styles.css 478 B 0 B
build/editor/index.js 42.5 kB 0 B
build/editor/style-rtl.css 3.85 kB 0 B
build/editor/style.css 3.85 kB 0 B
build/element/index.js 4.62 kB 0 B
build/escape-html/index.js 735 B 0 B
build/format-library/index.js 6.86 kB 0 B
build/format-library/style-rtl.css 547 B 0 B
build/format-library/style.css 548 B 0 B
build/hooks/index.js 2.16 kB 0 B
build/html-entities/index.js 623 B 0 B
build/i18n/index.js 3.57 kB 0 B
build/is-shallow-equal/index.js 698 B 0 B
build/keyboard-shortcuts/index.js 2.54 kB 0 B
build/keycodes/index.js 1.94 kB 0 B
build/list-reusable-blocks/index.js 3.1 kB 0 B
build/list-reusable-blocks/style-rtl.css 476 B 0 B
build/list-reusable-blocks/style.css 476 B 0 B
build/media-utils/index.js 5.32 kB 0 B
build/notices/index.js 1.81 kB 0 B
build/nux/index.js 3.42 kB 0 B
build/nux/style-rtl.css 671 B 0 B
build/nux/style.css 668 B 0 B
build/plugins/index.js 2.56 kB 0 B
build/primitives/index.js 1.43 kB 0 B
build/priority-queue/index.js 790 B 0 B
build/redux-routine/index.js 2.84 kB 0 B
build/reusable-blocks/index.js 3.06 kB 0 B
build/rich-text/index.js 13.3 kB 0 B
build/server-side-render/index.js 2.77 kB 0 B
build/shortcode/index.js 1.69 kB 0 B
build/token-list/index.js 1.27 kB 0 B
build/url/index.js 4.05 kB 0 B
build/viewport/index.js 1.86 kB 0 B
build/warning/index.js 1.14 kB 0 B
build/wordcount/index.js 1.22 kB 0 B

compressed-size-action

@carlomanf
Copy link

Thanks for this. I just have a few questions:

A special values is used to to mark templates that were created from a theme template file. This value is: wp_file_based.

What's the purpose of this? Is it necessary? It would conflict if there happened to be a theme called wp_file_based.

You will need to start clean. (Delete all templates and template parts)

I want to be able to keep the ones I already have and have them automatically update to the new system. Would it be a good idea to automatically associate templates and parts with the current theme by default, similar to how posts get automatically put in Uncategorized?

@david-szabo97
Copy link
Member Author

What's the purpose of this? Is it necessary? It would conflict if there happened to be a theme called wp_file_based.

It will used to differentiate between file-based theme templates and user-created templates.

I want to be able to keep the ones I already have and have them automatically update to the new system. Would it be a good idea to automatically associate templates and parts with the current theme by default, similar to how posts get automatically put in Uncategorized?

I've added a function that migrates from meta to taxonomy. Good point there, it would be difficult to ask everyone to start clean 😄 Later on we can remove this function.

@carlomanf
Copy link

What's the purpose of this? Is it necessary? It would conflict if there happened to be a theme called wp_file_based.

It will used to differentiate between file-based theme templates and user-created templates.

I gathered that much, but I mean where will this differentiation be needed?

I want to be able to keep the ones I already have and have them automatically update to the new system. Would it be a good idea to automatically associate templates and parts with the current theme by default, similar to how posts get automatically put in Uncategorized?

I've added a function that migrates from meta to taxonomy. Good point there, it would be difficult to ask everyone to start clean 😄 Later on we can remove this function.

Thanks, but I think we could go a step further and implement something permanent. My understanding is theme-less templates and parts are not being supported for the time being, so wouldn't it make sense for there to be some kind of "sanitisation" on saving the post?

@aristath
Copy link
Member

Is this a duplicate of #24720 ? 🤔

@gziolo
Copy link
Member

gziolo commented Nov 17, 2020

@aristath, good catch. I was about to ask exactly the same question. As far as I can tell it look nearly identical.

@david-szabo97
Copy link
Member Author

Is this a duplicate of #24720 ? 🤔

@aristath Would you like to rebase your PR and finish it up? I'd be happy to close this PR as a duplicate and review yours.

@aristath
Copy link
Member

No need... Yours is up to date and rebasing mine would be a waste of time since we already have this one.
I'll close mine as superseded by this one, review it and see if there's anything missing (though from a quick look it looks OK). As long as it gets done it doesn't matter who does it 👍

lib/templates.php Outdated Show resolved Hide resolved
@david-szabo97
Copy link
Member Author

No need... Yours is up to date and rebasing mine would be a waste of time since we already have this one.
I'll close mine as superseded by this one, review it and see if there's anything missing (though from a quick look it looks OK). As long as it gets done it doesn't matter who does it 👍

Thank you! Sorry for hijacking your PR 😅

Copy link
Member

@aristath aristath left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Overall this looks good!

I only found 1 part that was missed, but I didn't want to push directly to your branch so there's a small additional PR against this one here: #27042

@david-szabo97
Copy link
Member Author

Hopefully, perf test is now going to pass. Perf test was failing because we use the same database for testing the master and this branch. Since master branch is ran first in the perf test, it creates all the templates (with meta) and this branch won't find any templates because the theme terms are missing. (Since master created the templates) To fix this, we also delete the auto-draft templates so we can regenerate the templates with terms instead of metas.

Copy link
Member

@noahtallen noahtallen left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM! That bug is fixed now. :shipit:

@noahtallen noahtallen merged commit 69e19d7 into master Nov 18, 2020
@noahtallen noahtallen deleted the add/theme-taxonomy branch November 18, 2020 21:52
@github-actions github-actions bot added this to the Gutenberg 9.5 milestone Nov 18, 2020
@youknowriad
Copy link
Contributor

Hi there! Can we update the architecture docs to reflect these changes? Can we explain the purpose of each of these taxonomies?

Also what value the "_wp_file_based" provides? A user might edit the template entirely which might make this taxonomy meaningless.

@Copons
Copy link
Contributor

Copons commented Nov 23, 2020

Also what value the "_wp_file_based" provides? A user might edit the template entirely which might make this taxonomy meaningless.

@youknowriad _wp_file_based indicates the "origin" of a template/part.
As long as it's auto-draft, we know that it hasn't been modified, and so it's exactly like its file counterpart.

Once it's modified, its status will change to anything else (draft, publish). Combined with _wp_file_based it means that:

  • The template was originally converted from a file.
  • It is now customized.
  • We can "revert" it to be like the original file.

Without the _wp_file_based tag, we wouldn't have a good way to know if a template was automatically created from a file.
Initially we relied only on the auto-draft status, but that's not necessarily enough, as users can create auto-draft templates if they start editing a template with the normal post editor (e.g. from /wp-admin/edit.php?post_type=wp_template) and leave without saving.

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 this pull request may close these issues.

FSE Post Meta Queries Performance
7 participants