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

Avoid double-processing post content when parsing block template HTML #3549

Open
wants to merge 8 commits into
base: trunk
Choose a base branch
from
+190 −2
Open

Avoid double-processing post content when parsing block template HTML #3549

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
abba2ae
Avoid double-processing post content when parsing block template HTML.
felixarntz Nov 1, 2022
a810c7e
Yoda I use OK.
felixarntz Nov 1, 2022
3a89988
Ensure placeholder uniqueness and prevent tampering with global.
felixarntz Nov 2, 2022
ae0a332
Add comprehensive test coverage.
felixarntz Nov 2, 2022
f49d532
Fix PHPCS warning.
felixarntz Nov 2, 2022
ac75d2f
Add test to also verify https://core.trac.wordpress.org/ticket/56930 …
felixarntz Nov 2, 2022
61f2f21
Merge branch 'trunk' into fix/55996-double-post-content-parsing
felixarntz Nov 3, 2022
0ea23a3
Remove lazy-loading specific test now that 56930 is being fixed separ…
felixarntz Nov 3, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
47 changes: 46 additions & 1 deletion src/wp-includes/block-template.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -235,16 +235,61 @@ function get_the_block_template_html() {
return;
}

// Record all post content strings into this list to avoid double parsing.
$parsed_post_content = array();
add_filter(
'render_block_core/post-content',
function( $block_content ) use ( &$parsed_post_content ) {
$parsed_post_content[] = $block_content;
return $block_content;
},
PHP_INT_MAX
);

$content = $wp_embed->run_shortcode( $_wp_current_template_content );
$content = $wp_embed->autoembed( $content );
$content = do_blocks( $content );
Copy link
Member

Choose a reason for hiding this comment

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

Could we do this?

remove_filter( 'the_excerpt', 'wp_filter_content_tags' );
remove_filter( 'the_content', 'wp_filter_content_tags' );
$content = do_blocks( $content );
add_filter( 'the_excerpt', 'wp_filter_content_tags' );
add_filter( 'the_content', 'wp_filter_content_tags' );

Copy link

Choose a reason for hiding this comment

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

Could we do this?

remove_filter( 'the_excerpt', 'wp_filter_content_tags' );
remove_filter( 'the_content', 'wp_filter_content_tags' );
$content = do_blocks( $content );
add_filter( 'the_excerpt', 'wp_filter_content_tags' );
add_filter( 'the_content', 'wp_filter_content_tags' );

WordPress/gutenberg#44995 does exactly what you are suggesting. @felixarntz informed me that it does not actually fix the problem, and nor does your similar implementation #3833.

Copy link

Choose a reason for hiding this comment

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

He did not seem at all interested in assisting with a solution based on filter removal, even after I informed him that his implementation would break shortcodes. I just don't have the familiarity with wp_filter_content_tags to know how to implement it correctly, and all I can tell is that the $context variable needs to be set to 'the_content' for it to work as intended.

Copy link
Member

Choose a reason for hiding this comment

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

As noted in my PR, even after applying this PR, filters are applied double, as image filter is applied in template_part funciton.

Copy link

Choose a reason for hiding this comment

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

I'm not talking about the same thing. It's not because of the template part, it's because there are conditions that require the the $context variable to be set to 'the_content' rather than 'the_block_template_html', and I'm not sure of the ramifications of changing the condition to allow a different value or explicitly passing 'the_content' to make the condition pass. If @felixarntz is willing to assist in this area then we might be able to use WordPress/gutenberg#44995 as the solution instead of putting up with his messy implementation.

Copy link

Choose a reason for hiding this comment

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

Could we do this?

@spacedmonkey Removing the filters around the do_blocks() call is only a partial solution. This "record before, reinstate after" approach is the only one that solves both "double execution problem" and also the "avoid all execution problem" as discussed at length in 55996.

If I need to open another ticket on Trac so that the "avoid all execution problem" is considered of equal importance to the "double execution problem" then I guess I'll do that :) In the meantime, this approach is the one that works for both problems.


// Replace all post content within the template with a temporary placeholder so that the post content is not
// unnecessarily processed again. See https://core.trac.wordpress.org/ticket/55996.
Comment on lines +253 to +254
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
// Replace all post content within the template with a temporary placeholder so that the post content is not
// unnecessarily processed again. See https://core.trac.wordpress.org/ticket/55996.
/*
* Replace all post content within the template with a temporary placeholder so that the post content is not
* unnecessarily processed again. See https://core.trac.wordpress.org/ticket/55996.
*/

Should use the multiline comment format here.

if ( ! empty( $parsed_post_content ) ) {
// Ensure a placeholder is used that is not already present in the content.
$content_placeholder = '<!-- wp-post-content-placeholder-%d -->';
$existing_placeholder_count = 0;
while ( preg_match( '/' . sprintf( $content_placeholder, '[0-9]+' ) . '/', $content ) ) {
Copy link
Member

Choose a reason for hiding this comment

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

I don't love using a while loop and regex ( preg_match ) here. Is there no way we could do a single regex and loop through all the matching blocks?

Copy link
Contributor

Choose a reason for hiding this comment

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

Seems like it should be possible with preg_match_all().

$existing_placeholder_count++;
$content_placeholder = '<!-- wp' . $existing_placeholder_count . '-post-content-placeholder-%d -->';
}

foreach ( $parsed_post_content as $i => $post_content ) {
$pos = strpos( $content, $post_content );
if ( false !== $pos ) {
Comment on lines +265 to +266
Copy link
Member

Choose a reason for hiding this comment

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

Could we use str_contains here?

$content = substr_replace( $content, sprintf( $content_placeholder, $i ), $pos, strlen( $post_content ) );
}
}
}

$content = wptexturize( $content );
$content = convert_smilies( $content );
$content = shortcode_unautop( $content );
$content = wp_filter_content_tags( $content );
$content = wp_filter_content_tags( $content, 'the_block_template' );
Copy link
Member Author

Choose a reason for hiding this comment

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

This change is a bit unrelated, but since here this function is called outside of any filter, it is crucial to manually provide a $context. I went with the_block_template for now (similar to e.g. the_content), but certainly open to alternatives.

Copy link
Member

Choose a reason for hiding this comment

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

Let's do this in another commit. No need to block this change as of this ticket.

$content = do_shortcode( $content );
$content = str_replace( ']]>', ']]&gt;', $content );

// Now that the template has been fully processed, add back in all post content.
if ( ! empty( $parsed_post_content ) ) {
$content = str_replace(
array_map(
function( $i ) use ( $content_placeholder ) {
return sprintf( $content_placeholder, $i );
},
array_keys( $parsed_post_content )
),
$parsed_post_content,
$content
);
Comment on lines +281 to +290
Copy link
Member

Choose a reason for hiding this comment

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

I would not say this is specially readable. Any chance we could just use a simple for each loop here? It would mean not requiring this function.

Copy link
Contributor

Choose a reason for hiding this comment

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

That sounds like a good idea. Also, while not huge in real terms, using foreach() will be ~15-30% faster.

foreach ( $parsed_post_content as $item_number => $replacement ) {
    $content = str_replace(
        sprintf( $content_placeholder, $item_number ),
        $replacement,
        $content
    );
}

}

// Wrap block template in .wp-site-blocks to allow for specific descendant styles
// (e.g. `.wp-site-blocks > *`).
return '<div class="wp-site-blocks">' . $content . '</div>';
Expand Down
145 changes: 144 additions & 1 deletion tests/phpunit/tests/block-template.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,10 +11,48 @@
* @group block-templates
*/
class Tests_Block_Template extends WP_UnitTestCase {
private static $post;
private static $post_id;

private static $template_canvas_path = ABSPATH . WPINC . '/template-canvas.php';

private static $demo_block_template;
private static $demo_post_content;
private static $demo_parsed_template;
private static $demo_parsed_content;

public static function wpSetUpBeforeClass( WP_UnitTest_Factory $factory ) {
self::$demo_block_template = '<!-- wp:group {"tagName":"main"} --> <main class="wp-block-group">';
self::$demo_block_template .= '<!-- wp:post-title {"level":1,"align":"wide"} /-->';
self::$demo_block_template .= '<!-- wp:spacer {"height":32} --> <div style="height:32px" aria-hidden="true" class="wp-block-spacer"></div> <!-- /wp:spacer -->';
self::$demo_block_template .= '<!-- wp:post-content {"layout":{"inherit":true}} /-->';
self::$demo_block_template .= '</main> <!-- /wp:group -->';

self::$demo_post_content = '<!-- wp:image {"width":1500,"height":1500,"sizeSlug":"large"} -->';
self::$demo_post_content .= '<figure class="wp-block-image size-large"><img src="https://picsum.photos/1500" alt="" width="1500" height="1500"/></figure>';
self::$demo_post_content .= '<!-- /wp:image -->';

self::$post_id = $factory->post->create(
array(
'post_title' => 'Demo Block Editor Post',
'post_content' => self::$demo_post_content,
'post_type' => 'post',
'post_status' => 'publish',
)
);

self::$demo_parsed_content = '<figure class="wp-block-image size-large"><img decoding="async" src="https://picsum.photos/1500" alt="" width="1500" height="1500" /></figure>';

self::$demo_parsed_template = ' <main class="is-layout-flow wp-block-group">';
self::$demo_parsed_template .= '<h1 class="alignwide wp-block-post-title">Demo Block Editor Post</h1>';
self::$demo_parsed_template .= ' <div style="height:32px" aria-hidden="true" class="wp-block-spacer"></div> ';
self::$demo_parsed_template .= '<div class="is-layout-constrained entry-content wp-block-post-content">' . self::$demo_parsed_content . '</div>';
self::$demo_parsed_template .= '</main> ';
}

public static function wpTearDownAfterClass() {
wp_delete_post( self::$post_id, true );
Copy link
Contributor

Choose a reason for hiding this comment

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

I thought the parent method deletes all posts. Curious, do other tests in the test suite fail without this line?

}

public function set_up() {
parent::set_up();
switch_theme( 'block-theme' );
Expand Down Expand Up @@ -226,4 +264,109 @@ public function test_resolve_home_block_template_no_resolution() {

$this->assertNull( $template );
}

/**
* @ticket 55996
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we have test descriptions beginning with "Tests" per convention, and covers annotations?

/**
 * Tests that X does/does not Y when Z
 *
 * @ticket 55996
 *
 * @covers
 */

Same for other tests in the PR as well.

*/
public function test_get_the_block_template_html_parses_template_content() {
global $_wp_current_template_content, $wp_query, $wp_the_query;

$wp_query = new WP_Query( array( 'p' => self::$post_id ) );
$wp_the_query = $wp_query;
the_post();

$_wp_current_template_content = self::$demo_block_template;

$html = get_the_block_template_html();
$this->assertSame( '<div class="wp-site-blocks">' . self::$demo_parsed_template . '</div>', $html );
}

/**
* @ticket 55996
*/
public function test_get_the_block_template_html_parses_post_content_only_once() {
global $_wp_current_template_content, $wp_query, $wp_the_query;

// This filter should only be run once per image (and the post here only has a single image).
add_filter(
'wp_content_img_tag',
function( $img ) {
return str_replace( '<img ', '<img data-attr="value" ', $img );
}
);

$wp_query = new WP_Query( array( 'p' => self::$post_id ) );
$wp_the_query = $wp_query;
the_post();

$_wp_current_template_content = self::$demo_block_template;

$html = get_the_block_template_html();
$this->assertStringContainsString( '<img data-attr="value" ', $html );
$this->assertStringNotContainsString( '<img data-attr="value" data-attr="value" ', $html );
Comment on lines +305 to +306
Copy link
Contributor

Choose a reason for hiding this comment

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

Test methods with multiple assertions should have a final $message parameter to explain what went wrong should an assertion fail. Same for other tests in the PR as well.

}

/**
* @ticket 55996
*/
public function test_get_the_block_template_html_parses_post_content_only_once_with_render_block_filter() {
global $_wp_current_template_content, $wp_query, $wp_the_query;

// This filter should only be run once per image (and the post here only has a single image).
add_filter(
'wp_content_img_tag',
function( $img ) {
return str_replace( '<img ', '<img data-attr="value" ', $img );
}
);

// Add random block rendering filter that modifies post content in some way.
add_filter(
'render_block_core/post-content',
function( $block_content ) {
return str_replace( ' size-large', '', $block_content );
}
);

$wp_query = new WP_Query( array( 'p' => self::$post_id ) );
$wp_the_query = $wp_query;
the_post();

$_wp_current_template_content = self::$demo_block_template;

$html = get_the_block_template_html();
$this->assertStringContainsString( '<img data-attr="value" ', $html );
$this->assertStringNotContainsString( '<img data-attr="value" data-attr="value" ', $html );
}

/**
* @ticket 55996
*/
public function test_get_the_block_template_html_uses_unique_content_placeholder() {
global $_wp_current_template_content, $wp_query, $wp_the_query;

// Ensure the block template contains a placeholder similar to the placeholder that the replacement mechanism uses.
$demo_block_template = '<!-- wp-post-content-placeholder-0 -->' . self::$demo_block_template;

// Set the post content to contain a placeholder similar to the placeholder that the replacement mechanism uses.
$post_content = '<p>Test content with conflicting placeholder comment: <!-- wp-post-content-placeholder-0 --></p>' . "\n";
wp_update_post(
array(
'ID' => self::$post_id,
'post_content' => $post_content,
'post_content_filtered' => $post_content,
)
);

$expected_parsed_template = str_replace( self::$demo_parsed_content, $post_content, '<!-- wp-post-content-placeholder-0 -->' . self::$demo_parsed_template );

$wp_query = new WP_Query( array( 'p' => self::$post_id ) );
$wp_the_query = $wp_query;
the_post();

$_wp_current_template_content = $demo_block_template;

$html = get_the_block_template_html();
$this->assertSame( '<div class="wp-site-blocks">' . $expected_parsed_template . '</div>', $html );
}
}