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

Expose serialized template content on WP_Block_Template. #5656

Conversation

nerrad
Copy link

@nerrad nerrad commented Nov 10, 2023

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';
	}
   }
   return $hooked_blocks;
}

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)
CleanShot 2023-11-10 at 14 50 39@2x CleanShot 2023-11-10 at 14 50 13@2x

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


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.

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.
@ockham
Copy link
Contributor

ockham commented Nov 13, 2023

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.

Consolidates initial read from template file into the `content` property in the `WP_Block_Template` instance removing the need for a separate `$template_content` variable.
@nerrad
Copy link
Author

nerrad commented Nov 13, 2023

Yup makes sense 👍🏻

@nerrad
Copy link
Author

nerrad commented Nov 13, 2023

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.

@ockham
Copy link
Contributor

ockham commented Nov 14, 2023

Yeah, good point 🤔

Just to make sure, are you still okay if we proceed (with 9eebbbe)? 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
Copy link
Author

nerrad commented Nov 14, 2023

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.

@@ -532,6 +531,7 @@ function _build_block_template_result_from_file( $template_file, $template_type
$template->has_theme_file = true;
$template->is_custom = true;
$template->modified = null;
$template->content = file_get_contents( $template_file['path'] );
Copy link
Contributor

@ockham ockham Nov 16, 2023

Choose a reason for hiding this comment

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

One final suggestion: Let's set $template->content right below $template->theme (i.e. on R526 if I'm not mistaken).

That's the position where it originally was before we started porting Block Hooks code to Core, and it's also more consistent with _build_block_template_result_from_post:

$template->theme = $theme;
$template->content = $post->post_content;
$template->slug = $post->post_name;

Copy link
Contributor

@ockham ockham left a comment

Choose a reason for hiding this comment

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

One final note; otherwise LGTM!

@ockham
Copy link
Contributor

ockham commented Nov 17, 2023

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! 😊

@ockham ockham closed this Nov 17, 2023
@ockham
Copy link
Contributor

ockham commented Nov 17, 2023

Backported to the 6.4 branch in https://core.trac.wordpress.org/changeset/57119.

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

Successfully merging this pull request may close these issues.

2 participants