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

Reuse block metadata in WP_Theme_JSON::get_valid_block_style_variations() for better performance #7586

Conversation

mukeshpanchal27
Copy link
Member

@mukeshpanchal27 mukeshpanchal27 commented Oct 18, 2024

Copy link

Test using WordPress Playground

The changes in this pull request can previewed and tested using a WordPress Playground instance.

WordPress Playground is an experimental project that creates a full WordPress instance entirely within the browser.

Some things to be aware of

  • The Plugin and Theme Directories cannot be accessed within Playground.
  • All changes will be lost when closing a tab with a Playground instance.
  • All changes will be lost when refreshing the page.
  • A fresh instance is created each time the link below is clicked.
  • Every time this pull request is updated, a new ZIP file containing all changes is created. If changes are not reflected in the Playground instance,
    it's possible that the most recent build failed, or has not completed. Check the list of workflow runs to be sure.

For more details about these limitations and more, check out the Limitations page in the WordPress Playground documentation.

Test this pull request with WordPress Playground.

@mukeshpanchal27
Copy link
Member Author

Before:

Screenshot 2024-10-21 at 10 01 52 AM

After:

Screenshot 2024-10-21 at 10 01 59 AM

@mukeshpanchal27
Copy link
Member Author

@joemcgill @ramonjd Please review when you have moment. Thanks

@ramonjd
Copy link
Member

ramonjd commented Oct 24, 2024

This change makes sense to me. Good idea to reduce the calls to get_blocks_metadata(), which preserving backwards compatibility.

I did a quick sweep for other instances of get_valid_block_style_variations and couldn't find any.

@mukeshpanchal27 mukeshpanchal27 changed the title Performance assessment for WP_Theme_JSON Performance assessment: Reduce WP_Theme_JSON::get_blocks_metadata() calls Oct 24, 2024
@mukeshpanchal27 mukeshpanchal27 changed the title Performance assessment: Reduce WP_Theme_JSON::get_blocks_metadata() calls Reuse block metadata in WP_Theme_JSON::get_valid_block_style_variations() for better performance Oct 24, 2024
@mukeshpanchal27 mukeshpanchal27 marked this pull request as ready for review October 24, 2024 06:34
Copy link

github-actions bot commented Oct 24, 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.

Core Committers: Use this line as a base for the props when committing in SVN:

Props mukesh27, flixos90, ramonopoly, aaronrobertshaw.

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

Comment on lines +4541 to -4539
protected static function get_valid_block_style_variations( $blocks_metadata = array() ) {
$valid_variations = array();
foreach ( self::get_blocks_metadata() as $block_name => $block_meta ) {
Copy link
Member

Choose a reason for hiding this comment

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

Maybe we should still call self::get_blocks_metadata() in case no value was provided, i.e. as default?

Since this method is protected (not private), in theory someone else could have overwritten this class. Highly unlikely but possible. Additionally though, not requiring the parameter makes using the method more ergonomic in situations where it hasn't been computed before.

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 already added $blocks_metadata = empty( $blocks_metadata ) ? static::get_blocks_metadata() : $blocks_metadata; if the value is not passed of empty.

Let me know if i missed anything.

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

@ramonjd ramonjd left a comment

Choose a reason for hiding this comment

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

LGTM. Smoke tested locally. Tests pass and logs are clean.

Thanks!

Let me know if you need help backporting to the plugin.

@mukeshpanchal27
Copy link
Member Author

Let me know if you need help backporting to the plugin.

Yes please if you have moment. Thanks

ramonjd added a commit to WordPress/gutenberg that referenced this pull request Oct 28, 2024
@ramonjd
Copy link
Member

ramonjd commented Oct 28, 2024

Gutenberg backport PR:

Copy link

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

I've given this one a quick test as well. LGTM too.

As part of the testing I also ran through the various means of defining block style variations (theme.json partials, register_block_style etc). All the variations displayed correctly, could be modified in global styles, and had updates reflected appropriately on the frontend.

LGTM too.

I'll give the plugin backport a run shortly.

ramonjd added a commit to WordPress/gutenberg that referenced this pull request Oct 29, 2024
…block_style_variations() for better performance (#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]>
peterwilsoncc pushed a commit to peterwilsoncc/gutenberg-build that referenced this pull request Oct 30, 2024
…block_style_variations() for better performance (#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]>

Source: WordPress/gutenberg@828868d
@mukeshpanchal27 mukeshpanchal27 self-assigned this Nov 4, 2024
@ramonjd
Copy link
Member

ramonjd commented Nov 4, 2024

Is this ready to be committed? The Gutenberg backport PR is merged.

@mukeshpanchal27
Copy link
Member Author

Is this ready to be committed? The Gutenberg backport PR is merged.

Yes. If you have moment then commit it.

@ramonjd
Copy link
Member

ramonjd commented Nov 6, 2024

Committed in r59359 / 5ff9717

@ramonjd ramonjd closed this Nov 6, 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
None yet
Projects
Status: Done 😃
Development

Successfully merging this pull request may close these issues.

4 participants