Block Hooks: Fix in Navigation block #59021

ockham · 2024-02-14T15:44:54Z

What?

Update the Block Hooks mechanism as used in the Navigation block upon writing the REST API response received from a client to the DB.

Why?

The Block Hooks mechanism recently underwent some overhaul (in WordPress/wordpress-develop#6087), which decoupled hooked block injection from setting the ignoredHookedBlocks metadata. The latter is no longer done by insert_hooked_blocks but by a separate function called set_ignored_hooked_blocks_metadata (that can be passed as a callback to the make_{before|after}_block visitor factories).

Since the implementation of the Navigation block relied on the previous functionality, it needs to be updated to work properly.

How?

We need to run block tree traversal with set_ignored_hooked_blocks_metadata rather than insert_hooked_blocks on the template content received from the endpoint for the Navigation post type, upon persisting to the DB.

Testing Instructions

Verify that with current Core trunk, current versions of GB don't work properly w.r.t. hooked blocks insertion into the Navigation block (specifically when making changes to the Nav block).

Add the below code to your themes functions.php
Load frontend and check the Login/Logout block is an inner block of the core Navigation block and hasn't been added twice.
Load the Header template part in the Site Editor and check that the Login/Logout block is present as an inner block of the core Navigation and hasn't been added twice.
Make customisations to move the block and check it persists between reloads.
Make customisations to remove the block completely and check it persists between reloads.
Remove the PHP code added in step 1, and now add the below JSON to the same blocks block.json file and retest starting from step 2.

function register_logout_block_as_navigation_last_child( $hooked_blocks, $position, $anchor_block, $context ) {
	if ( $anchor_block === 'core/navigation' && $position === 'last_child' ) {
		$hooked_blocks[] = 'core/loginout';
	}

	return $hooked_blocks;
}

add_filter( 'hooked_block_types', 'register_logout_block_as_navigation_last_child', 10, 4 );

"blockHooks": {
	"core/navigation": "lastChild"
}

ockham · 2024-02-14T15:50:03Z

FYI @gziolo

ockham · 2024-02-14T15:53:14Z

packages/block-library/src/navigation/index.php

@@ -1383,7 +1383,7 @@ function block_core_navigation_get_most_recently_published_navigation() {
 * @param WP_Post $post `wp_navigation` post object corresponding to the block.
 * @return string Serialized inner blocks in mock Navigation block wrapper, with hooked blocks inserted, if any.
 */
-function block_core_navigation_insert_hooked_blocks( $inner_blocks, $post ) {
+function block_core_navigation_insert_hooked_blocks( $inner_blocks, $post, $callback = 'insert_hooked_blocks' ) {


The function name (block_core_navigation_insert_hooked_blocks) isn't quite accurate anymore since it can now either insert hooked blocks, or set the ignoredHookedBlocks metadata.

Furthermore, requiring the callback name as an arg here is also possibly exposing too many internals.

Not sure if we want two separate functions (maybe using the same underlying helper, just with different callbacks?), e.g. block_core_navigation_insert_hooked_blocks and block_core_navigation_set_ignored_hooked_blocks_metadata or so.

Done in 43a9276

There is still a small bit of code duplication by creating the visitor functions which in theory could be abstracted, but I think having it this way is more clear. Of course, open to opposing opinions on that.

ockham · 2024-02-14T15:54:51Z

packages/block-library/src/navigation/index.php

@@ -1441,6 +1440,10 @@ function block_core_navigation_update_ignore_hooked_blocks_meta( $post ) {
 }
 update_post_meta( $post->ID, '_wp_ignored_hooked_blocks', json_encode( $ignored_hooked_blocks ) );
 }
+
+ // TODO: wp_update_post() to set the post_content to the updated markup (to include the ignoredHookedBlocks metadata).


I.e. for anchor blocks other than the mock root Nav block -- in case a block hooks e.g. after core/page-list, we want the ignoredHookedBlocks metadata set on that anchor block, in the markup.

Come to think of it, we should ideally cover that case with a unit test somewhere 🤔

Done in 9da0b54

github-actions · 2024-02-15T15:22:43Z

Flaky tests detected in 894d356fc64b4c7b35a1e664067754c95e421399.
Some tests passed with failed attempts. The failures may not be related to this commit but are still reported for visibility. See the documentation for more information.

🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/7930437803
📝 Reported issues:

[Flaky Test] can add submenus #50976 in /test/e2e/specs/editor/blocks/navigation-list-view.spec.js
[Flaky Test] default to the only existing classic menu if there are no block menus #48033 in /test/e2e/specs/editor/blocks/navigation.spec.js
[Flaky Test] submenu converts to link automatically #58981 in /test/e2e/specs/editor/blocks/navigation.spec.js

tjcafferkey · 2024-02-15T16:11:38Z

@ockham I've also updated the feature gate to be set_ignored_hooked_blocks_metadata because this is a function that was recently introduced that we depend on. Let me know if you see a problem with this.

tjcafferkey · 2024-02-16T10:14:56Z

packages/block-library/src/navigation/index.php

+ */
+function block_core_navigation_remove_serialized_parent_block( $serialized_block ) {
+ $start = strpos( $serialized_block, '-->' ) + strlen( '-->' );
+ $end = strrpos( $serialized_block, '<!--' );
+ return substr( $serialized_block, $start, $end - $start );
+}


We were already doing this logic here. I've just moved it into its own function to reuse.

github-actions · 2024-02-16T10:15:41Z

The following accounts have interacted with this PR and/or linked issues. I will continue to update these lists as activity occurs. You can also manually ask me to refresh this list by adding the props-bot label.

If you're merging code through a pull request on GitHub, copy and paste the following into the bottom of the merge commit message.

Co-authored-by: ockham <[email protected]>
Co-authored-by: tjcafferkey <[email protected]>
Co-authored-by: gziolo <[email protected]>

To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook.

ockham · 2024-02-19T10:12:44Z

Thank you for your work @tjcafferkey!

My plan is to land this today 🙂

@youknowriad @getdave It'd be great to get this into WP 6.5 Beta 2. What's your ETA for the package sync? 😊

…s meta for clarity and readability

gziolo

Some creative work there to account for the fact that the content of the Navigation block doesn't contain the parent block wrapper 👍🏻

ockham · 2024-02-19T14:22:40Z

I think we had the wrong label on this one 😅

Update the Block Hooks mechanism as used in the Navigation block upon writing the REST API response received from a client to the DB. The Block Hooks mechanism recently underwent some overhaul (in WordPress/wordpress-develop#6087), which decoupled hooked block injection from setting the `ignoredHookedBlocks` metadata. The latter is no longer done by `insert_hooked_blocks` but by a separate function called `set_ignored_hooked_blocks_metadata` (that can be passed as a callback to the `make_{before|after}_block` visitor factories). Since the implementation of the Navigation block relied on the previous functionality, it needs to be updated to work properly: We need to run block tree traversal with `set_ignored_hooked_blocks_metadata` rather than `insert_hooked_blocks` on the template content received from the endpoint for the Navigation post type, upon persisting to the DB. Co-authored-by: ockham <[email protected]> Co-authored-by: tjcafferkey <[email protected]> Co-authored-by: gziolo <[email protected]>

getdave · 2024-02-20T09:31:34Z

I just cherry-picked this PR to the cherry-pick-beta-2 branch to get it included in the next release: 302d96f

Update the Block Hooks mechanism as used in the Navigation block upon writing the REST API response received from a client to the DB. The Block Hooks mechanism recently underwent some overhaul (in WordPress/wordpress-develop#6087), which decoupled hooked block injection from setting the `ignoredHookedBlocks` metadata. The latter is no longer done by `insert_hooked_blocks` but by a separate function called `set_ignored_hooked_blocks_metadata` (that can be passed as a callback to the `make_{before|after}_block` visitor factories). Since the implementation of the Navigation block relied on the previous functionality, it needs to be updated to work properly: We need to run block tree traversal with `set_ignored_hooked_blocks_metadata` rather than `insert_hooked_blocks` on the template content received from the endpoint for the Navigation post type, upon persisting to the DB. Co-authored-by: ockham <[email protected]> Co-authored-by: tjcafferkey <[email protected]> Co-authored-by: gziolo <[email protected]>

creativecoder · 2024-02-23T23:57:06Z

Note that this revert PR did not make it into the 17.7 release. However, it is in 17.8 RC1 and will be include in the 17.8 release next week.

scruffian · 2024-03-04T12:56:43Z

This PR caused this issue: #59516

I don't understand why.

ockham assigned tjcafferkey Feb 14, 2024

ockham added the [Type] Bug An existing feature does not function as intended label Feb 14, 2024

ockham commented Feb 14, 2024

View reviewed changes

tjcafferkey reviewed Feb 16, 2024

View reviewed changes

tjcafferkey marked this pull request as ready for review February 16, 2024 10:15

tjcafferkey requested a review from ajitbohra as a code owner February 16, 2024 10:15

tjcafferkey requested a review from gziolo February 16, 2024 10:16

ockham mentioned this pull request Feb 19, 2024

Epic: Block Hooks Features for WP 6.5 #54904

Closed

16 tasks

ockham and others added 4 commits February 19, 2024 11:40

Block Hooks: Fix in Navigation block

0e88fe4

Update wp_navigation post with ignoredHookedBlocks meta and feature gate

faf4448

Remove comment

0dcb9c0

Separate functions to insert hooked blocks and set ignoredHookedBlock…

6423090

…s meta for clarity and readability

ockham force-pushed the fix/navigation-block-hooks branch from 894d356 to 6423090 Compare February 19, 2024 10:40

gziolo approved these changes Feb 19, 2024

View reviewed changes

ockham merged commit 467aade into trunk Feb 19, 2024
56 of 57 checks passed

ockham deleted the fix/navigation-block-hooks branch February 19, 2024 14:01

github-actions bot added this to the Gutenberg 17.8 milestone Feb 19, 2024

ockham mentioned this pull request Feb 19, 2024

Navigation Block: Polish Block Hooks integration #59177

Draft

ockham added Backport to WP 6.6 Beta/RC Pull request that needs to be backported to the WordPress major release that's currently in beta and removed Backported to WP Core Pull request that has been successfully merged into WP Core labels Feb 19, 2024

getdave removed the Backport to WP 6.6 Beta/RC Pull request that needs to be backported to the WordPress major release that's currently in beta label Feb 20, 2024

getdave mentioned this pull request Feb 20, 2024

Cherry pick commits for WordPress 6.5. Beta 2 #59197

Merged

creativecoder removed the Backport to Gutenberg RC Pull request that needs to be backported to a Gutenberg release candidate (RC) label Feb 23, 2024

This was referenced Mar 4, 2024

Ampersand's in the Navigation block are converted to u0026 #59516

Closed

Navigation Block: Fix erroneous escaping of ampersands (etc) #59561

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Block Hooks: Fix in Navigation block #59021

Block Hooks: Fix in Navigation block #59021

ockham commented Feb 14, 2024 •

edited

Loading

ockham commented Feb 14, 2024

ockham Feb 14, 2024 •

edited

Loading

tjcafferkey Feb 15, 2024 •

edited

Loading

ockham Feb 14, 2024

ockham Feb 14, 2024

tjcafferkey Feb 15, 2024

github-actions bot commented Feb 15, 2024 •

edited

Loading

tjcafferkey commented Feb 15, 2024

tjcafferkey Feb 16, 2024 •

edited

Loading

github-actions bot commented Feb 16, 2024 •

edited

Loading

ockham commented Feb 19, 2024

gziolo left a comment

ockham commented Feb 19, 2024

getdave commented Feb 20, 2024

creativecoder commented Feb 23, 2024

scruffian commented Mar 4, 2024

Block Hooks: Fix in Navigation block #59021

Block Hooks: Fix in Navigation block #59021

Conversation

ockham commented Feb 14, 2024 • edited Loading

What?

Why?

How?

Testing Instructions

ockham commented Feb 14, 2024

ockham Feb 14, 2024 • edited Loading

Choose a reason for hiding this comment

tjcafferkey Feb 15, 2024 • edited Loading

Choose a reason for hiding this comment

ockham Feb 14, 2024

Choose a reason for hiding this comment

ockham Feb 14, 2024

Choose a reason for hiding this comment

tjcafferkey Feb 15, 2024

Choose a reason for hiding this comment

github-actions bot commented Feb 15, 2024 • edited Loading

tjcafferkey commented Feb 15, 2024

tjcafferkey Feb 16, 2024 • edited Loading

Choose a reason for hiding this comment

github-actions bot commented Feb 16, 2024 • edited Loading

ockham commented Feb 19, 2024

gziolo left a comment

Choose a reason for hiding this comment

ockham commented Feb 19, 2024

getdave commented Feb 20, 2024

creativecoder commented Feb 23, 2024

scruffian commented Mar 4, 2024

ockham commented Feb 14, 2024 •

edited

Loading

ockham Feb 14, 2024 •

edited

Loading

tjcafferkey Feb 15, 2024 •

edited

Loading

github-actions bot commented Feb 15, 2024 •

edited

Loading

tjcafferkey Feb 16, 2024 •

edited

Loading

github-actions bot commented Feb 16, 2024 •

edited

Loading