#59646 closed enhancement (fixed)
Block Hooks: Enable for user-modified templates/parts/patterns
Reported by: | Bernhard Reiter | Owned by: | Bernhard Reiter |
---|---|---|---|
Milestone: | 6.5 | Priority: | normal |
Severity: | normal | Version: | |
Component: | Editor | Keywords: | has-patch has-unit-tests add-to-field-guide |
Focuses: | Cc: |
Description
The biggest tradeoff we made in the implementation of Block Hooks was that we had to limit them to templates, template parts, and patterns that didn't have any user modifications (see #59313 for the reason). We would like to remove this limitation, so they’ll also work with user-modified templates, parts, and patterns.
The crucial problem we need to solve is to acknowledge if a user has opted to remove or persist a hooked block, so that the auto-insertion mechanism won't run again and inject an extraneous hooked block on the frontend when none is solicited.
Change History (39)
This ticket was mentioned in PR #5523 on WordPress/wordpress-develop by @Bernhard Reiter.
14 months ago
#3
- Keywords has-patch added
From a pair-programming session with @gziolo.
Description TBD.
TODO:
- [ ] Make sure
hookedBlocks
global attribute default is set correctly (to empty array). - [ ] Make sure hooked block is correctly added to
hookedBlocks
attribute from "after" block visitor. - [ ] Inject hooked blocks into DB-based templates.
Trac ticket: https://core.trac.wordpress.org/ticket/59646
#4
@
14 months ago
More notes from the aforementioned pairing session. We tried to figure out the necessary changes on the server to enable Block Hooks in the modified templates and content. We discussed a few ideas, and it all looks promising to use block attributes as the place to remember which blocks were hooked in the past. The next steps could look as follows:
- Finish refactoring the server implementation to always mark anchor blocks with the list of block types hooked to it at any given time in the history.
- Rethink the implementation for the Plugins panel in the block sidebar, to use the list of hooked blocks stored in the block attribute to present toggles for hooked blocks.
- Enable the same functionality for modified templates and content, taking into account that the list of hooked blocks stored for the anchor block needs to support additions (for unmodified templates and patterns, we always inject the attribute from scratch).
- The only identified challenge would be deciding what should happen when you insert a new block that is also an anchor block for other block types.
@Bernhard Reiter commented on PR #5523:
14 months ago
#5
If I apply the following patch to this branch, I can see that the Mini Cart block is added to the Navigation block's blockHooks
attribute in the preloaded routes:
-
src/wp-includes/blocks.php
diff --git a/src/wp-includes/blocks.php b/src/wp-includes/blocks.php index 77c7716434..29d30017c2 100644
a b function insert_hooked_blocks( &$block, $relative_position, $anchor_block_type, 772 772 if ( ! isset( $block['attrs']['blockHooks'] ) || ! in_array( $hooked_block_type, $block['attrs']['blockHooks'] ) ) { 773 773 $markup .= get_comment_delimited_block_content( $hooked_block_type, array(), '' ); 774 774 } 775 if ( defined( 'REST_REQUEST' ) && REST_REQUEST ) {775 //if ( defined( 'REST_REQUEST' ) && REST_REQUEST ) { 776 776 if ( ! isset( $block['attrs']['blockHooks'] ) ) { 777 777 $block['attrs']['blockHooks'] = array(); 778 778 } 779 779 $block['attrs']['blockHooks'][] = $hooked_block_type; 780 }780 //} 781 781 } 782 782 return $markup; 783 783 }
The attribute still seems to be filtered out on the client side (in the editor) though -- I guess we need to make the global blockHooks
attribute known to the editor.
I also can't see the attribute injected into the Comment Template block (for the Like Button) yet.
Needs more tweaking, I guess 😅
14 months ago
#6
The attribute still seems to be filtered out on the client side (in the editor) though -- I guess we need to make the global blockHooks attribute known to the editor.
Right, that would be necessary here. Unless we reuse metadata
attribute that is already being injected in the editor here:
https://github.com/WordPress/gutenberg/blob/trunk/packages/block-editor/src/hooks/metadata.js
In that case, we would need to make metadata
the globally registered attribute on the server (which is probably needed anyway). The information would get stored as attributes.metadata.blockHooks
.
So whatever works for you best 😀
#8
in reply to:
↑ 2
;
follow-up:
↓ 9
@
13 months ago
Replying to Bernhard Reiter:
The syntax could be as follows:
<!-- wp:navigation {"blockHooks":{"after":["mycommerce/mini-cart"]}} -->
As discussed with @matveb and @gziolo, we can probably flatten the syntax by removing the relative position, i.e. something like
<!-- wp:navigation {"blockHooks":["mycommerce/mini-cart"]} -->
We're still brainstorming to find a more descriptive / less confusing name for the attribute. We're also likely to put it inside of the metadata
attribute (that's been introduced on the client side to allow e.g. naming of blocks IIRC).
#9
in reply to:
↑ 8
@
13 months ago
Replying to Bernhard Reiter:
We're still brainstorming to find a more descriptive / less confusing name for the attribute. We're also likely to put it inside of the
metadata
attribute (that's been introduced on the client side to allow e.g. naming of blocks IIRC).
We'll go with ignoredHookedBlocks
for a start.
#10
@
13 months ago
Per discussion with @gziolo and @matveb, we'll indeed wrap the ignoredHookedBlocks
property in the metadata
attribute, i.e.
<!-- wp:navigation {"metadata":{"ignoredHookedBlocks":["mycommerce/mini-cart"]}} -->
@Bernhard Reiter commented on PR #5523:
13 months ago
#11
@Bernhard Reiter commented on PR #5523:
13 months ago
#12
On the note of the global metadata
attribute: It seems that per https://github.com/WordPress/gutenberg/pull/54426, it's no longer experimental 👍
@Bernhard Reiter commented on PR #5523:
13 months ago
#13
Update: With the attribute renamed to ignoredHookedBlocks
, and moved into metadata
, the following patch
-
src/wp-includes/blocks.php
diff --git a/src/wp-includes/blocks.php b/src/wp-includes/blocks.php index 93fad3ee95..d28274ab3a 100644
a b function insert_hooked_blocks( &$block, $relative_position, $anchor_block_type, 781 781 ) { 782 782 $markup .= get_comment_delimited_block_content( $hooked_block_type, array(), '' ); 783 783 } 784 if ( defined( 'REST_REQUEST' ) && REST_REQUEST ) {784 //if ( defined( 'REST_REQUEST' ) && REST_REQUEST ) { 785 785 if ( ! isset( $block['attrs']['metadata']['ignoredHookedBlocks'] ) ) { 786 786 $block['attrs']['metadata']['ignoredHookedBlocks'] = array(); 787 787 } 788 788 $block['attrs']['metadata']['ignoredHookedBlocks'][] = $hooked_block_type; 789 }789 //} 790 790 } 791 791 return $markup; 792 792 }
now successfully adds the information to the block in the editor 🎉
@Bernhard Reiter commented on PR #5523:
13 months ago
#14
Also working for lastChild
insertion, although it seems like it's attaching the info to the wrong block 🤔
This ticket was mentioned in PR #5598 on WordPress/wordpress-develop by @Bernhard Reiter.
13 months ago
#15
_tl;dr:_ In 6.5, we'll need arguments to two functions introduced during the 6.4 cycle to become references. Since that can be deemed a back-compat breaking change, I'm suggesting to make that change already.
The callbacks returned by make_before_block_visitor
and make_after_block_visitor
, respectively (which are passed as arguments to traverse_and_serialize_block(s)
) currently accept three arguments, all of which are block arrays (i.e. with properties blockName
, attrs
, etc.):
- A _reference_ to the block they're currently visiting,
&$block
; - the block's
$parent_block
; and - the
$prev
ious block (formake_before_block_visitor
), or the$next
block (formake_after_block_visitor
), respectively.
Those arguments are passed to the "block visitor" callbacks by traverse_and_serialize_block(s)
during traversal. The block that the callback is currently visiting is passed _by reference_ to allow modifying it, which is e.g. used to inject the theme
attribute into Template Part blocks.
One major limitation with Block Hooks is that they currently only work with templates, parts, and patterns that _don't have any user modifications_ (i.e. that come straight from the corresponding theme files, rather than the DB). For WP 6.5, we're planning to change that to also make Block Hooks work for templates, parts, and patterns that _do_ have user modifications: `#59646`.
The way we'll make this work is by storing an attribute on the "anchor" block. This is currently being developed in https://github.com/WordPress/wordpress-develop/pull/5523.
While working on that PR, I noticed that that means that we'll also need to allow the aforementioned callbacks to modify not only the currently visited $block
, but also the $parent_block
-- i.e. to pass the latter argument by reference, too. This is consistent with the requirement of adding an attribute to an anchor block, as it's not only the currently visited block that can serve as an anchor block (in the case of before
or after
sibling insertion), but also its parent (for first_child
and last_child
insertion).
Per discussion with @hellofromtonya, changing the $parent_block
argument to become a reference can be considered a backwards-compatibility breaking change. I'm thus suggesting to make that change already as part of 6.4, which is the cycle where the relevant functions were first introduced. This should have no impact on existing code, since nothing currently relies on $parent_block
remaining unmodified by the respective callback, nor is anything currently modifying that argument.
Trac ticket: https://core.trac.wordpress.org/ticket/59776
@Bernhard Reiter commented on PR #5523:
13 months ago
#16
Better now:
(Also, no more patching needed -- the PR now Just Works™️)
@Bernhard Reiter commented on PR #5598:
13 months ago
#17
This ticket was mentioned in Slack in #core by azaozz. View the logs.
13 months ago
This ticket was mentioned in PR #5608 on WordPress/wordpress-develop by @Bernhard Reiter.
13 months ago
#20
- Keywords has-unit-tests added
For use cases such as allowing the user to assign custom names to blocks or for making Block Hooks work with user-modified templates/parts/patterns (`#59646`), there's a requirement for a new global attribute (i.e. an attribute that can be added to all and any blocks) to hold that sort of metadata
.
Spun off from https://github.com/WordPress/wordpress-develop/pull/5523.
Fixes https://github.com/WordPress/gutenberg/issues/55194.
Trac ticket: https://core.trac.wordpress.org/ticket/59797
@Bernhard Reiter commented on PR #5608:
13 months ago
#21
Thanks all!
I also made sure to test this change with Gutenberg installed and activated, to make sure this change wouldn't commit with code from https://github.com/WordPress/gutenberg/pull/54426 ✅ (Fortunately, the latter has a number of isset
guards to avoid that kind of collision.)
Committed to Core in https://core.trac.wordpress.org/changeset/57068.
@get_dave commented on PR #5608:
13 months ago
#22
I was very pleased to return to work and find this completed. Thank you so much 🙇
@Bernhard Reiter commented on PR #5523:
13 months ago
#23
Some notes:
- https://github.com/WordPress/gutenberg/pull/55811 has been merged; we might want to do a package sync to pull in those changes, as we'll need them for hooked block insertion into modified templates. (I hope that a package sync won't have any adversary side effects, e.g. if other blocks rely on PHP code that's not in Core yet? Might be less risky since we're only at the beginning of the release cycle, and there's not too much stuff yet in GB's `lib/compat/wordpress-6.5`. Curious to hear your thoughts @gziolo)
- In a similar vein, we need to test the Block Hooks UI with this PR. We will need to update how it works, which has to be done in GB. Depending on the logic we'll have to implement there (to possibly accommodate both for the "old" way of hooked block insertion and the "new" way), this might raise the question if we should also implement insertion into modified templates in GB first 🤔 (We will probably be able to make an informed decision based on what those changes to the UI logic will need to look like.)
This ticket was mentioned in PR #5712 on WordPress/wordpress-develop by @Bernhard Reiter.
12 months ago
#24
WIP. Alternative approach to #5523.
Trac ticket: https://core.trac.wordpress.org/ticket/59646
12 months ago
#25
Alternative approach to https://github.com/WordPress/wordpress-develop/pull/5523. See https://github.com/WordPress/wordpress-develop/pull/5609#discussion_r1408393016 for the rationale.
The proposed alternative looks very appealing. I need to think more about it, but on the first look I would go with this approach.
12 months ago
#26
It's less important, but the Ci job reports some coding style violations.
The concept introduced around ignoredHookedBlocks
on the anchor block works great, and I'm unable to find any drawbacks of this approach on the server side. If you still favor the new helper proposed get_hooked_block_markup
(most likely annotated as private), feel free to commit it together with related unit tests.
I'm a bit hesitant to open modified templates todat to fully integrate with Block Hooks because of a single edge discovered. Everything works like charm with the special ignoredHookedBlocks
metadata value for existing and modified blocks. The only blocker is the case where a new block gets inserted in the editor that is also an anchor block. We discussed with Bernie possible solutions and it isn't that trivial. However, I'm leaning towards trying to avoid auto-inserting hooked blocks for the newly inserted anchor blocks and finding a way to annotate it with the list of hooked blocks to make it as close as possible to what happens on the server. The only limitation on the client is no ability to apply WP filter that can modify the list of hooked blocks for a certain target position.
@Bernhard Reiter commented on PR #5523:
12 months ago
#27
Closing in favor or #5712.
@Bernhard Reiter commented on PR #5712:
12 months ago
#28
I'm a bit hesitant to open modified templates todat to fully integrate with Block Hooks because of a single edge discovered. Everything works like charm with the special
ignoredHookedBlocks
metadata value for existing and modified blocks. The only blocker is the case where a new block gets inserted in the editor that is also an anchor block. We discussed with Bernie possible solutions and it isn't that trivial. However, I'm leaning towards trying to avoid auto-inserting hooked blocks for the newly inserted anchor blocks and finding a way to annotate it with the list of hooked blocks to make it as close as possible to what happens on the server. The only limitation on the client is no ability to apply WP filter that can modify the list of hooked blocks for a certain target position.
Here's a short screencast to illustrate the issue:
#29
@
12 months ago
I've filed #60008 to have a dedicated ticket for introducing the ignoredBlockHooks
metadata, without actually enabling the new technique for modified layouts. For the latter, we'll continue to use this ticket.
@Bernhard Reiter commented on PR #5712:
12 months ago
#30
As stated in this comment,
I've filed `#60008` to have a dedicated ticket for introducing the
ignoredBlockHooks
metadata, without actually enabling the new technique for modified layouts. For the latter, we'll continue to use this ticket.
I'll link this PR to the new ticket, will update the title and description, and remove the changes to _build_block_template_result_from_post
.
@Bernhard Reiter commented on PR #5712:
12 months ago
#32
Committed to Core in https://core.trac.wordpress.org/changeset/57157.
This ticket was mentioned in PR #5726 on WordPress/wordpress-develop by @Bernhard Reiter.
12 months ago
#33
Using the new technique of storing information about whether or not a hooked block should be considered for injection or not, extend said injection to encompass _modified_ templates and parts.
Follow-up to https://github.com/WordPress/wordpress-develop/pull/5712.
Trac ticket: https://core.trac.wordpress.org/ticket/59646
@Bernhard Reiter commented on PR #5726:
10 months ago
#35
This is one of the major changes planned for 6.5 and has been in the works for a while. It has been discussed quite extensively on the accompanying ticket, and most work has happened on preparatory changesets such as https://core.trac.wordpress.org/changeset/57157 and https://github.com/WordPress/gutenberg/pull/58553. Unit test coverage in this PR should ensure that it works properly.
I'll commit shortly 🙂
@Bernhard Reiter commented on PR #5726:
10 months ago
#37
Committed to Core in https://core.trac.wordpress.org/changeset/57594.
Per discussion with @matveb, the most straight-forward mechanism to achieve this is probably an extra attribute on the anchor block (not on the hooked block'').
It seems that we might not even need to store specifically whether a block was persisted or dismissed; it might be enough to simply store the information that an anchor block has had a dependent hooked block inserted. (Think of this as somehow mimicking the current approach of unmodified vs user-modified templates.)
The syntax could be as follows:
Here’s an example scenario:
blockHooks
attribute to see if it contains the shopping cart block. It doesn't, so we proceed to inject the shopping cart block after the Navigation block.blockHooks
attribute that's set to{"after":["mycommerce/mini-cart"]}
.blockHooks
attribute on the Navigation block. Since it now contains the shopping cart block, we refrain from automatically injecting the shopping cart block, thus preventing extraneous insertion.blockHooks
attribute doesn't contain the Login/out button, it is rendered on the frontend.blockHooks
attribute. This ensures that any further user modifications to the template will be reflected on the frontend; specifically, no extraneous instance of the Login/out block will be auto-inserted.The biggest change that this will require is a global
blockHooks
attribute (or some other sort of global per-block metadata).