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

Editor: Return block patterns to server-generated settings #2672

Closed

Conversation

jsnajdr
Copy link
Member

@jsnajdr jsnajdr commented May 4, 2022

This reverts commit 31c2639.

As a companion to Gutenberg WordPress/gutenberg#40818, start adding __experimentaBlockPatterns and __experimentalBlockPatternCategories to server-generated editor settings. That makes sure that patterns registered in admin_init or current_screen are not lost.

Fixes WordPress/gutenberg#40736

Trac ticket: https://core.trac.wordpress.org/ticket/55567

@johnstonphilip
Copy link

I have tested and confirm that this change solves the problem in WordPress/gutenberg#40736, which was that calling register_block_pattern from the current_screen hook no longer worked on 6.0.

On this branch it works again.

Comment on lines 205 to 206
'__experimentalBlockPatterns' => WP_Block_Patterns_Registry::get_instance()->get_all_registered(),
'__experimentalBlockPatternCategories' => WP_Block_Pattern_Categories_Registry::get_instance()->get_all_registered(),
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
'__experimentalBlockPatterns' => WP_Block_Patterns_Registry::get_instance()->get_all_registered(),
'__experimentalBlockPatternCategories' => WP_Block_Pattern_Categories_Registry::get_instance()->get_all_registered(),
'blockPatterns' => WP_Block_Patterns_Registry::get_instance()->get_all_registered(),
'blockPatternCategories' => WP_Block_Pattern_Categories_Registry::get_instance()->get_all_registered(),

WP is the stable product. The philosophy to maintain backward compatibility means that once these are added, Core is stuck with them. So lets just call them stable.

If a future iteration of this code does end up using the rest API, these would be need to be maintained but become a wrapper for the API call.

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 can't change the names, they are names of the settings fields that the wp.editPost.initializeEditor API expects. The consumer of the API doesn't get to choose them.

And we don't want the "pass the block patterns data object as a setting when initializing the editor" API to become stable, because using it causes performance issues. The block pattern data are 200kB even on vanilla WP with Twenty Twenty-Two, and they are slowing down the editor load and initialization. They should be loaded solely with REST endpoint.

This patch merely makes pattern registration in current_screen work again.

Copy link
Contributor

@adamziel adamziel May 11, 2022

Choose a reason for hiding this comment

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

@jsnajdr that 200kB is my only concern here. Do you think there's any way to:

  • Check if any additional patterns were registered by the extenders
  • If yes, include the __experimentalBlockPatterns setting
  • If not, do not include the __experimentalBlockPatterns setting

? Otherwise that 200+kB will always be a part of the config and will likely keep growing over time.

Copy link
Member Author

Choose a reason for hiding this comment

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

@adamziel When registering a pattern I could use a 'init' !== current_action() check to see if the pattern is being registered with an action different than init, where the REST API controller doesn't see it. And then the __experimentalBlockPatterns setting would include only these registered-outside-init patterns.

Most patterns are registered on init, e.g., Core and themes shipped with Core do that. We can rely on them to be sent in the REST response and don't need to include them in a big initial setting JSON.

What do you think?

Copy link
Contributor

Choose a reason for hiding this comment

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

I think that's a brilliant idea @jsnajdr! It saves everyone 200kB and only increases the config size for those developers who specifically want to do that. I don't see any downsides really.

Copy link
Member Author

Choose a reason for hiding this comment

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

@adamziel I implemented the idea in 7d32ca9. I'm going to update also the patch in WordPress/gutenberg#40818 and test them together.

One thing I'm unhappy about is the $outside_init_only parameter for the get_all_registered method. I don't want it to become a permanent part of the public API. It should be eventually removed after all patterns are registered the new way. Is there a way to flag it as "unstable"?

Copy link
Member Author

Choose a reason for hiding this comment

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

Tested this with patched Core, patched Gutenberg and the Blockmeister plugin. The result is that initial HTML has __experimentalBlockPatterns setting with just the patterns (and categories) registered by Blockmeister (I created one), and the rest is loaded with REST API. Namely the core/* and twentytwentytwo/* patterns, and the remote ones, featured patterns from wp.org. And the editor UI merges them together.

Copy link
Contributor

Choose a reason for hiding this comment

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

Once plugins move the registration to the recommended hooks, the HTML will become [] so I don't think it's a huge problem.

If I understand correctly, the fallback is for plugins that register the patterns on hooks that occur after the admin/front-end and REST API execution path fork.

I'm in the process of confirming, but it looks like that happens shortly after the wp_loaded hook fires so you might be able to do if ( ! did_action( 'wp_loaded' ) ) { /* don't populate config */ }.

I love this idea @jsnajdr, it's a really nice way to maintain the improvement without breaking back-compat.

Copy link
Member Author

Choose a reason for hiding this comment

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

All execution paths, frontend, admin and REST, execute the wp-settings.php file, with the actions that are there: plugin(s)_loaded, init and wp_loaded. And they diverge shortly after that. The REST path diverges in $wp->parse_request, where it hooks into the parse_request action, where if it detects a REST request it performs it and dies.

@jsnajdr
Copy link
Member Author

jsnajdr commented May 5, 2022

I have tested and confirm that this change solves the problem in WordPress/gutenberg#40736, which was that calling register_block_pattern from the current_screen hook no longer worked on 6.0.

Thanks for testing! Let me just note that applying only this patch, and not also the Gutenberg one (WordPress/gutenberg#40818) fixes only a part of the issue. Important things remain broken. Namely, remote patterns, loaded from api.wordpress.org, would never be loaded and available. These are loaded solely in the REST endpoint, and without WordPress/gutenberg#40818 the settings data and REST data wouldn't get merged. Only the settings data would be used.

Copy link
Contributor

@adamziel adamziel left a comment

Choose a reason for hiding this comment

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

Thank you @jsnajdr! This works as expected, is well thought out, and maintains the BC without the added burden of delivering all block patterns on each page load.

@jsnajdr
Copy link
Member Author

jsnajdr commented May 16, 2022

Thanks for reviewing @adamziel 👍 Note that this patch needs to be committed together with WordPress/gutenberg#40818, both in the Gutenberg plugin and in the block editor shipped with Core, otherwise block patterns will stop working.

@jsnajdr jsnajdr force-pushed the revert/block-patterns-server-settings branch from f534e90 to 352a284 Compare May 16, 2022 14:11
@gziolo
Copy link
Member

gziolo commented May 17, 2022

Committed with https://core.trac.wordpress.org/changeset/53404.

@gziolo gziolo closed this May 17, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Regression: Cannot register_block_pattern using current_screen hook.
5 participants