Patterns: `theme` attribute not inserted in Template Part blocks

[Bernhard Reiter]

First reported by @scruffian in ​https://github.com/WordPress/gutenberg/issues/55202.

[56805] introduced a regression: The theme attribute is no longer injected into Template Part blocks inside of patterns. This causes errors on the frontend, where instead of a given template part, an error message such as Template part has been deleted or is unavailable: header is seen.

Fixes: ​https://github.com/WordPress/gutenberg/issues/55202
Trac ticket: https://core.trac.wordpress.org/ticket/59583

This ensures that the theme slug is correctly inserted into template parts that are inside patterns.

Props @gziolo for finding the problem

Looking good. Thanks all, I'll land it 😊

[Bernhard Reiter]

In 56818:

Patterns: Inject theme attribute into Template Part blocks.

[56805] introduced a regression: The theme attribute was no longer injected into Template Part blocks inside of patterns. This caused errors on the frontend, where instead of a given template part, an error message such as Template part has been deleted or is unavailable: header was seen.

This changeset rectifies that problem, and adds unit test coverage to guard against future regressions.

Follow-up to [56805].
Props scruffian, gziolo.
Fixes #59583.

Committed to Core in https://core.trac.wordpress.org/changeset/56818.

[flixos90]

Flagging that this bug fix caused a major performance regression and effectively reverted most of the benefit that [56805] introduced. See ​Slack thread.

While a functional bug fix is more important than a performance improvement, I am reopening this so we can further explore alternative ways that get us closer to good performance while maintaining functionality as needed. The performance impact of the change is significant enough to warrant that IMO.

WIP. Result of a ​discussion with @felixarntz, aiming to fix the performance regression re-introduced by https://core.trac.wordpress.org/changeset/56818.

Needs its ​Gutenberg counterpart PR, which has the Template Part block fall back to the current theme if no theme attribute is present. This should OTOH allow removing theme attr injection on the frontend; specifically, from the pattern registry. I believe that the relevant method is get_registered, whereas get_all_registered is used by the patterns REST API controller -- which is still needed e.g. for the editor (see TODO note below).

To test locally, patch src/wp-includes/blocks/template-part.php with the changes from ​https://github.com/WordPress/gutenberg/pull/55217.diff.

TODO:
Make sure the patterns registry doesn't inject the theme attr on the frontend. It probably will continue to need to inject it into the REST API so it's present in the editor (so that the attr is present when a pattern that includes a Template Part block is persisted to the DB).

I'm not very comfortable with how there doesn't seem to be a great way to delineate the frontend from the REST API use case; we might want to find a better way to do so.

cc/ @felixarntz @gziolo

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

[marybaum]

The Tuesday bug scrubbers will leave this alone.

@ockham In order to be able to benchmark the performance impact, it would be great if you could incorporate the full change into this PR as well, i.e. including the Gutenberg changes from ​https://github.com/WordPress/gutenberg/pull/55217. Obviously this will cause a CI workflow error and shouldn't be merged, but it makes it easier to compare and discuss the changes.

This is a bit unfortunate, but I think for issues that require work in both Gutenberg and Core, it's the easiest way to holistically reason about the solution. Once we agree on a solution, we can:

1. remove the Gutenberg specific part from here
2. merge it in Gutenberg
3. backport the Gutenberg change to Core
4. merge this PR

@ockham In order to be able to benchmark the performance impact, it would be great if you could incorporate the full change into this PR as well, i.e. including the Gutenberg changes from ​WordPress/gutenberg#55217. Obviously this will cause a CI workflow error and shouldn't be merged, but it makes it easier to compare and discuss the changes.

Makes sense! Done in ​https://github.com/WordPress/wordpress-develop/pull/5455/commits/62e0be60b443915be6ab9b3635ae36877857b1ca 😊

One open question is whether the Pattern returned from REST API needs to have the current theme injected as the theme attribute in the Template Part block included in the pattern. If the answer is yes, then we can probably move the logic that injects it to the REST API controller rather than doing it conditionally in the patterns registry. If the answer is no, we should stop injecting it when accessing all registered patterns.

I'm testing locally with the following diff:

src/wp-includes/blocks/template-part.php

@@ -18 +18 @@
         $template_part_id = null;
         $content          = null;
         $area             = WP_TEMPLATE_PART_AREA_UNCATEGORIZED;
+        $theme            = isset( $attributes['theme'] ) ? $attributes['theme'] : get_stylesheet();
 
-        if (
-                isset( $attributes['slug'] ) &&
-                isset( $attributes['theme'] ) &&
-                get_stylesheet() === $attributes['theme']
-        ) {
-                $template_part_id    = $attributes['theme'] . '//' . $attributes['slug'];
+        if ( isset( $attributes['slug'] ) && get_stylesheet() === $theme ) {
+                $template_part_id    = $theme . '//' . $attributes['slug'];
                 $template_part_query = new WP_Query(
                         array(
                                 'post_type'           => 'wp_template_part',
@@ -34 +31 @@
                                         array(
                                                 'taxonomy' => 'wp_theme',
                                                 'field'    => 'name',
-                                                'terms'    => $attributes['theme'],
+                                                'terms'    => $theme,
                                         ),
                                 ),
                                 'posts_per_page'      => 1,

src/wp-includes/class-wp-block-patterns-registry.php

@@ -165 +165 @@
         private function prepare_content( $pattern, $hooked_blocks ) {
                 $content = $pattern['content'];
 
-                $before_block_visitor = '_inject_theme_attribute_in_template_part_block';
+                $before_block_visitor = ( defined( 'REST_REQUEST' ) && REST_REQUEST ) ? '_inject_theme_attribute_in_template_part_block' : null;
                 $after_block_visitor  = null;
                 if ( ! empty( $hooked_blocks ) || has_filter( 'hooked_block_types' ) ) {
                         $before_block_visitor = make_before_block_visitor( $hooked_blocks, $pattern );
                         $after_block_visitor  = make_after_block_visitor( $hooked_blocks, $pattern );
                 }
-                $blocks  = parse_blocks( $content );
-                $content = traverse_and_serialize_blocks( $blocks, $before_block_visitor, $after_block_visitor );
+                if ( ! is_null( $before_block_visitor ) || ! is_null( $after_block_visitor ) ) {
+                        $blocks  = parse_blocks( $content );
+                        $content = traverse_and_serialize_blocks( $blocks, $before_block_visitor, $after_block_visitor );
+                }
 
                 return $content;
         }

I could confirm in my testing that the theme attribute needs to be injected into Template Part blocks in all block patterns when sending to the editor with REST API.

I'm testing locally with the following diff:

src/wp-includes/blocks/template-part.php

@@ -18 +18 @@
         $template_part_id = null;
         $content          = null;
         $area             = WP_TEMPLATE_PART_AREA_UNCATEGORIZED;
+        $theme            = isset( $attributes['theme'] ) ? $attributes['theme'] : get_stylesheet();
 
-        if (
-                isset( $attributes['slug'] ) &&
-                isset( $attributes['theme'] ) &&
-                get_stylesheet() === $attributes['theme']
-        ) {
-                $template_part_id    = $attributes['theme'] . '//' . $attributes['slug'];
+        if ( isset( $attributes['slug'] ) && get_stylesheet() === $theme ) {
+                $template_part_id    = $theme . '//' . $attributes['slug'];
                 $template_part_query = new WP_Query(
                         array(
                                 'post_type'           => 'wp_template_part',
@@ -34 +31 @@
                                         array(
                                                 'taxonomy' => 'wp_theme',
                                                 'field'    => 'name',
-                                                'terms'    => $attributes['theme'],
+                                                'terms'    => $theme,
                                         ),
                                 ),
                                 'posts_per_page'      => 1,

src/wp-includes/class-wp-block-patterns-registry.php

@@ -165 +165 @@
         private function prepare_content( $pattern, $hooked_blocks ) {
                 $content = $pattern['content'];
 
-                $before_block_visitor = '_inject_theme_attribute_in_template_part_block';
+                $before_block_visitor = ( defined( 'REST_REQUEST' ) && REST_REQUEST ) ? '_inject_theme_attribute_in_template_part_block' : null;
                 $after_block_visitor  = null;
                 if ( ! empty( $hooked_blocks ) || has_filter( 'hooked_block_types' ) ) {
                         $before_block_visitor = make_before_block_visitor( $hooked_blocks, $pattern );
                         $after_block_visitor  = make_after_block_visitor( $hooked_blocks, $pattern );
                 }
-                $blocks  = parse_blocks( $content );
-                $content = traverse_and_serialize_blocks( $blocks, $before_block_visitor, $after_block_visitor );
+                if ( ! is_null( $before_block_visitor ) || ! is_null( $after_block_visitor ) ) {
+                        $blocks  = parse_blocks( $content );
+                        $content = traverse_and_serialize_blocks( $blocks, $before_block_visitor, $after_block_visitor );
+                }
 
                 return $content;
         }

I could confirm in my testing that the theme attribute needs to be injected into Template Part blocks in all block patterns when sending to the editor with REST API.

I also tried applying the same mechanism to patterns, as to template, and template parts:

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

@@ -549 +549 @@
                 $template->area = $template_file['area'];
         }
 
-        $before_block_visitor = '_inject_theme_attribute_in_template_part_block';
+        $before_block_visitor = ( defined( 'REST_REQUEST' ) && REST_REQUEST ) ? '_inject_theme_attribute_in_template_part_block' : null;
         $after_block_visitor  = null;
         $hooked_blocks        = get_hooked_blocks();
         if ( ! empty( $hooked_blocks ) || has_filter( 'hooked_block_types' ) ) {
                 $before_block_visitor = make_before_block_visitor( $hooked_blocks, $template );
                 $after_block_visitor  = make_after_block_visitor( $hooked_blocks, $template );
         }
-        $blocks            = parse_blocks( $template_content );
-        $template->content = traverse_and_serialize_blocks( $blocks, $before_block_visitor, $after_block_visitor );
+        if ( ! is_null( $before_block_visitor ) || ! is_null( $after_block_visitor ) ) {
+                $blocks           = parse_blocks( $template_content );
+                $template_content = traverse_and_serialize_blocks( $blocks, $before_block_visitor, $after_block_visitor );
+        }
+        $template->content = $template_content;
 
         return $template;
 }

When combined with the diff included in ​https://github.com/WordPress/wordpress-develop/pull/5455#issuecomment-1757470060 I saw better results.

## Benchmarking

I used the benchmark-server-timing command, with 100 runs each, and I did that 3 times (i.e. 3 medians, each based on 100 runs). All benchmarks without any plugins active (other than Performance Lab for the Server-Timing headers). Below is a summary of the wp-total metrics my data.

With TT4 home page and this patch:

256.74ms (PR) vs 265.09ms (trunk) --> 3.1% faster 👍🏻
256.67ms (PR) vs 265.11ms (trunk) --> 3.2% faster 👍🏻
256.99ms (PR) vs 265.78ms (trunk) --> 3.3% faster 👍🏻

Given @ockham is out today, I have refreshed this PR with the latest updates from ​https://github.com/WordPress/gutenberg/pull/55217, as well as the changes suggested above from @gziolo: The _inject_theme_attribute_in_template_part_block() should only be called within a REST API request, but not a regular frontend request.

[flixos90]

@bernhard-reiter @gziolo I have spent some time researching this further in terms of performance impact. Here is my summary / conclusion:

• [56805] seemed to have a massive performance impact (in my benchmarks, wp-total improved from ~105ms to ~85ms for TT4).
• However it introduced a functional bug, which [56818] then addressed, while however also reverting almost all the performance benefit.
• The solution implemented in ​https://github.com/WordPress/gutenberg/pull/55217 and ​https://github.com/WordPress/wordpress-develop/pull/5455 improves performance again slightly, however far from how [56805] initially seemed to do (wp-total 102.5ms vs 103.8ms in my current benchmark with TT4)
• Unfortunately, the apparent performance enhancement [56805] came almost entirely from the bug it introduced. Due to no longer injecting the theme attribute to all core/template-part blocks, those blocks would no run any of the logic within ​https://github.com/WordPress/gutenberg/pull/55217/files#diff-28f36bb64fbbc8ce728db401b1e4cf30c9f44db7f5dcce1b7b0cc9ce60b1a209L22, which has a considerable cost.
• This led us to believe there was a major performance improvement behind the change, but unfortunately most of it came from the bug.
• I think it is worth completing the work on ​https://github.com/WordPress/gutenberg/pull/55217 and ​https://github.com/WordPress/wordpress-develop/pull/5455, since they bring a decent performance improvement. However, the greater part of the associated cost comes from retrieving the file contents from block template part files (and block template files), for which I have identified a separate solution in a new ticket #59600. The PR on that ticket leads to performance improvements between 5-10% for all block themes I've tested it with so far.

@ockham I have replaced the tests that are no longer applicable now (​https://github.com/WordPress/wordpress-develop/pull/5455/commits/68bfea6c3c68d65cd6c3bf93b2150a26ab02b1a0) with new tests ensuring the template part block still works correctly with the theme attribute (​https://github.com/WordPress/wordpress-develop/pull/5455/commits/8522bc87d7389292ade13af166d872cea5f906ad).

Let me know your thoughts, from my perspective this is good to go pending the backport of ​https://github.com/WordPress/gutenberg/pull/55217.

Running some tests and I hope they are helpful.

I want to compare three states here:

• [control] before the original refactor in 56805
• [broken] with the refactor in 56805 [git sha 9d039e6c]
• [fixed] with this fix from 56818 [git sha a9bb470f]

Obviously it would be unfair to compare all three of these against each other directly, though, since other improvements may have appeared in trunk along the way.

To do this I'm going to create all new commits based on the latest trunk.

• [control] is now trunk at a8d53284 with reverts of broken and fixed
  • branch control at 76a1aded
• [broken] at trunk with revert of fixed
  • branch broken at 02db6af7
• [fixed] at trunk
  • branch fixed at a8d53284

• Wiped out the database, reset everything on the site.
• Activate Performance Lab and output buffering plugins, disable all Performance Lab options.
• curl the home page 500 times in a row for each branch, switch branches, and repeat until enough samples for all three branches have been collected

<details><summary>performance testing is hard - detour!</summary>

this initial assessment reports that things are insignificantly faster on my test server after the fix in a9bb470f, but interestingly enough there's not that much difference in either of the three branches; plausibly because of the caching that was introduced? I'm guessing that there's a more profound impact when that cache isn't so readily available.

to try and confirm this I reset to the parent commit of 9d039e6c and re-ran the experiment, cherry-picking the fix on that old commit.

• [control] is now a55dcf46
• [broken] is now 9d039e6c
• [fixed] is now 72b829c3

unfortunately the results are almost identical on these old branches, so I think something was wrong with the tests.

restarting every 500 samples when I change branches, I'm now restoring a fresh copy of the database after activating the plugins, from the control branch.

going back in time, here are the control, broken, and fixed patches in comparison to each other. these look right.
</details>

and here is the same plot if we went back in time to the original parent commit of the breaking patch, then cherry-picked this fix onto the broken patch.

so the fixes have less impact today than they had when they were originally applied because trunk got faster otherwise. there's only one more interesting feature here I noticed in a plot that deserves exploration: my sneaky suspicion. I'm going to run the tests again but wipe out the database between every request and see if the outliers in these plots (not shown because they peak at around 1400 ms) are outliers or represent the true cost of this stuff when caching isn't available.

<details><summary>But what if the caches aren't primed?</summary>

given that this testing requires resetting the data on every single request, it's very slow and I have set it to switch branches every five requests instead of every five hundred, and I'm going to look at the data before we have nearly as many samples.

and going back in time…

</details>

there's a _very_ interesting change in the plots if we look at what I'm trying to create as the "first page render without any primed caches." I'm trying to recreate a situation more common to large-scale shared hosting where caches may not be as readily available. these numbers are the worst case, but trunk today takes ten times as long to render on the first page load as it did when we started patching block hooks. I have no explanation and I'm concluding my investigation for today, but it's fascinating to see such a drastic change in behavior. to re-confirm: I'm measuring a reduction in the average render times when many requests have already run, when caches are already primed, and when database fields have been created.

[gziolo]

Unfortunately, the apparent performance enhancement [56805] came almost entirely from the bug it introduced. Due to no longer injecting the theme attribute to all core/template-part blocks, those blocks would no run any of the logic within ​https://github.com/WordPress/gutenberg/pull/55217/files#diff-28f36bb64fbbc8ce728db401b1e4cf30c9f44db7f5dcce1b7b0cc9ce60b1a209L22, which has a considerable cost.

Excellent discovery, @flixos90. It took me a while to realize that even after reading your comment too fast yesterday. That's the crux of it for TT4 which uses many patterns referencing template parts. In particular, on the homepage the impact was high because with the bug introduced, all these template part weren't processed because of the missing theme attribute, which prevented displaying them at all.

This led us to believe there was a major performance improvement behind the change, but unfortunately most of it came from the bug.

That's very unfortunate, but this is 100% true. We still see some small improvements with this patch, but given the scale reported before it's much harder to offer equally satisfying fix. The good news is that the implementation around Block Hooks, or Templater Parts seems to be very optimized.

Thanks a lot folks for pushing this along and for the great performance analysis! With ​https://github.com/WordPress/gutenberg/pull/55217 merged and ​ready to be sync'ed, I'll remove the block code from this PR.

Thanks a lot folks for pushing this along and for the great performance analysis! With ​WordPress/gutenberg#55217 merged and ​ready to be sync'ed, I'll remove the block code from this PR.

Done in ​https://github.com/WordPress/wordpress-develop/pull/5455/commits/8394dfd60a675c7dab54e4e86105efd81292b495.

We now need to wait for ​https://github.com/WordPress/wordpress-develop/pull/5468 to land before we can merge this one 😊

[hellofromTonya]

In 56849:

Editor: Update npm packages ahead of 6.4 RC1.

Updates the npm packages and code for:

• ​List: fix forward merging of nested list

• ​Update consent string for using private APIs.

• ​useBlockSettings: add missing useMemo dependencies

• ​Remove the lightbox filter and view file when the lightbox setting is disabled.

• ​Patterns: Remove the version enforcement for npm in engines field

• ​Remove `@return void` from PHP function docs

• ​Image: Disable lightbox editor UI for linked images

• ​Image: Stop crashing with Lightbox on image blocks without an image

• ​Update fullscreen icon

• ​Template Part block: Fall back to current theme if no theme attribute is given. This change is part of fix for a performance regression re-introduced by [56818].

References:

• ​Gutenberg PR 55298 Cherry-pick commits

Follow-up to [56818], [56816].

Props ellatrix, peterwilsoncc, jsnajdr, afercia, gziolo, isabel_brison, artemiosans, richtabor, bernhard-reiter, flixos90, mikachan, spacedmonkey, hellofromTonya.
See #59583, #59411.

Heads up: The Gutenberg npm packages are now updated in Core's trunk via https://core.trac.wordpress.org/changeset/56849. This changeset includes the package side fixes from ​https://github.com/WordPress/gutenberg/pull/55217 which were part of ​https://github.com/WordPress/wordpress-develop/pull/5468.

With the package updates now in trunk, I'd suggest rebasing this PR to bring in the latest changes. Then another round of testing just to make sure everything works as expected, i.e. before committing.

Noting (you may already know in which case sorry for my redundant note): The PHPUnit tests are failing with this PR:

There was 1 failure:

1) Tests_Blocks_RenderBlockCoreTemplatePart::test_render_block_core_template_part_without_theme_attribute
Failed asserting that two strings are identical.
--- Expected
+++ Actual
@@ @@
-'<header class="wp-block-template-part">
-<p>Small Header Template Part</p>
-</header>'
+'Template part has been deleted or is unavailable: small-header'

/var/www/tests/phpunit/tests/blocks/renderBlockCoreTemplatePart.php:60
phpvfscomposer:///var/www/vendor/phpunit/phpunit/phpunit:106
/var/www/vendor/bin/phpunit:118

Rebased; this should now include the changes to the Pattern and Template Part blocks from ​https://github.com/WordPress/wordpress-develop/pull/5468, and thus get the PHPUnit tests to pass 🤞

[Bernhard Reiter]

In 56896:

Patterns: Don't inject theme attribute on frontend.

Having the patterns registry inject the theme attribute into all Template Part blocks inside every pattern was found to negatively impact performance. It turns out that it's only required for the editor (i.e. in the REST API) but not on the frontend; there, it's instead possible to fall back to the currently active theme.

The latter change was made to the Pattern and Template Part blocks in ​https://github.com/WordPress/gutenberg/pull/55217, which was sync'ed to Core in [56849]. Consequently, this changeset removes theme attribute insertion from the frontend.

Props flixos90, gziolo, dmsnell, hellofromtonya.
Fixes #59583.

Committed to Core in https://core.trac.wordpress.org/changeset/56896.

[gziolo]

There was a bug fix in the editor to ensure that patterns work correctly with the post type templates: ​https://github.com/WordPress/gutenberg/pull/55858. It's very similar to what we observed when removing the logic on the server to inject the theme attribute in template parts used with patterns. I'm inclined to revisit the implementation on the client in the editor to ensure that the default theme name is always properly read so we no longer need to inject the same value in the REST API endpoint.

[gziolo]

I opened ​https://github.com/WordPress/gutenberg/pull/55965, where I'm looking at adding the fallback for the current theme in places where template parts are loaded from the server without preprocessing through _inject_theme_attribute_in_template_part_block. In essence, I'm seeking a ways to the whole logic to the client to bring back the performance improvements we measured for the homepage in Twenty Twenty-Four theme.