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

Navigation block: Check Block Hooks API callback hasn't already been added. #58772

Merged

Conversation

tjcafferkey
Copy link
Contributor

What?

We check for the presence of a filter callback hooked in Core (here and here) before adding ours in Gutenberg

Why?

This results in the same hooked block being inserted twice since the filter runs both in Core and in Gutenberg. So we need to check if Core has hooked into the filter before Gutenberg does.

How?

We use the has_filter() check before adding our callback to it. We have to concatenate Cores callback name to prevent the Gutenberg build step from prefixing it with gutenberg_ which is not what its called in Core.

Testing Instructions

Please ensure you have WordPress core trunk (or the latest nightly) running.

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

Testing Instructions for Keyboard

Screenshots or screencast

@tjcafferkey tjcafferkey added [Type] Bug An existing feature does not function as intended [Feature] Extensibility The ability to extend blocks or the editing experience [Block] Navigation Affects the Navigation Block Needs PHP backport Needs PHP backport to Core labels Feb 7, 2024
@tjcafferkey tjcafferkey requested a review from ockham February 7, 2024 11:33
@tjcafferkey tjcafferkey self-assigned this Feb 7, 2024
Copy link
Contributor

@ockham ockham left a comment

Choose a reason for hiding this comment

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

Thank you very much for the fix!
LGTM; with GB 17.7 RC1 just around the corner, let’s land this soon 👍

In the long run, we might want to look for a nicer solution (probably allowing for some exceptions in the mangling logic, or moving some of our functions out of the Nav block’s dynamic PHP and into lib/compat/wordpress-6.x/ (which doesn’t get prefixed)), but for now, LGTM!

Copy link

github-actions bot commented Feb 8, 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: tjcafferkey <tomjcafferkey@git.wordpress.org>
Co-authored-by: ockham <bernhard-reiter@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.

@gziolo
Copy link
Member

gziolo commented Feb 8, 2024

moving some of our functions out of the Nav block’s dynamic PHP and into lib/compat/wordpress-6.x/ (which doesn’t get prefixed)), but for now, LGTM!

That would be the preferred way to move it later to the lib/compat folder in Gutenberg and find a way to integrate the logic into WordPress core without filters.

@ockham
Copy link
Contributor

ockham commented Feb 8, 2024

moving some of our functions out of the Nav block’s dynamic PHP and into lib/compat/wordpress-6.x/ (which doesn’t get prefixed)), but for now, LGTM!

That would be the preferred way to move it later to the lib/compat folder in Gutenberg and find a way to integrate the logic into WordPress core without filters.

Yeah. The problem is that AFAICT, the Nav block has so far set a precedent of including pretty much all of its code inside its index.php (and a change to move code to lib/compat was later reverted). (That said, it's still worth considering for "our" filters and functions; but not something that should block WP 6.5 :)

@ockham
Copy link
Contributor

ockham commented Feb 8, 2024

I'll rebase to get tests to pass.

@ockham ockham force-pushed the fix/core-navigation-hooked-blocks-rest-api-filter-callback branch from 0c095e8 to 624a388 Compare February 8, 2024 16:57
@ockham ockham enabled auto-merge (squash) February 8, 2024 17:10
@ockham ockham merged commit 6f8a7dd into trunk Feb 8, 2024
56 checks passed
@ockham ockham deleted the fix/core-navigation-hooked-blocks-rest-api-filter-callback branch February 8, 2024 17:30
@github-actions github-actions bot added this to the Gutenberg 17.7 milestone Feb 8, 2024
@youknowriad youknowriad removed the Needs PHP backport Needs PHP backport to Core label Feb 9, 2024
@ockham ockham mentioned this pull request Feb 19, 2024
16 tasks
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] Extensibility The ability to extend blocks or the editing experience [Type] Bug An existing feature does not function as intended
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants