Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Block Hooks: Fix in Navigation block #59021

Merged
merged 4 commits into from
Feb 19, 2024
Merged

Block Hooks: Fix in Navigation block #59021

merged 4 commits into from
Feb 19, 2024

Conversation

ockham
Copy link
Contributor

@ockham ockham commented Feb 14, 2024

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

  1. Add the below code to your themes functions.php
  2. Load frontend and check the Login/Logout block is an inner block of the core Navigation block and hasn't been added twice.
  3. 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.
  4. Make customisations to move the block and check it persists between reloads.
  5. Make customisations to remove the block completely and check it persists between reloads.
  6. 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 ockham added [Feature] Block API API that allows to express the block paradigm. [Block] Navigation Affects the Navigation Block Backport to Gutenberg RC Pull request that needs to be backported to a Gutenberg release candidate (RC) Backported to WP Core Pull request that has been successfully merged into WP Core labels Feb 14, 2024
@ockham ockham added the [Type] Bug An existing feature does not function as intended label Feb 14, 2024
@ockham
Copy link
Contributor Author

ockham commented Feb 14, 2024

FYI @gziolo

@@ -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' ) {
Copy link
Contributor Author

@ockham ockham Feb 14, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

@tjcafferkey tjcafferkey Feb 15, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@@ -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).
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done in 9da0b54

Copy link

github-actions bot commented Feb 15, 2024

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:

@tjcafferkey
Copy link
Contributor

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

Comment on lines +1376 to +1381
*/
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 );
}
Copy link
Contributor

@tjcafferkey tjcafferkey Feb 16, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

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

github-actions bot commented Feb 16, 2024

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 <bernhard-reiter@git.wordpress.org>
Co-authored-by: tjcafferkey <tomjcafferkey@git.wordpress.org>
Co-authored-by: gziolo <gziolo@git.wordpress.org>

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

@tjcafferkey tjcafferkey requested a review from gziolo February 16, 2024 10:16
@ockham
Copy link
Contributor Author

ockham commented Feb 19, 2024

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

@ockham ockham mentioned this pull request Feb 19, 2024
16 tasks
@ockham ockham force-pushed the fix/navigation-block-hooks branch from 894d356 to 6423090 Compare February 19, 2024 10:40
Copy link
Member

@gziolo gziolo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@ockham ockham merged commit 467aade into trunk Feb 19, 2024
56 of 57 checks passed
@ockham ockham deleted the fix/navigation-block-hooks branch February 19, 2024 14:01
@github-actions github-actions bot added this to the Gutenberg 17.8 milestone Feb 19, 2024
@ockham ockham added Backport to WP 6.7 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
@ockham
Copy link
Contributor Author

ockham commented Feb 19, 2024

I think we had the wrong label on this one 😅

getdave pushed a commit that referenced this pull request Feb 20, 2024
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 <bernhard-reiter@git.wordpress.org>
Co-authored-by: tjcafferkey <tomjcafferkey@git.wordpress.org>
Co-authored-by: gziolo <gziolo@git.wordpress.org>
@getdave
Copy link
Contributor

getdave commented Feb 20, 2024

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

@getdave getdave removed the Backport to WP 6.7 Beta/RC Pull request that needs to be backported to the WordPress major release that's currently in beta label Feb 20, 2024
youknowriad pushed a commit that referenced this pull request Feb 20, 2024
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 <bernhard-reiter@git.wordpress.org>
Co-authored-by: tjcafferkey <tomjcafferkey@git.wordpress.org>
Co-authored-by: gziolo <gziolo@git.wordpress.org>
@creativecoder
Copy link
Contributor

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.

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

This PR caused this issue: #59516

I don't understand why.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Block] Navigation Affects the Navigation Block [Feature] Block API API that allows to express the block paradigm. [Type] Bug An existing feature does not function as intended
Projects
No open projects
Status: Done
Development

Successfully merging this pull request may close these issues.

6 participants