-
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: Respect "multiple": false
#7443
Block Hooks: Respect "multiple": false
#7443
Conversation
Test using WordPress PlaygroundThe changes in this pull request can previewed and tested using a WordPress Playground instance. WordPress Playground is an experimental project that creates a full WordPress instance entirely within the browser. Some things to be aware of
For more details about these limitations and more, check out the Limitations page in the WordPress Playground documentation. |
4af6a2b
to
dcbdba9
Compare
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 Core Committers: Use this line as a base for the props when committing in SVN:
To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook. |
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 reviewing this, a few preliminary stylistic notes.
Co-authored-by: Jon Surrell <[email protected]>
Note to self, this is going to need a Dev Note. |
This reverts commit 40bac1c.
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.
Nice unit tests coverage. I see all the cases I could think of covered which gives a lot of confidence into the implementation proposed.
* `$content` at first and we're allowed to insert it once -- but not again. | ||
*/ | ||
$suppress_single_instance_blocks = static function ( $hooked_block_types ) use ( &$block_allows_multiple_instances, $content ) { | ||
static $single_instance_blocks_present_in_content = array(); |
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.
That's the only place where I'm not entirely sure what are the consequences of using the static variable. However my understanding is that $suppress_single_instance_blocks
should be instantiated once per apply_block_hooks_to_content
call, so all the checks are applied in a specific context - a template, template part, pattern, post content or navigation.
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.
In my testing, it works correctly as explained in #7443 (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.
Yeah, IIUC, we're instantiating a new function here every time apply_block_hooks_to_content()
is called, and for the lifetime of that $suppress_single_instance_blocks
, we're using $single_instance_blocks_present_in_content
as a cache. The next call to apply_block_hooks_to_content()
then creates a new $suppress_single_instance_blocks
, which in turn creates a new (empty) $single_instance_blocks_present_in_content
.
(I added a var_dump( $single_instance_blocks_present_in_content );
on the line below this one and ran tests to verify this 😅 )
Thank you very much for reviewing and approving!
Right, that's a good point that hadn't occurred to me. The same template part is used multiple times — but since we only enforce single-instance insertion on a per-context (here: per-template-part) basis, there is still one Like Button per template part — and thus, multiple Like buttons in the Comments section, as you said 😅 I'm not sure there's an easy way to change this so there would be only one instance in the entire rendered output, as that would have to work across context boundaries. We might look into that later (for 6.8?) -- assuming we can even easily define the desired behavior in that case -- but it's clearly out of scope for 6.7. I'll go and commit this change 😄 |
To be fair, it's exactly the same problem that exists in the editor. It's even more difficult to resolve there as there are multiple factors involved, like you can use a zoom out mode to edit only a pattern, or template part, etc. However, the more challenging factor is that some aspects of the theme might change through external updates, and the same applies to the content. At the end of the day, it all has to be assembled into a single page on the site when rendering for the visitor. It's rather impossible to find a way to represent that in the UI that such block might or might not display in the future. |
Committed to Core in https://core.trac.wordpress.org/changeset/59124. |
Block Hooks currently do not respect the
"multiple": false
setting: If this is set in a prospective hooked block'sblock.json
, and there's already an instance of that block present in a given context (i.e. template, template part, pattern, or navigation menu), then Block Hooks should not insert another instance of that block.In a similar vein, if no other instance of that block is present in the given context, then Block Hooks should insert only one instance of that block -- i.e. the first time a matching anchor block is encountered, but not for any other matching anchor blocks.
This will help extenders avoid checking for the presence of another instance of their hooked block inside of the
hooked_block_types
filter, which is called for every block found in a given context -- thus introducing an unnecessary performance overhead.Implementation notes
This is implemented in
apply_block_hooks_to_content
-- which as of59101
is the main entrypoint for applying Block Hooks to block markup -- as follows:First, we iterate over all "statically" registered hooked blocks (i.e. via a
block.json
'sblockHooks
field) and check for each of them if it hasmultiple
set tofalse
. If it does, and if there's already an instance of the block present in the markup, then we remove the block from the list of hooked blocks passed to the Block Hooks logic. Otherwise (i.e. ifmultiple
istrue
, or if the block is not present in the markup), we cache the value of itsmultiple
field in the$block_allows_multiple_instances
array.We then create an anonymous function for use with the
hooked_block_types
filter, withPHP_INT_MAX
as its priority. This allows us to keep track if we've already inserted one instance of a hooked block that has"multiple": false
, and to also "intercept" hooked blocks that are "dynamically" added, i.e. via thehooked_block_types
filter.Architectural notes
The chosen architecture has several benefits. One, unlike most other Block Hooks related functions,
apply_block_hooks_to_content
gets$content
as an argument (i.e. a block markup string), which means we don't have to awkwardly extract it from the$context
variable (that's conteXt, not conteNt), as seen e.g. here in this alternative PR.Two, using the
hooked_block_types
filter (at maximum priority) allows us to communicate both the aforementioned$content
string further "down" (so we can check for the presence of the hooked block in the markup), as well as the$block_allows_multiple_instances
cache. Otherwise, we'd likely have to add those as arguments to all functions along the call stack fromapply_block_hooks_to_content
all the way down toinsert_hooked_blocks
(thus also diluting separation of concerns). The present approach OTOH keeps things neatly separated.Testing instructions
You can use the Like Button block plugin to test; the latest version (0.9.0) has
"multiple": false
set in itsblock.json
.However, note that you need to modify the plugin prior to activating it:
hooked_block_types
filter that would normally auto-insert the block. (You can also do this manually via Tools > Plugin File Editor after installing the plugin -- just make sure it's before activating it!)"multiple": false
line fromblock.json
to verify that it will auto-insert the block if that line is not present.Trac ticket: https://core.trac.wordpress.org/ticket/61902