Correctly parse synced patterns with nested blocks#87
Merged
alecgeatches merged 7 commits intotrunkfrom Mar 21, 2025
Merged
Conversation
alecgeatches
previously approved these changes
Mar 20, 2025
Contributor
alecgeatches
left a comment
alecgeatches
left a comment
There was a problem hiding this comment.
Great fix! I like that the core/block addition is actually smaller now, and we've doubled the size of the tests on synced patterns. The new tests cover a lot more.
It'd be great to have @toshotosho verify on real data first per the bug in #86, but I'm happy to merge when verified.
| public function test_synced_pattern_with_override_in_nested_content() { | ||
| $synced_pattern_content = ' | ||
| <!-- wp:group --> | ||
| <div class=k"wp-block-group"> |
Contributor
There was a problem hiding this comment.
Minor note, there's 3 k"s scattered in here that appear to be an accident, unless I'm missing something.
| 'inner_blocks' => [], | ||
| 'locked' => false, | ||
| ]; | ||
| if ( ! $post instanceof \WP_Post ) { |
Contributor
There was a problem hiding this comment.
Extreme nit - can we use use WP_Block; up top and this for consistency?
Suggested change
| if ( ! $post instanceof \WP_Post ) { | |
| if ( ! $post instanceof WP_Post ) { |
|
Thanks guys! I just deployed the fix and it solves our issue without breaking anything else, so great work! |
alecgeatches
approved these changes
Mar 20, 2025
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
Fixes #86.
Our current solution for extracting and parsing blocks from synced patterns is a bit too experimental. It hooks into the render cycle and attempts to detect when the block being rendered has a synced pattern parent block. It does this via a somewhat fragile inspection of
WP_Block_Supports.The issue with that approach is that it can't tell when the synced pattern is a direct parent block vs. an ancestor block, so it ends up duplicating blocks in nested trees. There's no way to make this determination reliably, so we need to abandon that approach.
Instead of hooking into the existing render cycle, we can just render synced patterns manually. This carries some of its own fragility, because we need to duplicate some code from Core to support synced pattern overrides. But it's an overall improvement and the duplicated code is just a few lines.
It does require making
ContentParser#render_parsed_blocka public method, which was the path I found that required the least refactoring.@toshotosho Thanks for the failing test case. Can you check out this branch and make sure it resolves the issue for you? Feel free to contribute additional test cases.