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

Regression: Cannot register_block_pattern using current_screen hook. #40736

Closed
johnstonphilip opened this issue Apr 29, 2022 · 16 comments · Fixed by #40818
Closed

Regression: Cannot register_block_pattern using current_screen hook. #40736

johnstonphilip opened this issue Apr 29, 2022 · 16 comments · Fixed by #40818
Assignees
Labels
[Feature] Patterns A collection of blocks that can be synced (previously reusable blocks) or unsynced [Type] Bug An existing feature does not function as intended [Type] Regression Related to a regression in the latest release

Comments

@johnstonphilip
Copy link
Contributor

johnstonphilip commented Apr 29, 2022

Description

On WP 5.9.4-alpha-53081 I was able to register block patterns using the current_screen hook.

On WP 6.0-beta3-53305, the same code does not register block patterns any longer.

Example code:

function register_block_patterns() {
	register_block_pattern(
		'wpdocs-my-plugin/my-awesome-pattern',
		array(
			'title'       => __( 'My Awesome Pattern', 'wpdocs-my-plugin' ),
			'description' => _x( 'Two horizontal buttons, the left button is filled in, and the right button is outlined.', 'Block pattern description', 'wpdocs-my-plugin' ),
			'content'     => "<!-- wp:buttons {\"align\":\"center\"} -->\n<div class=\"wp-block-buttons aligncenter\"><!-- wp:button {\"backgroundColor\":\"very-dark-gray\",\"borderRadius\":0} -->\n<div class=\"wp-block-button\"><a class=\"wp-block-button__link has-background has-very-dark-gray-background-color no-border-radius\">" . esc_html__( 'Button One', 'wpdocs-my-plugin' ) . "</a></div>\n<!-- /wp:button -->\n\n<!-- wp:button {\"textColor\":\"very-dark-gray\",\"borderRadius\":0,\"className\":\"is-style-outline\"} -->\n<div class=\"wp-block-button is-style-outline\"><a class=\"wp-block-button__link has-text-color has-very-dark-gray-color no-border-radius\">" . esc_html__( 'Button Two', 'wpdocs-my-plugin' ) . "</a></div>\n<!-- /wp:button --></div>\n<!-- /wp:buttons -->",
		)
	);
}
add_action( 'current_screen', 'register_block_patterns' );

Step-by-step reproduction instructions

  1. On WP 5.9.4-alpha-53081 use the example code above to register a pattern on the current_screen hook.
  2. Go to "Posts" > "Add New", and notice you can insert that pattern using the normal pattern insertion flow.
  3. On WP 6.0-beta3-53305, notice it no longer works, and no pattern is registered or shown in the block editor.

Screenshots, screen recording, code snippet

  • WP version 5.9.4-alpha-53081 ✅ Works as expected.
  • WP version 6.0-beta3-53305 ❌ Broken

Environment info

  • Not using Gutenberg on this install, but am using WP 6.0-beta3-53305

Please confirm that you have searched existing issues in the repo.

Yes

Please confirm that you have tested with all plugins deactivated except Gutenberg.

Yes

@johnstonphilip johnstonphilip added the [Type] Bug An existing feature does not function as intended label Apr 29, 2022
@johnstonphilip
Copy link
Contributor Author

This might be related to https://github.com/WordPress/gutenberg/pull/39493/files but I am not 100% sure yet.

@Mamaduka

@ocean90
Copy link
Member

ocean90 commented Apr 29, 2022

Patterns are now loaded through the REST API, see bdd024e. current_screen is/was not really a proper action to register patterns IMO.

@johnstonphilip
Copy link
Contributor Author

johnstonphilip commented Apr 29, 2022

@ocean90 If that's true, should there be warnings thrown when using register_block_pattern with the wrong hook?

@johnstonphilip
Copy link
Contributor Author

Also, with this new REST API approach, there seems to be no way of limiting which patterns load for specific post types. For example if I want to load "woocommerce specific" block patterns only on product posts, I can no longer do that with this change.

@johnstonphilip johnstonphilip added the [Type] Regression Related to a regression in the latest release label Apr 29, 2022
@gziolo gziolo added the [Feature] Patterns A collection of blocks that can be synced (previously reusable blocks) or unsynced label Apr 30, 2022
@ndiego
Copy link
Member

ndiego commented May 2, 2022

Hey @johnstonphilip, thanks for submitting this issue! Is the use of current_screen a specific approach that you discovered, or an implementation technique pulled from some other documentation? The handbook indicates that init should be used. If you did see current_screen somewhere else, I would like to get that updated.

Given the move to loading patterns through the REST API, it's unlikely that registering patterns using current_screen will continue to be possible. That said, the concept of conditional patterns based on post type is quite interesting and something that I could see baked into pattern registration itself. Or perhaps registering certain pattern categories that are only available on certain post types, or in certain contexts, like the page Editor versus the Site Editor. 🤔

@johnstonphilip
Copy link
Contributor Author

It's not something from any documentation, but just the only hook available where registering by post_type was possible.

I suppose it's fine to not do anything to fix this regression since it wasn't part of the official documentation, but anyone with knowledge of how WordPress hooks work under the hood might have done it this way to improve performance, and/or to filter patterns by any number of reasons (post_type, site editor, etc).

I guess the thing to be aware of here is that this change could break implementations that are currently working.

If using any other hook than init truly is wrong, it should likely throw a doing_it_wrong so people know why their patterns aren't working all of the sudden on 6.0. Throwing that is normal practice for functions that need to be called under very specific hooks/pretenses.

@johnstonphilip
Copy link
Contributor Author

I opened a trac ticket to add _doing_it_wrong to register_block_pattern if not using init hook here:
https://core.trac.wordpress.org/ticket/55655

@hellofromtonya
Copy link
Contributor

The docs say:

register_block_pattern() should be called from a handler attached to the init hook.

"should" is not the same as "must". Changing the code to throw a _doing_it_wrong() when not hooked to init means any block patterns not registered to the init hook will no longer work. In other words, it changes it to a "must be registered to init hard requirement.

Is this problematic for extenders? Should registration only be with init?

@costdev
Copy link
Contributor

costdev commented May 3, 2022

Here's some examples of extender usage from this search result:

Most seem to use init, but these are a few examples of where that's not the case.

@peterwilsoncc
Copy link
Contributor

peterwilsoncc commented May 3, 2022

I've put this on the WP 6.0 project as it's a regression.

@ndiego
Copy link
Member

ndiego commented May 3, 2022

@peterwilsoncc given the current/new implementation of patterns, I'm not sure that current_screen can be supported as a valid action to register patterns moving forward. @ntsekouras any thoughts on this? As Phil noted, I think the change might have happened here: https://github.com/WordPress/gutenberg/pull/39493/files

@johnstonphilip
Copy link
Contributor Author

Is this problematic for extenders? Should registration only be with init?

@hellofromtonya Note that the changes in bdd024e likely already broke every extender's implementations that do not use init. Therefore this change to add _doing_it_wrong is unlikely to be the cause of an actual problem, but rather will provide guidance as to why patterns are all the sudden not working on 6.0.

@peterwilsoncc
Copy link
Contributor

WordPress has a core philosophy to maintain backward compatibility, so neither a doing it wrong call or saying it's not possible is the answer. Something needs to be fixed.

bdd024e removed examples of Gutenberg registering block patterns on hooks other than init. Extenders learnt to use current_screen from Core & Gutenberg.

@ntsekouras
Copy link
Contributor

I'm not really sure how to proceed with this one.. Any thoughts @jsnajdr ?

@jsnajdr
Copy link
Member

jsnajdr commented May 4, 2022

Ideally we should add a register_block_patterns action (not there yet, tracked in #40305) that everyone hooks into, without caring where exactly in the WP lifecycle (admin_init, current_screen, rest_dispatch_request?) is the action executed. But that doesn't solve the back-compat issues.

To be able to still register patterns the old way, on the admin screen instead of in the REST endpoint, we could:

  1. On the server, when producing the editor page HTML, look at registered patterns and send them as serialized JSON as part of the HTML. We stopped doing that in __experimentalBlockPatterns: load them with REST API #39185, but it seem we'll have to reintroduce that.
  2. On the client, read block patterns from this JSON, and also load them from REST, and merge together.

In other words, instead of removing block pattern registration from the editor page HTML and moving it to REST completely, we would continue to support both methods.

@jsnajdr
Copy link
Member

jsnajdr commented May 4, 2022

A combination of WordPress/wordpress-develop#2672 (Core) and #40818 (Gutenberg) should fix this.

How to test:
On a WP box with both patches (patched Core and patched Gutenberg plugin), install also the Blockmeister plugin. In that plugin, create a block pattern (saved as regular post).

Then open a post editor for a post. The Blockmeister pattern should be visible in the Block Inserter UI. Before these patches, it wouldn't be there. You can also verify that the Blockmeister pattern is part of the initial HTML (__experimentalBlockPatterns array passed to wp.editPost.initializeEditor), but not of the /block-patterns/pattern response.

@priethor priethor removed the [Status] In Progress Tracking issues with work in progress label May 22, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Feature] Patterns A collection of blocks that can be synced (previously reusable blocks) or unsynced [Type] Bug An existing feature does not function as intended [Type] Regression Related to a regression in the latest release
Projects
No open projects
Archived in project
10 participants