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

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

Merged
merged 3 commits into from
May 1, 2024

Conversation

Mamaduka
Copy link
Member

@Mamaduka Mamaduka commented Apr 30, 2024

What?

Fixes #61031.

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

  1. Using TT4, remove the customTemplates setting.
  2. Create a page.
  3. Open the "Swap templates" modal.
  4. Confirm all the templates listed in the customTemplates setting are visible.
  5. 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
CleanShot 2024-04-30 at 18 21 01

Templates without theme.json setting
CleanShot 2024-04-30 at 18 20 15

@Mamaduka Mamaduka self-assigned this Apr 30, 2024
@Mamaduka 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 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
@Mamaduka Mamaduka force-pushed the fix/block-template-files-post-type-query branch from b6ba41c to bd46b25 Compare April 30, 2024 13:30
Comment on lines +216 to +219
// 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;
}
Copy link
Member Author

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Member Author

@Mamaduka Mamaduka May 1, 2024

Choose a reason for hiding this comment

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

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

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

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

github-actions bot commented Apr 30, 2024

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.

Copy link
Contributor

@creativecoder creativecoder left a comment

Choose a reason for hiding this comment

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

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.

Comment on lines +139 to +142
$default_template_types = array();
if ( 'wp_template' === $template_type ) {
$default_template_types = get_default_block_template_types();
}
Copy link
Contributor

Choose a reason for hiding this comment

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

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?

Copy link
Member Author

Choose a reason for hiding this comment

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

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

Comment on lines +216 to +219
// 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;
}
Copy link
Contributor

Choose a reason for hiding this comment

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

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

* 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 );
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Member Author

Choose a reason for hiding this comment

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

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

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

Mamaduka commented May 1, 2024

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
Copy link
Member Author

Mamaduka commented May 1, 2024

@Mamaduka Mamaduka merged commit f9e725b into trunk May 1, 2024
60 checks passed
@Mamaduka Mamaduka deleted the fix/block-template-files-post-type-query branch May 1, 2024 11:27
@github-actions github-actions bot added this to the Gutenberg 18.3 milestone May 1, 2024
@oandregal
Copy link
Member

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 ecrã 2024-05-02, à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
Copy link
Member Author

Mamaduka commented May 2, 2024

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

CleanShot 2024-05-02 at 12 22 08

@oandregal
Copy link
Member

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 ecrã 2024-05-02, à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
Copy link
Member

@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
Copy link
Contributor

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
Copy link
Member

@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
Copy link
Member Author

Mamaduka commented May 2, 2024

@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
Copy link
Member

oandregal commented May 2, 2024

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
Copy link
Member Author

Mamaduka commented May 2, 2024

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
Copy link
Member

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
Copy link
Member

oandregal commented May 2, 2024

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
Copy link
Member Author

Mamaduka commented May 2, 2024

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
Copy link
Contributor

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
Copy link
Member Author

Mamaduka commented May 2, 2024

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
Copy link
Contributor

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
Copy link
Member Author

Mamaduka commented May 6, 2024

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.

pento pushed a commit to WordPress/wordpress-develop that referenced this pull request Jun 4, 2024
See WordPress/gutenberg#61244.
See #6468.

Fixes #61110.

Props mamaduka, mukesh27, grantmkin, vcanales, ellatrix, oandregal.



git-svn-id: https://develop.svn.wordpress.org/trunk@58323 602fd350-edb4-49c9-b593-d223f7449a82
markjaquith pushed a commit to markjaquith/WordPress that referenced this pull request Jun 4, 2024
See WordPress/gutenberg#61244.
See WordPress/wordpress-develop#6468.

Fixes #61110.

Props mamaduka, mukesh27, grantmkin, vcanales, ellatrix, oandregal.


Built from https://develop.svn.wordpress.org/trunk@58323


git-svn-id: http://core.svn.wordpress.org/trunk@57780 1a063a9b-81f0-0310-95a4-ce76da25c4cd
github-actions bot pushed a commit to platformsh/wordpress-performance that referenced this pull request Jun 4, 2024
See WordPress/gutenberg#61244.
See WordPress/wordpress-develop#6468.

Fixes #61110.

Props mamaduka, mukesh27, grantmkin, vcanales, ellatrix, oandregal.


Built from https://develop.svn.wordpress.org/trunk@58323


git-svn-id: https://core.svn.wordpress.org/trunk@57780 1a063a9b-81f0-0310-95a4-ce76da25c4cd
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Feature] Templates API Related to API powering block template functionality in the Site Editor [Type] Bug An existing feature does not function as intended
Projects
Development

Successfully merging this pull request may close these issues.

Can't swap templates in the post editor
6 participants