Add required logic for Block template themes #1267

ockham · 2021-05-19T19:23:58Z

Trac ticket: https://core.trac.wordpress.org/ticket/53176

List of files in Gutenberg, and where to map them to in Core

Attempting to fit into Core's existing file hierarchy and naming scheme.

lib/template-canvas.php -> src/wp-includes/

lib/full-site-editing/:

phpunit/:

class-block-templates-test.php -> tests/phpunit/tests/block-template-loader.php
- Unit test coverage for the block-template loading functions (load from file or wp_template, respectively).
~~class-edit-site-export-test.php~~ Not needed for WP 5.8.
- Test coverage for edit-site-export.php.
class-template-loader-test.php -> tests/phpunit/tests/block-template.php
- Unit test coverage for the block-template relevant modifications for the template resolution algorithm.
class-wp-rest-template-controller-test.php -> tests/phpunit/tests/rest-api/rest-templates-controller.php

phpunit/fixtures/themes/:

test-theme/ -> tests/phpunit/data/themedir1/block-template-theme/
test-theme-child/ -> tests/phpunit/data/themedir1/block-template-theme-child/

TODO

Fix theme loading in tests/phpunit/tests/block-template.php.
Use different theme in tests/phpunit/tests/rest-api/rest-templates-controller.php (i.e. not TT1 Blocks, since it's not included with Core) to make it pass. Should ideally be a test block template theme fixture that we're including with wordpress-develop. Maybe we can use tests/phpunit/data/themedir1/block-template-theme/ (or extend it a bit).
Remove template parts specific bits from the templates REST API controller and its unit test (unless we decide to include template parts in WP 5.8 after all).
Remove _gutenberg_ and gutenberg_ prefixes.
Make sure the loading order in src/wp-settings.php is fine.
Revisit if we need if ( gutenberg_supports_block_templates() ) conditionals (e.g. for post type registration in in post.php)
Add/update PHPDocs to cover added/changed functionality; add @since items where needed.

Testing instructions

To run unit tests locally:

npm run test:php tests/phpunit/tests/block-template.php
npm run test:php tests/phpunit/tests/block-template-loader.php
npm run test:php tests/phpunit/tests/rest-api/rest-templates-controller.php

(or just omit the file args altogether to run all unit tests)

Furthermore, try in your browser: Activate a block template theme (e.g. TT1 Blocks), and verify that it renders properly on the frontend. (Make sure that the GB plugin isn't active!)

Any necessary customizations to GB can be done after the fact and shouldn't be a blocker to this PR.

There's a chance that this list isn't exhaustive -- I might've missed some required files.

This Pull Request is for code review only. Please keep all other discussion in the Trac ticket. Do not merge this Pull Request. See GitHub Pull Requests for Code Review in the Core Handbook for more details.

## 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)).

youknowriad · 2021-05-24T07:57:25Z

Some updates here:

Removed all changes related to the WP-Admin menus for the CPT (these are not going to be shipped).
Moved some files that are not "admin" into "wp-includes".
Enable the block-templates support by default.

The next step for me is going to make everything work as it should in the UI before attacking the code cleaning.

youknowriad · 2021-05-24T11:56:27Z

@ockham I removed all the logic that fetches templates from files (block based themes) from the code and left just the part about custom template (the CPT). I wonder if resolve_block_template is still necessary. At least I think there's a few things we can remove from the resolution logic (as we don't care about theme files)

youknowriad · 2021-05-24T17:40:34Z

@ockham I wanted to add that I had to change the "prepare_for_database" function in the REST API controller because the create_item unit test was failing (compare it with Gutenberg). I think the failure was legitimate and we probably should fix that function similarly in Gutenberg as well, though not sure why the unit test was not failing there.

(Just wanted to note this comment somewhere so I don't forget about it :P )

src/wp-includes/post.php

carlomanf · 2021-05-25T07:03:36Z

src/wp-includes/theme-templates.php

+	// environment.  Unfortunately using `get_the_terms` for the 'wp-theme'
+	// term does not work in the case of new entities since is too early in
+	// the process to have been saved to the entity.  So for now we use the
+	// currently activated theme for creation.


Is it really such a good idea to merge this to core with an unresolved todo note? I have WordPress/gutenberg#31971 open, which basically solves this fully and will eliminate a lot of future headaches. Although it must be thoroughly tested, it's not intended to change any existing behaviour.

Today, is the last day for feature freeze so we need a solution in. I think the current solution is decent but your approach in the PR look promising potentially. We can look at it at ease and make the right call for beta1...

I don't think the current solution is scalable because taxonomies and slugs offer too many features that are not suitable for this purpose, nor are those systems aware of the constraints that are needed here. For example, the use of get_the_terms()[0] is problematic, because if a template somehow has two theme terms, and one is deleted, it can suddenly change theme. That could then trigger a slug conflict that causes the front-end to render differently.

If the taxonomy makes it into 5.8, I can see a potentially complicated and risky database migration needing to be done in a later version, and it would be nice to avoid that.

@carlomanf I think Riad's point is that we need to merge this PR today in order to get the template infrastructure into Core, since it's the last possible date (due to feature freeze) for 5.8. A PR like your WordPress/gutenberg#31971 (which looks quite good to me at first glance BTW!) could be considered a bug fix that we can merge even after feature freeze, so it makes it into 5.8.

… wp-includes/theme.php

See WordPress/gutenberg#32116

ockham · 2021-05-25T10:16:43Z

src/wp-includes/post.php

@@ -310,6 +310,68 @@ function create_initial_post_types() {
 		)
 	);

+	if ( theme_supports_block_templates() ) {


@youknowriad I was wondering about these checks (especially here and in taxonomy.php where we register the wp_theme taxonomy). There doesn't seem to be any precedent in Core for conditionally registering a post type or taxonomy; and I was wondering if this made sense UX-wise.

I take it that the main reason for this is to hide the Template Editor for themes that don't support block templates. But this seems to make less sense if we're not even actually using any block template files from a given theme for now -- but only wp_template CPTs. In other words, why should the theme decide whether or not the user can create custom templates, if it's not even providing the block template file fallback?

why should the theme decide whether or not the user can create custom templates, if it's not even providing the block template file fallback?

Because we want themes to have the possibility to opt-out of this feature and stay as they are today. Whether or not the CPT should always be registered or not is another concern though.

Making CPT and taxonomy registration unconditional in 3471404 and cba7648, respectively.

Making CPT and taxonomy registration unconditional in 3471404 and cba7648, respectively.

👍 This is a good call, as it prevents problems with the post type and taxonomy disappearing when importing/exporting because the current theme doesn't happen to use it.

ockham · 2021-05-25T11:16:37Z

src/wp-includes/block-template.php

+ * @param string $template_file Template file name.
+ * @return string Template file name without extension.
+ */
+function _strip_php_suffix( $template_file ) {


The naming isn't accurate here, since the line below also removes .html suffixes. We should either change the name, or the implementation. Since I only see one callsite which seems .php specific, it might be okay to only remove .php?

It's internal anyway, your call

Ah, I think the .html case is needed for custom page block templates. Renaming to _strip_template_file_suffix in bf8d81e.

ockham · 2021-05-25T12:25:30Z

src/wp-includes/theme.php

+function theme_supports_block_templates() {
+	return current_theme_supports( 'block-templates' );


How about just inlining this? The check is super simple now; if we need more criteria in the future, we can always add them later, but maybe it's best to start simple (YAGNI!)

We do need more criteria in the future (like the Gutenberg plugin). Again, I'm not very opinionated here.

Inlining in e4ecc6b. My rationale is that we don't add more API surface (even if only internal) for now; and in some places, we might need a slightly different check anyway later, so it might make sense to keep individual checks separate. (Feel free to revert tho.)

ockham · 2021-05-25T13:16:23Z

Pushed a few changes; feel free to revert if you disagree.

This LGTM overall. I haven't reviewed the controller closely, but I think this PR should be in good enough shape to merge.

ockham · 2021-05-26T13:13:36Z

@ockham I wanted to add that I had to change the "prepare_for_database" function in the REST API controller because the create_item unit test was failing (compare it with Gutenberg). I think the failure was legitimate and we probably should fix that function similarly in Gutenberg as well, though not sure why the unit test was not failing there.

(Just wanted to note this comment somewhere so I don't forget about it :P )

Working on a fix here: WordPress/gutenberg#32233

ockham mentioned this pull request May 20, 2021

Templates: Remove now-obsolete gutenberg_get_template_paths() WordPress/gutenberg#32066

Merged

ockham mentioned this pull request May 21, 2021

FSE: Remove now-obsolete get_template_hierarchy() WordPress/gutenberg#32116

Merged

youknowriad marked this pull request as ready for review May 24, 2021 16:58

carlomanf reviewed May 25, 2021

View reviewed changes

src/wp-includes/post.php Outdated Show resolved Hide resolved

carlomanf reviewed May 25, 2021

View reviewed changes

ockham added 21 commits May 25, 2021 09:07

Add block templates based theme fixtures from GB

fcb8068

Update names

96146a8

Add class-wp-rest-templates-controller.php

0a529e3

Add templates controller test

808babb

Add class-wp-block-template.php

c7889f1

Register wp_template post type

107e917

Register wp_theme taxonomy

dac3189

Add src/wp-admin/includes/templates-utils.php

26712c7

Add src/wp-admin/theme-templates.php

f991abe

Remove post type and taxonomy registration from theme-templates.php

4ca78c2

Add wp_template caps to src/wp-includes/post.php

ba9f4ea

Add src/wp-includes/block-templates.php

daeba90

Add src/wp-includes/default-template-types.php

fc62150

Add tests/phpunit/tests/block-templates.php

ab6fa38

Move gutenberg_is_fse_theme and gutenberg_supports_block_templates to…

c6a71b2

… wp-includes/theme.php

Add custom block template support

832e464

Ugh, forgot to negate

e9c64a4

Uncomment theme.json specific criterion

cc4393e

Remove now-obsolete test setup methods

41db3dc

Add src/wp-includes/block-template.php

31c1ce6

FSE: Remove now-obsolete get_template_hierarchy()

15d49d6

See WordPress/gutenberg#32116

youknowriad added 8 commits May 25, 2021 09:08

Format changes

600db7c

Fix unit tests

2fc65d0

more formatting issues

0270bb2

Fix theme unit test

8f6f435

Fix rest api test

595b6c8

Move filters to default-filters.php

de87bd2

no need for taxonomy in classic themes

575a506

Fix capabilities

71d29fc

youknowriad mentioned this pull request May 25, 2021

Adapt the Gutenberg plugin's code to work with FSE infrastructure in Core. WordPress/gutenberg#32183

Merged

ockham commented May 25, 2021

View reviewed changes

ockham added 4 commits May 25, 2021 15:04

Inline theme_supports_block_templates

e4ecc6b

Make CPT registration unconditional

3471404

Make wp_theme taxonomy registration unconditional

cba7648

Rename _strip_php_suffix to _strip_template_file_suffix

bf8d81e

youknowriad closed this May 25, 2021

ockham deleted the add/block-template-themes branch May 25, 2021 16:24

ockham mentioned this pull request May 26, 2021

Template Controller: Make test use test theme WordPress/gutenberg#32233

Closed

ockham mentioned this pull request Dec 2, 2021

Make ?_wp-find-template=true work for new posts #1997

Closed

oandregal mentioned this pull request Feb 17, 2023

Improve get_block_templates performance #4097

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add required logic for Block template themes #1267

Add required logic for Block template themes #1267

ockham commented May 19, 2021 •

edited by youknowriad

Loading

youknowriad commented May 24, 2021

youknowriad commented May 24, 2021

youknowriad commented May 24, 2021

carlomanf May 25, 2021

youknowriad May 25, 2021

carlomanf May 25, 2021 •

edited

Loading

ockham May 25, 2021

ockham May 25, 2021

youknowriad May 25, 2021

ockham May 25, 2021

creativecoder May 25, 2021

ockham May 25, 2021

youknowriad May 25, 2021

ockham May 25, 2021

ockham May 25, 2021

youknowriad May 25, 2021

ockham May 25, 2021

ockham commented May 25, 2021

ockham commented May 26, 2021

		function theme_supports_block_templates() {
		return current_theme_supports( 'block-templates' );

Add required logic for Block template themes #1267

Add required logic for Block template themes #1267

Conversation

ockham commented May 19, 2021 • edited by youknowriad Loading

List of files in Gutenberg, and where to map them to in Core

TODO

Testing instructions

youknowriad commented May 24, 2021

youknowriad commented May 24, 2021

youknowriad commented May 24, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

carlomanf May 25, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ockham commented May 25, 2021

ockham commented May 26, 2021

ockham commented May 19, 2021 •

edited by youknowriad

Loading

carlomanf May 25, 2021 •

edited

Loading