Blocks: Refactor the check against self for block hooks

[gziolo]

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

I added some refactorings discussed in #5203 (review):

• Block Hooks: Add field to block registration, REST API #5203 (comment) - gutenberg text domain remove committed with https://core.trac.wordpress.org/changeset/56592.
• There is custom logic for mapping blockHooks to block_hooks when handling metadata. I covered that part in unit tests.
• One last bit of feedback was for the _doing_it_wrong logic. I moved the check to the WP_Block_Type class to cover also the case when someone registers the block with a regular register_block_type call without using metadata file.

---

This Pull Request is for code review only. Please keep all other discussion in the Trac ticket. Do not merge this Pull Request. See GitHub Pull Requests for Code Review in the Core Handbook for more details.

[ockham]

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

[ockham]

Sorry, I really think we should keep this in register_block_type_from_metadata 😬 AFAICS, no other field is being validated here in WP_Block_Type -- they're all processed in register_block_type_from_metadata, so I'd rather keep it consistent.

Note that register_block_type actually calls register_block_type_from_metadata for block registration, so any block type registered via register_block_type will in fact undergo the validation in register_block_type_from_metadata:

wordpress-develop/src/wp-includes/blocks.php

Lines 584 to 590 in e005108

	function register_block_type( $block_type, $args = array() ) {
	if ( is_string( $block_type ) && file_exists( $block_type ) ) {
	return register_block_type_from_metadata( $block_type, $args );
	}
	return WP_Block_Type_Registry::get_instance()->register( $block_type, $args );
	}

Finally, note that there's even an "inner" inconsistency with this PR, as validation of the allowed relative positions is still done in register_block_type_from_metadata 😅

[gziolo]

I left a #5218 (comment) with the details from my debugging after trying to use the test with the version before refactoring.

Finally, note that there's even an "inner" inconsistency with this PR, as validation of the allowed relative positions is still done in register_block_type_from_metadata 😅

Yes, that could get improved, but we first need to settle on the responsibilities of processing block.json files. So far, it is mostly about mapping the JSON object to the matching fields in WP_Block_Type.

AFAICS, no other field is being validated here in WP_Block_Type

There are some issues filed in Gutenberg to address it:

• Validate each arg before setting WP_Block_Type properties gutenberg#48039
• Blocks: Add a warning for developers if they use reserved words for attributes gutenberg#48112

[ockham]

👍

[gziolo]

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

---

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

wordpress-develop/src/wp-includes/blocks.php

Lines 618 to 624 in 227fdb8

	function register_block_type( $block_type, $args = array() ) {
	if ( is_string( $block_type ) && file_exists( $block_type ) ) {
	return register_block_type_from_metadata( $block_type, $args );
	}
	return WP_Block_Type_Registry::get_instance()->register( $block_type, $args );
	}

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. It's also what register_block_type_from_metadata calls after all transformations applied to the loaded block.json file:

wordpress-develop/src/wp-includes/blocks.php

Lines 594 to 597 in 227fdb8

	return WP_Block_Type_Registry::get_instance()->register(
	$metadata['name'],
	$settings
	);

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.

[ockham]

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

wordpress-develop/src/wp-includes/blocks.php

Lines 618 to 624 in 227fdb8

	function register_block_type( $block_type, $args = array() ) {
	if ( is_string( $block_type ) && file_exists( $block_type ) ) {
	return register_block_type_from_metadata( $block_type, $args );
	}
	return WP_Block_Type_Registry::get_instance()->register( $block_type, $args );
	}

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:

wordpress-develop/src/wp-includes/blocks.php

Lines 594 to 597 in 227fdb8

	return WP_Block_Type_Registry::get_instance()->register(
	$metadata['name'],
	$settings
	);

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.

[gziolo]

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

[ockham]

Thank you! LGTM

[gziolo]

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

[ockham]

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

[gziolo]

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: WordPress/gutenberg#48039.