General: Add `block_hooks` field to block types

[Bernhard Reiter]

In order to implement Block Hooks (see #59313), we need to add a new block_hooks field to the WP_Block_Type class, as well as to block registration related functions, the block type REST API controller, etc.

[Bernhard Reiter]

In 56587:

General: Add block_hooks field to block type registration, REST API.

In order to implement Block Hooks, we need to add a new block_hooks field to the WP_Block_Type class, as well as to block registration related functions, the block types REST API controller, etc.

Props gziolo.
Fixes #59346. See #59313.

[spacedmonkey]

@bernhard-reiter Why was this committed with outstanding feedback from the maintainer of the rest api?

[spacedmonkey]

I have left some feedback ​here.

[gziolo]

In 56588:

Tests: Improve the assertions for REST API endpoint for block types

Follow-up to [56587], [55673]. While working on #59346, it was noted that selectors fiels is not always included in the assertions. While looking at it is was difficult to spot the issue because the random order of how REST API fields where listed.

Reorderd the REST API fields in the test cases to follow the list from the documentation: ​https://github.com/WordPress/gutenberg/blob/trunk/docs/reference-guides/block-api/block-metadata.md. This way it's going to be easier to maintain the list moving forward.

Props ockham.
See #59346, #59313, #57585.

[Soean]

We should remove the text domain 'gutenberg'.

​https://github.com/WordPress/wordpress-develop/pull/5203/files#r1326102815

[gziolo]

@Soean, I noticed that, too. I'm refactoring the related code anyway. I should open a PR soon.

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

See my comment -- I really think we should keep the check for self-insertion in register_block_type_from_metadata.

I tried locally reverting the changes for the code moved to WP_Block_Type::set_props. I basically used the current trunk with the same unit test but modified @expectedIncorrectUsage line:

`diff
diff --git a/tests/phpunit/tests/blocks/register.php b/tests/phpunit/tests/blocks/register.php
index 55faf297a5..2c1cf64937 100644
--- a/tests/phpunit/tests/blocks/register.php
+++ b/tests/phpunit/tests/blocks/register.php
@@ -1069,4 +1069,29 @@ class Tests_Blocks_Register extends WP_UnitTestCase {

$actual = register_block_style( 'core/query', $block_styles );
$this->assertTrue( $actual );

}

+
+ /
+ * @ticket 59346
+ *
+ * @covers ::register_block_type
+ *
+ * @expectedIncorrectUsage register_block_type_from_metadata
+ */
+ public function test_register_block_hooks_targeting_itself() {
+ $block_name = 'tests/block-name';
+ $block_type = register_block_type(
+ $block_name,
+ array(
+ 'block_hooks' => array(
+ $block_name => 'first',
+ 'tests/other-block' => 'last',
+ ),
+ )
+ );
+
+ $this->assertSameSets(
+ array( 'tests/other-block' => 'last' ),
+ $block_type->block_hooks
+ );
+ }

}

The test fails:

No changes get applied.

[gziolo]

In 56592:

Blocks: Remove the gutenberg textdomain accidently added to translation

Props soean.
See #59346.
Follow-up [56587].

When inspecting the code it turns out that register_block_type_from_metadata isn't the universal way to register block type:

​https://github.com/WordPress/wordpress-develop/blob/227fdb8b741a71e300062d80e9471fef87a5ff11/src/wp-includes/blocks.php#L618-L624

If no path to the block.json file gets passed as the first param, then the registration goes directly through the WP_Block_Type_Registry.

You're of course right 🤦‍♂️ My apologies, I'd read that code but totally glossed over the if ( is_string( $block_type ) && file_exists( $block_type ) ) conditional.

It's also what register_block_type_from_metadata calls after all transformations applied to the loaded block.json file:

​https://github.com/WordPress/wordpress-develop/blob/227fdb8b741a71e300062d80e9471fef87a5ff11/src/wp-includes/blocks.php#L594-L597

We can keep the implementation as it is currently works in trunk and refactor the test to use some mocked block.json file. The drawback is that _doing_it_wrong check won't run in the case when someone opts into registering the block type using the syntax that has been present in WordPress core since version 5.0. The same applies to the case when someone uses the registry directly with WP_Block_Type_Registry::get_instance()->register(). Let me know which approach you prefer, given all the above.

I think since there's no precedent of doing any processing or validation in WP_Block_Type, I'd like to keep it that way, even if it means that people could bypass the check in register_block_type_from_metadata. In any case, I'd like to have a check that prevents infinite recursion by disallowing self-insertion later in insert_hooked_block (or whatever we're going to call it), regardless of how the block was registered.

Thanks for the feedback @ockham, I will refactor the code accordingly.

[gziolo]

In 56607:

Tests: Add additional tests covering Block Hooks registration

Props ockham.
See #59346.
Follow-up [56587].

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

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

The goal of this PR is to align the schema between block.json and the REST API endpoint for block types. It looks like the name field isn't validated in all places and if it uses pattern matching in the REST API code, then it's slightly different.

​https://github.com/WordPress/gutenberg/blob/aa938166f28bcfff6c7b8d1caac5a95f78852f97/schemas/json/block.json#L26

​https://github.com/WordPress/gutenberg/blob/f6d16f0b9f0a71d51c7178d35902c078b0d33c08/packages/blocks/src/api/registration.js#L231

I opted for using the same pattern as during block registration in JavaScript:

'^[a-z][a-z0-9-]*/[a-z][a-z0-9-]*$'

@gziolo Don't we need to regenerate this line -​https://github.com/WordPress/wordpress-develop/blob/trunk/tests/qunit/fixtures/wp-api-generated.js

@gziolo Don't we need to regenerate this line -​https://github.com/WordPress/wordpress-develop/blob/trunk/tests/qunit/fixtures/wp-api-generated.js

Do you mean we should also update the pattern used with URLs, like:

​https://github.com/WordPress/wordpress-develop/blob/e413569b67d86608485d59a0b90898207eeb5067/tests/qunit/fixtures/wp-api-generated.js#L9931

or

​https://github.com/WordPress/wordpress-develop/blob/e413569b67d86608485d59a0b90898207eeb5067/tests/qunit/fixtures/wp-api-generated.js#L9962

I executed all unit tests and this file hasn't changed. The comment included explains that the file is auto-generated with test_build_wp_api_client_fixtures.

[costdev]

In 56668:

REST API: Correct spelling error in block_hooks field documentation.

In [56587], a spelling error was introduced in the documentation of the new block_hooks field in WP_REST_Block_Types_Controller.

This fixes the spelling error.

Follow-up to [56587].

Props kebbet, mukesh27, tahmina1du.
Fixes #59426. See #59346.

Thank you! I'll have a closer look in a bit, but one thing I was wondering -- could we express the fact that the keys in blockHooks are block names (as defined earlier in the schema) even more semantically (rather than using the same RegEx as a constant)? I've seen there's something called $ref in the JSON schema spec -- think that could work here? 🤔

I've seen there's something called $ref in the JSON schema spec -- think that could work here? 🤔

There is a code comment included in the same method which suggests it can't be used:

​https://github.com/WordPress/wordpress-develop/blob/e00f028a676490178b06f985cabe3d8dd8c5adc9/src/wp-includes/rest-api/endpoints/class-wp-rest-block-types-controller.php#L397

[gziolo]

In 56676:

REST API: Improve the block type schema for the name field

Align the schema between block.json defined in Gutenberg and the REST API endpoint for block types. It looks like the name field isn't validated in all places and when it uses pattern matching in the REST API code, then it was slightly different.

Props spacedmonkey, ockham.
See #59346.

[gziolo]

@spacedmonkey, is there any follow-up necessary after committing [56676]? I want to clarify what's necessary to close this ticket.

Committed with 56676.

[spacedmonkey]

@gziolo Not that I can think of.

Just a short note: The method we landed on for ​inserting hooked blocks into their designated positions should make infinite loops of blocks inserting themselves next to each other impossible, if I'm not mistaken.

The crux is that we no longer insert parsed hooked blocks into the block tree, where they might be traversed themselves by traverse_and_serialized, and blocks that are hooked to _them_ would be inserted, potentially causing infinite recursion. Instead, we now only concatenate the hooked block's serialized markup during the traversal and serialization step to its anchor block's. That serialized markup is _not_ subjected to any further parsing or traversal that could cause recursive block insertion.

In other words, while it's nice to prohibit "self-insertion" at block type registration stage, it's not technically required -- things won't break if a block does end up registering itself for "self-insertion".

In other words, while it's nice to prohibit "self-insertion" at block type registration stage, it's not technically required -- things won't break if a block does end up registering itself for "self-insertion".

That's another excellent side-effect of the revised implementation. I really like how you integrated Block Hooks into WordPress core, taking advantage of the access to all internals. It seems fine to leave the validation as is. By the way, there is an open issue to add validation for every argument to WP_Block_Type: ​https://github.com/WordPress/gutenberg/issues/48039.