Templates perf: resolve patterns server side #60349
Merged
Conversation
This file contains 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
|
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. |
|
Can we remove the pattern block entirely? |
Mamaduka
reviewed
Apr 2, 2024
|
Can we only limit the logic to REST API requests? I've not seen the theme performance stats, but technically, double parsing blocks with |
|
Size Change: 0 B Total Size: 1.73 MB ℹ️ View Unchanged
|
Mamaduka
reviewed
Apr 2, 2024
We're already parsing the blocks to run the Adding a new visitor like that shouldn't have an impact on performance IMO. |
The visitor is actually the exact same logic that the pattern block does on the server, so I suspect that it should be strictly equivalent in terms of performance for frontend (assuming we use a visitor and don't do another parse which we might be obligated to do on Gutenberg) |
|
It might temporarily impact performance because we add a new parse/serialise loop, but the goal is to merge it with the existing one. |
youknowriad
reviewed
Apr 2, 2024
| array_splice( $inner_content, $content_index, 1, $nulls ); | ||
| } | ||
| } elseif ( ! empty( $blocks[ $i ]['innerBlocks'] ) ) { | ||
| $blocks[ $i ]['innerBlocks'] = gutenberg_replace_pattern_blocks( |
There was a problem hiding this comment.
Is it possible to guard against recursions. Like a pattern block containing the same pattern block or something like that. cc @mcsf has some experience here.
There was a problem hiding this comment.
Right, I guess we can pass down the parent pattern name and check
ellatrix
commented
Apr 2, 2024
| add_filter( 'get_block_template', 'gutenberg_replace_pattern_blocks_get_block_template' ); | ||
| // Similarly, for patterns, we can avoid the double parse here: | ||
| // https://github.com/WordPress/wordpress-develop/blob/02fb53498f1ce7e63d807b9bafc47a7dba19d169/src/wp-includes/class-wp-block-patterns-registry.php#L175 | ||
| add_filter( 'rest_post_dispatch', 'gutenberg_replace_pattern_blocks_patterns_endpoint', 10, 3 ); |
There was a problem hiding this comment.
@youknowriad @Mamaduka I added some comments here regarding the double parse and core merge.
youknowriad
reviewed
Apr 2, 2024
| @@ -8,14 +8,25 @@ | |||
| * @return array Array of blocks with patterns replaced. | |||
| */ | |||
| function gutenberg_replace_pattern_blocks( $blocks, &$inner_content = null ) { | |||
| // Keep track of seen references to avoid infinite loops. | |||
| static $seen_refs = array(); | |||
There was a problem hiding this comment.
I'm not sure I ever used a static variable before but I guess this works.
youknowriad
approved these changes
Apr 2, 2024
|
Thanks! Even though it's "just" 6%, it makes it easier to figure out what else is going on that's causing store updates and re-renders. |
mcsf
reviewed
Apr 2, 2024
| @@ -8,14 +8,25 @@ | |||
| * @return array Array of blocks with patterns replaced. | |||
| */ | |||
| function gutenberg_replace_pattern_blocks( $blocks, &$inner_content = null ) { | |||
| // Keep track of seen references to avoid infinite loops. | |||
| static $seen_refs = array(); | |||
Comment on lines
+19
to
+20
| // Skip recursive patterns. | ||
| array_splice( $blocks, $i, 1 ); |
There was a problem hiding this comment.
A downside (but not a blocker) of this simple skipping is that recursion violations become silent in the editor, reducing users' ability to spot and fix them, but they will still be flagged in the front end by a different mechanism.
In a production environment (with false == WP_DEBUG && WP_DEBUG_DISPLAY), that's fine, but in a development environment it could cause situations where a user sees a notice ([block rendering halted]) in the front end but no signs of recursion in the editor. At least theoretically — I haven't tried this.
There was a problem hiding this comment.
Yes, but we don't have mechanism to "store" errors in block markup. You just injected some text, but that doesn't work in the editor. We can't just inject a paragraph block that is then editable. 🤔 Maybe set an attribute and error in the editor if that's present? Anyway, I think we could explore this separately if needed. I guess the main thing is preventing an infinite loop. :)
There was a problem hiding this comment.
Anyway, I think we could explore this separately if needed. I guess the main thing is preventing an infinite loop. :)
Like I said, not a blocker. :)
Yes, but we don't have mechanism to "store" errors in block markup. You just injected some text, but that doesn't work in the editor. We can't just inject a paragraph block that is then editable. 🤔 Maybe set an attribute and error in the editor if that's present?
Could be as simple as a special block, no? Inject:
[
'blockName' => 'core/error',
'attrs' => [
'type' => 'recursion',
'details' => $pattern_slug,
],
'innerBlocks' => [],
'innerHTML' => '',
'innerContent' => [],
];
There was a problem hiding this comment.
We could also try to use the core/missing block if we don't want to introduce a new core/error block.
ellatrix
force-pushed
the
try/resolve-patterns-server-side
branch
from
cc89b23
to
7ba871e
Compare
andrewserong
reviewed
Apr 4, 2024
|
|
||
| $registry = WP_Block_Patterns_Registry::get_instance(); | ||
| $pattern = $registry->get_registered( $slug ); | ||
| $blocks_to_insert = parse_blocks( $pattern['content'] ); |
There was a problem hiding this comment.
I'm seeing a PHP warning in WP 6.4.3 while running Gutenberg trunk on this line:
Should there be a check that the pattern could be found? I wasn't sure if there's a bug here, or if it's something specific to my testing environment.
There was a problem hiding this comment.
I had the same issue fwiw, and wp-admin wasn't even loading for me but pretty sure the version I was on was 6.5-alpha-something.
Destroying my env and upgrading to 6.6-alpha fixed it, but we do need to make sure the plugin works on 6.4 and 6.5.
There was a problem hiding this comment.
Thanks, I created #60464
|
@ellatrix this is really interesting. I wonder if it would be good to get an issue/PR going for this in Core early in order to catch potential side effects. |
cbravobernal
pushed a commit
to garridinsi/gutenberg
that referenced
this pull request
Apr 9, 2024
Co-authored-by: ellatrix <ellatrix@git.wordpress.org> Co-authored-by: Mamaduka <mamaduka@git.wordpress.org> Co-authored-by: youknowriad <youknowriad@git.wordpress.org> Co-authored-by: mcsf <mcsf@git.wordpress.org>
|
Speaking of side-effects, I've run into an issue this will cause in the new WordPress.org/patterns theme — WordPress/pattern-directory#662 Basically, I was taking advantage of the pattern block being rendered like other blocks to filter the I'm not sure if this situation is too edge-case, or if I should create an issue for GB, what do you all think? |
|
I guess we could still fire those hooks when resolving the pattern? |
|
Yeah, I think that would work, assuming this happens late enough that things like the current user & query context are set up. |
|
That could be tough, I'm not sure. TBH I don't think I'm familiar enough with the PHP side to try this, so I'd appreciate any help for this use case |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
What?
This should render template previews faster since the editor does not have replace these patterns in the editor, causing blocks to re-render.
Why?
A 6% improvement for template/pattern loading.
How?
Testing Instructions
Testing Instructions for Keyboard
Screenshots or screencast