Refresh: Implement 'the_block_template_html' filter for the block template #6533

joemcgill · 2024-05-09T20:50:14Z

This refreshes #5188

Merges trunk and resolves conflicts
Updates docblocks

Trac ticket: https://core.trac.wordpress.org/ticket/55996

This Pull Request is for code review only. Please keep all other discussion in the Trac ticket. Do not merge this Pull Request. See GitHub Pull Requests for Code Review in the Core Handbook for more details.

github-actions · 2024-05-09T20:50:28Z

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 props-bot label.

Core Committers: Use this line as a base for the props when committing in SVN:

Props flixos90, joemcgill, westonruter, ryelle.

To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook.

github-actions · 2024-05-09T21:03:20Z

Test using WordPress Playground

The changes in this pull request can previewed and tested using a WordPress Playground instance.

WordPress Playground is an experimental project that creates a full WordPress instance entirely within the browser.

Some things to be aware of

The Plugin and Theme Directories cannot be accessed within Playground.
All changes will be lost when closing a tab with a Playground instance.
All changes will be lost when refreshing the page.
A fresh instance is created each time the link below is clicked.
Every time this pull request is updated, a new ZIP file containing all changes is created. If changes are not reflected in the Playground instance,
it's possible that the most recent build failed, or has not completed. Check the list of workflow runs to be sure.

For more details about these limitations and more, check out the Limitations page in the WordPress Playground documentation.

Test this pull request with WordPress Playground.

westonruter · 2024-05-09T21:07:19Z

@joemcgill This is still pending tests, correct?

joemcgill · 2024-05-09T21:41:47Z

@westonruter I think adding some tests could be good. However, the way this was implemented seems to be fully compatible with the previous behavior, so previous tests for block-templates should cover this change. I'll spend some time verifying and see if there are some places where additional test coverage should be added.

westonruter · 2024-05-09T22:22:37Z

The fix here is purely for performance, so there's no bug that can be tested otherwise?

joemcgill · 2024-05-10T13:25:34Z

As far as I can tell, this wouldn't have any direct impact on performance—if anything, moving from hard coded functions to hooks, is probably slower—but it does provide the opportunity for post processing that needs to be run across the whole template to be handled in one place in the future, which would be better.

westonruter · 2024-05-10T17:01:20Z

I thought Core-55996 is intended to be a performance fix by avoiding the same functions being run on the content more than once?

ryelle · 2024-05-10T19:00:58Z

I've manually tested this, and everything still works as expected (embeds, shortcodes, etc) across patterns, templates, and content. I also tried using the the_block_template_html filter for the issue I raised in WordPress/gutenberg#61454, and it works perfectly.

joemcgill · 2024-05-13T17:12:00Z

@westonruter

I thought Core-55996 is intended to be a performance fix by avoiding the same functions being run on the content more than once?

That's correct. Though, @felixarntz questioned whether running certain filters twice was a bad thing in this comment. This PR does not address that question, but only moves the hard-coded function calls for the block template canvas to a filter. To finish addressing the original ticket, I think we would need to move some post-processing off of the post content block since it is being run on the full template, but that will need a bit more thinking. Either way, having this filter in place is a step towards addressing the original ticket concerns and addresses the use case @ryelle ran into. I'd be happy to open a separate ticket specifically for proposing the filter, but since he already had the implementation here, I just refreshed his PR.

westonruter

LGTM

felixarntz

@joemcgill This looks solid to me. Sharing a few considerations we may want to think through.

felixarntz · 2024-05-13T17:39:40Z

src/wp-includes/default-filters.php

+add_filter( 'the_block_template_html', 'wptexturize' );
+add_filter( 'the_block_template_html', 'wp_filter_content_tags' );


Instead of having the order of these two purely defined by the order of these two add_filter() calls, I think we should use priority 12 on wp_filter_content_tags, also for consistency with how it is intentionally done elsewhere as of 6560f46.

Suggested change

add_filter( 'the_block_template_html', 'wptexturize' );

add_filter( 'the_block_template_html', 'wp_filter_content_tags' );

add_filter( 'the_block_template_html', 'wptexturize' );

add_filter( 'the_block_template_html', 'wp_filter_content_tags', 12 );

felixarntz · 2024-05-13T17:42:46Z

src/wp-includes/default-filters.php

+add_filter( 'the_block_template_html', 'wp_filter_content_tags' );
+add_filter( 'the_block_template_html', 'convert_smilies', 20 );


In the original code that is being changed here, convert_smilies() was run before wp_filter_content_tags(). While I'm not opposed to use the same priority as convert_smilies uses elsewhere, we should make sure using a different order here wasn't an intentional decision (and there is a good chance it wasn't).

But if we don't feel strongly, it would be safer to leave out the priority on convert_smilies() so that the original order is maintained.

Suggested change

add_filter( 'the_block_template_html', 'wp_filter_content_tags' );

add_filter( 'the_block_template_html', 'convert_smilies', 20 );

add_filter( 'the_block_template_html', 'wp_filter_content_tags', 12 );

add_filter( 'the_block_template_html', 'convert_smilies' );

felixarntz · 2024-05-13T17:48:33Z

src/wp-includes/default-filters.php

+add_filter( 'the_block_template_html', 'shortcode_unautop', 7 );
+add_filter( 'the_block_template_html', 'do_shortcode', 8 );


While this maintains the original order of the function, I wonder whether we could align this with the filter order execution on the other similar scenarios, e.g. on the_content. On those, shortcode_unautop uses the default 10 and do_shortcode uses 11.

I know this comment is a bit opposite of my other comment, but I'm purely throwing it out there for consideration. I wonder why it was even decided in the first place to use a different order here.

On another note: If we used 10 and 11 here, it would also allow to use 8 for the filters in the WP_Embed class, which is another place to make things more consistent.

Suggested change

add_filter( 'the_block_template_html', 'shortcode_unautop', 7 );

add_filter( 'the_block_template_html', 'do_shortcode', 8 );

add_filter( 'the_block_template_html', 'shortcode_unautop' );

add_filter( 'the_block_template_html', 'do_shortcode', 11 );

felixarntz · 2024-05-13T17:49:06Z

src/wp-includes/class-wp-embed.php

@@ -30,6 +30,7 @@ class WP_Embed {
 	 */
 	public function __construct() {
 		// Hack to get the [embed] shortcode to run before wpautop().
+		add_filter( 'the_block_template_html', array( $this, 'run_shortcode' ), 6 );


If we go with my other suggestion on adjusting the shortcode filter priorities, we could align this:

Suggested change

add_filter( 'the_block_template_html', array( $this, 'run_shortcode' ), 6 );

add_filter( 'the_block_template_html', array( $this, 'run_shortcode' ), 8 );

felixarntz · 2024-05-13T17:49:15Z

src/wp-includes/class-wp-embed.php

@@ -38,6 +39,7 @@ public function __construct() {
 		add_shortcode( 'embed', '__return_false' );

 		// Attempts to embed all URLs in a post.
+		add_filter( 'the_block_template_html', array( $this, 'autoembed' ), 6 );


If we go with my other suggestion on adjusting the shortcode filter priorities, we could align this:

Suggested change

add_filter( 'the_block_template_html', array( $this, 'autoembed' ), 6 );

add_filter( 'the_block_template_html', array( $this, 'autoembed' ), 8 );

felixarntz and others added 5 commits September 11, 2023 16:26

Implement 'template' filter for the block template HTML.

8a85987

Merge branch 'trunk' into add/55996-block-template-filter

cc87df3

Address CR feedback.

ec9a2ff

Merge branch 'trunk' into update/55996-block-template-filter

36fa8bf

Update docblocks

b42de40

joemcgill mentioned this pull request May 9, 2024

Implement 'template' filter for the block template HTML #5188

Closed

joemcgill requested review from felixarntz, westonruter, youknowriad and ryelle May 9, 2024 20:52

joemcgill mentioned this pull request May 9, 2024

Pattern render hooks no longer run WordPress/gutenberg#61454

Open

westonruter approved these changes May 13, 2024

View reviewed changes

felixarntz reviewed May 13, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refresh: Implement 'the_block_template_html' filter for the block template #6533

Refresh: Implement 'the_block_template_html' filter for the block template #6533

joemcgill commented May 9, 2024

github-actions bot commented May 9, 2024 •

edited

github-actions bot commented May 9, 2024

westonruter commented May 9, 2024

joemcgill commented May 9, 2024

westonruter commented May 9, 2024

joemcgill commented May 10, 2024

westonruter commented May 10, 2024

ryelle commented May 10, 2024

joemcgill commented May 13, 2024

westonruter left a comment

felixarntz left a comment

felixarntz May 13, 2024

felixarntz May 13, 2024

felixarntz May 13, 2024 •

edited

felixarntz May 13, 2024

felixarntz May 13, 2024

		add_filter( 'the_block_template_html', 'wptexturize' );
		add_filter( 'the_block_template_html', 'wp_filter_content_tags' );

	add_filter( 'the_block_template_html', array( $this, 'run_shortcode' ), 6 );
	add_filter( 'the_block_template_html', array( $this, 'run_shortcode' ), 8 );

Refresh: Implement 'the_block_template_html' filter for the block template #6533

Are you sure you want to change the base?

Refresh: Implement 'the_block_template_html' filter for the block template #6533

Conversation

joemcgill commented May 9, 2024

github-actions bot commented May 9, 2024 • edited

github-actions bot commented May 9, 2024

Test using WordPress Playground

Some things to be aware of

westonruter commented May 9, 2024

joemcgill commented May 9, 2024

westonruter commented May 9, 2024

joemcgill commented May 10, 2024

westonruter commented May 10, 2024

ryelle commented May 10, 2024

joemcgill commented May 13, 2024

westonruter left a comment

Choose a reason for hiding this comment

felixarntz left a comment

Choose a reason for hiding this comment

felixarntz May 13, 2024

Choose a reason for hiding this comment

felixarntz May 13, 2024

Choose a reason for hiding this comment

felixarntz May 13, 2024 • edited

Choose a reason for hiding this comment

felixarntz May 13, 2024

Choose a reason for hiding this comment

felixarntz May 13, 2024

Choose a reason for hiding this comment

github-actions bot commented May 9, 2024 •

edited

felixarntz May 13, 2024 •

edited