Block Hooks: Absorb logic to set ignored hooked blocks post meta into `apply_block_hooks_to_content`

[Bernhard Reiter]

Per [59523] and [59543], Block Hooks are applied to post content and synced patterns, respectively.

In order to allow inserting hooked blocks as first and last children of a Post Content block (core/post-content), synced pattern (core/block), Template Part block (core/template-part, [60854]), and Navigation block (core/navigation), the block markup gathered from the corresponding post type is temporarily wrapped in a "virtual" instance of the respective block type. That temporary wrapper block furthermore has its metadata.ignoredHookedBlocks attribute set based on the corresponding post meta from the DB.

The Block Hooks algorithm is then applied to the full markup (including the wrapper block), thus reading and/or updating the block wrapper's metadata.ignoredHookedBlocks. (During a write operation, that metadata is then synced back to the post meta in the DB). Finally, the wrapper block is removed.

This pattern -- first introduced for the Navigation block -- is now repeated across a number of block types. ​It has been suggested by @gziolo that we absorb it into apply_block_hooks_to_content.

[gziolo]

Thank you for opening this ticket!

[Bernhard Reiter]

Looks like we'll need to prioritize this to fix a bug I introduced when enabling Block Hooks for post content, see ​https://github.com/WordPress/gutenberg/issues/68605#issuecomment-2618970655.

Port of ​https://github.com/WordPress/gutenberg/pull/68926.

More details TBD.

Fixes `#61074`, `62716`.

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

Adding a unit test for unhappy paths might be a good idea. When post_content returns something different due to the filter.

@ockham, is this ready for review?

@ockham, is this ready for review?

Not quite yet -- I still need to increase test coverage, and audit other callsites of apply_block_hooks_to_content to see if they should be changed to call apply_block_hooks_to_content_from_post_object instead.

I won't be able to work on this PR this week, but I'll continue work next week. I hope that works for you time-wise, @Mamaduka?

Thanks, @ockham!

That timeline should be okay for now. We just need to make sure this is merged before Beta 1.

[joemcgill]

Adding the gutenberg-merge keyword since this is related to changes from ​https://github.com/WordPress/gutenberg/pull/68926.

A note for any reviewers, this fixes issues noted in #61074 which effect the display of post content:

• ​https://github.com/WordPress/gutenberg/issues/68605
• ​https://github.com/WordPress/gutenberg/issues/68614

I've confirmed that I can reproduce the issue described in the second link and confirmed that the current PR (in progress) addresses the issue.

Switching back to draft mode since I might've encountered a bug while testing.

Note that when testing this with a block theme (e.g. TT5), hooked blocks in the first_child or last_child positions of the Post Content block will be inserted twice. E.g. when using ​this plugin for testing:

This problem will go away once ​changes to the Post Content block that were made in ​https://github.com/WordPress/gutenberg/pull/68926 are synced over to Core.

<details>
<summary>
You can simulate this by applying this patch.
</summary>

src/wp-includes/blocks/post-content.php

@@ -46 +46 @@
                 $content .= wp_link_pages( array( 'echo' => 0 ) );
         }
 
-        $ignored_hooked_blocks = get_post_meta( $post_id, '_wp_ignored_hooked_blocks', true );
-        if ( ! empty( $ignored_hooked_blocks ) ) {
-                $ignored_hooked_blocks  = json_decode( $ignored_hooked_blocks, true );
-                $attributes['metadata'] = array(
-                        'ignoredHookedBlocks' => $ignored_hooked_blocks,
-                );
-        }
-
-        // Wrap in Post Content block so the Block Hooks algorithm can insert blocks
-        // that are hooked as first or last child of `core/post-content`.
-        $content = get_comment_delimited_block_content(
-                'core/post-content',
-                $attributes,
-                $content
-        );
-
-        // We need to remove the `core/post-content` block wrapper after the Block Hooks algorithm,
-        // but before `do_blocks` runs, as it would otherwise attempt to render the same block again --
-        // thus recursing infinitely.
-        add_filter( 'the_content', 'remove_serialized_parent_block', 8 );
-
         /** This filter is documented in wp-includes/post-template.php */
         $content = apply_filters( 'the_content', str_replace( ']]>', ']]>', $content ) );
         unset( $seen_ids[ $post_id ] );
 
-        remove_filter( 'the_content', 'remove_serialized_parent_block', 8 );
-
         if ( empty( $content ) ) {
                 return '';
         }

</details>

Heads-up, I'm thinking to include changes from ​https://github.com/WordPress/gutenberg/pull/69241 in this. (Should take a couple of minutes only.)

Heads-up, I'm thinking to include changes from ​WordPress/gutenberg#69241 in this. (Should take a couple of minutes only.)

Done: ​https://github.com/WordPress/wordpress-develop/pull/8212/commits/970eef515bb910bf8c68a1414a8637263a67763b

[Bernhard Reiter]

In 59838:

Block Hooks: Add function to encapsulate wrapping in ad-hoc parent.

Introduce a new function, apply_block_hooks_to_content_from_post_object, to colocate the logic used to temporarily wrap content in a parent block (with ignoredHookedBlocks information fetched from post meta) alongside the call to apply_block_hooks_to_content. Fetching that information from post meta is required for all block types that get their content from post objects, i.e. Post Content, Synced Pattern, and Navigation blocks.

Additionally, the newly introduced function contains logic to ensure that insertion of a hooked block into the first_child or last_child position of a given Post Content block works, even if that block only contains "classic" markup (i.e. no blocks).

Props bernhard-reiter, gziolo, mamaduka.
Fixes #61074, #62716.

[johnjamesjacoby]

Seeing this notice running PHP version 8.4:

Deprecated: apply_block_hooks_to_content_from_post_object(): Implicitly marking parameter $post as nullable is deprecated, the explicit nullable type must be used instead in /wp-includes/blocks.php on line 1163

Recommend to remove the WP_Post type hint.

(I had to double-check, but a search through the core code revealed we only use the WP_Post $post pattern in docs; not in function definition parameters.)

[johnjamesjacoby]

In 59839:

Block Hooks: Remove WP_Post type hint.

This change prevents a PHP deprecation notice in the new apply_block_hooks_to_content_from_post_object() function, introduced in r59838.

Fixes #62716.

[jorbin]

We can still get the benefit of the type hint by making it nullable instead of removing it all together. This will work down to php7.1 which is below the minimum WP version.

See ​https://3v4l.org/1Mdrl for the deprecation and ​https://3v4l.org/o764l for the nullable version

We can still get the benefit of the type hint by making it nullable instead of removing it all together. This will work down to php7.1 which is below the minimum WP version.

See ​https://3v4l.org/1Mdrl for the deprecation and ​https://3v4l.org/o764l for the nullable version

Originally added in https://core.trac.wordpress.org/changeset/59838 Removed in https://core.trac.wordpress.org/changeset/59839

[johnjamesjacoby]

We can still get the benefit of the type hint by making it nullable instead of removing it all together. This will work down to php7.1 which is below the minimum WP version.

That syntax does get used on ints & arrays in a few random'ish places (SimplePie, wp_getimagesize(), etc...); just not with objects (and specifically not with ?WP_Post).

I'm not confident this ticket is the best place to introduce type hinting on a $post parameter that gets run through the_content. Hypothetically, plugins using the the_content filter on a non-WP_Post object would now fatal error when it would not previously, and that seems unintentional.

[audrasjb]

@jorbin do you think it's still realistic to keep this ticket in the milestone? About the PR added for further improvements, should we rather open a new ticket for these changes and close this one as fixed?

[jorbin]

@audrasjb Being that we should get it decided during this milestone (since adding a typehint to already released code could be a backcompat break), I don't think a new ticket would be helpful.

Nullable object hints are used in the Interactivity API and HTML API code so restoring them here wouldn't be something that has never been done before.

That said, I did ​a search to see what plugins applying filters to the content are doing and some are passing various things as the second param, so this would break sites running those plugins. Therefore, re-closing this.