Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Block Style Variations: Reuse block metadata in WP_Theme_JSON::get_valid_block_style_variations() for better performance #66539

Merged
merged 2 commits into from
Oct 29, 2024

Conversation

ramonjd
Copy link
Member

@ramonjd ramonjd commented Oct 28, 2024

What?

Backporting theme json performance optimizations from WordPress/wordpress-develop#7586

Props @mukeshpanchal27

Why?

Reduce unnecessary calls to ::get_blocks_metadata().

How?

Passing already-fetched data from ::get_blocks_metadata() to ::get_valid_block_style_variations(()

Testing Instructions

CI tests should pass. Smoke test a theme with style variations in the site editor.

Here's an example from #57908 that you can add under /styles.

{
	"$schema": "https://schemas.wp.org/trunk/theme.json",
	"version": 2,
	"title": "Variation A",
	"blockTypes": [ "core/group", "core/columns", "core/media-text" ],
	"styles": {
		"color": {
			"background": "#eed8d3",
			"text": "#201819"
		},
		"elements": {
			"heading": {
				"color": {
					"text": "#201819"
				}
			}
		},
		"blocks": {
			"core/group": {
				"color": {
					"background": "#825f58",
					"text": "#eed8d3"
				},
				"elements": {
					"heading": {
						"color": {
							"text": "#eed8d3"
						}
					}
				}
			}
		}
	}
}

@ramonjd ramonjd added [Type] Performance Related to performance efforts Backport from WordPress Core Pull request that needs to be backported to a Gutenberg release from WordPress Core No Core Sync Required Indicates that any changes do not need to be synced to WordPress Core labels Oct 28, 2024
@ramonjd ramonjd self-assigned this Oct 28, 2024
Copy link

github-actions bot commented Oct 28, 2024

Warning: Type of PR label mismatch

To merge this PR, it requires exactly 1 label indicating the type of PR. Other labels are optional and not being checked here.

  • Type-related labels to choose from: [Type] Automated Testing, [Type] Breaking Change, [Type] Bug, [Type] Build Tooling, [Type] Code Quality, [Type] Copy, [Type] Developer Documentation, [Type] Enhancement, [Type] Experimental, [Type] Feature, [Type] New API, [Type] Task, [Type] Technical Prototype, [Type] Performance, [Type] Project Management, [Type] Regression, [Type] Security, [Type] WP Core Ticket, Backport from WordPress Core, Gutenberg Plugin.
  • Labels found: [Type] Performance, Backport from WordPress Core, [Feature] Block Style Variations, No Core Sync Required.

Read more about Type labels in Gutenberg. Don't worry if you don't have the required permissions to add labels; the PR reviewer should be able to help with the task.

Copy link

Warning: Type of PR label mismatch

To merge this PR, it requires exactly 1 label indicating the type of PR. Other labels are optional and not being checked here.

  • Type-related labels to choose from: [Type] Automated Testing, [Type] Breaking Change, [Type] Bug, [Type] Build Tooling, [Type] Code Quality, [Type] Copy, [Type] Developer Documentation, [Type] Enhancement, [Type] Experimental, [Type] Feature, [Type] New API, [Type] Task, [Type] Technical Prototype, [Type] Performance, [Type] Project Management, [Type] Regression, [Type] Security, [Type] WP Core Ticket, Backport from WordPress Core, Gutenberg Plugin.
  • Labels found: [Type] Performance, Backport from WordPress Core, No Core Sync Required.

Read more about Type labels in Gutenberg. Don't worry if you don't have the required permissions to add labels; the PR reviewer should be able to help with the task.

Copy link

github-actions bot commented Oct 28, 2024

The following accounts have interacted with this PR and/or linked issues. I will continue to update these lists as activity occurs. You can also manually ask me to refresh this list by adding the props-bot label.

If you're merging code through a pull request on GitHub, copy and paste the following into the bottom of the merge commit message.

Co-authored-by: ramonjd <[email protected]>
Co-authored-by: mukeshpanchal27 <[email protected]>
Co-authored-by: aaronrobertshaw <[email protected]>

To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook.

$registry = WP_Block_Type_Registry::get_instance();
$valid_block_names = array_keys( $registry->get_all_registered() );
$blocks_metadata = static::get_blocks_metadata();
$valid_block_names = array_keys( $blocks_metadata );
Copy link
Member Author

Choose a reason for hiding this comment

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

I'm tentatively backporting this change too.

Core uses ::get_blocks_metadata() to get valid block names. See: https://github.com/mukeshpanchal27/wordpress-develop/blob/trunk/src/wp-includes/class-wp-theme-json.php#L762

It has done it this way since 5.9 😄

Then I found this Gutenberg PR from 2022, which doesn't appear to have been backported:

Which is correct?

I think maybe we could use what Core has since ::get_valid_block_style_variations() will now call ::get_blocks_metadata() anyway.

cc @ajlende @oandregal @aaronrobertshaw @mukeshpanchal27 for advice.

Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks for highlighting this. It might have been a factor in why the valid variations function evolved as it did.

I think maybe we could use what Core has since ::get_valid_block_style_variations() will now call ::get_blocks_metadata() anyway.

Agreed. It saves another update in core too.

Copy link
Member

Choose a reason for hiding this comment

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

mmm, perhaps something that was added in gutenberg but not backported later to core? anyway, this seems a safe change to me given blocks metadata will recalculate the blocks that are registered (if they're different).

Copy link
Member

@mukeshpanchal27 mukeshpanchal27 left a comment

Choose a reason for hiding this comment

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

Thanks @ramonjd, LGTM.

Copy link
Contributor

@aaronrobertshaw aaronrobertshaw left a comment

Choose a reason for hiding this comment

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

Thanks for wrangling this one @ramonjd 👍

✅ Code changes match #66539
✅ Block style variation registration works as before
✅ Block styles can be updated via Global Styles
✅ Updated block styles are reflected on the frontend

LGTM 🚢

@ramonjd
Copy link
Member Author

ramonjd commented Oct 29, 2024

Thanks for the help and reviews, everyone!

@ramonjd ramonjd merged commit 828868d into trunk Oct 29, 2024
67 checks passed
@ramonjd ramonjd deleted the backport/wordpress-develop-7586 branch October 29, 2024 20:24
@github-actions github-actions bot added this to the Gutenberg 19.6 milestone Oct 29, 2024
@cbravobernal cbravobernal added the [Feature] Block Style Variations Issues or PRs that are related to the style variations for blocks label Oct 30, 2024
@cbravobernal cbravobernal changed the title Backport from Core: Reuse block metadata in WP_Theme_JSON::get_valid_block_style_variations() for better performance Block Style Variations: Reuse block metadata in WP_Theme_JSON::get_valid_block_style_variations() for better performance Oct 30, 2024
karthick-murugan pushed a commit to karthick-murugan/gutenberg that referenced this pull request Nov 13, 2024
…block_style_variations() for better performance (WordPress#66539)

* Backporting theme json performance optimizations from WordPress/wordpress-develop#7586

* Tentatively backport using `get_blocks_metadata` instead of `WP_Block_Type_Registry` to get block names.

Co-authored-by: ramonjd <[email protected]>
Co-authored-by: mukeshpanchal27 <[email protected]>
Co-authored-by: aaronrobertshaw <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Backport from WordPress Core Pull request that needs to be backported to a Gutenberg release from WordPress Core [Feature] Block Style Variations Issues or PRs that are related to the style variations for blocks No Core Sync Required Indicates that any changes do not need to be synced to WordPress Core [Type] Performance Related to performance efforts
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants