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: Extract insert_hooked_blocks() function #5609

Closed

Conversation

ockham
Copy link
Contributor

@ockham ockham commented Nov 1, 2023

Spun off from #5523.

TODO:

  • Add test coverage for insert_hooked_blocks().

Trac ticket: TBD


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 Nov 1, 2023
@ockham ockham changed the title Block Hooks: Extract insert_hooked_blocks() function Block Hooks: Extract insert_hooked_blocks() function Nov 1, 2023
src/wp-includes/blocks.php Outdated Show resolved Hide resolved
@ockham ockham force-pushed the update/extract-insert-hooked-blocks branch from 016e3a8 to a258a01 Compare November 14, 2023 12:44
Comment on lines 766 to 777
/**
* Filters the list of hooked block types for a given anchor block type and relative position.
*
* @since 6.4.0
*
* @param string[] $hooked_block_types The list of hooked block types.
* @param string $relative_position The relative position of the hooked blocks.
* Can be one of 'before', 'after', 'first_child', or 'last_child'.
* @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.

Might consider applying the filter outside of insert_hooked_blocks, to keep it ignorant of $context 🤔

Copy link
Member

Choose a reason for hiding this comment

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

You still need to document the filter in one place so it works with the documentation tool. What would be the benefit of having it repeated in all places where it has to be used?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

What would be the benefit of having it repeated in all places where it has to be used?

Oh, that aspect is definitely a tradeoff. The benefit would be separation of concerns; we would no longer need to pass $context to insert_hooked_blocks; I believe there's even a way to get rid of $relative_position and $anchor_block.

However, I'm now also leaning to keep the filter inside insert_hooked_blocks, even though I don't love the mix of concerns.

Another idea I had was to additionally extract an even finer-grained insert_hooked_block (singular), to have the best of both worlds. I might explore that separately.


Note that I've started considering not merging this PR separately, but rather do it all as part of #5523. I've noticed that the ideal shape of helper functions is strongly determined by the eventual desired use case, which in this case includes insertion into modified layouts. I've noticed that "massaging" the function signatures and implementation into the right shape might lead to somewhat different results when done with those changes present, versus when done without them 🤔

Copy link
Member

Choose a reason for hiding this comment

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

I see, so you were concerned about the helper function signature. The thing is that you need to run the filter before you can serialize the block(s) into markup, so the helper makes less sense if you further break it down. Anyway, it might be fine to land the same changes with the original PR if that makes things simpler for you. This PR is still useful as we can discuss the problem in isolation.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

While adding some test coverage for this (0b0f7cb), I became even more convinced that I didn't like insert_block_hooks() 😬

It's just weird: It takes a nested $hooked_blocks array, i.e. something like

$hooked_blocks = array(
	'tests/anchor1' => array(
		'after' => array( 'tests/hooked-before' ),
	),
	'tests/anchor2' => array(
		'last_child' => array( 'tests/hooked-last-child' ),
	)
);

plus an $anchor_block and a $relative_position arg (among others), but only really looks up the hooked blocks that are relevant for that anchor block and relative position; any information about blocks hooked to other anchor blocks and/or relative positions is ignored. OTOH, that's also the only thing that the $relative_position is used for.

So I'd actually rather continue to duplicate the filter calls 😬
Instead, I filed a PR using the insert_hooked_block approach discussed here; although I ended up calling the function differently: #5712.

Copy link
Member

Choose a reason for hiding this comment

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

It looks like your bad feelings about the excessive function's signature where spot on 👍🏻

@gziolo
Copy link
Member

gziolo commented Nov 15, 2023

Add test coverage for insert_hooked_blocks().

We also will need a proper PHPDoc which marks the function as private to mirror what we did with other utils. Otherwise, this PR is on track to landing soon.

@ockham
Copy link
Contributor Author

ockham commented Nov 30, 2023

Closing in favor or #5712.

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.

2 participants