Templates API: Fix template files query by post-type #61244

Mamaduka · 2024-04-30T13:25:52Z

What?

PR fixes the "block template" files query by post-type when a theme doesn't provide customTemplates mapping in the theme.json`

The PHP side regression was introduced in 55687 changeset but became more evident after recent editor updates. See #61031 (comment).

Why?

The custom templates with no associated post-types should be available for all post-types.

How?

Update _get_block_templates_files to match the original logic. The actual fix is just a few lines, but I had to copy multiple functions to allow overriding core.

Testing Instructions

Using TT4, remove the customTemplates setting.
Create a page.
Open the "Swap templates" modal.
Confirm all the templates listed in the customTemplates setting are visible.
Confirm any custom templates created in the side editor are also visible as before.

Testing Instructions for Keyboard

Same.

Screenshots or screencast

Templates with theme.json setting

Templates without theme.json setting

Mamaduka · 2024-04-30T13:32:03Z

lib/compat/wordpress-6.6/block-template-utils.php

+				// The custom templates with no associated post-types are available for all post-types.
+				if ( $post_type && ! isset( $candidate['postTypes'] ) && $is_custom ) {
+					$template_files[ $template_slug ] = $candidate;
+				}


This is the actual fix. Everything else is just a boilerplate we have to use to override the core.

I think it would be helpful to mark the start and end of the new code being inserted with a @core-merge comment or similar, so we can see what's being added without manually running a diff.

(I did look for a way to override these functions with less duplicated code, but didn't see one.)

That's a good point; I totally forgot about those.

Yeah, couldn't come up with a better override for this case :/

github-actions · 2024-04-30T14:31:30Z

The following accounts have interacted with this PR and/or linked issues. I will continue to update these lists as activity occurs. You can also manually ask me to refresh this list by adding the props-bot label.

If you're merging code through a pull request on GitHub, copy and paste the following into the bottom of the merge commit message.

Co-authored-by: Mamaduka <mamaduka@git.wordpress.org>
Co-authored-by: creativecoder <grantmkin@git.wordpress.org>
Co-authored-by: mike-henderson <mikehend@git.wordpress.org>
Co-authored-by: youknowriad <youknowriad@git.wordpress.org>

To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook.

creativecoder

Thanks for this fix!

Following the replication instructions from #61031 (comment), this fixes the issue for me. Block templates that aren't declared in theme.json are available to both posts and pages for swapping templates.

I left some suggestions for consolidating and commenting to more easily see what code is actually different compared to the equivalent core functions. But overall I think this approach makes sense and can be backported for WP 6.5.3, so I'll go ahead and approve.

creativecoder · 2024-05-01T00:59:18Z

lib/compat/wordpress-6.6/block-template-utils.php

+	$default_template_types = array();
+	if ( 'wp_template' === $template_type ) {
+		$default_template_types = get_default_block_template_types();
+	}


It looks like $default_template_types is being used only by the new code below and is only needed when 'wp_template' === $template_type.

Perhaps move $default_template_types into the existing if ( 'wp_template_part' === $template_type ) { conditional block, right above $is_custom to consolidate the fix?

I tried to avoid calling the function in a loop. Its values can be retrieved at the top level.

creativecoder · 2024-05-01T01:01:28Z

lib/compat/wordpress-6.6/block-template-utils.php

+				// The custom templates with no associated post-types are available for all post-types.
+				if ( $post_type && ! isset( $candidate['postTypes'] ) && $is_custom ) {
+					$template_files[ $template_slug ] = $candidate;
+				}


I think it would be helpful to mark the start and end of the new code being inserted with a @core-merge comment or similar, so we can see what's being added without manually running a diff.

(I did look for a way to override these functions with less duplicated code, but didn't see one.)

creativecoder · 2024-05-01T01:02:37Z

lib/compat/wordpress-6.6/block-template-utils.php

+		 * over the theme-provided ones, so we skip querying and building them.
+		 */
+		$query['slug__not_in'] = wp_list_pluck( $query_result, 'slug' );
+		$template_files        = _gutenberg_get_block_templates_files( $template_type, $query );


Same as above, I think we should mark what's being changed with a @core-merge comment or similar.

There is no need here. I had to duplicate the whole gutenberg_get_block_templates just for this line override :/

Mamaduka · 2024-05-01T05:20:57Z

PR now includes PHPUnit tests.

It modifies the existing test_get_block_templates_should_respect_posttypes_property, which seems like the right place. We can add a separate test case, but that would require adding yet another block-theme for testing.

Mamaduka · 2024-05-01T06:17:08Z

Trac ticket: https://core.trac.wordpress.org/ticket/61110
Core PR: WordPress/wordpress-develop#6468

oandregal · 2024-05-02T07:30:18Z

I want to look at this today (was AFK yesterday), but one thing I noticed is how this affects the performance of themes (blocks&classic) in front-end (timeToFirstByte):

Captura de ecrã 2024-05-02, às 09 27 34

Improving that performance was the reason for the PR that apparently introduce the feature regression, so I'd like to see if there's an alternative fix that doesn't come with a performance regression as a side-effect.

Mamaduka · 2024-05-02T07:42:51Z

Thanks for having a look, @oandregal!

Do you have any ideas about what the bottleneck could be here?

The get_default_block_template_types is mostly a static list; we're just adding an extra call.
I can combine conditions and see if that makes a difference, but I doubt that.

The WP 6.5.3 RC is scheduled for today, so we'll have to make a call if this fix will be included in it.

Update: The effect in the core is smaller.

Screenshot

oandregal · 2024-05-02T09:11:39Z

I've looked into the "Swap templates" behavior. According to #51477 the button should be present if:

There're custom templates available for the given post type.
There's an alternative custom template for the given post type. This is: if the post type is already using the only custom template, the button should be hidden and we should show "Use default template" instead.

Is this correct?

Based on the feedback at #61031 (comment) the "Swap templates" button wasn't available when the theme didn't register any custom template. Register as in: declare customTemplates via theme.json. Isn't this how it should work?

After this PR, block themes that don't register custom templates via the customTemplates key in theme.json will see all custom templates, because there's no information about what post type they are attached to. In the following screenshot, all custom templates are listed for the pages post type in the TT4 theme:

Captura de ecrã 2024-05-02, às 10 47 48

I understand how it's confusing that there's no "Swap button" but the custom templates are still displayed in the site editor. In my view, unregistered custom templates shouldn't be listed in the site editor. This is the core issue we should clarify: how a block theme register custom templates? What should happen when a theme doesn't register the custom templates via theme.json but they are present in the templates/ folder? I consider the theme.json the source of truth (provides the metadata for the custom template to work properly), so my expectation is that they shouldn't be listed anywhere.

I haven't been involved in templates work, so asking for feedback on this before jumping into implementation/code review. @ntsekouras @youknowriad @ockham @Mamaduka @creativecoder @carolinan

fabiankaegy · 2024-05-02T09:18:27Z

@oandregal From my perspective the problem here is that the behavior changed in 6.5. Prior to that the customTemplates in theme.json was only additive to add more metadata. The templates that weren't defined still showed up everywhere.

With 6.5 they now only seem to show up if they are defined in theme.json

ntsekouras · 2024-05-02T09:31:10Z

With 6.5 they now only seem to show up if they are defined in theme.json

@fabiankaegy so this PR fixes that, correct?

fabiankaegy · 2024-05-02T09:37:26Z

@ntsekouras Yes it does. I understood the comment from @oandregal as questioning whether this was indeed a bug or whether this was the intended behavior

Mamaduka · 2024-05-02T09:37:43Z

@ntsekouras, this PR restores behavior from pre-6.5 versions.

IIRC, the customTemplates definition was never a requirement for block themes. Anything in the templates directory was considered a custom unless it was a default WP template (get_default_block_template_types).

P.S. Behavior is similar to how classic themes handled it. When the Template Post Type header wasn't defined, the template was available for every post type.

oandregal · 2024-05-02T10:24:01Z

Some thread in slack 6-5-release-leads https://wordpress.slack.com/archives/C065MBW03EH/p1714637576481459

Summarizing it a bit:

Thanks to mamaduka, I learned more about this. Before the "swap button" PR, users were able to do this, just using a list not a preview. So, we need to fix this user-facing issue.

The fix in this PR

In addition to update the visualization, the Swap button PR switched which templates were shown: before, we merged data from different sources instead of taking it only from available templates. The swap button now only uses useAvailableTemplates hook that dispatches a request to the /wp/v2/templates endpoint giving a post_type.

Before this PR, the response was a empty array:

Gravacao.do.ecra.2024-05-02.as.11.51.38.mov

After this PR, the response also returns the custom templates that don't have any post type attached:

Gravacao.do.ecra.2024-05-02.as.11.53.31.mov

I mentioned in slack that this is unexpected to me. I also mentioned that I lack much context about this feature, so I welcome more feedback from people that actually worked on this. If this the proper fix (endpoint providing all the custom templates that don't declare a post type?), I can look at the implementation to see if there's any chance to improve the performance regression.

Alternative fix?

I wonder if there's an alternative fix where we do at the client-level what we did before the swap button PR: take data from available templates (the templates that are registered for a post type) + take other custom templates that are postType unbound from elsewhere.

Mamaduka · 2024-05-02T10:52:58Z

I think we need to decide if adding a customTemplates property to define custom templates is required for the block themes. But making it a requirement seems like a breaking change because it was previously optional.

cc @WordPress/theme-team

fabiankaegy · 2024-05-02T10:55:55Z

But making it a requirement seems like a breaking change because it was previously optional.

Exactly this. If we want to make that change that fine. But it needs to be opt in. Like for example behind a version change of the theme.json file such as we just experienced with version 3 to introduce a breaking change for defaultFontSize handling.

From my perspective what's in 6.5 today is broken compared to earlier version and we should ensure that we ship this fix

oandregal · 2024-05-02T10:56:51Z

I think we need to decide if adding a customTemplates property to define custom templates is required for the block themes. But making it a requirement seems like a breaking change because it was previously optional.

Actually, you've convinced me about this already :) I don't think it's a requirement: this provides metadata information about the custom templates. Even if we wanted, this ship already sailed.

The question for me is how to fix this for users.

I see two alternatives: either make the /wp/v2/templates with post_type return custom templates that are postType unbound (this PR) or return empty as before. If we return empty, we need to do client reconciliation as we did before 6.5.

Mamaduka · 2024-05-02T11:03:04Z

Restoring behavior pre-changelog 55687 made the most sense when investigating the issue. Why?

Here, client UI represents data returned by the server.
If I want to grab templates that can be assigned to a post type, the client reconciliation knowledge shouldn't be a requirement.

creativecoder · 2024-05-02T15:02:24Z

The WP 6.5.3 RC is scheduled for today, so we'll have to make a call if this fix will be included in it.

It's good to see that the performance impact of this fix is less/minimal in the wordpress-develop PR. But I worry that the comparatively worse Gutenberg performance results could become the core results after all of the PHP and JS package changes in Gutenberg trunk land in core trunk. I don't have a lot of experience monitoring the performance tests, is that a reasonable fear?

Personally I'm leaning toward punting this fix from the 6.5.3 release until we have more time to investigate and continue discussing alternatives.

Mamaduka · 2024-05-02T15:08:45Z

Personally I'm leaning toward punting this fix from the 6.5.3 release until we have more time to investigate and continue discussing alternatives.

Personally, that's fine with me as well since there's not enough time to gather more feedback.

Re-performance tests: I don't think " trunk " shows the real picture now. The code there runs with regression. If the original change accounted for the scenario we're trying to fix here, the original perf results would have been different.

youknowriad · 2024-05-06T07:41:26Z

I see #61244 (comment): either make the /wp/v2/templates with post_type return custom templates that are postType unbound (this PR) or return empty as before. If we return empty, we need to do client reconciliation as we did before 6.5.

Thanks for the discussion.

For me, the former is more logical. if there's no "post type" for a template, it just means "all post types" and the endpoint should reflect that. (there might be other places where we want the templates for a given post type, and we shouldn't have to add logic any time we want that).

Now, If I understand properly, this has some frontend performance impact. I'd like to understand how much are we talking? Is there a way (I mean some code/caching) to avoid the regression?

Mamaduka · 2024-05-06T09:19:29Z

I'm AFK today, but as I shared before, the performance effect shouldn't be a concern here. Why?

If you check stats on trunk PR, the difference is a range of accepted fluctuation.
The fix itself is simple and doesn't make any expensive calls.
The trunk is testing broken logic, and I think its metrics should be taken with a grain of salt.

Mamaduka self-assigned this Apr 30, 2024

Mamaduka added [Type] Bug An existing feature does not function as intended [Feature] Templates API Related to API powering block template functionality in the Site Editor labels Apr 30, 2024

Mamaduka changed the title ~~Block Template Utils: Fix template files query by post-type~~ Templates API: Fix template files query by post-type Apr 30, 2024

Templates API: Fix template files query by post-type

bd46b25

Mamaduka force-pushed the fix/block-template-files-post-type-query branch from b6ba41c to bd46b25 Compare April 30, 2024 13:30

Mamaduka commented Apr 30, 2024

View reviewed changes

Mamaduka marked this pull request as ready for review April 30, 2024 14:31

Mamaduka requested a review from spacedmonkey as a code owner April 30, 2024 14:31

Mamaduka requested review from youknowriad, oandregal and fabiankaegy April 30, 2024 14:31

Mamaduka mentioned this pull request Apr 30, 2024

Can't swap templates in the post editor #61031

Closed

creativecoder approved these changes May 1, 2024

View reviewed changes

Mamaduka added 2 commits May 1, 2024 08:36

Add '@core-merge' comments

8818200

Add PHPUnit tests

60f8453

Mamaduka force-pushed the fix/block-template-files-post-type-query branch from 855e996 to 60f8453 Compare May 1, 2024 05:18

Mamaduka mentioned this pull request May 1, 2024

Editor: Fix block template files query for a post-type WordPress/wordpress-develop#6468

Open

Mamaduka merged commit f9e725b into trunk May 1, 2024
60 checks passed

Mamaduka deleted the fix/block-template-files-post-type-query branch May 1, 2024 11:27

github-actions bot added this to the Gutenberg 18.3 milestone May 1, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Templates API: Fix template files query by post-type #61244

Templates API: Fix template files query by post-type #61244

Mamaduka commented Apr 30, 2024 •

edited

Mamaduka Apr 30, 2024

creativecoder May 1, 2024

Mamaduka May 1, 2024 •

edited

github-actions bot commented Apr 30, 2024 •

edited

creativecoder left a comment

creativecoder May 1, 2024

Mamaduka May 1, 2024

creativecoder May 1, 2024

creativecoder May 1, 2024

Mamaduka May 1, 2024

Mamaduka commented May 1, 2024

Mamaduka commented May 1, 2024

oandregal commented May 2, 2024

Mamaduka commented May 2, 2024 •

edited

oandregal commented May 2, 2024

fabiankaegy commented May 2, 2024

ntsekouras commented May 2, 2024

fabiankaegy commented May 2, 2024

Mamaduka commented May 2, 2024 •

edited

oandregal commented May 2, 2024 •

edited

Mamaduka commented May 2, 2024

fabiankaegy commented May 2, 2024

oandregal commented May 2, 2024 •

edited

Mamaduka commented May 2, 2024

creativecoder commented May 2, 2024

Mamaduka commented May 2, 2024

youknowriad commented May 6, 2024

Mamaduka commented May 6, 2024

Templates API: Fix template files query by post-type #61244

Templates API: Fix template files query by post-type #61244

Conversation

Mamaduka commented Apr 30, 2024 • edited

What?

Why?

How?

Testing Instructions

Testing Instructions for Keyboard

Screenshots or screencast

Mamaduka Apr 30, 2024

Choose a reason for hiding this comment

creativecoder May 1, 2024

Choose a reason for hiding this comment

Mamaduka May 1, 2024 • edited

Choose a reason for hiding this comment

github-actions bot commented Apr 30, 2024 • edited

creativecoder left a comment

Choose a reason for hiding this comment

creativecoder May 1, 2024

Choose a reason for hiding this comment

Mamaduka May 1, 2024

Choose a reason for hiding this comment

creativecoder May 1, 2024

Choose a reason for hiding this comment

creativecoder May 1, 2024

Choose a reason for hiding this comment

Mamaduka May 1, 2024

Choose a reason for hiding this comment

Mamaduka commented May 1, 2024

Mamaduka commented May 1, 2024

oandregal commented May 2, 2024

Mamaduka commented May 2, 2024 • edited

Screenshot

oandregal commented May 2, 2024

fabiankaegy commented May 2, 2024

ntsekouras commented May 2, 2024

fabiankaegy commented May 2, 2024

Mamaduka commented May 2, 2024 • edited

oandregal commented May 2, 2024 • edited

The fix in this PR

Alternative fix?

Mamaduka commented May 2, 2024

fabiankaegy commented May 2, 2024

oandregal commented May 2, 2024 • edited

Mamaduka commented May 2, 2024

creativecoder commented May 2, 2024

Mamaduka commented May 2, 2024

youknowriad commented May 6, 2024

Mamaduka commented May 6, 2024

Mamaduka commented Apr 30, 2024 •

edited

Mamaduka May 1, 2024 •

edited

github-actions bot commented Apr 30, 2024 •

edited

Mamaduka commented May 2, 2024 •

edited

Mamaduka commented May 2, 2024 •

edited

oandregal commented May 2, 2024 •

edited

oandregal commented May 2, 2024 •

edited