Skip to content
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

Blocks: Insert hooked blocks #5261

Closed
wants to merge 1 commit into from
Closed

Conversation

ockham
Copy link
Contributor

@ockham ockham commented Sep 20, 2023

Synopsis

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:

  1. 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.
  2. 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, and patterns.

It turns out that there's a rather natural stage for automatic insertion of hooked blocks to happen, which is during template file retrieval (and loading of patterns, respectively).

Trac ticket: https://core.trac.wordpress.org/ticket/59313

Supersedes #5158.


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.

@ockham ockham self-assigned this Sep 20, 2023
@ockham ockham changed the title Demonstrate new block insertion technique Blocks: Insert hooked blocks Sep 20, 2023
@ockham
Copy link
Contributor Author

ockham commented Sep 20, 2023

@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 🙂

src/wp-includes/blocks.php Outdated Show resolved Hide resolved
src/wp-includes/blocks.php Outdated Show resolved Hide resolved
src/wp-includes/blocks.php Outdated Show resolved Hide resolved
src/wp-includes/blocks.php Outdated Show resolved Hide resolved

if ( $parent && ! $prev ) {
// Candidate for first-child insertion.
$hooked_blocks_for_parent = get_hooked_blocks( $parent['blockName'] );
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

get_hooked_blocks is going to be called too many times. I will work on some optimizations tomorrow.

@ockham
Copy link
Contributor Author

ockham commented Sep 21, 2023

Weird, unit tests are currently failing:

Invalid argument supplied for foreach()

/var/www/src/wp-includes/blocks.php:754

That's this line:

foreach ( $block_type->block_hooks as $anchor_block_type => $relative_position ) {

I wonder if/why the block_hooks field isn't available at all (rather than defaulting to the empty array)?

@ockham
Copy link
Contributor Author

ockham commented Sep 21, 2023

Workaround in bd1c57b. Let's see if that works.

src/wp-includes/blocks.php Outdated Show resolved Hide resolved
@@ -751,6 +751,9 @@ function get_hooked_blocks( $name ) {
$block_types = WP_Block_Type_Registry::get_instance()->get_all_registered();
$hooked_blocks = array();
foreach ( $block_types as $block_type ) {
if ( ! property_exists( $block_type, 'block_hooks' ) ) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wonder if/why the block_hooks field isn't available at all (rather than defaulting to the empty array)?

It might be the tests for REST API that try to inject invalid values. ! empty( $block_type->block_hooks ) check would be faster and it would also cover the case when there is an empty array.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It might be an issue with the missing test clean up. I'm trying unregistering the block with invalid data: d783702.

@ockham
Copy link
Contributor Author

ockham commented Sep 21, 2023

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.

@ockham
Copy link
Contributor Author

ockham commented Sep 21, 2023

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.

22c75d9

Copy link
Member

@gziolo gziolo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We still need to increase the code coverage with unit tests, but overall, it works as expected based on the smoke tests performed. I'm very confident that traverse_and_serialize_blocks is implemented correctly. Some follow-up work will require:

  • Increased test coverage for make_before_block_visitor and make_after_block_visitor (they are tested indirectly to some degree).
  • Integration test for Block Hooks that spans a simplified theme based on Twenty Twenty-Three that contains template file with a template part and pattern sourced from the theme folders.
  • Performance audit for get_hooked_blocks and potentially also to traverse_and_serialize_blocks.

@ockham ockham marked this pull request as ready for review September 21, 2023 15:40
@ockham
Copy link
Contributor Author

ockham commented Sep 21, 2023

Thanks a lot for the test coverage @gziolo, I quite like your approach! 😄

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 underlying idea is to provide an extensibility mechanism for Block Themes, in analogy to WordPress' [https://developer.wordpress.org/plugins/hooks/ Hooks] concept that has allowed extending Classic Themes through filters and actions.

The two core tenets for Block Hooks are:

1. 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.
2. 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 (i.e. remove), customize, or move it will be respected (and reflected on the frontend).

To account for both tenets, the **tradeoff** was made to limit automatic block insertion to 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.

To accommodate for that tradeoff, [WordPress/gutenberg#52969 UI controls (toggles)] are being added to increase visibility of hooked blocks, and to allow for their later insertion into templates (or parts) that already have been modified by the user.

For hooked blocks to appear both in the frontend and in the editor (see tenet number 2), they need to be 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, the ''serialized'' (but ''unrendered'') markup needs to be modified.

Furthermore, hooked blocks also have to be inserted into block patterns. Since practically no filters exist for the patterns registry, this has to be done in the registry's `get_registered` and `get_all_registered` methods.

Props gziolo.
Fixes #59313.
@ockham
Copy link
Contributor Author

ockham commented Sep 21, 2023

Committed to Core in https://core.trac.wordpress.org/changeset/56649.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants