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: Stop using auto-draft for auto-generated templates and template parts #27277

Closed
wants to merge 5 commits into from

Conversation

Copons
Copy link
Contributor

@Copons Copons commented Nov 25, 2020

Description

This PR proposes a change of direction regarding templates and template parts automatically generated from theme-provided files.
Instead of creating auto-draft posts, we create publish posts with a new internal _wp_is_original term (in the recently introduced wp_theme taxonomy).

The _wp_is_original term indicates those templates and template parts provided by the theme, that haven't been customized by the user yet.

Auto-generated templates and template parts will be created with a _wp_is_original term.
Once updated, they will lose that term, and won't be able to get it back.

✨ Props to @david-szabo97 for the idea! ✨

Reasons

The auto-draft status is a rather unwieldy beast.
It's supposed to be used for posts that have been created and not manually saved yet; once they are saved, they gain a "stable" status, such as draft or publish. While the description seems to fit auto-generated templates, there are more quirks to consider.

auto-draft is not supposed to be handled by the user. It doesn't appear in normal queries (we always need to query specifically for that status) and it doesn't show up in wp-admin. WP doesn't even provide localized labels to it when it's registered.

In Gutenberg, auto-draft posts are not supposed to have titles. While auto-draft posts per se can have titles, Gutenberg strips it from them.

auto-draft posts don't create revisions. This is logical, given the "auto-draft" state should only be a bridge between editor load and first save.
But even manually calling wp_save_post_revision isn't of any help, since it's a no-op for auto-draft posts.

auto-draft posts can be pruned by functions such as wp_delete_auto_draft, and also typically by database cleaning plugins. (Props to @carlomanf for noticing this!)

Impact

  • This PR shouldn't change the behaviour of Site Editor and front-end with one exception: the Template Part block's preview selector doesn't return "theme-agnostic" template parts anymore.
    I'm considering this not an issue, considering we are removing that feature for the foreseeable future in Site Editor: Only request templates and template parts for the current theme #27152 anyway.

  • I'm marking this PR as "breaking change", but in fact it should be relatively backward-compatible.
    Templates and template parts that have been already customized won't be affected.
    Old auto-drafts will be ignored (not deleted, though), and replaced with new auto-generated posts with status publish and _is_wp_original term.

How has this been tested?

Smoke test everything! 🎉

Types of changes

Breaking change (fix or feature that would cause existing functionality to not work as expected)

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 25, 2020

Size Change: +328 B (0%)

Total Size: 1.2 MB

Filename Size Change
build/block-editor/index.js 134 kB +13 B (0%)
build/block-editor/style.css 11.3 kB +4 B (0%)
build/block-library/editor-rtl.css 8.95 kB +1 B
build/block-library/editor.css 8.96 kB +3 B (0%)
build/block-library/index.js 148 kB -1 B
build/edit-post/index.js 306 kB +30 B (0%)
build/edit-site/index.js 23.9 kB +348 B (1%)
build/edit-site/style-rtl.css 3.92 kB -2 B (0%)
build/edit-site/style.css 3.92 kB -2 B (0%)
build/edit-widgets/index.js 26.3 kB +6 B (0%)
build/editor/index.js 43.3 kB -72 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/style-rtl.css 11.3 kB 0 B
build/block-library/style-rtl.css 8.23 kB 0 B
build/block-library/style.css 8.23 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.1 kB 0 B
build/components/index.js 172 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.95 kB 0 B
build/edit-navigation/index.js 11.1 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/style-rtl.css 6.42 kB 0 B
build/edit-post/style.css 6.4 kB 0 B
build/edit-widgets/style-rtl.css 3.13 kB 0 B
build/edit-widgets/style.css 3.13 kB 0 B
build/editor/editor-styles-rtl.css 476 B 0 B
build/editor/editor-styles.css 478 B 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.27 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 2.92 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

@Copons Copons changed the title Try/fse templates original term Full Site Editing: Stop using auto-draft for auto-generated templates and template parts Nov 25, 2020
@Copons Copons added [Feature] Full Site Editing [Status] In Progress Tracking issues with work in progress labels Nov 25, 2020
@Copons Copons marked this pull request as ready for review November 25, 2020 17:14
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.

Overall, this makes sense to me. It seems to clean up the code quite a bit, and I'm not really noticing any issues in the site editor. A bit tricky to test, though :)

I wonder, is _wp_is_original verbose enough? Should it be something like _wp_is_original_template_file?

Also curious if others have opinions about going with this approach instead. But from my PoV, I'm in favor

* [
* 'is_file_based', // Whether the template is based upon a theme-provided file.
* 'is_original', // Whether the template is the original theme-provided file.
* 'themes', // An array of themes that this template belongs to.
Copy link
Member

Choose a reason for hiding this comment

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

How can a template belong to multiple themes?

Copy link
Member

Choose a reason for hiding this comment

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

It can't right now. But in the future we might want to share templates between multiple themes. Which would be possible by assigning it to multiple themes.

Something like that I guess.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We will also eventually decide what to do with child themes that inherit templates from their parent.
One (naive, probably) way to handle it is to have both child and parent themes slugs in the array. 🤔

select( 'core' ).getEntityRecords( 'postType', 'wp_template_part', {
theme: currentTheme,
status: [ 'auto-draft' ],
theme: [ currentTheme, '_wp_is_original' ],
Copy link
Member

Choose a reason for hiding this comment

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

Why does it all go into a single array?

Copy link
Member

Choose a reason for hiding this comment

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

They are stored in the same taxonomy.

Copy link
Contributor

Choose a reason for hiding this comment

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

What if a theme is called _wp_is_original or _wp_file_based

@carlomanf
Copy link

I like it, but I find the naming of _wp_file_based and _wp_is_original confusing, especially since the one that matches the file is NOT the one that includes the word "file" in its name. Perhaps _wp_theme_based and _wp_theme_original?

@noahtallen
Copy link
Member

I like those names better too 👍

@carlomanf
Copy link

Although even wp_theme_original seems to imply that it "originated" from the theme, not necessarily that it matches the theme. The point is, some thought should be given to arrive at the right naming.

@carlomanf
Copy link

I can also imagine users saying something like, "I made an original template," which would mean the exact opposite of what "original" means here.

@carlomanf
Copy link

_wp_was_file and _wp_is_file?

$themes = wp_get_post_terms( $post_id, 'wp_theme' );
if ( ! $themes ) {
wp_set_post_terms( $post_id, array( wp_get_theme()->get_stylesheet() ), 'wp_theme', true );
}
if ( $update && has_term( '_wp_is_original', 'wp_theme', $post_id ) ) {
wp_remove_object_terms( $post_id, '_wp_is_original', 'wp_theme' );
}

Choose a reason for hiding this comment

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

I think this would wrongly remove the term if it's being updated based on a file change?

Copy link
Contributor

@youknowriad youknowriad left a comment

Choose a reason for hiding this comment

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

I like where this is going, good job;

select( 'core' ).getEntityRecords( 'postType', 'wp_template_part', {
theme: currentTheme,
status: [ 'auto-draft' ],
theme: [ currentTheme, '_wp_is_original' ],
Copy link
Contributor

Choose a reason for hiding this comment

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

What if a theme is called _wp_is_original or _wp_file_based

@Copons
Copy link
Contributor Author

Copons commented Nov 26, 2020

I've pushed a rather big update to this in 27f2166.

Remember: names are up for discussion!

  • Add a new wp_flag taxonomy, to avoid conflating themes and flags.
  • wp_flag handles three terms.
    Note that they are not namespaced anymore, as now they can't conflict with theme slugs.
    • theme_file_original: indicates posts that are the direct conversion of the original theme files.
    • theme_file_based: indicates posts that originated from theme files, and have been customized.
    • theme_sync: temporary flag to indicate that a "original" post is updating automatically in a sync.
  • Update the wp_template_part REST requests to accept a new flags array parameter to filter by flags.
  • Update the wp_template_part REST responses to include theme_file_original and theme_file_based.

The theme_sync flag

As pointed out by @carlomanf, removing the theme_file_original flag on all updates means that we would also remove it when updating original templates because their file counterparts changed.

Obviously, we only want to remove the _original flag when the file is customized by the user, but as far as I can see, we don't have a good way to determine that.

My solution proposed here (which I'm not very fond of), is to add a theme_sync flag when we update the post, then check it in the save_post hook.

  • If the save is an update, and the template has the theme_sync flag, it means that it's an "original file update". We can remove theme_sync and leave the other flags unaffected.
  • If the save is an update, and the template has the theme_file_original flag but doesn't have theme_sync, it means that it's a user customization. In this case we can remove the theme_file_original and turn the template into a custom one.

@bobbingwide
Copy link
Contributor

Good. I'm glad you've done this. I'm currently battling with auto-draft posts for templates where I've renamed the .html file, but the auto-draft has persisted since the reconciliation doesn't check for deleted files.
eg A page-$id.html file where the post ID was incorrectly chosen.

I've also noted a problem where the auto-draft was created for different user IDs. In my case 0 and 1.
The original post was being loaded, not the one most recently synchronsed.
Can you confirm what value the post_author will be set to?

Finally, when will synchronization be performed?
On every single load or only for logged in users who have the authority to manage templates?

@Copons
Copy link
Contributor Author

Copons commented Nov 26, 2020

Good. I'm glad you've done this. I'm currently battling with auto-draft posts for templates where I've renamed the .html file, but the auto-draft has persisted since the reconciliation doesn't check for deleted files.
eg A page-$id.html file where the post ID was incorrectly chosen.

@bobbingwide Excellent reminder.
I did notice this problem and totally forgot to file an issue for it. Will do it ASAP. #27304

I've also noted a problem where the auto-draft was created for different user IDs. In my case 0 and 1.
The original post was being loaded, not the one most recently synchronsed.
Can you confirm what value the post_author will be set to?

We are not explicitly setting the post author of templates and template parts, so it will default to the current user.
I presume that author 0 is for templates synced during a load by a logged out user.

Finally, when will synchronization be performed?
On every single load or only for logged in users who have the authority to manage templates?

Currently it's on every single load.
There are a number of discussions about this (most recent one that comes to mind: #27284), but the short answer would be: we used to have a very complicated sync logic, which became unmanageable.

I wonder if we might only sync while in wp-admin, though.
It might be enough to cover all cases (except, like, a manual theme file change over FTP?) 🤔

@Copons Copons removed the [Status] In Progress Tracking issues with work in progress label Nov 26, 2020
@noahtallen
Copy link
Member

I wonder if we might only sync while in wp-admin, though.

Yeah, I guess that would be good enough, assuming this flow still works:

  1. activate new theme
  2. Immediately close wp-admin
  3. visit front end as a logged-out user

In that case, the templates would need to have been synced for the site to work. I'm not sure if wp_load is run at all after the theme is activated before you visit a different wp-admin page.

@mattwiebe
Copy link
Contributor

mattwiebe commented Nov 26, 2020

I'm not sure if wp_load is run at all after the theme is activated before you visit a different wp-admin page.

In wp-admin, the sequence is something like

  1. Visit themes page
  2. Activate new theme - request to wp-admin finishes loading, redirects to:
  3. a new load of the themes page where "you've activated __ theme" banner shows

So, wp_load will fire during phase 3, before the frontend is visited.

Now, if we want to start locking down the re-sync logic, we can look instead at using the after_switch_theme hook, and then it appears upgrader_process_complete for when the theme updates.

That won't, however, deal with resyncing a theme that's in active development locally. Some ideas for that:

  1. button somewhere in wp-admin to perform the sync
  2. set a constant (or even the presence of a file in the theme) that performs the sync on every load, like now
  3. wp-cli command

@noahtallen
Copy link
Member

So, wp_load will fire during phase 3, before the frontend is visited.

Nice, thanks for that extra detail! Does that also happen if activating a theme programmatically?

@carlomanf
Copy link

We are not explicitly setting the post author of templates and template parts, so it will default to the current user.
I presume that author 0 is for templates synced during a load by a logged out user.

Could there be a way to use the post author to determine whether the update is being done by the system or by the user, as an alternative to all these extra flags? I'm thinking to set it to 0 when the system is updating it, and set it to the current user when they are doing a manual edit?

@mattwiebe
Copy link
Contributor

Does that also happen if activating a theme programmatically?

It won't be exactly that, but the very next request to the site will call after_switch_theme on the very next request to the site, whatever it is. It's ultimately called on init at 99 priority, and init fires on both admin and frontend requests.

@Copons Copons closed this Jan 11, 2021
@Copons Copons deleted the try/fse-templates-original-term branch January 11, 2021 10:53
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.

7 participants