-
Notifications
You must be signed in to change notification settings - Fork 2.7k
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
Block Hooks: Apply to Post Content (on frontend and in editor) #7898
base: trunk
Are you sure you want to change the base?
Block Hooks: Apply to Post Content (on frontend and in editor) #7898
Conversation
Test using WordPress PlaygroundThe changes in this pull request can previewed and tested using a WordPress Playground instance. WordPress Playground is an experimental project that creates a full WordPress instance entirely within the browser. Some things to be aware of
For more details about these limitations and more, check out the Limitations page in the WordPress Playground documentation. |
$content = apply_block_hooks_to_content( | ||
$content, | ||
$post, | ||
'insert_hooked_blocks_and_set_ignored_hooked_blocks_metadata' | ||
); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We're now inserting hooked blocks and setting ignoredHookedBlocks
metadata. Previously, we were only inserting hooked blocks (i.e. the default behavior of apply_block_hooks_to_content
when no third argument is given).
This change also affects the Navigation block. We need to make sure that any hooked blocks inside of the Navigation block -- both as first/last child of core/navigation
itself, but also after
/before
blocks inside of the Navigation block -- still work (both on the frontend and in the editor.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To test if the Navigation block still works, we can follow the testing instructions from WordPress/gutenberg#59021.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Turns out there was an unrelated issue with this. I've filed a fix: #7941
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The following accounts have interacted with this PR and/or linked issues. I will continue to update these lists as activity occurs. You can also manually ask me to refresh this list by adding the Core Committers: Use this line as a base for the props when committing in SVN:
To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook. |
FYI @kmanijak. Maybe you can give this a try and let me know if it works for the purpose you're interested in! 😊 |
@@ -192,6 +192,7 @@ | |||
add_filter( 'the_title', 'convert_chars' ); | |||
add_filter( 'the_title', 'trim' ); | |||
|
|||
add_filter( 'the_content', 'apply_block_hooks_to_content', 8 ); // BEFORE do_blocks(). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Quoting #7889 (comment):
apply_block_hooks_to_content
parses blocks, traverses the block tree (to insert hooked blocks), and reserializes them. Note that it's followed immediately bydo_blocks
(on the line below), which also parses blocks.This means that in theory, there's some potential for optimization (parse only once inside of
do_blocks
, and traverse to insert hooked blocks there).In practice, I'm not sure if that's viable, or if that might cause some collisions; in particular if
do_blocks
is already used in other contexts where hooked blocks are already inserted (through a different mechanism), e.g. in templates orwp_navigation
posts.
Thank you @ockham for working on this! 🚀 I gave it a try and at first glance it works as expected! I can achieve the goals raised in scope of the discussion in WooCommerce. I'll play with it more tomorrow to check if I see any problems. One potential feature I can already see (but obviously not the scope for this PR!) is the ability for more strict selectors. For example: Anyway, looking good, thank you! 🙌 |
Just following up on @kmanijak's comments. We flagged this as an issue to the Woo team, specifically about hooking blocks into Woo's Product Collection block when it was included in pages. Unfortunately, the patch here doesn't appear to resolve the issues we were seeing (we're unable to add hooked blocks inside the Product Collection block). It's possible that the issue is Woo-specific (or on our side), but I just wanted to drop my observations here in case it is fundamental to the solution. |
I'm testing with the WP 6.7.1 with the patch from this issue. Are there other dependent changes to make this work (ie would I have to start with WP trunk?) |
Thanks a lot for testing, @leewillis77! I'm a bit puzzled as to why this wouldn't work if patched against 6.7.1; there shouldn't really be any dependent changes required for this. Can you maybe try with the playground link (from this comment)? |
For reviewers: @kmanijak has kindly tested this using the Woo Product Collection block and my Like Button block in a number of different scenarios 🙂 |
@ockham @kmanijak I've re-tested this today (fresh install, re-patch etc.), and can confirm that block hooks are fired correctly on posts/pages. No idea why we couldn't get this working last week. However, trying our actual implementation rather than just artificial tests I don't think this is going to give us what we actually need (what was originally raised on the WooCommerce discussion) due to the lack of contextual information (as @kmanijak alluded to in #7898 (comment)). Ideally what we need in block hooks when we're deciding whether or not to hook a block in or not is the context of where it is. This is the same issue I originally raised in WordPress/gutenberg#54904 (comment). It's particularly bad here as the |
@@ -1286,22 +1293,44 @@ function insert_hooked_blocks_into_rest_response( $response, $post ) { | |||
'ignoredHookedBlocks' => $ignored_hooked_blocks, | |||
); | |||
} | |||
|
|||
if ( 'wp_navigation' === $post->post_type ) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What about synced patterns wp/block
? Should it be extensible? These post types are special do maybe it’s fine as is but a filter here could be useful.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, good point. I guess the wp_block
post type should map to core/block
.
A filter might make sense here 👍 Something to map post types to block types. (I wonder if we already have a function or data structure to do that somewhere 🤔 )
add_filter( 'rest_prepare_page', 'insert_hooked_blocks_into_rest_response', 10, 2 ); | ||
add_filter( 'rest_prepare_post', 'insert_hooked_blocks_into_rest_response', 10, 2 ); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This follows the precedent of rest_prepare_wp_navigation
below. However, it has the unfortunate side effect that Block Hooks aren't automatically applied to user-defined CPTs; those would need to be added manually (via add_filter( 'rest_prepare_my_cpt', ... )
). (AFAIK, there's no "generic" rest_prepare
filter that could cover all post types.)
I wonder if we should run insert_hooked_blocks_into_rest_response
from inside the Posts controller instead, in order to insert hooked blocks into every CPT that has show_in_rest
set to true
🤔 cc/ @gziolo
This reverts commit 41d13d0.
ff37889
to
7635d23
Compare
@gziolo Since insertion as the Post Content block's first or last child required changes to that block's PHP, I've updated the Gutenberg PR -- it also includes the required compat layer changes backported from this PR. Would you mind reviewing giving the GB PR a look? There are a few open questions on this PR:
However, I don't think that those should be blockers for the Gutenberg PR. Both of them deserve some more consideration, but we might do that as part of this PR -- possibly even after the GB PR has landed 😊 |
I was following the testing instructions from the description and when I add more headings the separator is not being added 😕
|
When I add more headers they all look like this, which I believe is not intended according to the testing instructions: <!-- wp:heading {"level":1,"metadata":{"ignoredHookedBlocks":["core/separator"]}} -->
<h1 class="wp-block-heading">text</h1>
<!-- /wp:heading --> |
@sirreal Good spot! Turns out @gziolo noticed the same thing over at the GB PR. I replied there. The tl;dr is
So we might keep it this way. I'll add a note to the testing instructions. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This tests well for me. I'm not well versed in block hooks but the changes seem reasonable. I've left a few notes, but I think this can be merged if you're confident.
if ( null === $context ) { | ||
$context = get_post(); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could this still be null and would that be a problem? null
is in the return type of get_post()
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm, good question 🤔 The Block Hooks algorithm should work even with no $context
specified -- it started out as an optional argument that filters such as hooked_block_types
could use to conditionally insert hooked blocks based on context.
I think that that should still be the case, but I might wanna double-check 👍
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Seems fine. I've added test coverage in a7693b0.
Co-authored-by: Jon Surrell <[email protected]>
Why does this work? And how?
It works the same way as in templates:
ignoredHookedBlocks
metadata (into anchor blocks).ignoredHookedBlocks
metadata on individual anchor blocks persisted when saving.ignoredHookedBlocks
. OTOH, any hooked blocks that were present when saving the post have now simply become part of the markup.Testing Instructions, Part One
Start by installing the following plugin, but do not activate it yet:
Plugin code
Screenshots or screencast
Testing Instructions, Part Two
Edit: The behavior covered by the following testing instructions changed after I wrote them, as @sirreal noticed here. We ended up deciding that the new behavior -- where hooked blocks would not be inserted if the anchor block was added after the post was added might be preferable, see this comment on the companion GB PR.
Now edit the post, and insert another heading at the bottom. Note that no separator is inserted at the client side "right away" -- the reason is that Block Hooks are almost exclusively handled on the server side. This is one minor UX inconsistency -- the same inconsistency is present in templates.Save the post, and reload it on the frontend. Note that a separator has now been inserted before the newly added heading!✨Finally, reload the post in the editor. Note that the separator is now also present there.Screenshots or screencast, Part Two
Trac ticket: https://core.trac.wordpress.org/ticket/61074
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.