Make WordPress Core

Opened 14 months ago

Closed 14 months ago

Last modified 13 months ago

#59583 closed defect (bug) (fixed)

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

Reported by: bernhard-reiter's profile Bernhard Reiter Owned by: bernhard-reiter's profile Bernhard Reiter
Milestone: 6.4 Priority: high
Severity: major Version:
Component: General Keywords: has-patch has-unit-tests
Focuses: performance Cc:

Description

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.

Change History (35)

This ticket was mentioned in PR #5450 on WordPress/wordpress-develop by @scruffian.


14 months ago
#1

  • Keywords has-patch has-unit-tests added

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

@Bernhard Reiter commented on PR #5450:


14 months ago
#2

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

#3 @Bernhard Reiter
14 months ago

  • Resolution set to fixed
  • Status changed from assigned to closed

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.

#5 @flixos90
14 months ago

  • Resolution fixed deleted
  • Status changed from closed to reopened

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.

This ticket was mentioned in Slack in #core by gziolo. View the logs.


14 months ago

#7 @gziolo
14 months ago

  • Focuses performance added

This ticket was mentioned in PR #5455 on WordPress/wordpress-develop by @Bernhard Reiter.


14 months ago
#8

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

This ticket was mentioned in Slack in #core by marybaum. View the logs.


14 months ago

#10 @marybaum
14 months ago

The Tuesday bug scrubbers will leave this alone.

@flixos90 commented on PR #5455:


14 months ago
#11

@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

@Bernhard Reiter commented on PR #5455:


14 months ago
#12

@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 😊

@gziolo commented on PR #5455:


14 months ago
#13

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.

@gziolo commented on PR #5455:


14 months ago
#14

I'm testing locally with the following diff:

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

    diff --git a/src/wp-includes/blocks/template-part.php b/src/wp-includes/blocks/template-part.php
    index a7bd4033af..3ad4009069 100644
    a b function render_block_core_template_part( $attributes ) { 
    1818        $template_part_id = null;
    1919        $content          = null;
    2020        $area             = WP_TEMPLATE_PART_AREA_UNCATEGORIZED;
     21        $theme            = isset( $attributes['theme'] ) ? $attributes['theme'] : get_stylesheet();
    2122
    22         if (
    23                 isset( $attributes['slug'] ) &&
    24                 isset( $attributes['theme'] ) &&
    25                 get_stylesheet() === $attributes['theme']
    26         ) {
    27                 $template_part_id    = $attributes['theme'] . '//' . $attributes['slug'];
     23        if ( isset( $attributes['slug'] ) && get_stylesheet() === $theme ) {
     24                $template_part_id    = $theme . '//' . $attributes['slug'];
    2825                $template_part_query = new WP_Query(
    2926                        array(
    3027                                'post_type'           => 'wp_template_part',
    function render_block_core_template_part( $attributes ) { 
    3431                                        array(
    3532                                                'taxonomy' => 'wp_theme',
    3633                                                'field'    => 'name',
    37                                                 'terms'    => $attributes['theme'],
     34                                                'terms'    => $theme,
    3835                                        ),
    3936                                ),
    4037                                'posts_per_page'      => 1,
  • src/wp-includes/class-wp-block-patterns-registry.php

    diff --git a/src/wp-includes/class-wp-block-patterns-registry.php b/src/wp-includes/class-wp-block-patterns-registry.php
    index e516277e42..ddf73446e7 100644
    a b final class WP_Block_Patterns_Registry { 
    165165        private function prepare_content( $pattern, $hooked_blocks ) {
    166166                $content = $pattern['content'];
    167167
    168                 $before_block_visitor = '_inject_theme_attribute_in_template_part_block';
     168                $before_block_visitor = ( defined( 'REST_REQUEST' ) && REST_REQUEST ) ? '_inject_theme_attribute_in_template_part_block' : null;
    169169                $after_block_visitor  = null;
    170170                if ( ! empty( $hooked_blocks ) || has_filter( 'hooked_block_types' ) ) {
    171171                        $before_block_visitor = make_before_block_visitor( $hooked_blocks, $pattern );
    172172                        $after_block_visitor  = make_after_block_visitor( $hooked_blocks, $pattern );
    173173                }
    174                 $blocks  = parse_blocks( $content );
    175                 $content = traverse_and_serialize_blocks( $blocks, $before_block_visitor, $after_block_visitor );
     174                if ( ! is_null( $before_block_visitor ) || ! is_null( $after_block_visitor ) ) {
     175                        $blocks  = parse_blocks( $content );
     176                        $content = traverse_and_serialize_blocks( $blocks, $before_block_visitor, $after_block_visitor );
     177                }
    176178
    177179                return $content;
    178180        }

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.

@gziolo commented on PR #5455:


14 months ago
#15

I'm testing locally with the following diff:

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

    diff --git a/src/wp-includes/blocks/template-part.php b/src/wp-includes/blocks/template-part.php
    index a7bd4033af..3ad4009069 100644
    a b function render_block_core_template_part( $attributes ) { 
    1818        $template_part_id = null;
    1919        $content          = null;
    2020        $area             = WP_TEMPLATE_PART_AREA_UNCATEGORIZED;
     21        $theme            = isset( $attributes['theme'] ) ? $attributes['theme'] : get_stylesheet();
    2122
    22         if (
    23                 isset( $attributes['slug'] ) &&
    24                 isset( $attributes['theme'] ) &&
    25                 get_stylesheet() === $attributes['theme']
    26         ) {
    27                 $template_part_id    = $attributes['theme'] . '//' . $attributes['slug'];
     23        if ( isset( $attributes['slug'] ) && get_stylesheet() === $theme ) {
     24                $template_part_id    = $theme . '//' . $attributes['slug'];
    2825                $template_part_query = new WP_Query(
    2926                        array(
    3027                                'post_type'           => 'wp_template_part',
    function render_block_core_template_part( $attributes ) { 
    3431                                        array(
    3532                                                'taxonomy' => 'wp_theme',
    3633                                                'field'    => 'name',
    37                                                 'terms'    => $attributes['theme'],
     34                                                'terms'    => $theme,
    3835                                        ),
    3936                                ),
    4037                                'posts_per_page'      => 1,
  • src/wp-includes/class-wp-block-patterns-registry.php

    diff --git a/src/wp-includes/class-wp-block-patterns-registry.php b/src/wp-includes/class-wp-block-patterns-registry.php
    index e516277e42..ddf73446e7 100644
    a b final class WP_Block_Patterns_Registry { 
    165165        private function prepare_content( $pattern, $hooked_blocks ) {
    166166                $content = $pattern['content'];
    167167
    168                 $before_block_visitor = '_inject_theme_attribute_in_template_part_block';
     168                $before_block_visitor = ( defined( 'REST_REQUEST' ) && REST_REQUEST ) ? '_inject_theme_attribute_in_template_part_block' : null;
    169169                $after_block_visitor  = null;
    170170                if ( ! empty( $hooked_blocks ) || has_filter( 'hooked_block_types' ) ) {
    171171                        $before_block_visitor = make_before_block_visitor( $hooked_blocks, $pattern );
    172172                        $after_block_visitor  = make_after_block_visitor( $hooked_blocks, $pattern );
    173173                }
    174                 $blocks  = parse_blocks( $content );
    175                 $content = traverse_and_serialize_blocks( $blocks, $before_block_visitor, $after_block_visitor );
     174                if ( ! is_null( $before_block_visitor ) || ! is_null( $after_block_visitor ) ) {
     175                        $blocks  = parse_blocks( $content );
     176                        $content = traverse_and_serialize_blocks( $blocks, $before_block_visitor, $after_block_visitor );
     177                }
    176178
    177179                return $content;
    178180        }

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.

@gziolo commented on PR #5455:


14 months ago
#16

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

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

    diff --git a/src/wp-includes/block-template-utils.php b/src/wp-includes/block-template-utils.php
    index c5953e1d4c..81f306be93 100644
    a b function _build_block_template_result_from_file( $template_file, $template_type 
    549549                $template->area = $template_file['area'];
    550550        }
    551551
    552         $before_block_visitor = '_inject_theme_attribute_in_template_part_block';
     552        $before_block_visitor = ( defined( 'REST_REQUEST' ) && REST_REQUEST ) ? '_inject_theme_attribute_in_template_part_block' : null;
    553553        $after_block_visitor  = null;
    554554        $hooked_blocks        = get_hooked_blocks();
    555555        if ( ! empty( $hooked_blocks ) || has_filter( 'hooked_block_types' ) ) {
    556556                $before_block_visitor = make_before_block_visitor( $hooked_blocks, $template );
    557557                $after_block_visitor  = make_after_block_visitor( $hooked_blocks, $template );
    558558        }
    559         $blocks            = parse_blocks( $template_content );
    560         $template->content = traverse_and_serialize_blocks( $blocks, $before_block_visitor, $after_block_visitor );
     559        if ( ! is_null( $before_block_visitor ) || ! is_null( $after_block_visitor ) ) {
     560                $blocks           = parse_blocks( $template_content );
     561                $template_content = traverse_and_serialize_blocks( $blocks, $before_block_visitor, $after_block_visitor );
     562        }
     563        $template->content = $template_content;
    561564
    562565        return $template;
    563566}

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 👍🏻

This ticket was mentioned in Slack in #core-performance by mukeshpanchal27. View the logs.


14 months ago

@flixos90 commented on PR #5455:


14 months ago
#18

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.

This ticket was mentioned in Slack in #core-performance by flixos90. View the logs.


14 months ago

#20 @flixos90
14 months ago

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

This ticket was mentioned in Slack in #core by hellofromtonya. View the logs.


14 months ago

@flixos90 commented on PR #5455:


14 months ago
#22

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

@dmsnell commented on PR #5455:


14 months ago
#23

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>

https://i0.wp.com/github.com/WordPress/wordpress-develop/assets/5431237/dce47f29-2271-45f8-9d8a-cac094622a42

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>

https://i0.wp.com/github.com/WordPress/wordpress-develop/assets/5431237/5dab87bb-c81b-475c-9565-c670e9002118

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.

https://i0.wp.com/github.com/WordPress/wordpress-develop/assets/5431237/1999c424-d0b4-4d3d-8aa1-9f01e103df41

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.

https://i0.wp.com/github.com/WordPress/wordpress-develop/assets/5431237/af124110-9d41-4534-a677-a1f772ab1d2f

and going back in time…

https://i0.wp.com/github.com/WordPress/wordpress-develop/assets/5431237/6116844b-ee2b-4442-8072-8630ecfea1df

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

#24 @gziolo
14 months ago

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.

Last edited 14 months ago by gziolo (previous) (diff)

@Bernhard Reiter commented on PR #5455:


14 months ago
#25

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.

@Bernhard Reiter commented on PR #5455:


14 months ago
#26

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 commented on PR #5455:


14 months ago
#28

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.

@hellofromTonya commented on PR #5455:


14 months ago
#29

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.

@hellofromTonya commented on PR #5455:


14 months ago
#30

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

@Bernhard Reiter commented on PR #5455:


14 months ago
#31

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 🤞

#32 @Bernhard Reiter
14 months ago

  • Resolution set to fixed
  • Status changed from reopened to closed

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.

#34 @gziolo
13 months ago

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.

#35 @gziolo
13 months ago

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.

Note: See TracTickets for help on using tickets.