-
Notifications
You must be signed in to change notification settings - Fork 3.2k
Block Hooks: Respect "multiple": false
#7443
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
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. |
sirreal
left a 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'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.
gziolo
left a 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.
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. |



Block Hooks currently do not respect the
"multiple": falsesetting: 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_typesfilter, 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 of59101is 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'sblockHooksfield) and check for each of them if it hasmultipleset 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. ifmultipleistrue, or if the block is not present in the markup), we cache the value of itsmultiplefield in the$block_allows_multiple_instancesarray.We then create an anonymous function for use with the
hooked_block_typesfilter, withPHP_INT_MAXas 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_typesfilter.Architectural notes
The chosen architecture has several benefits. One, unlike most other Block Hooks related functions,
apply_block_hooks_to_contentgets$contentas an argument (i.e. a block markup string), which means we don't have to awkwardly extract it from the$contextvariable (that's conteXt, not conteNt), as seen e.g. here in this alternative PR.Two, using the
hooked_block_typesfilter (at maximum priority) allows us to communicate both the aforementioned$contentstring further "down" (so we can check for the presence of the hooked block in the markup), as well as the$block_allows_multiple_instancescache. Otherwise, we'd likely have to add those as arguments to all functions along the call stack fromapply_block_hooks_to_contentall 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": falseset in itsblock.json.However, note that you need to modify the plugin prior to activating it:
hooked_block_typesfilter 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": falseline fromblock.jsonto verify that it will auto-insert the block if that line is not present.Trac ticket: https://core.trac.wordpress.org/ticket/61902