Make WordPress Core

Opened 7 months ago

Closed 7 months ago

Last modified 7 months ago

#59882 closed defect (bug) (fixed)

Expose serialized template content to callbacks registered to the `hooked_block_types` filter.

Reported by: nerrad's profile nerrad Owned by: bernhard-reiter's profile Bernhard Reiter
Milestone: 6.4.2 Priority: normal
Severity: normal Version:
Component: Editor Keywords: has-patch fixed-major commit
Focuses: template Cc:

Description

The recently introduced Block Hooks API exposes a filter (hooked_block_types) which exposes $context as an argument. When a template file is built, WP_Block_Template::content is not exposed to the callbacks registered on hooked_block_types.

This would be useful for callbacks that might want to detect the presence of a serialized block instance (or potentially in the future utilize the HTML API) to restrict where the block is injected (before the template is rendered).

Addressing this would also achieve parity with the structure of $context when it represents a pattern (where pattern serialized content is present).

Change History (14)

This ticket was mentioned in PR #5656 on WordPress/wordpress-develop by nerrad.


7 months ago
#1

  • Keywords has-patch added; needs-patch removed

When building the template, initialize WP_Block_Template with the serialized block content so callbacks registered to the hooked_block_types filter can access the content.

If you registered a callback to hooked_block_types when the callback receives an instance of a template or template_part via the $context argument, the $content property will be null. Minimally reproducible example:

<?php
add_action( 'hooked_block_types', 'testing_auto_hook', 10, 4 );
function testing_auto_hook( $hooked_blocks, $position, $anchor_block, $context ) {
   if ( $context instanceof \WP_Block_Template ) {
        $content = $context instanceof \WP_Block_Template && $context->content ? $context->content : '';
        if ( strpos( $content, 'wp:post-title' ) !== false ) {
                // the `core/post-title` block already exists in content so abort.
                return $hooked_blocks;
        }
        if (
                $context instanceof \WP_Block_Template &&
                'after' === $position &&
                'core/post-title' === $anchor_block
        ) {
                $hooked_blocks[] = 'core/navigation';
        }
   }
}

In the above example, the code is checking to see if the $context is an instance of WP_Block_Template and if it is, looks in the $content property for the serialized representation of the core/post-title block. If it's present, then bail early and don't inject navigation. For the pages template, I'd _expect_ this to not inject core/navigation because the core/post-title block is present in the content, but in fact, it does get injected because WP_Block_Template::$content === null.

Screenshots:

Before After (with this PR/patch)
https://i0.wp.com/github.com/nerrad/wordpress-develop/assets/1429108/4958e2f9-f67b-435c-9cd0-c06bef472b0d https://i0.wp.com/github.com/nerrad/wordpress-develop/assets/1429108/b6c78b55-f93b-431c-bcae-7eaa8739f54e

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

@Bernhard Reiter commented on PR #5656:


7 months ago
#2

Makes sense, thank you for your contribution!

If we do this, should we also drop the $template_content variable altogether? It's currently parsed for the actual hooked blocks insertion; it might be less confusing for people if we changed that to also used $template->content.

nerrad commented on PR #5656:


7 months ago
#3

Yup makes sense 👍🏻

nerrad commented on PR #5656:


7 months ago
#4

One thing to be aware of with this change is that it might appear to extenders that it's possible to mutate $template->content. The blocks are already parsed from content by this point but one extension mutating the property could mess it up for every other callback utilizing block_hooks and checking this property (the content wouldn't match the rendered blocks). Ideally, $template->content would be a private property exposed via getter so that extenders can't mutate it.

@Bernhard Reiter commented on PR #5656:


7 months ago
#5

Yeah, good point 🤔

Just to make sure, are you still okay if we proceed (with https://github.com/WordPress/wordpress-develop/pull/5656/commits/9eebbbed451cd63ac52f3a04ad52dfd5df6a3b20)? Personally, I'm inclined to say that it's okay to expect an extender _not_ to mess up $template->content, or at least I don't think it's a blocker.

Also agree that a getter would be nice to have for the template content, but OTOH, we're also overriding it with new markup returned from `traverse_and_serialize_blocks` (with hooked blocks inserted) so I'm not sure we can make the content property private, as PHP doesn't seem to have a `friend` concept like C++ does 🤔

Anyway, mostly just curious if you think this PR is blocked by any of the above 😄

nerrad commented on PR #5656:


7 months ago
#6

Yup, I'm okay with the latest commit. I just wanted to flag it as a potential issue to get further thoughts. The only way (I can currently think of) that we'd be able to lock this down is to require a new object constructed and use something the value object pattern to keep WP_Block_Template immutable. Practically, I'm not sure how big of a deal that is.

#7 @Bernhard Reiter
7 months ago

  • Milestone changed from Awaiting Review to 6.4.2
  • Owner set to Bernhard Reiter
  • Status changed from new to accepted

#8 @Bernhard Reiter
7 months ago

  • Type changed from enhancement to defect (bug)

#9 @Bernhard Reiter
7 months ago

  • Resolution set to fixed
  • Status changed from accepted to closed

In 57118:

Block Hooks: Expose serialized template content to filter.

The recently introduced Block Hooks API exposes a filter (hooked_block_types) which is given a $context argument, among others. If the filter is called on a block that's part of a template or template part, $context is set to the corresponding WP_Block_Template object.

However, that object's $content property is currently not exposed to the filter. This changeset amends that shortcoming.

This is useful for callbacks that might want to detect the presence of a serialized block instance (or potentially in the future utilize the HTML API) to restrict where the block is injected (before the template is rendered).

Addressing this also achieves parity with the structure of $context when it represents a pattern (where pattern serialized content is present).

Props nerrad.
Fixes #59882.

@Bernhard Reiter commented on PR #5656:


7 months ago
#10

I've taken the liberty of moving the line and committed this change to Core: https://core.trac.wordpress.org/changeset/57118.

Hope that's okay! 😊

#11 @Bernhard Reiter
7 months ago

  • Keywords fixed-major commit added
  • Resolution fixed deleted
  • Status changed from closed to reopened

Reopening for backporting to the 6.4 branch.

#12 @Bernhard Reiter
7 months ago

  • Resolution set to fixed
  • Status changed from reopened to closed

In 57119:

Block Hooks: Expose serialized template content to filter.

The recently introduced Block Hooks API exposes a filter (hooked_block_types) which is given a $context argument, among others. If the filter is called on a block that's part of a template or template part, $context is set to the corresponding WP_Block_Template object.

However, that object's $content property is currently not exposed to the filter. This changeset amends that shortcoming.

This is useful for callbacks that might want to detect the presence of a serialized block instance (or potentially in the future utilize the HTML API) to restrict where the block is injected (before the template is rendered).

Addressing this also achieves parity with the structure of $context when it represents a pattern (where pattern serialized content is present).

Merges [57118] to the 6.4 branch.

Props nerrad.
Fixes #59882.

This ticket was mentioned in Slack in #core by jorbin. View the logs.


7 months ago

Note: See TracTickets for help on using tickets.