Make WordPress Core

Opened 9 months ago

Closed 9 months ago

Last modified 9 months ago

#59346 closed enhancement (fixed)

General: Add `block_hooks` field to block types

Reported by: bernhard-reiter's profile Bernhard Reiter Owned by: bernhard-reiter's profile Bernhard Reiter
Milestone: 6.4 Priority: normal
Severity: normal Version:
Component: General Keywords: commit has-patch has-unit-tests
Focuses: Cc:

Description

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.

Change History (27)

#1 @Bernhard Reiter
9 months ago

  • Keywords commit added

#2 @Bernhard Reiter
9 months ago

  • Resolution set to fixed
  • Status changed from assigned to closed

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.

#3 @spacedmonkey
9 months ago

  • Resolution fixed deleted
  • Status changed from closed to reopened

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

#4 @spacedmonkey
9 months ago

I have left some feedback here.

#5 @gziolo
9 months ago

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.

#7 @gziolo
9 months ago

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

This ticket was mentioned in PR #5218 on WordPress/wordpress-develop by @gziolo.


9 months ago
#8

  • Keywords has-patch has-unit-tests added

@gziolo commented on PR #5218:


9 months ago
#9

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:

https://i0.wp.com/github.com/WordPress/wordpress-develop/assets/699132/6e73690f-5cf8-4edc-8cf1-6c535a69297c

No changes get applied.

#10 @gziolo
9 months ago

In 56592:

Blocks: Remove the gutenberg textdomain accidently added to translation

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

@Bernhard Reiter commented on PR #5218:


9 months ago
#11

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.

@gziolo commented on PR #5218:


9 months ago
#12

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

#13 @gziolo
9 months ago

In 56607:

Tests: Add additional tests covering Block Hooks registration

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

This ticket was mentioned in PR #5278 on WordPress/wordpress-develop by @gziolo.


9 months ago
#15

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 commented on PR #5278:


9 months ago
#17

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

#18 @costdev
9 months ago

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.

@Bernhard Reiter commented on PR #5278:


9 months ago
#19

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? 🤔

@gziolo commented on PR #5278:


9 months ago
#20

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

#21 @gziolo
9 months ago

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.

#22 @gziolo
9 months ago

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

@gziolo commented on PR #5278:


9 months ago
#23

Committed with 56676.

#24 @spacedmonkey
9 months ago

@gziolo Not that I can think of.

#25 @gziolo
9 months ago

  • Resolution set to fixed
  • Status changed from reopened to closed

@Bernhard Reiter commented on PR #5218:


9 months ago
#26

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 commented on PR #5218:


9 months ago
#27

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.

Note: See TracTickets for help on using tickets.