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 API: Add hooked_blocks filter to allow hooked block extensibility #6228

Draft
wants to merge 1 commit into
base: trunk
Choose a base branch
from

Conversation

joshuatf
Copy link

@joshuatf joshuatf commented Mar 5, 2024

This PR is a POC that introduces a new hook hooked_blocks that allows insertion of an entire hooked block, including its attributes. This is beneficial for inserting generic blocks into templates that are only differentiated by their attributes. It should be fully backwards compatible existing block hook filters. cc @ockham

To avoid the issue with ignoredHookedBlocks blocking generic blocks from being added of similar types, it also adds a hooked_block_signature to help discern hooked blocks.

  • Block signature without attributes: woocommerce/text-field
  • Block signature with attributes: woocommerce/text-field-{"label":"Name"}

Note that it's possible this should be a separate API from block hooks, but it seems the core editor could also benefit from the ability to use generic blocks.

Trac ticket: https://core.trac.wordpress.org/ticket/60696#ticket


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.

Copy link

github-actions bot commented Mar 5, 2024

Hi @joshuatf! 👋

Thank you for your contribution to WordPress! 💖

It looks like this is your first pull request to wordpress-develop. Here are a few things to be aware of that may help you out!

No one monitors this repository for new pull requests. Pull requests must be attached to a Trac ticket to be considered for inclusion in WordPress Core. To attach a pull request to a Trac ticket, please include the ticket's full URL in your pull request description.

Pull requests are never merged on GitHub. The WordPress codebase continues to be managed through the SVN repository that this GitHub repository mirrors. Please feel free to open pull requests to work on any contribution you are making.

More information about how GitHub pull requests can be used to contribute to WordPress can be found in this blog post.

Please include automated tests. Including tests in your pull request is one way to help your patch be considered faster. To learn about WordPress' test suites, visit the Automated Testing page in the handbook.

If you have not had a chance, please review the Contribute with Code page in the WordPress Core Handbook.

The Developer Hub also documents the various coding standards that are followed:

Thank you,
The WordPress Project

Comment on lines +982 to +984
/** This filter is documented in wp-includes/blocks.php */
$hooked_blocks = apply_filters( 'hooked_blocks', $hooked_blocks, $relative_position, $parsed_anchor_block, $context );
$hooked_blocks = array_merge( $hooked_blocks, $hooked_block_types );
Copy link
Contributor

Choose a reason for hiding this comment

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

My suggestion would be to remove the filter from here (and only keep it in insert_hooked_blocks). set_ignored_hooked_blocks_metadata is the mechanism we use to tell Block Hooks not to (re-)insert a hooked block if the containing template has already been edited by the user in the Site Editor, and thus, the hooked block has become part of the template (or was removed from it; either way, we want Block Hooks to respect that).

As mentioned elsewhere, if our use case is static templates, and if we don't have any concrete plans to make them editable by the user, we shouldn't include blocks that are added by the hooked_blocks filter in ignoredHookedBlocks. (We might then want to rename the filter to injected_blocks, to distinguish it better from hooked blocks.) Specifically, this will allow us to avoid using block signatures as an identifier.

(I think that the latter would introduce significant complexity; I'll elaborate in a follow-up comment.)

Copy link
Contributor

@ockham ockham Mar 21, 2024

Choose a reason for hiding this comment

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

I'll try to give an example for the complexities that block signatures would introduce:

Let's assume a plugin wants to insert the Mini Cart block after the Page List block, but it wants it to display the "bag" style rather than the "cart". This would look about like this (modulo escaping and whatnot):

<!-- wp:page-list {"metadata":{"ignoredHookedBlocks":["woocommerce/mini-cart-{"miniCartIcon":"bag"}"]}} /-->
<!-- wp:woocommerce/mini-cart {"miniCartIcon":"bag"} /-->

image

Now let's assume the user opens the Site Editor to change the icon to bag-alt:

<!-- wp:page-list {"metadata":{"ignoredHookedBlocks":["woocommerce/mini-cart-{"miniCartIcon":"bag"}"]}} /-->
<!-- wp:woocommerce/mini-cart {"miniCartIcon":"bag-alt"} /-->

Block Hooks will continue to compare the block and its attribute to the signature in ignoredHookedBlocks, which is now out of sync, so it'll wrongly conclude that it needs to inject the Mini Cart block, resulting in two instances:

image

We could probably circumvent the above by taking the changed block into account when updating the ignoredHookedBlocks metadata while saving (which we don't do now; it's not needed for ignoredHookedBlocks that contain only block types). It's less straight-forward than it might seem (and might involve some level of heuristics, since we cannot know for sure that a the block was inserted by Block Hooks in the first place, or added later by the user). This extra complexity is avoidable if we only apply the filter within insert_hooked_blocks.

Copy link
Author

Choose a reason for hiding this comment

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

I'm not opposed to keeping this in insert_hooked_blocks but there's a good chance that if we keep the product editor in blocks, it will be editable at some point. I think it might be good to solve this issue of generic blocks for core blocks insertion as well.

Do you have any ideas for how might solve that? An ID stands out to me as an easy way of doing this, but I know that blocks has been averse to adding IDs to blocks on the server.

Copy link
Contributor

Choose a reason for hiding this comment

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

Do you have any ideas for how might solve that? An ID stands out to me as an easy way of doing this, but I know that blocks has been averse to adding IDs to blocks on the server.

Yeah, it's tricky. I've tried to think about other alternatives, but so far, I keep coming back to named block variations. I'll try to elaborate why.

If we use attributes to distinguish blocks from each other -- as done in this PR by means of a block signature -- we most likely wouldn't want the entirety of attributes to enter into the picture. This is both for "information parsimony" and UX reasons: We'd likely want to avoid including every attribute of a given block in its signature (including style various block-supports related attributes), as that could yield a fairly massive string of serialized attributes.

For the user, it would also mean that if they change any of those attributes, Block Hooks will no longer recognize it as a hooked block, and re-insert the hooked block itself (see the shopping cart icon example above, but think even "smaller" changes, such as changing a font size or so).

To solve this problem, I think we'd need to select an attribute -- or a set of attributes -- that we actually want to distinguish one block from another, if they are of the same type. E.g. for the Social Icon block, we might want to distinguish block instances based on their service attribute (which can be facebook, x, tumblr, etc).

But this means that we need to introduce a way for blocks to declare which attributes carry that kind of meaning. And AFAICT, this is pretty exactly what named block variations are about.

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