#59583 closed defect (bug) (fixed)
Patterns: `theme` attribute not inserted in Template Part blocks
Reported by: | Bernhard Reiter | Owned by: | 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
@Bernhard Reiter commented on PR #5450:
14 months ago
#2
Looking good. Thanks all, I'll land it 😊
@Bernhard Reiter commented on PR #5450:
14 months ago
#4
Committed to Core in https://core.trac.wordpress.org/changeset/56818.
#5
@
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
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
@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:
- remove the Gutenberg specific part from here
- merge it in Gutenberg
- backport the Gutenberg change to Core
- 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 😊
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.
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 ) { 18 18 $template_part_id = null; 19 19 $content = null; 20 20 $area = WP_TEMPLATE_PART_AREA_UNCATEGORIZED; 21 $theme = isset( $attributes['theme'] ) ? $attributes['theme'] : get_stylesheet(); 21 22 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']; 28 25 $template_part_query = new WP_Query( 29 26 array( 30 27 'post_type' => 'wp_template_part', … … function render_block_core_template_part( $attributes ) { 34 31 array( 35 32 'taxonomy' => 'wp_theme', 36 33 'field' => 'name', 37 'terms' => $ attributes['theme'],34 'terms' => $theme, 38 35 ), 39 36 ), 40 37 '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 { 165 165 private function prepare_content( $pattern, $hooked_blocks ) { 166 166 $content = $pattern['content']; 167 167 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; 169 169 $after_block_visitor = null; 170 170 if ( ! empty( $hooked_blocks ) || has_filter( 'hooked_block_types' ) ) { 171 171 $before_block_visitor = make_before_block_visitor( $hooked_blocks, $pattern ); 172 172 $after_block_visitor = make_after_block_visitor( $hooked_blocks, $pattern ); 173 173 } 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 } 176 178 177 179 return $content; 178 180 }
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.
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 ) { 18 18 $template_part_id = null; 19 19 $content = null; 20 20 $area = WP_TEMPLATE_PART_AREA_UNCATEGORIZED; 21 $theme = isset( $attributes['theme'] ) ? $attributes['theme'] : get_stylesheet(); 21 22 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']; 28 25 $template_part_query = new WP_Query( 29 26 array( 30 27 'post_type' => 'wp_template_part', … … function render_block_core_template_part( $attributes ) { 34 31 array( 35 32 'taxonomy' => 'wp_theme', 36 33 'field' => 'name', 37 'terms' => $ attributes['theme'],34 'terms' => $theme, 38 35 ), 39 36 ), 40 37 '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 { 165 165 private function prepare_content( $pattern, $hooked_blocks ) { 166 166 $content = $pattern['content']; 167 167 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; 169 169 $after_block_visitor = null; 170 170 if ( ! empty( $hooked_blocks ) || has_filter( 'hooked_block_types' ) ) { 171 171 $before_block_visitor = make_before_block_visitor( $hooked_blocks, $pattern ); 172 172 $after_block_visitor = make_after_block_visitor( $hooked_blocks, $pattern ); 173 173 } 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 } 176 178 177 179 return $content; 178 180 }
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.
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 549 549 $template->area = $template_file['area']; 550 550 } 551 551 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; 553 553 $after_block_visitor = null; 554 554 $hooked_blocks = get_hooked_blocks(); 555 555 if ( ! empty( $hooked_blocks ) || has_filter( 'hooked_block_types' ) ) { 556 556 $before_block_visitor = make_before_block_visitor( $hooked_blocks, $template ); 557 557 $after_block_visitor = make_after_block_visitor( $hooked_blocks, $template ); 558 558 } 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; 561 564 562 565 return $template; 563 566 }
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
@
14 months ago
@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 allcore/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.
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.
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
- branch
- [broken] at
trunk
with revert of fixed- branch
broken
at 02db6af7
- branch
- [fixed] at
trunk
- branch
fixed
at a8d53284
- branch
- 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.
#24
@
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.
@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.
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 🤞
@Bernhard Reiter commented on PR #5455:
14 months ago
#33
Committed to Core in https://core.trac.wordpress.org/changeset/56896.
#34
@
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
@
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.
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