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

Blocks: Introduce filter to allow easy addition of hooked blocks #5271

Closed
wants to merge 1 commit into from

Conversation

ockham
Copy link
Contributor

@ockham ockham commented Sep 21, 2023

Introduce a hooked_block_types filter that allows even easier conditional addition (or removal) of hooked blocks for a given anchor block and relative position.

function insert_shopping_cart_hooked_block( $hooked_blocks, $position, $anchor_block, $context ) {
	if ( 'after' === $position && 'core/navigation' === $anchor_block && /** $context is header template part **/ ) {
		$hooked_blocks[] = 'mycommerce/shopping-cart';
	}
	return $hooked_blocks;
}
add_filter( 'hooked_block_types', 'insert_shopping_cart_hooked_block', 10, 4 );

Based on an idea by @gziolo that we discussed during a call earlier today.

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


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 ockham self-assigned this Sep 21, 2023
Comment on lines +834 to +846
/**
* Filters the serialized markup of a hooked block.
*
* @since 6.4.0
*
* @param string $hooked_block_markup The serialized markup of the hooked block.
* @param string $hooked_block_type The type of the hooked block.
* @param string $relative_position The relative position of the hooked block.
* Can be one of 'before', 'after', 'first_child', or 'last_child'.
* @param array $block The anchor block.
* @param WP_Block_Template|array $context The block template, template part, or pattern that the anchor block belongs to.
*/
$markup .= apply_filters( 'inject_hooked_block_markup', $hooked_block_markup, $hooked_block_type, $relative_position, $block, $context );
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@gziolo We might consider removing the inject_hooked_block_markup filter in a follow-up; it might be a bit low-level (and less useful?) after all.

Copy link
Member

Choose a reason for hiding this comment

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

I need to think a bit more about it, but at first glance, it looks like they are mostly the same.

Copy link
Member

Choose a reason for hiding this comment

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

inject_hooked_block_markup operates on the markup level, so it basically allows setting any string as the return value. The difference in the params available in the filter is that hooked_block_types uses the block name as the anchor, in contrast to the block type object that inject_hooked_block_markup offers for the parent or sibling block. I would be in favor of keeping only hooked_block_types which is a nice abstraction as it's just a list of block names.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sounds good. We can either land this PR and remove inject_hooked_block_markup in a follow-up, or do it as part of this PR.

@ockham ockham requested a review from gziolo September 21, 2023 16:58
@ockham
Copy link
Contributor Author

ockham commented Sep 21, 2023

FYI @nerrad @ndiego

* @param string $anchor_block_type The anchor block type.
* @param WP_Block_Template|array $context The block template, template part, or pattern that the anchor block belongs to.
*/
$hooked_block_types = apply_filters( 'hooked_block_types', $hooked_block_types, $relative_position, $anchor_block_type, $context );
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Should we call the filter just hooked_blocks?

Copy link
Member

Choose a reason for hiding this comment

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

I like hooked_block_types personally, but defer to you all. It makes it clear to me that you are filtering the "block types" and not the entire block markup or something else.

Copy link

Choose a reason for hiding this comment

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

Agree with Nick here although it does make me wonder how consistent the codebase is in differentiating between "blocks" and "block types".

Copy link
Member

Choose a reason for hiding this comment

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

how consistent the codebase is in differentiating between "blocks" and "block types".

😬 I support any effort to draw those distinctions where they haven't already been blurred.

Copy link
Member

Choose a reason for hiding this comment

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

hooked_block_types sounds good 👍🏻

*/
$markup .= apply_filters( 'inject_hooked_block_markup', $hooked_block_markup, $hooked_block_type, $relative_position, $block, $context );
}
$relative_position = 'before';
Copy link

Choose a reason for hiding this comment

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

Minor, but I wonder if the various $relative_position values should be constants (something like WP_Block_Hooks::RELATIVE_POSITION_BEFORE, WP_Block_Hooks::RELATIVE_POSITION_FIRST_CHILD ...etc). This could reduce some variable declaration and make it more straightforward/clear for extension use.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah yeah, good point. I think it hadn't occurred to me since we need to use the position values verbatim in block.json -- but it might make sense to add constants in the code 👍

Copy link
Member

Choose a reason for hiding this comment

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

did we update the block.json schema so that these values can be drawn from an auto-completed list with inline documentation?

Copy link
Member

Choose a reason for hiding this comment

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

Copy link
Member

Choose a reason for hiding this comment

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

Speaking of schema, I opened #5278 to align the schemas between Gutenberg and WordPress core.

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.

Excellent idea with the new better scoped filters hooked_block_types. I would be in favor of removing inject_hooked_block_markup filters as they are low-level and offer too much power in contrast to the new proposal. We can always add them back later if it turns out they are essential for more advanced usage that I can't think of at the moment.

@ockham
Copy link
Contributor Author

ockham commented Sep 25, 2023

Excellent idea with the new better scoped filters hooked_block_types. I would be in favor of removing inject_hooked_block_markup filters as they are low-level and offer too much power in contrast to the new proposal. We can always add them back later if it turns out they are essential for more advanced usage that I can't think of at the moment.

Great, thank you! I agree 100% -- YAGNI and all 😉
I'll land this change, and will then do a follow-up to remove inject_hooked_block_markup.

Introduce a `hooked_block_types` filter that allows easier conditional addition (or removal) of hooked blocks for a given anchor block and relative position.

{{{#!php
function insert_shopping_cart_hooked_block( $hooked_blocks, $position, $anchor_block, $context ) {
	if ( 'after' === $position && 'core/navigation' === $anchor_block && /** $context is header template part **/ ) {
		$hooked_blocks[] = 'mycommerce/shopping-cart';
	}
	return $hooked_blocks;
}
add_filter( 'hooked_block_types', 'insert_shopping_cart_hooked_block', 10, 4 );
}}}

Props gziolo, nerrad, dmsnell, ndiego.
Fixes #59424.
@ockham ockham force-pushed the add/hooked-block-filter branch from ac7f792 to 373d176 Compare September 25, 2023 08:41
@ockham
Copy link
Contributor Author

ockham commented Sep 25, 2023

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants