-
Notifications
You must be signed in to change notification settings - Fork 4.3k
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
Synced Patterns: Apply Block Hooks #68058
Conversation
Flaky tests detected in 256ebad. 🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/12378234118
|
@@ -170,7 +170,7 @@ function gutenberg_apply_block_hooks_to_post_content( $content ) { | |||
* @return WP_REST_Response The response object. | |||
*/ | |||
function gutenberg_insert_hooked_blocks_into_rest_response( $response, $post ) { | |||
if ( empty( $response->data['content']['raw'] ) || empty( $response->data['content']['rendered'] ) ) { |
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 that synced patterns don't have rendered content set, so I had to loosen the criterion here.
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 If you're merging code through a pull request on GitHub, copy and paste the following into the bottom of the merge commit message.
To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook. |
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.
It isn't that much code to support Synced Patterns. Code looks good and it's similar to what I saw for other similar blocks updated recently. I will give it a round of testing next.
@@ -206,6 +208,11 @@ function gutenberg_insert_hooked_blocks_into_rest_response( $response, $post ) { | |||
|
|||
$response->data['content']['raw'] = $content; | |||
|
|||
// If the rendered content was previously empty, we leave it like that. | |||
if ( empty( $response->data['content']['rendered'] ) ) { |
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.
Makes perfect sense together with other changes 👍🏻
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.
It works like a charm! So great to see all these enhancements 💯
@@ -87,6 +87,26 @@ function render_block_core_block( $attributes ) { | |||
add_filter( 'render_block_context', $filter_block_context, 1 ); | |||
} | |||
|
|||
$ignored_hooked_blocks = get_post_meta( $attributes['ref'], '_wp_ignored_hooked_blocks', true ); |
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.
I couldn't find a better place, so I will ask here. How feasible would it be to move the logic to apply_block_hooks_to_content
in WordPress core? A very similar logic exists now in a few places. As the helper function gets the current post object as param, it should be able to handle everything as it happens inside the rest_pre_insert_{$post_type}
filter.
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.
Hah, I just had a similar thought yesterday!
I'll need to check if we can really absorb this logic into apply_block_hooks_to_content
. Note that templates and template parts are still handled quite differently from posts, synced patterns, and navigation blocks:
- They're instances of the
WP_Block_Template
class rather thanWP_Post
. - While first/last child insertion is implemented for template parts, it is not implemented for templates. (The reason is that there is a Template Part block,
core/template-part
, that can be used as an anchor block; but of course, there's no Template block, so it cannot be used as an anchor block for Block Hooks.) - I need to think a bit more about patterns.
Anyway, I'll file a ticket for this and will look into it in January 😊
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.
They're instances of the WP_Block_Template class rather than WP_Post.
I missed the nuance of the templates. Anyway, it could still be context-based, so the additional logic would only run if the WP_Post
instance is provided.
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.
Anyway, I'll file a ticket for this and will look into it in January 😊
What?
Apply Block Hooks to synced patterns (i.e.
core/block
).Follow-up to #67272 and WordPress/wordpress-develop#7898. See WordPress/wordpress-develop#7898 (comment) for more context.
Why?
For consistency with post content, to which Block Hooks are now also applied.
How?
By applying the same technique as to other post types, e.g.
post
orwp_navigation
. See #67272 for a recent example.Follow-up
In a follow-up PR, I'll add a filter for the post type/block type mappings, so that they can be customized and used for CPTs.
Testing Instructions
Start by installing the following plugin, but do not activate it yet:
Plugin code
Screenshots or screencast