-
Notifications
You must be signed in to change notification settings - Fork 2.7k
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
base: trunk
Are you sure you want to change the base?
Conversation
Hi @joshuatf! 👋 Thank you for your contribution to WordPress! 💖 It looks like this is your first pull request to 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, |
/** 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 ); |
There was a problem hiding this comment.
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.)
There was a problem hiding this comment.
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"} /-->
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:
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
.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
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 @ockhamTo avoid the issue with
ignoredHookedBlocks
blocking generic blocks from being added of similar types, it also adds ahooked_block_signature
to help discern hooked blocks.woocommerce/text-field
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.