Template Part block: Fall back to current theme if no theme attribute is given. #55217

ockham · 2023-10-10T18:37:42Z

Part of the task in WordPress Core seeking performance improvements:

An alternative approach to #53423.

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

What?

If a Template Part block is encountered that doesn't have a theme attribute, fall back to the current them (get_stylesheet()) instead.

Why?

This should allow removing injection of the theme attribute during rendering of the Template Part block.

How?

By falling back to the current theme (get_stylesheet()) if $attributes['theme'] isn't set.

Testing Instructions

Verify that Template Parts blocks that are part of patterns still work, both on the frontend and in the editor. Specifically, verify that there are no Template part has been deleted or is unavailable errors.

Will be followed up by a Core PR to remove theme attribute injection on the frontend.

cc/ @gziolo

… is given.

felixarntz

I know this is a WIP, just sharing very early feedback here :)

packages/block-library/src/template-part/index.php

Co-authored-by: Felix Arntz <[email protected]>

gziolo · 2023-10-11T07:31:43Z

After looking at how #55217 is implemented, I think we should also remove the following line that get executed only in the Gutenberg plugin:

gutenberg/packages/block-library/src/pattern/index.php

Line 47 in 8a9cb70

$content = _inject_theme_attribute_in_block_template_content( $content );

The same logic should be handled now on the front end with the fallback logic for the theme attribute in the Template Part block.

mikachan

Thank you for working on this! I can confirm this fixes the original issue in #53194.

I tested using the same themes (Overlaid and Lativ) as when I tested #53423, and this PR works well 👍

Resolves WordPress/gutenberg#55217 When rendering images with the lightbox "expand on click" function enabled, if an image block is encountered with no image (for example, a block was added but no image was ever selected), then WordPress would create a warning that an "undefined array key 0" was being accessed because the server code assumes an IMG element exists in the block's HTML. In this patch a check is performed to ensure that an IMG block exists before processing the block. If one isn't, for any reason, the original given HTML for the block is passed through un-modified. This patch needs to be backported into Gutenberg from where it's sourced but this issue arose during the WordPress 6.4 beta testing and needs to be resolved immediately in Core.

pbking · 2023-10-11T14:45:35Z

Tested in all scenarios I can dream up and works as I would expect. My only suggestion is that, as @gziolo suggested, the call to _inject_theme_attribute_in_block_template_content() be removed in this change. It's unnecessary with this change.

Once that change has been made this is ready to ship.

felixarntz

Pending the additional feedback in #55217 (comment), the change so far looks good to me.

…ependency

…() call from pattern block.

felixarntz · 2023-10-11T15:37:54Z

Given @ockham is out today, I took the liberty of pushing the additionally requested change in 47608ff. @gziolo @pbking @mikachan Could you please take another look?

mikachan

LGTM and works well in my testing, thanks @felixarntz!

felixarntz

I am currently benchmarking and profiling this change further, and it seems the approach here is not right in terms of performance at least. Marking as request for changes for now, let's hold off merging at this point.

felixarntz

I have concluded my performance investigation here and left a summary in https://core.trac.wordpress.org/ticket/59583#comment:20.

TL;DR This fix makes sense and is good to merge. However, it doesn't fully address the performance issues (partly because the initial "performance win" was mostly due to the bug introduced.

I have identified another very promising performance opportunity related to this https://core.trac.wordpress.org/ticket/59600 and WordPress/wordpress-develop#5463

felixarntz · 2023-10-11T20:26:11Z

@ockham @gziolo @mikachan I think this is good to merge. I'll leave that to you since I'm not sure on the workflow in relation to backporting to the 6.4 release, but LGTM from my end. Once backported to core, we can update WordPress/wordpress-develop#5455 accordingly.

ockham · 2023-10-12T09:18:47Z

Thanks all! I'll go ahead and merge this then; also going to mark for inclusion in WP RC.

@mikachan We need this in Core to unblock WordPress/wordpress-develop#5455. In principle, it's probably fine to land on Monday, but it might be nice to do it even earlier to give us some more time to discover any potential issues with this. Are y'all planning a package sync this week still by any chance? 😊

ockham · 2023-10-12T09:19:50Z

packages/block-library/src/template-part/index.php

@@ -18,12 +18,7 @@ function render_block_core_template_part( $attributes ) {
 	$template_part_id = null;
 	$content          = null;
 	$area             = WP_TEMPLATE_PART_AREA_UNCATEGORIZED;
-
-	if ( isset( $attribues['theme'] ) ) {


… is given. (#55217) * Template Part block: Fall back to current theme if no theme attribute is given. * Remove now unnecessary _inject_theme_attribute_in_template_part_block() call from pattern block. --------- Co-authored-by: Greg Ziółkowski <[email protected]> Co-authored-by: Felix Arntz <[email protected]> Co-authored-by: Felix Arntz <[email protected]>

mikachan · 2023-10-12T09:58:35Z

I just cherry-picked this PR to the 6.4-rc1 branch to get it included in the next release: fd15da3

* List: fix forward merging of nested list (#55121) * Private APIs: Update consent string for unlocking access. (#55182) Replace the consent string with `I know using unstable features means my theme or plugin will inevitably break in the next version of WordPress`. * useBlockSettings: add missing useMemo dependencies (#55204) * Remove the lightbox filter and view file when the lightbox setting is disabled. (#55120) * Remove the filter and view file of the lightbox when the lightbox setting is disabled. * Make sure to add filter and view file when needed. * Fix block comment indentation. * Patterns: Remove the version enforcement for npm in `engines` field (#55245) * Patterns: Remove the version enforcement for npm in `engines` * Regenerate the lock file * Remove `@return void` from PHP function docs. (#55237) # Conflicts: # packages/block-library/src/form/index.php * Image: Disable lightbox editor UI for linked images (#55141) * Disable lightbox UI and add help text for linked images * Update help text * Image: Stop crashing with Lightbox on image blocks without an image (#55269) * Stop crashing with Lightbox on image blocks without an image * Fix PHPCS error * Update fullscreen icon (#55021) * Template Part block: Fall back to current theme if no theme attribute is given. (#55217) * Template Part block: Fall back to current theme if no theme attribute is given. * Remove now unnecessary _inject_theme_attribute_in_template_part_block() call from pattern block. --------- Co-authored-by: Greg Ziółkowski <[email protected]> Co-authored-by: Felix Arntz <[email protected]> Co-authored-by: Felix Arntz <[email protected]> --------- Co-authored-by: Ella <[email protected]> Co-authored-by: Peter Wilson <[email protected]> Co-authored-by: Jarda Snajdr <[email protected]> Co-authored-by: Andrea Fercia <[email protected]> Co-authored-by: Greg Ziółkowski <[email protected]> Co-authored-by: tellthemachines <[email protected]> Co-authored-by: Artemio Morales <[email protected]> Co-authored-by: Rich Tabor <[email protected]> Co-authored-by: Bernie Reiter <[email protected]> Co-authored-by: Felix Arntz <[email protected]> Co-authored-by: Felix Arntz <[email protected]>

Updates the npm packages and code for: * [WordPress/gutenberg#55121 List: fix forward merging of nested list] * [WordPress/gutenberg#55182 Update consent string for using private APIs.] * [WordPress/gutenberg#55204 useBlockSettings: add missing useMemo dependencies] * [WordPress/gutenberg#55120 Remove the lightbox filter and view file when the lightbox setting is disabled.] * [WordPress/gutenberg#55245 Patterns: Remove the version enforcement for npm in engines field] * [WordPress/gutenberg#55237 Remove `@return void` from PHP function docs] * [WordPress/gutenberg#55141 Image: Disable lightbox editor UI for linked images] * [WordPress/gutenberg#55269 Image: Stop crashing with Lightbox on image blocks without an image] * [WordPress/gutenberg#55021 Update fullscreen icon] * [WordPress/gutenberg#55217 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: * [WordPress/gutenberg#55298 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. git-svn-id: https://develop.svn.wordpress.org/trunk@56849 602fd350-edb4-49c9-b593-d223f7449a82

Updates the npm packages and code for: * [WordPress/gutenberg#55121 List: fix forward merging of nested list] * [WordPress/gutenberg#55182 Update consent string for using private APIs.] * [WordPress/gutenberg#55204 useBlockSettings: add missing useMemo dependencies] * [WordPress/gutenberg#55120 Remove the lightbox filter and view file when the lightbox setting is disabled.] * [WordPress/gutenberg#55245 Patterns: Remove the version enforcement for npm in engines field] * [WordPress/gutenberg#55237 Remove `@return void` from PHP function docs] * [WordPress/gutenberg#55141 Image: Disable lightbox editor UI for linked images] * [WordPress/gutenberg#55269 Image: Stop crashing with Lightbox on image blocks without an image] * [WordPress/gutenberg#55021 Update fullscreen icon] * [WordPress/gutenberg#55217 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: * [WordPress/gutenberg#55298 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. Built from https://develop.svn.wordpress.org/trunk@56849 git-svn-id: http://core.svn.wordpress.org/trunk@56361 1a063a9b-81f0-0310-95a4-ce76da25c4cd

Updates the npm packages and code for: * [WordPress/gutenberg#55121 List: fix forward merging of nested list] * [WordPress/gutenberg#55182 Update consent string for using private APIs.] * [WordPress/gutenberg#55204 useBlockSettings: add missing useMemo dependencies] * [WordPress/gutenberg#55120 Remove the lightbox filter and view file when the lightbox setting is disabled.] * [WordPress/gutenberg#55245 Patterns: Remove the version enforcement for npm in engines field] * [WordPress/gutenberg#55237 Remove `@return void` from PHP function docs] * [WordPress/gutenberg#55141 Image: Disable lightbox editor UI for linked images] * [WordPress/gutenberg#55269 Image: Stop crashing with Lightbox on image blocks without an image] * [WordPress/gutenberg#55021 Update fullscreen icon] * [WordPress/gutenberg#55217 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: * [WordPress/gutenberg#55298 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. Built from https://develop.svn.wordpress.org/trunk@56849 git-svn-id: https://core.svn.wordpress.org/trunk@56361 1a063a9b-81f0-0310-95a4-ce76da25c4cd

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 WordPress/gutenberg#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. git-svn-id: https://develop.svn.wordpress.org/trunk@56896 602fd350-edb4-49c9-b593-d223f7449a82

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 WordPress/gutenberg#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. Built from https://develop.svn.wordpress.org/trunk@56896 git-svn-id: http://core.svn.wordpress.org/trunk@56407 1a063a9b-81f0-0310-95a4-ce76da25c4cd

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 WordPress/gutenberg#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. Built from https://develop.svn.wordpress.org/trunk@56896 git-svn-id: https://core.svn.wordpress.org/trunk@56407 1a063a9b-81f0-0310-95a4-ce76da25c4cd

Updates the npm packages and code for: * [WordPress/gutenberg#55121 List: fix forward merging of nested list] * [WordPress/gutenberg#55182 Update consent string for using private APIs.] * [WordPress/gutenberg#55204 useBlockSettings: add missing useMemo dependencies] * [WordPress/gutenberg#55120 Remove the lightbox filter and view file when the lightbox setting is disabled.] * [WordPress/gutenberg#55245 Patterns: Remove the version enforcement for npm in engines field] * [WordPress/gutenberg#55237 Remove `@return void` from PHP function docs] * [WordPress/gutenberg#55141 Image: Disable lightbox editor UI for linked images] * [WordPress/gutenberg#55269 Image: Stop crashing with Lightbox on image blocks without an image] * [WordPress/gutenberg#55021 Update fullscreen icon] * [WordPress/gutenberg#55217 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: * [WordPress/gutenberg#55298 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. git-svn-id: https://develop.svn.wordpress.org/trunk@56849 602fd350-edb4-49c9-b593-d223f7449a82

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 WordPress/gutenberg#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. git-svn-id: https://develop.svn.wordpress.org/trunk@56896 602fd350-edb4-49c9-b593-d223f7449a82

Template Part block: Fall back to current theme if no theme attribute…

8fa1ff8

… is given.

ockham self-assigned this Oct 10, 2023

felixarntz reviewed Oct 10, 2023

View reviewed changes

packages/block-library/src/template-part/index.php Outdated Show resolved Hide resolved

packages/block-library/src/template-part/index.php Outdated Show resolved Hide resolved

ockham mentioned this pull request Oct 10, 2023

Patterns: Don't inject theme attr on frontend WordPress/wordpress-develop#5455

Closed

gziolo and others added 2 commits October 11, 2023 08:37

Update packages/block-library/src/template-part/index.php

f9d9d35

Co-authored-by: Felix Arntz <[email protected]>

Fix the type in the variable name

06af195

gziolo added [Block] Pattern Affects the Patterns Block [Type] Enhancement A suggestion for improvement. [Block] Template Part Affects the Template Parts Block and removed [Block] Pattern Affects the Patterns Block labels Oct 11, 2023

gziolo mentioned this pull request Oct 11, 2023

Inject theme stylesheet value as template part theme attribute #53423

Merged

gziolo requested review from pbking and scruffian October 11, 2023 07:28

gziolo requested a review from mikachan October 11, 2023 07:32

mikachan reviewed Oct 11, 2023

View reviewed changes

dmsnell mentioned this pull request Oct 11, 2023

Fix: Stop crashing with Lightbox on image blocks without an image. WordPress/wordpress-develop#5461

Closed

artemiomorales mentioned this pull request Oct 11, 2023

Image: Stop crashing with Lightbox on image blocks without an image #55269

Merged

felixarntz approved these changes Oct 11, 2023

View reviewed changes

felixarntz added 2 commits October 11, 2023 10:33

Merge branch 'trunk' into try/fixing-template-part-block-theme-attr-d…

b769297

…ependency

Remove now unnecessary _inject_theme_attribute_in_template_part_block…

47608ff

…() call from pattern block.

felixarntz marked this pull request as ready for review October 11, 2023 15:38

felixarntz requested a review from ajitbohra as a code owner October 11, 2023 15:38

felixarntz added a commit to ockham/wordpress-develop that referenced this pull request Oct 11, 2023

Reflect latest changes from WordPress/gutenberg#55217.

4fb3a58

mikachan approved these changes Oct 11, 2023

View reviewed changes

felixarntz requested changes Oct 11, 2023

View reviewed changes

felixarntz approved these changes Oct 11, 2023

View reviewed changes

ockham added the Backport to WP 6.7 Beta/RC Pull request that needs to be backported to the WordPress major release that's currently in beta label Oct 12, 2023

ockham merged commit 1e0c9c6 into trunk Oct 12, 2023
52 checks passed

ockham deleted the try/fixing-template-part-block-theme-attr-dependency branch October 12, 2023 09:19

ockham commented Oct 12, 2023

View reviewed changes

github-actions bot added this to the Gutenberg 16.9 milestone Oct 12, 2023

mikachan removed the Backport to WP 6.7 Beta/RC Pull request that needs to be backported to the WordPress major release that's currently in beta label Oct 12, 2023

ockham mentioned this pull request Oct 15, 2023

Epic: Block Hooks Features for WP 6.5 #54904

Closed

16 tasks

gziolo mentioned this pull request Nov 8, 2023

Template Part: Add fallback to the current theme when not provided #55965

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Template Part block: Fall back to current theme if no theme attribute is given. #55217

Template Part block: Fall back to current theme if no theme attribute is given. #55217

ockham commented Oct 10, 2023 •

edited

Loading

felixarntz left a comment

gziolo commented Oct 11, 2023

mikachan left a comment

pbking commented Oct 11, 2023

felixarntz left a comment

felixarntz commented Oct 11, 2023

mikachan left a comment

felixarntz left a comment

felixarntz left a comment

felixarntz commented Oct 11, 2023

ockham commented Oct 12, 2023

ockham Oct 12, 2023

mikachan commented Oct 12, 2023

Template Part block: Fall back to current theme if no theme attribute is given. #55217

Template Part block: Fall back to current theme if no theme attribute is given. #55217

Conversation

ockham commented Oct 10, 2023 • edited Loading

What?

Why?

How?

Testing Instructions

felixarntz left a comment

Choose a reason for hiding this comment

gziolo commented Oct 11, 2023

mikachan left a comment

Choose a reason for hiding this comment

pbking commented Oct 11, 2023

felixarntz left a comment

Choose a reason for hiding this comment

felixarntz commented Oct 11, 2023

mikachan left a comment

Choose a reason for hiding this comment

felixarntz left a comment

Choose a reason for hiding this comment

felixarntz left a comment

Choose a reason for hiding this comment

felixarntz commented Oct 11, 2023

ockham commented Oct 12, 2023

ockham Oct 12, 2023

Choose a reason for hiding this comment

mikachan commented Oct 12, 2023

ockham commented Oct 10, 2023 •

edited

Loading