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.

Sign up for GitHub

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

Jump to bottom

Templates perf: resolve patterns server side #60349

Merged
merged 9 commits into from Apr 2, 2024
Merged

Templates perf: resolve patterns server side #60349

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
243f72a
Templates perf: resolve patterns server side
ellatrix Apr 1, 2024
19136e0
Also filter get_block_template
ellatrix Apr 1, 2024
a5fdfc7
Also resolve for patterns
ellatrix Apr 1, 2024
b159d95
Make PHP linter happy
ellatrix Apr 2, 2024
1ad5980
Fix recursion e2e test
ellatrix Apr 2, 2024
7f09e52
Address feedback
ellatrix Apr 2, 2024
0bfe0d6
Guard for pattern recursion
ellatrix Apr 2, 2024
a65767e
Add e2e test for recursion server side
ellatrix Apr 2, 2024
7ba871e
That linter again
ellatrix Apr 2, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
97 changes: 97 additions & 0 deletions lib/compat/wordpress-6.6/resolve-patterns.php
View file
Open in desktop
@@ -0,0 +1,97 @@
<?php

/**
* Replaces pattern blocks with their content.
*
* @param array $blocks Array of blocks.
* @param array $inner_content Optional array of inner content.
* @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();
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not sure I ever used a static variable before but I guess this works.

Copy link
Member Author

Choose a reason for hiding this comment

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

I shamelessly copied @mcsf #56511.

Copy link
Contributor

Choose a reason for hiding this comment

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

I approve this technique :trollface:

$i = 0;
while ( $i < count( $blocks ) ) {
if ( 'core/pattern' === $blocks[ $i ]['blockName'] ) {
$slug = $blocks[ $i ]['attrs']['slug'];

if ( isset( $seen_refs[ $slug ] ) ) {
// Skip recursive patterns.
array_splice( $blocks, $i, 1 );
Comment on lines +19 to +20
Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Member Author

Choose a reason for hiding this comment

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

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. :)

Copy link
Contributor

@mcsf mcsf Apr 2, 2024 •
edited

Choose a reason for hiding this comment

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

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' => [],
];

Copy link
Member

Choose a reason for hiding this comment

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

We could also try to use the core/missing block if we don't want to introduce a new core/error block.

continue;
}

$registry = WP_Block_Patterns_Registry::get_instance();
$pattern = $registry->get_registered( $slug );
$blocks_to_insert = parse_blocks( $pattern['content'] );
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm seeing a PHP warning in WP 6.4.3 while running Gutenberg trunk on this line:

image

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Member Author

@ellatrix ellatrix Apr 4, 2024 •
edited

Choose a reason for hiding this comment

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

Thanks, I created #60464

$seen_refs[ $slug ] = true;
$blocks_to_insert = gutenberg_replace_pattern_blocks( $blocks_to_insert );
unset( $seen_refs[ $slug ] );
array_splice( $blocks, $i, 1, $blocks_to_insert );

// If we have inner content, we need to insert nulls in the
// inner content array, otherwise serialize_blocks will skip
// blocks.
if ( $inner_content ) {
$null_indices = array_keys( $inner_content, null, true );
$content_index = $null_indices[ $i ];
$nulls = array_fill( 0, count( $blocks_to_insert ), null );
array_splice( $inner_content, $content_index, 1, $nulls );
}

// Skip inserted blocks.
$i += count( $blocks_to_insert );
} else {
if ( ! empty( $blocks[ $i ]['innerBlocks'] ) ) {
$blocks[ $i ]['innerBlocks'] = gutenberg_replace_pattern_blocks(
Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Member Author

Choose a reason for hiding this comment

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

Right, I guess we can pass down the parent pattern name and check

$blocks[ $i ]['innerBlocks'],
$blocks[ $i ]['innerContent']
);
}
++$i;
}
}
return $blocks;
}

function gutenberg_replace_pattern_blocks_get_block_templates( $templates ) {
foreach ( $templates as $template ) {
$blocks = parse_blocks( $template->content );
$blocks = gutenberg_replace_pattern_blocks( $blocks );
$template->content = serialize_blocks( $blocks );
}
return $templates;
}

function gutenberg_replace_pattern_blocks_get_block_template( $template ) {
$blocks = parse_blocks( $template->content );
$blocks = gutenberg_replace_pattern_blocks( $blocks );
$template->content = serialize_blocks( $blocks );
return $template;
}

function gutenberg_replace_pattern_blocks_patterns_endpoint( $result, $server, $request ) {
if ( $request->get_route() !== '/wp/v2/block-patterns/patterns' ) {
return $result;
}

$data = $result->get_data();

foreach ( $data as $index => $pattern ) {
$blocks = parse_blocks( $pattern['content'] );
$blocks = gutenberg_replace_pattern_blocks( $blocks );
$data[ $index ]['content'] = serialize_blocks( $blocks );
}

$result->set_data( $data );

return $result;
}

// For core merge, we should avoid the double parse and replace the patterns in templates here:
// https://github.com/WordPress/wordpress-develop/blob/02fb53498f1ce7e63d807b9bafc47a7dba19d169/src/wp-includes/block-template-utils.php#L558
add_filter( 'get_block_templates', 'gutenberg_replace_pattern_blocks_get_block_templates' );
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 );
Copy link
Member Author

Choose a reason for hiding this comment

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

@youknowriad @Mamaduka I added some comments here regarding the double parse and core merge.

1 change: 1 addition & 0 deletions lib/load.php
View file
Open in desktop
Expand Up @@ -125,6 +125,7 @@ function gutenberg_is_experiment_enabled( $name ) {
require __DIR__ . '/compat/wordpress-6.5/script-loader.php';

// WordPress 6.6 compat.
require __DIR__ . '/compat/wordpress-6.6/resolve-patterns.php';
require __DIR__ . '/compat/wordpress-6.6/block-bindings/pattern-overrides.php';
require __DIR__ . '/compat/wordpress-6.6/option.php';
require __DIR__ . '/compat/wordpress-6.6/class-gutenberg-rest-templates-controller-6-6.php';
Expand Down
68 changes: 59 additions & 9 deletions test/e2e/specs/editor/plugins/pattern-recursion.spec.js
View file
Open in desktop
Expand Up @@ -3,7 +3,42 @@
*/
const { test, expect } = require( '@wordpress/e2e-test-utils-playwright' );

test.describe( 'Preventing Pattern Recursion', () => {
test.describe( 'Preventing Pattern Recursion (client)', () => {
test.beforeEach( async ( { admin, editor, page } ) => {
await admin.createNewPost();
await editor.canvas
.locator( 'role=button[name="Add default block"i]' )
.click();
await page.evaluate( () => {
window.wp.data.dispatch( 'core/block-editor' ).updateSettings( {
__experimentalBlockPatterns: [
{
name: 'evil/recursive',
title: 'Evil recursive',
description: 'Evil recursive',
content:
'<!-- wp:paragraph --><p>Hello</p><!-- /wp:paragraph --><!-- wp:pattern {"slug":"evil/recursive"} /-->',
},
],
} );
} );
} );

test( 'prevents infinite loops due to recursive patterns', async ( {
editor,
} ) => {
await editor.insertBlock( {
name: 'core/pattern',
attributes: { slug: 'evil/recursive' },
} );
const warning = editor.canvas.getByText(
'Pattern "evil/recursive" cannot be rendered inside itself'
);
await expect( warning ).toBeVisible();
} );
} );

test.describe( 'Preventing Pattern Recursion (server)', () => {
test.beforeAll( async ( { requestUtils } ) => {
await requestUtils.activatePlugin(
'gutenberg-test-protection-against-recursive-patterns'
Expand All @@ -21,15 +56,30 @@ test.describe( 'Preventing Pattern Recursion', () => {
} );

test( 'prevents infinite loops due to recursive patterns', async ( {
page,
editor,
} ) => {
await editor.insertBlock( {
name: 'core/pattern',
attributes: { slug: 'evil/recursive' },
} );
const warning = editor.canvas.getByText(
'Pattern "evil/recursive" cannot be rendered inside itself'
);
await expect( warning ).toBeVisible();
// Click the Toggle block inserter button
await page
.getByRole( 'button', { name: 'Toggle block inserter' } )
.click();
// Click the Patterns tab
await page.getByRole( 'tab', { name: 'Patterns' } ).click();
// Click the Uncategorized tab
await page.getByRole( 'button', { name: 'Uncategorized' } ).click();
// Click the Evil recursive pattern
await page.getByRole( 'option', { name: 'Evil recursive' } ).click();
// By simply checking the editor content, we know that the pattern
// endpoint did not crash.
expect( await editor.getBlocks() ).toMatchObject( [
{
name: 'core/paragraph',
attributes: { content: 'Hello' },
},
{
name: 'core/paragraph',
attributes: { content: 'Hello' },
},
] );
} );
} );