#59313 closed enhancement (fixed)
Implement Block Hooks
Reported by: | Bernhard Reiter | Owned by: | Bernhard Reiter |
---|---|---|---|
Milestone: | 6.4 | Priority: | high |
Severity: | normal | Version: | |
Component: | Editor | Keywords: | has-patch gutenberg-merge has-unit-tests commit |
Focuses: | Cc: |
Description (last modified by )
This ticket is about porting Gutenberg PHP code that implemented Block Hooks to Core. Skip to the bottom of this ticket for a TODO list.
Synopsis
First implemented in Gutenberg under the name Auto-inserting Blocks and recently renamed to "Block Hooks", this feature is meant to provide an extensibility mechanism for Block Themes, in analogy to WordPress' Hooks concept that has allowed extending Classic Themes through filters and actions.
Specifically, Block Hooks allow a third-party block to specify a position relative to a given block into which it will then be automatically inserted (e.g. a "Like" button block can ask to be inserted after the Post Content block, or an eCommerce shopping cart block can ask to be inserted after the Navigation block).
The two core tenets for block hooks are:
- Insertion into the frontend should happen right after a plugin containing a hooked block is activated (i.e. the user isn't required to insert the block manually in the editor first); similarly, disabling the plugin should remove the hooked block from the frontend.
- The user has the ultimate power to customize that automatic insertion: The hooked block is also visible in the editor, and the user's decision to persist, dismiss, customize, or move it will be respected (and reflected on the frontend).
To account for both tenets, we've made the tradeoff that automatic block insertion only works for unmodified templates (and template parts, respectively). The reason for this is that the simplest way of storing the information whether a block has been persisted to (or dismissed from) a given template (or part) is right in the template markup. This was first suggested here.
To accommodate for that tradeoff, we've added UI controls (toggles) to increase visibility of hooked blocks, and to allow their later insertion into templates (or parts) that already have been modified by the user.
Implementation
Since we wanted hooked blocks to appear both in the frontend and in the editor (see tenet number 2), we had to make sure they were inserted into both the frontend markup and the REST API (templates and patterns endpoints) equally. As a consequence, this means that automatic insertion couldn't only be implemented at block render stage, as for the editor, we needed to modify the serialized (but unrendered) markup.
However, thanks to the tradeoff we made (see above), we could at least limit ourselves to only inserting hooked blocks into unmodified templates (and parts), i.e. those that were coming directly from a Block Theme's template (or parts) file, rather than the database.
It turns out that there's a rather natural stage for automatic insertion of hooked blocks to happen, which is during template file retrieval.
While we had to leverage the get_block_templates
and get_block_file_template
hooks in Gutenberg to insert those blocks, we'll be able to directly modify the _build_block_template_result_from_file
function in Core.
Furthermore, hooked blocks also have to be inserted into block patterns. Since almost no filters exist for the patterns registry, this was done in the patterns REST API controller in the Gutenberg codebase; for Core, we'll likely want to this at a lower level.
Core merge
Based on an early exploration in this experimental PR, I believe that we can break down the implementation of Block Hooks in Core into the following steps. Note that due to the nature of the existing code, the first steps might appear somewhat surprising and unrelated; however, they are very much relevant and will allow us to extend existing functionality through a series of self-contained steps.
- #59325: Add proper unit test coverage to verify that
_build_block_template_result_from_file
injects thetheme
attribute.- While we have coverage for
_inject_theme_attribute_in_block_template_content
, those tests only verify that that function does what is supposed to do; there's however no guarantee that_build_block_template_result_from_file
uses that function (or whatever other technique) to actually inject the theme attribute (see).
- While we have coverage for
- #59327: Add a
$callback
argument toserialize_block()
(see) andserialize_blocks()
(see).- In order to insert our hooked blocks, we need to traverse the parsed block tree (before it is eventually serialized). In order to minimize the number of tree traversals (which is a potentially costly operation), it seems that adding a callback function argument to
serialize_block()
-- which inevitable has to traverse the entire tree anyway -- is a natural fit.
- In order to insert our hooked blocks, we need to traverse the parsed block tree (before it is eventually serialized). In order to minimize the number of tree traversals (which is a potentially costly operation), it seems that adding a callback function argument to
- #59338: Use the latter to replace the call to
_inject_theme_attribute_in_block_template_content
in_build_block_template_result_from_file
with a newly written_inject_theme_attribute_in_template_part_block
(see).- Note that
_inject_theme_attribute_in_block_template_content
is still used in another spot: The Template Part block’s render.php (i.e. in GB). We might also want to replace that with_inject_theme_attribute_in_template_part_block
so that we can deprecate_inject_theme_attribute_in_block_template_content
.
- Note that
- #59346: Add
block_hooks
field toWP_Block_Type
,block.json
loading, REST API endpoint, etc (see here and here). - #59383: Implement
get_hooked_blocks()
to receive a list of blocks hooked into a given "anchor" block (see).- We probably want to cache this.
- #59385: Implement block insertion functions.
- #59412: Add post-block callback arg to
traverse_and_serialize_block(s)
, change signature. - #59399: Implement
insert_hooked_blocks
(to insert all hooked blocks for a given anchor block). - Call the latter alongside
_inject_theme_attribute_in_template_part_block
in a newly written visitor function that's passed as$callback
toserialize_blocks
(see). - Apply automatic insertion of hooked blocks to block patterns (see here and here).
- Add a filter to allow people to e.g. limit automatic insertion of hooked blocks to a certain template or template part type only (also see).
- This somewhat informs the architecture of the code (e.g. function signatures, passing of
$block_template
context) and goes beyond what is currently in Gutenberg. - Note that this hasn't been implemented in the code sync PR yet and is thus still a bit more tentative.
- This somewhat informs the architecture of the code (e.g. function signatures, passing of
The TODO list is based on my notes from a call with @gziolo earlier today.
Change History (118)
This ticket was mentioned in PR #5158 on WordPress/wordpress-develop by @Bernhard Reiter.
13 months ago
#2
- Keywords has-patch added
@Bernhard Reiter commented on PR #5158:
13 months ago
#7
Hey @ockham! I know this PR is still in draft. I'm just leaving some nitpick feedback early in case I miss a chance to review this one later, and to avoid additional instances of these being introduced later in the PR's development. 🙂
Feel free to ping me at any stage if I can help to review the implementation, tests, docs, etc. 🙂
Thank you very much @costdev! Good to address these early one, before I break the PR down into smaller ones 😊
This ticket was mentioned in PR #5185 on WordPress/wordpress-develop by @Bernhard Reiter.
13 months ago
#8
Spun off from https://github.com/WordPress/wordpress-develop/pull/5158.
See https://core.trac.wordpress.org/ticket/59313 for details
TODO: Add unit test coverage.
Trac ticket: TBD
This ticket was mentioned in PR #5186 on WordPress/wordpress-develop by @Bernhard Reiter.
13 months ago
#9
- Keywords has-unit-tests added
While we have coverage for _inject_theme_attribute_in_block_template_content
, those tests only verify that that function does what is supposed to do; there's however no guarantee that _build_block_template_result_from_file
uses that function (or whatever other technique) to actually inject the theme attribute (see).
Needed for https://core.trac.wordpress.org/ticket/59313.
### Testing Instructions
- Apply the following patch first on
trunk
, and run unit tests (npm run test:php -- --group=block-templates
). Verify that they pass (although they shouldn't) ✅ 👎 - Then, apply the patch on top of this branch, and run unit tests again. Verify that this time, they fail. :x: 👍
- Finally, remove the patch (or check CI for this branch): Unit tests should pass ✅ 👍
-
src/wp-includes/block-template-utils.php
diff --git a/src/wp-includes/block-template-utils.php b/src/wp-includes/block-template-utils.php index b8538ee3b43e..11d4141aa342 100644
a b function _build_block_template_result_from_file( $template_file, $template_type 565 565 $template = new WP_Block_Template(); 566 566 $template->id = $theme . '//' . $template_file['slug']; 567 567 $template->theme = $theme; 568 $template->content = _inject_theme_attribute_in_block_template_content( $template_content );568 $template->content = $template_content; 569 569 $template->slug = $template_file['slug']; 570 570 $template->source = 'theme'; 571 571 $template->type = $template_type;
Trac ticket: TBD.
@Bernhard Reiter commented on PR #5185:
13 months ago
#10
Ah, used the wrong repo to create the branch. Apologies.
This ticket was mentioned in PR #5187 on WordPress/wordpress-develop by @Bernhard Reiter.
13 months ago
#11
For some operations, we need to traverse the parsed block tree (before it is eventually serialized). In order to minimize the number of tree traversals (which is a potentially costly operation), it seems that adding a callback function argument to serialize_block()
-- which inevitable has to traverse the entire tree anyway -- is a natural fit.
Examples where this could come in handy:
theme
arg injection into a block template loaded from a file.- automatic insertion of hooked blocks (see https://core.trac.wordpress.org/ticket/59313)
Trac ticket: https://core.trac.wordpress.org/ticket/59327
@Bernhard Reiter commented on PR #5185:
13 months ago
#12
Opened #5187 instead.
13 months ago
#13
Hi! Please forgive my dropping in with a few comments, but this PR was interesting to me.
As a regular reader of the block API, I found the $callback
parameter somewhat unclear. In the context #5158, of course, it makes more sense. Otherwise, $callback
seems just as likely to apply at the end of the function as to the beginning. Perhaps it acts as replacement logic for the whole function. A longer name like $tree_callback
or something might help disambiguate.
Next, after thinking about the variable name, I wondered why this function would introduce a dedicated parameter for what most other functions would make accessible as a filter — pre_serialize_block
or something. My instinct as a developer at an agency would be that if core has a need to modify the behavior of serialize_block()
, doing so with a filter would be more in keeping with the extensibility of the project and preserve the ability for developers to integrate with core.
Still, I can think of some reasons against a filter, such as being wary about adding a filter to a hot code path, a desire to keep the new behavior in #5158 "private," or disliking the pattern of adding and removing one-off filters. So that leads to my final thought, which is, rather than add a callback to serialize_block()
, create a separate function for traversing the block tree, and call that before serialize_blocks()
, e.g.
$pattern = $this->registered_patterns[ $pattern_name ]; $blocks = parse_blocks( $pattern['content'] ); $visitor = _parsed_block_visitor( $pattern ); $blocks = map_block_tree( $visitor, $blocks ); // Here $pattern['content'] = serialize_blocks( $blocks );
This new function could be @access private
if you thought that was important, although I can think of some cases where we could have used such a function with our clients.
Regardless, a new function would keep an added responsibility out of serialize_block(s)()
and still allow you to modify the block tree without interference when Block Hooks required doing so.
Thanks for considering it!
13 months ago
#14
As for the functionality, these new tests cover the part of the functionality that is going to be refactored as outlined in https://github.com/WordPress/wordpress-develop/pull/5158 to bring support for the new Block Hooks feature. I can confirm that the tests look as intended.
13 months ago
#15
@dlh01, you shared great feedback in your comment about $callback
. It's a difficult topic as serialize_blocks
needs to be extremely performant.
This new function could be @access private if you thought that was important, although I can think of some cases where we could have used such a function with our clients.
Regardless, a new function would keep an added responsibility out of serialize_block(s)() and still allow you to modify the block tree without interference when Block Hooks required doing so.
This would work, but that would require traversing the tree of blocks twice. You would essentially have to recreate recursion inside map_block_tree
. What's interesting, a very similar helper is already present in the code: _inject_theme_attribute_in_block_template_content
.
My instinct as a developer at an agency would be that if core has a need to modify the behavior of serialize_block(), doing so with a filter would be more in keeping with the extensibility of the project and preserve the ability for developers to integrate with core.
Yes, that would definitely be my favorite approach. I'm very hesitant to go this path though, as it's hard to predict the consequences. In this case, it's safer to start small and allow modifications only when loading the templates and patterns from the files on the server, as we know it will get processed only once. In fact, Block Hooks feature will most likely include a filter as part of the flow so plugin devs can cover more advanced use cases where they have specific limitations for automatically inserting blocks.
Anyway, let's keep the discussion going as we implement the basic functionality to enable the feature for WP 6.4 beta 1.
@Bernhard Reiter commented on PR #5187:
13 months ago
#16
Hey @dlh01, thanks a lot for your thoughtful feedback! I think you're making a number of great points there, so it's much appreciated 😄
As a regular reader of the block API, I found the
$callback
parameter somewhat unclear. In the context #5158, of course, it makes more sense. Otherwise,$callback
seems just as likely to apply at the end of the function as to the beginning. Perhaps it acts as replacement logic for the whole function. A longer name like$tree_callback
or something might help disambiguate.
Yeah, I found the naming part hard. Eventually, I went with the fairly generic $callback
, as is also used by somewhat similar PHP functions such as `array_map`.
I'm a bit reluctant to use $tree_callback
as it seems to put too much focus on the tree aspect 🤔 $block_callback
also comes to mind, but to me, that evokes more something like a render_block
callback... Naming is hard! 😅 Maybe I can solve this by a better argument description in the PHPDoc?
Next, after thinking about the variable name, I wondered why this function would introduce a dedicated parameter for what most other functions would make accessible as a filter —
pre_serialize_block
or something. My instinct as a developer at an agency would be that if core has a need to modify the behavior ofserialize_block()
, doing so with a filter would be more in keeping with the extensibility of the project and preserve the ability for developers to integrate with core.
Still, I can think of some reasons against a filter, such as being wary about adding a filter to a hot code path, a desire to keep the new behavior in #5158 "private," or disliking the pattern of adding and removing one-off filters.
Yes, that was pretty much my thought process! I'll add that the main reason against a filter was that serialize_block
is a fairly low-level function that's used in a variety of different contexts, so it seemed risky to require the user to remove the filter after serialization to make sure it wouldn't be accidentally applied when running serialization in a different context. Finally, if we have different parts of the codebase add different filters for serialize_blocks
(to be used in different contexts), it could also get a bit hard to tell which ones apply to a certain code path, since the addition and removal of filters isn't necessarily colocated with the serialize_block()
call.
So that leads to my final thought, which is, rather than add a callback to
serialize_block()
, create a separate function for traversing the block tree, and call that beforeserialize_blocks()
, e.g. [...]
I also considered this option 😅 but ultimately decided against. Two reasons here:
- Efficiency: It would duplicate tree traversal, which is a potentially costly operation. As stated in the PR description,
serialize_block
_has_ to recursively traverse the tree; if we need to traverse it for another purpose, we might as well do it here 😄 - Established pattern: There's arguable _some_ precedent for a pattern like this -- application of a function to every element of a data structure while it is being traversed (the basic one being once again
array_map
). In cases like these, languages like PHP (but also JS) tend to accept the callback as a function argument.
---
FWIW, I was considering still some other alternatives/variations (e.g. even re-implementing serialize_blocks
itself using a variation of your map_block_tree
function, and/or accepting an array of callbacks rather than just an individual one) but eventually landed on the present approach since it seemed to strike a good balance between the requirements described above, and pretty good backwards-compatibility (via adding a new argument to an existing function). It also allowed keeping serialize_blocks
perfectly generic and ignorant of applications such as hooked block insertion, as that can be completely contained in the callback.
With all that said, I could see us adding a generic filter to serialize_block
like you suggested later if a strong need arises, but for the time being, I'd rather go with the callback argument, for the above reasons 😊
@Bernhard Reiter commented on PR #5186:
13 months ago
#17
@costdev Thank you for your feedback! I believe I've addressed it all 😊
Would you mind giving it another look when you have a moment? I'd love to land this soon as it's only the first step for Block Hooks to land in 6.4 😊
@Bernhard Reiter commented on PR #5187:
13 months ago
#18
Thank you @gziolo!
I'll make one minor change to the PHPDoc for $callback
to clarify what it's for.
@Bernhard Reiter commented on PR #5187:
13 months ago
#20
Committed to Core in https://core.trac.wordpress.org/changeset/56557/.
@Bernhard Reiter commented on PR #5187:
13 months ago
#21
BTW here's a basic first change that's going to leverage the new callback arg: https://github.com/WordPress/wordpress-develop/pull/5192 😊
13 months ago
#22
Thanks @ockham and @gziolo for your comments! (And thanks @ockham for the generous props.)
Both of you are right, of course, that a separate function would require traversing the block tree again, which is not optimal. I would say in response, first, that a separate function leaves the door open to refactoring the logic in both functions to reduce the duplication with minimal effects on backwards compatibility.
Second, the performance effect of traversing the tree twice would be felt in only the locations where the new function was called, whereas the current implementation commits core to checking is_callable()
before serializing every block everywhere. On a client project that we recently finished, for example, serialize_blocks()
is called three times just within the scope of the_content
as part of fulfilling various requirements for the site. I don't have the numbers to show it one way or the other, but I wonder whether these is_callable()
s running all the time would soon add up to more than the cost of the traversals running some of the time, an effect that would reach our projects, at least.
@Bernhard Reiter commented on PR #5186:
13 months ago
#24
Thanks all!
Committed to Core in https://core.trac.wordpress.org/changeset/56562/.
This ticket was mentioned in PR #5192 on WordPress/wordpress-develop by @Bernhard Reiter.
13 months ago
#25
Rather than using _inject_theme_attribute_in_block_template_content
to inject the theme
attribute into all Template Part blocks found in a given file-based Block Template, introduce a new function tentatively called _inject_theme_attribute_in_template_part_block
, and use that as second argument to serialize_blocks()
(introduced in #5187) in order to inject said attribute during tree traversal for serialization.
This allows for a more modular approach that will eventually be extended to implement automatic insertion of hooked blocks (see).
Note that we're guarding _build_block_template_result_from_file()
(i.e. the callsite of _inject_theme_attribute_in_template_part_block
and previously of _inject_theme_attribute_in_block_template_content
) against regressions through additional unit test coverage added in #5186.
TODO:
- [ ] Add test coverage for
_inject_theme_attribute_in_template_part_block
(same scenarios as_inject_theme_attribute_in_block_template_content
). - [x] Rebase once https://github.com/WordPress/wordpress-develop/pull/5186 has been committed.
Trac ticket: https://core.trac.wordpress.org/ticket/59338
@Bernhard Reiter commented on PR #5187:
13 months ago
#26
Thanks @ockham and @gziolo for your comments! (And thanks @ockham for the generous props.)
Both of you are right, of course, that a separate function would require traversing the block tree again, which is not optimal. I would say in response, first, that a separate function leaves the door open to refactoring the logic in both functions to reduce the duplication with minimal effects on backwards compatibility.
But if it needs serialization in addition to another operation, it will always traverse the tree at least twice, won't it? I'd really rather avoid that as a lower boundary for file-based template processing, when it's clear that it _is_ avoidable.
(Furthermore, I think that there's a risk that with this pattern, people will end up introducing another full tree traversal for each additional operation they would like to perform on the tree, rather than "merging" that operation into an existing traversal.)
Second, the performance effect of traversing the tree twice would be felt in only the locations where the new function was called, whereas the current implementation commits core to checking
is_callable()
before serializing every block everywhere. On a client project that we recently finished, for example,serialize_blocks()
is called three times just within the scope ofthe_content
as part of fulfilling various requirements for the site. I don't have the numbers to show it one way or the other, but I wonder whether theseis_callable()
s running all the time would soon add up to more than the cost of the traversals running some of the time, an effect that would reach our projects, at least.
That's fair enough (although my hunch is that is_callable
will be a fairly quick check). I would still like to continue to stick with the API of a callback argument for serialize_blocks
, and to avoid a lower boundary of two tree traversals for file-based templates.
I believe that it should be easy enough to optimize the chosen approach: If the is_callable
call proves too costly, we could do something like the following:
function serialize_blocks( $blocks, $callback = null ) {
$result = '';
if ( is_callable( $callback ) ) {
foreach ( $blocks as $block ) {
$result .= serialize_block( $block, $callback );
};
} else {
$result .= _serialize_block_without_callback( $block );
}
return $result;
}
With _serialize_block_without_callback
defined like the "old" (pre-callback) serialize_block
. A similar check could be added to serialize_block
(singular). AFAICS, that would limit the overhead to just one is_callable
check.
13 months ago
#27
For visibility, this is the performance impact from this PR included on CI https://github.com/WordPress/wordpress-develop/actions/runs/6173429870/job/16778816255?pr=5192:
13 months ago
#28
I checked out of the curiosity whether this change impacted performance metrics reported in trunk
with https://github.com/WordPress/wordpress-develop/actions/runs/6159812895/job/16715609415:
It gets compensated with the further refactoring as documented in https://github.com/WordPress/wordpress-develop/pull/5192#issuecomment-1719000338.
@Bernhard Reiter commented on PR #5192:
13 months ago
#29
Thank you very much!
@Bernhard Reiter commented on PR #5192:
13 months ago
#31
Committted to Core in https://core.trac.wordpress.org/changeset/56578.
This ticket was mentioned in PR #5203 on WordPress/wordpress-develop by @Bernhard Reiter.
13 months ago
#32
In order to implement Block Hooks (see `#59313`), we need to add a new block_hooks
field to the WP_Block_Type
class, as well as to block registration related functions, the block type REST API controller, etc.
Based on #5158.
Trac ticket: https://core.trac.wordpress.org/ticket/59346
13 months ago
#35
Let's tackle splitting the test case in a follow-up. I volunteer for doing it.
Tests splitted with https://core.trac.wordpress.org/changeset/56584.
@Bernhard Reiter commented on PR #5203:
13 months ago
#36
Thank you @gziolo!
[...] the key is the block name that can be anything that matches the pattern for the block name. If that helps we could include the pattern also for the
name
field:
That reminded me of something else; maybe we can cross-reference the name
field in the block_hooks
schema definition? Would make it more semantic -- we'd basically state, "The keys in block_hooks
are block name
s.".
I see there is custom logic for mapping
blockHooks
toblock_hooks
. I'd like to work on a follow-up to cover that part with unit tests.before
case is covered indirectly by other values still could use some testing.
Ah yeah, that's a fair point; especially for the camelCase -> snake_case mappings.
One last bit of feedback is for the
_doing_it_wrong
logic. I like that there is additional security enforced. I'm wondering if we should move it to theWP_Block_Type
class though, to cover also the case when someones registers the block with:
register_block_type( 'my-plugin/my-block', array( 'block_hooks' => array( 'my-plugin/my-block': 'first_child', ), ) );
FWIW, register_block_type
actually calls register_block_type_from_metadata
for block registration, so your example should already be covered: https://github.com/WordPress/wordpress-develop/blob/e005108fe18e6278f4e6dd0fec030e503fe0ff46/src/wp-includes/blocks.php#L584-L590
(We might want to add some unit test for this too, BTW 🤔 )
It also reminds me that in genera; there isn't that much sanitization present on the code for settings provided during block registration which I find a bit unforunate.
Yeah, good point!
@Bernhard Reiter commented on PR #5203:
13 months ago
#38
Committed to Core in https://core.trac.wordpress.org/changeset/56587.
13 months ago
#41
In https://github.com/WordPress/wordpress-develop/pull/5218 I added some refactorings discussed above:
- https://github.com/WordPress/wordpress-develop/pull/5203#discussion_r1326102815 -
gutenberg
text domain remove - There is custom logic for mapping
blockHooks
toblock_hooks
when handling metadata. I covered that part in unit tests. - One last bit of feedback was for the _doing_it_wrong logic. I moved the check to the
WP_Block_Type
class to cover also the case when someone registers the block with a regularregister_block_type
call without using metadata file.
13 months ago
#42
But if it needs serialization in addition to another operation, it will always traverse the tree at least twice, won't it? I'd really rather avoid that as a lower boundary for file-based template processing, when it's clear that it _is_ avoidable. (Furthermore, I think that there's a risk that with this pattern, people will end up introducing another full tree traversal for each additional operation they would like to perform on the tree, rather than "merging" that operation into an existing traversal.)
I think you sort of answered your own question 😄 Traversing the tree twice is definitely avoidable. In my original comment, I suggested a separate function to apply the callback throughout the tree before serialization occurs. Another way of achieving the goal of minimizing the impact to serialize_block(s)
would be to reverse your suggestion of a new function that serializes without a callback and create a _serialize_block(s)_with_callback()
.
Both new functions would, though, put us back to where we started, which is that the logic to recurse through the tree would be recreated.
I think at the heart of my original comment was the sense that it would be preferable to accept that duplicated logic in a new function that served the specific purposes presented by block hooks, compared to permanently encoding it in the main serialize_block(s)
functions.
A new internal function can be deprecated and forgotten as the block API takes on new use cases (for example, if it becomes necessary to merge multiple operations into a traversal), but the new arguments to the existing functions will live on.
Anyway: This is fun to talk about, but I know you have other things to work on, so we can leave it here. I appreciate you indulging me in this discussion and will be happy to talk about it more if it ever comes up again!
This ticket was mentioned in PR #5239 on WordPress/wordpress-develop by @gziolo.
13 months ago
#43
Trac ticket: https://core.trac.wordpress.org/ticket/59313
Extracted from https://github.com/WordPress/wordpress-develop/pull/5158 and covered with unit tests.
@Bernhard Reiter commented on PR #5239:
13 months ago
#44
Looking good overall, just one request regarding tests.
For the other PRs we've filed for block hooks, I've actually filed separate tickets; it'd be great if we could do the same here 😊 (I can do that if you're busy with other things.)
Finally, just noting that I agree that we should eventually cache get_hooked_blocks
as we discussed; but let's keep that for a follow-up.
13 months ago
#45
For the other PRs we've filed for block hooks, I've actually filed separate tickets; it'd be great if we could do the same here 😊 (I can do that if you're busy with other things.)
Feel free to open another ticket. Otherwise, I'm fine referencing the existing ticket that covers the block hooks feature.
13 months ago
#46
I addressed the feedback, and I will open a ticket for this functionality now that I have some additional time.
13 months ago
#47
The ticket is https://core.trac.wordpress.org/ticket/59383.
This ticket was mentioned in PR #5240 on WordPress/wordpress-develop by @Bernhard Reiter.
13 months ago
#49
WIP. See https://core.trac.wordpress.org/ticket/59313 and #5158.
TODO:
- [ ] Better name for
_parsed_block_visitor
. - [ ] Add test coverage for
insert_hooked_block
,insert_hooked_blocks
. - [ ] Implement pattern insertion?
Trac ticket: TBD
13 months ago
#51
Committed with https://core.trac.wordpress.org/changeset/56610.
This ticket was mentioned in PR #5241 on WordPress/wordpress-develop by @gziolo.
13 months ago
#52
Trac ticket: https://core.trac.wordpress.org/ticket/59313
Related to https://github.com/WordPress/wordpress-develop/pull/5240.
Still work in progress...
@Bernhard Reiter commented on PR #5240:
13 months ago
#53
@gziolo I've decided to only implement insert_block_hook
and inser_block_hooks
here (plus unit test coverage). We'll only wire them to block templates and patterns to actually insert blocks in a follow-up 😄
@Bernhard Reiter commented on PR #5240:
13 months ago
#55
@gziolo There's another fix we might have to backport to GB: e9931e7.
@Bernhard Reiter commented on PR #5240:
13 months ago
#56
@gziolo This needs test coverage for insert_hooked_blocks
(plural) (see https://github.com/WordPress/wordpress-develop/pull/5241#discussion_r1328811091); otherwise, it might be about ready 🤞
This ticket was mentioned in PR #5242 on WordPress/wordpress-develop by @Bernhard Reiter.
13 months ago
#57
See https://core.trac.wordpress.org/ticket/59313.
Trac ticket: TBD
@Bernhard Reiter commented on PR #5242:
13 months ago
#58
@gziolo I think this (plus #5240) is how we can implement sibling insertion 🤞
@Bernhard Reiter commented on PR #5240:
13 months ago
#59
Will need https://github.com/WordPress/wordpress-develop/pull/5242 to eventually pass the parent arg to insert_hooked_blocks
.
@Bernhard Reiter commented on PR #5240:
13 months ago
#60
I was considering doing one more thing: Split insert_hooked_block
into insert_hooked_child_block
and insert_hooked_sibling_block
, since they're pretty different. I also might adjust the function signature of the latter a bit more to align with this.
@Bernhard Reiter commented on PR #5242:
13 months ago
#61
@gziolo Since we're adding quite a bit of complexity to serialize_block(s)
, I'm starting to consider renaming it to e.g. traverse_and_serialize_block(s)
, and keep serialize_block(s)
as it was before #5187. (This will allow us to also optimize traverse_and_serialize_block(s)
, as it would enable it to call serialize_block(s)
for its children if no callback is given -- thus making the is_callable
check only at the top level, but avoiding it for all descendants, as basically discussed here.)
WDYT?
13 months ago
#63
traverse_and_serialize_block
- that sounds like a good idea given that it's going to be customized more than we discussed 👍🏻 The only question is whether it's possible to share as much as possible code between both implementations so they don't diverge over time. Maybe, it won't be a problem. Let's see later.
@Bernhard Reiter commented on PR #5242:
13 months ago
#64
traverse_and_serialize_block
- that sounds like a good idea given that it's going to be customized more than we discussed 👍🏻 The only question is whether it's possible to share as much as possible code between both implementations so they don't diverge over time. Maybe, it won't be a problem. Let's see later.
I'm afraid we won't be able to share much code between the two -- it's the tradeoff we make for optimizing. (Otherwise, we'd likely end up again with a bunch of extra conditionals in serialize_block(s)
, which is what we'd try to avoid.)
@Bernhard Reiter commented on PR #5240:
13 months ago
#65
I was considering doing one more thing: Split
insert_hooked_block
intoinsert_hooked_child_block
andinsert_hooked_sibling_block
, since they're pretty different. I also might adjust the function signature of the latter a bit more to align with this.
Might actually rename to insert_child_block
and insert_sibling_block
, since they're arguably not even that specific to hooked blocks.
@Bernhard Reiter commented on PR #5187:
13 months ago
#66
@dlh01 Just a quick update: Since we need to add a bit more logic, we'll be reverting the change to serialize_block(s)
and will introduce a new `traverse_and_serialize_block(s)` instead. Thanks again for the discussion, it was insightful feedback that eventually influenced this decision! 😄
@Bernhard Reiter commented on PR #5240:
13 months ago
#67
@gziolo Should be ready for review! I've changed it a lot, but I'm really happy with it now. It's much less specific to hooked blocks now, yet the functions are exactly the way we'll need them 😄
13 months ago
#68
Test failures seem unrelated, so maybe there is a need to rebase the PR.
I like how the code is split now into smaller helper functions that are specialized and nicely encapsulate their task. One final note, they will be exposed in the dev docs portal, which I'm fine with.
13 months ago
#69
I'm afraid we won't be able to share much code between the two -- it's the tradeoff we make for optimizing. (Otherwise, we'd likely end up again with a bunch of extra conditionals in serialize_block(s), which is what we'd try to avoid.)
I know realized that we are going to add more changes later in the process. That's fine 👍🏻
This ticket was mentioned in PR #5246 on WordPress/wordpress-develop by @gziolo.
13 months ago
#70
Trac ticket: https://core.trac.wordpress.org/ticket/59313.
Reverts changes from #5187 and implements changes discussed in #5242.
13 months ago
#71
I opened https://github.com/WordPress/wordpress-develop/pull/5246, which addresses the changes mentioned by @ockham.
13 months ago
#72
Opened https://github.com/WordPress/wordpress-develop/pull/5246 with the changes discussed in the comments.
@Bernhard Reiter commented on PR #5240:
13 months ago
#73
Test failures seem unrelated, so maybe there is a need to rebase the PR.
Yeah, looks like unit tests have been failing for all recent PRs 😕
I like how the code is split now into smaller helper functions that are specialized and nicely encapsulate their task. One final note, they will be exposed in the dev docs portal, which I'm fine with.
Yeah, I think that's perfectly okay -- the functions are now generic enough that they might even come in handy for different purposes.
@Bernhard Reiter commented on PR #5242:
13 months ago
#74
Closing in favor of https://github.com/WordPress/wordpress-develop/pull/5246.
@Bernhard Reiter commented on PR #5240:
13 months ago
#76
Committed to Core in https://core.trac.wordpress.org/changeset/56618.
13 months ago
#77
I don't think it's necessary anymore after https://core.trac.wordpress.org/changeset/56618 landed.
This ticket was mentioned in PR #5247 on WordPress/wordpress-develop by @Bernhard Reiter.
13 months ago
#78
WIP. See https://core.trac.wordpress.org/ticket/59313.
Trac ticket: TBD
13 months ago
#80
Committed with https://core.trac.wordpress.org/changeset/56620.
13 months ago
#82
Very interesting, @ockham — thanks for the heads up!
On the subject of sharing code between the functions, the decorator pattern might become helpful as the block API evolves. Here's an example implementation that I was experimenting with after our initial discussion, based on the original changes to serialize_block(s)
. WordPress doesn't really go for interfaces, but in this example I think they would allow for sharing the innerContent
loop without conditionals.
This ticket was mentioned in PR #5261 on WordPress/wordpress-develop by @Bernhard Reiter.
13 months ago
#85
Replaces #5158.
Rebase after #5257 is merged -- the latter has now some additional polish.
Trac ticket: https://core.trac.wordpress.org/ticket/59313
@Bernhard Reiter commented on PR #5261:
13 months ago
#86
@gziolo I'm hoping that this PR will become the final piece to implement Block Hooks. It should be pretty much feature-complete if I didn't miss anything 🙂
@Bernhard Reiter commented on PR #5158:
13 months ago
#88
Superseded by #5261.
@Bernhard Reiter commented on PR #5261:
13 months ago
#89
Weird, unit tests are currently failing:
Invalid argument supplied for foreach() /var/www/src/wp-includes/blocks.php:754
That's this line: https://github.com/WordPress/wordpress-develop/blob/aa033cba5cd3424a0445c6cd95e34307986880a1/src/wp-includes/blocks.php#L754
I wonder if/why the block_hooks
field isn't available at all (rather than defaulting to the empty array)?
@Bernhard Reiter commented on PR #5261:
13 months ago
#90
Workaround in bd1c57bb20. Let's see if that works.
@Bernhard Reiter commented on PR #5261:
13 months ago
#91
Seeing the same test failure I had earlier:
Invalid argument supplied for foreach() /var/www/src/wp-includes/blocks.php:757
Maybe I need to add an is_array
check alongside property_exists
.
@Bernhard Reiter commented on PR #5261:
13 months ago
#92
Seeing the same test failure I had earlier:
Invalid argument supplied for foreach() /var/www/src/wp-includes/blocks.php:757Maybe I need to add an
is_array
check alongsideproperty_exists
.
@Bernhard Reiter commented on PR #5261:
13 months ago
#93
Thanks a lot for the test coverage @gziolo, I quite like your approach! 😄
@Bernhard Reiter commented on PR #5261:
13 months ago
#96
Committed to Core in https://core.trac.wordpress.org/changeset/56649.
This ticket was mentioned in PR #5279 on WordPress/wordpress-develop by @gziolo.
13 months ago
#97
Trac ticket: https://core.trac.wordpress.org/ticket/59313
Follow-up for: https://core.trac.wordpress.org/changeset/56649
These two new unit tests document how Block Hooks behave with firstChild
and lastChild
relative positions. The hooked blocks will only get inserted in the case where the parent block has at least one child block present. While it seems like a limitation, in practice, it's hard to think of a case where the template would use a parent block without its children. It's more likely to happen with patterns in general, but in the case of patterns wired with the block theme, it also seems unlikely. The reasoning here is that out of the box, the block theme should produce a fully functional and valid HTML.
Replaces unit tests that cover similar cases in the Gutenberg plugin that are going to be removed with https://github.com/WordPress/gutenberg/pull/54651.
13 months ago
#98
I assume we no longer need it after landing an alternative version. I see now that https://core.trac.wordpress.org/ticket/59399 is closed, so I will close this one, too.
13 months ago
#100
Committed with https://core.trac.wordpress.org/changeset/56701.
This ticket was mentioned in PR #5340 on WordPress/wordpress-develop by @gziolo.
12 months ago
#101
Trac ticket: https://core.trac.wordpress.org/ticket/59313
For start, I created a simplified version of Twenty Twenty-Three theme that we used for testing Block Hooks integration that contains:
- The required
index.html
and thesingle.html
template that I plan to use with tests. - 3 template oarts where two of them reference patterns.
- 3 Patterns referenced in templates and template parts.
I want to automatically register 4 custom blocks where each of them hooks into another block using all four target relative positions: before
, after
, firstChild
, lastChild
.
Now, the goal would be to verify that the block gets hooked into the correct positions when targeting:
- template
- template part
- pattern
Bonus points would be for using a filter to prevent insertion of the block based on some made-up conditions.
Separately from that, with such a block theme in place and four hooked blocks, we could run some profiling on the block pattern registry that will contain at least 3 patterns to see if get_hooked_blocks
is really the hot path we should be worried about.
12 months ago
#102
Bonus points would be for using a filter to prevent insertion of the block based on some made-up conditions.
Separately from that, with such a block theme in place and four hooked blocks, we could run some profiling on the block pattern registry that will contain at least 3 patterns to see if get_hooked_blocks is really the hot path we should be worried about.
This PR is big enough, so I would prefer to split other test scenarios into a follow-up work.
12 months ago
#103
I addressed all comments, but https://github.com/WordPress/wordpress-develop/pull/5340#discussion_r1342899705. As noted, I'm happy to iterate on the test theme and reduce it further after profiling Block Hooks usage in a real-life scenario when rendering a single post page.
12 months ago
#105
Committed with https://core.trac.wordpress.org/changeset/56759.
This ticket was mentioned in PR #5523 on WordPress/wordpress-develop by @Bernhard Reiter.
11 months ago
#106
From a pair-programming session with @gziolo.
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.
TODO:
- [x] https://github.com/WordPress/wordpress-develop/pull/5525
- [ ] Make sure
hookedBlocks
global attribute default is set correctly (to empty array). - [ ] Make sure hooked block is correctly added to parent's
hookedBlocks
attribute for first and last child cases. - [ ] Inject hooked blocks into DB-based templates.
- [ ] Add unit tests to make sure
ignoredHookedBlocks
is added to the correct block.
Consider this PR experimental; we might want to break it down into smaller pieces to land invididually:
- [ ] Register global
metadata
attribute. - [ ] Extract
insert_hooked_blocks
function.
Trac ticket: https://core.trac.wordpress.org/ticket/59646
@Bernhard Reiter commented on PR #5523:
11 months ago
#109
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.
11 months ago
#111
WIP. Alternative approach to #5523.
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.
Trac ticket: https://core.trac.wordpress.org/ticket/59646
10 months ago
#112
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.
10 months ago
#113
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:
10 months ago
#114
Closing in favor or #5712.
@Bernhard Reiter commented on PR #5712:
10 months ago
#115
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:
@Bernhard Reiter commented on PR #5712:
10 months ago
#116
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:
10 months ago
#118
Committed to Core in https://core.trac.wordpress.org/changeset/57157.
Port the Block Hooks feature (formerly known as Auto-inserting Blocks) from Gutenberg to Core.
WIP. Might break this into separate PRs later.
Trac ticket: https://core.trac.wordpress.org/ticket/59313