Block Hooks: Add field to block registration, REST API #5203

ockham · 2023-09-13T15:34:18Z

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.

Based on #5158.

See #3942 for precedent of adding a new field like this.

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

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 · 2023-09-13T15:47:09Z

src/wp-includes/rest-api/endpoints/class-wp-rest-block-types-controller.php

+				'block_hooks'           => array(
+					'description'       => __( 'This block is automatically inserted near any occurence of the block types used as keys of this map, into a relative position given by the corresponding value.' ),
+					'type'              => 'object',
+					'patternProperties' => array(
+						'^[a-zA-Z0-9-]+/[a-zA-Z0-9-]+$' => array(
+							'type' => 'string',
+							'enum' => array( 'before', 'after', 'first_child', 'last_child' ),
+						),
+					),
+					'default'           => array(),
+					'context'           => array( 'embed', 'view', 'edit' ),
+					'readonly'          => true,
+				),


I've updated this based on https://github.com/WordPress/gutenberg/blob/8c5dbabe5361e94f18e4a55070db9a54d3c59d0b/lib/compat/wordpress-6.4/block-hooks.php#L341-L351. Hope that makes sense!

I don't think that is correct and should changed in gutenberg. This pattern is not needed and make it extremely hard to read.

See my other comment 😄

spacedmonkey · 2023-09-13T15:49:54Z

src/wp-includes/rest-api/endpoints/class-wp-rest-block-types-controller.php

@@ -707,15 +707,17 @@ public function get_item_schema() {
 				'keywords'              => $keywords_definition,
 				'example'               => $example_definition,
 				'block_hooks'           => array(
-					'description' => __( 'Block hooks.' ),
-					'type'        => 'object',
-					'properties'  => array(),


Can't we just use properties. Like this

wordpress-develop/src/wp-includes/rest-api/endpoints/class-wp-rest-widgets-controller.php

Lines 836 to 858 in 6f74f61

'instance' => array(

'description' => __( 'Instance settings of the widget, if supported.' ),

'type' => 'object',

'context' => array( 'edit' ),

'default' => null,

'properties' => array(

'encoded' => array(

'description' => __( 'Base64 encoded representation of the instance settings.' ),

'type' => 'string',

'context' => array( 'edit' ),

),

'hash' => array(

'description' => __( 'Cryptographic hash of the instance settings.' ),

'type' => 'string',

'context' => array( 'edit' ),

),

'raw' => array(

'description' => __( 'Unencoded instance settings, if supported.' ),

'type' => 'object',

'context' => array( 'edit' ),

),

),

),

I don't think so, but maybe I'm mistaken? 🤔

The kind of shape we support is

"blockHooks": { "core/comment-template": "lastChild", "core/postContent": "after", }

So the keys can be pretty much any block type, whereas the values can only be one of before, after, firstChild, and lastChild.

It was my understanding that properties is used to define an object with a set of fixed keys -- which we don't really have here. I thus chose patternProperties to allow any block type -- i.e. any alphanumeric string -- as key, while limiting the values to the aforementioned set of possible relative positions.

The way the schema is defined for block.json is at https://github.com/WordPress/gutenberg/blob/40c9f17199b067c75d58f1d918fc26c31b08897f/schemas/json/block.json#L709-L718:

"blockHooks": { "description": "Block Hooks allow a block to automatically insert itself next to all instances of a given block type.\n\nSee the Block Hooks documentation at https://developer.wordpress.org/block-editor/reference-guides/block-api/block-registration/#block-hooks-optional for more details.", "type": "object", "patternProperties": { "^[a-z][a-z0-9-]*/[a-z][a-z0-9-]*$": { "enum": [ "before", "after", "firstChild", "lastChild" ] } }, "additionalProperties": false },

I see now the comment from @ockham #5203 (comment) that uses:

array( 'description' => __( 'This block is automatically inserted near any occurence of the block types used as keys of this map, into a relative position given by the corresponding value.', 'gutenberg' ), 'patternProperties' => array( '^[a-zA-Z0-9-]+/[a-zA-Z0-9-]+$' => array( 'type' => 'string', 'enum' => array( 'before', 'after', 'first_child', 'last_child' ), ), ), )

It's very close. We should probably unify some aspects. As far as I follow the spec, when using enum, you don't need to define the type, but maybe it's required here.

Right:

You can use enum even without a type, to accept values of different types. Let’s extend the example to use null to indicate “off”, and also add 42, just for fun.

{ "enum": ["red", "amber", "green", null, 42] }

So I don't think the type is required but at least permissible.

As for the description, I think it's okay if they're different: After all, in the REST API, we kind of inform the consumer what the field is about:

This block is automatically inserted near any occurence of the block types used as keys of this map, into a relative position given by the corresponding value.

...whereas in the block.json schema, we inform people who write their own block what the block_hooks field is used for:

Block Hooks allow a block to automatically insert itself next to all instances of a given block type.\n\nSee the Block Hooks documentation at https://developer.wordpress.org/block-editor/reference-guides/block-api/block-registration/#block-hooks-optional for more details.

I think what we have now kind of covers each target audience just fine 👍

Yes, the description can differ, but for example, pattern matching for the block name should always be the same.

By the way, it should be as simple as adding pattern to the name field:

https://json-schema.org/understanding-json-schema/reference/regular_expressions.html#example

ockham · 2023-09-14T08:52:36Z

Rebased.

tests/phpunit/tests/rest-api/rest-block-type-controller.php

gziolo

That should cover adding the block_hooks field to WP_Block_Type and expose it in the REST API endpoint for block types.

Let's continue discussing the shape for the schema after landing initial changes. It's a more complex case as pointed out by @ockham, because the key is the block name that can be anything that matches the pattern for the block name. If that helps we could include the pattern also for the name field:

wordpress-develop/src/wp-includes/rest-api/endpoints/class-wp-rest-block-types-controller.php

Lines 478 to 484 in 72de120

    
           'name'                  => array( 
        
           	'description' => __( 'Unique name identifying the block type.' ), 
        
           	'type'        => 'string', 
        
           	'default'     => '', 
        
           	'context'     => array( 'embed', 'view', 'edit' ), 
        
           	'readonly'    => true, 
        
           ),

I see there is custom logic for mapping blockHooks to block_hooks. I'd like to work on a follow-up to cover that part with unit tests. before case is covered indirectly by other values still could use some testing.

One last bit of feedback is for the _doing_it_wrong logic. I like that there is additional security enforced. I'm wondering if we should move it to the WP_Block_Type class though, to cover also the case when someones registers the block with:

register_block_type( 'my-plugin/my-block', array(
    'block_hooks' => array(
        'my-plugin/my-block': 'first_child',
    ),
) );

It also reminds me that in general there isn't that much sanitization present on the code for settings provided during block registration which I find a bit unfortunate.

There is nothing blocking this PR, and we can iterate on the items I listed if they make sense.

ockham · 2023-09-14T13:04:30Z

Thank you @gziolo!

[...] the key is the block name that can be anything that matches the pattern for the block name. If that helps we could include the pattern also for the name field:

wordpress-develop/src/wp-includes/rest-api/endpoints/class-wp-rest-block-types-controller.php

Lines 478 to 484 in 72de120

'name' => array(

'description' => __( 'Unique name identifying the block type.' ),

'type' => 'string',

'default' => '',

'context' => array( 'embed', 'view', 'edit' ),

'readonly' => true,

),

That reminded me of something else; maybe we can cross-reference the name field in the block_hooks schema definition? Would make it more semantic -- we'd basically state, "The keys in block_hooks are block names.".

I see there is custom logic for mapping blockHooks to block_hooks. I'd like to work on a follow-up to cover that part with unit tests. before case is covered indirectly by other values still could use some testing.

Ah yeah, that's a fair point; especially for the camelCase -> snake_case mappings.

One last bit of feedback is for the _doing_it_wrong logic. I like that there is additional security enforced. I'm wondering if we should move it to the WP_Block_Type class though, to cover also the case when someones registers the block with:
register_block_type( 'my-plugin/my-block', array(
    'block_hooks' => array(
        'my-plugin/my-block': 'first_child',
    ),
) );

FWIW, register_block_type actually calls register_block_type_from_metadata for block registration, so your example should already be covered:

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 ); 
        
           }

(We might want to add some unit test for this too, BTW 🤔 )

It also reminds me that in genera; there isn't that much sanitization present on the code for settings provided during block registration which I find a bit unforunate.

Yeah, good point!

ockham · 2023-09-14T13:23:55Z

Committed to Core in https://core.trac.wordpress.org/changeset/56587.

spacedmonkey · 2023-09-14T13:30:17Z

src/wp-includes/rest-api/endpoints/class-wp-rest-block-types-controller.php

+					'description'       => __( 'This block is automatically inserted near any occurence of the block types used as keys of this map, into a relative position given by the corresponding value.' ),
+					'type'              => 'object',
+					'patternProperties' => array(
+						'^[a-zA-Z0-9-]+/[a-zA-Z0-9-]+$' => array(


@ockham How something like this.

$properties = array(); $registry = WP_Block_Type_Registry::get_instance(); foreach( $registry->get_all_registered() as $block ){ $properties[ $block ] = array( 'type' => 'string', 'enum' => array( 'before', 'after', 'first_child', 'last_child' ), ); }

Noting that there might be hundreds of registered blocks, but the block author might still list the block type name that isn't present on the site and we would consider it valid. What's the reasoning behind listing all available blocks?

src/wp-includes/blocks.php

gziolo · 2023-09-14T15:40:19Z

In #5218 I added some refactorings discussed above:

Block Hooks: Add field to block registration, REST API #5203 (comment) - gutenberg text domain remove
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.

ockham assigned ockham and gziolo Sep 13, 2023

ockham commented Sep 13, 2023

View reviewed changes

spacedmonkey reviewed Sep 13, 2023

View reviewed changes

ockham added 6 commits September 14, 2023 10:52

Add block_hooks property

3186d24

Map camelCased position string to snake_case

27fcdc5

Move block_hooks field under provides_context

42166c8

Fix PHPDoc types

99ab6c7

Add block_hooks to block types REST API controller

67135e3

Update REST API field schema

49bf14e

ockham force-pushed the add/block-hooks-field branch from 1b56611 to 49bf14e Compare September 14, 2023 08:52

Fix (and extend) block type controller test coverage

ad879ca

ockham commented Sep 14, 2023

View reviewed changes

tests/phpunit/tests/rest-api/rest-block-type-controller.php Show resolved Hide resolved

ockham added 4 commits September 14, 2023 12:02

Add to get_block_editor_server_block_settings()

8fd46e9

Add @SInCE PHPDoc to register_block_type_from_metadata

e71e581

Verify that block type contains block hooks from metadata

6de2dc3

Add test coverage for get_block_editor_server_block_settings

f3a04cb

ockham marked this pull request as ready for review September 14, 2023 10:22

Fix unit test

72de120

gziolo approved these changes Sep 14, 2023

View reviewed changes

ockham closed this Sep 14, 2023

ockham deleted the add/block-hooks-field branch September 14, 2023 13:24

spacedmonkey reviewed Sep 14, 2023

View reviewed changes

Soean reviewed Sep 14, 2023

View reviewed changes

src/wp-includes/blocks.php Show resolved Hide resolved

gziolo mentioned this pull request Sep 14, 2023

Blocks: Refactor the check against self for block hooks #5218

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Block Hooks: Add field to block registration, REST API #5203

Block Hooks: Add field to block registration, REST API #5203

ockham commented Sep 13, 2023 •

edited

Loading

ockham Sep 13, 2023

spacedmonkey Sep 14, 2023

ockham Sep 14, 2023

spacedmonkey Sep 13, 2023

ockham Sep 14, 2023

gziolo Sep 14, 2023

gziolo Sep 14, 2023

ockham Sep 14, 2023

ockham Sep 14, 2023

gziolo Sep 14, 2023 •

edited

Loading

ockham commented Sep 14, 2023

gziolo left a comment •

edited

Loading

ockham commented Sep 14, 2023

ockham commented Sep 14, 2023

spacedmonkey Sep 14, 2023

gziolo Sep 14, 2023

gziolo commented Sep 14, 2023

	'instance' => array(
	'description' => __( 'Instance settings of the widget, if supported.' ),
	'type' => 'object',
	'context' => array( 'edit' ),
	'default' => null,
	'properties' => array(
	'encoded' => array(
	'description' => __( 'Base64 encoded representation of the instance settings.' ),
	'type' => 'string',
	'context' => array( 'edit' ),
	),
	'hash' => array(
	'description' => __( 'Cryptographic hash of the instance settings.' ),
	'type' => 'string',
	'context' => array( 'edit' ),
	),
	'raw' => array(
	'description' => __( 'Unencoded instance settings, if supported.' ),
	'type' => 'object',
	'context' => array( 'edit' ),
	),
	),
	),

	'name' => array(
	'description' => __( 'Unique name identifying the block type.' ),
	'type' => 'string',
	'default' => '',
	'context' => array( 'embed', 'view', 'edit' ),
	'readonly' => true,
	),

Block Hooks: Add field to block registration, REST API #5203

Block Hooks: Add field to block registration, REST API #5203

Conversation

ockham commented Sep 13, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gziolo Sep 14, 2023 • edited Loading

Choose a reason for hiding this comment

ockham commented Sep 14, 2023

gziolo left a comment • edited Loading

Choose a reason for hiding this comment

ockham commented Sep 14, 2023

ockham commented Sep 14, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gziolo commented Sep 14, 2023

ockham commented Sep 13, 2023 •

edited

Loading

gziolo Sep 14, 2023 •

edited

Loading

gziolo left a comment •

edited

Loading