Remove the_content filter for block_template_parts

[johnstonphilip]

Description

This PR fixes #20342, where content filtered to the_content appears after every block template part is output.

How has this been tested?

1. Create a block-based theme, with the header.html and footer.html in the block-template-parts directory, and the index.html file pulling those in.
2. Add this code to a custom plugin:

function my_content_after_the_content( $the_content ) {
	return $the_content .
	'<div class="sharing_icons">I am hooked to the_content, to appear after every output of the_content</div>';
}
 add_filter( 'the_content', 'my_content_after_the_content' );

3. View a post.
4. Notice you see the hooked output after each template_part is shown.

Types of changes

Bug fix.

Screenshots:

Before:

After:

Checklist:

• [✔] My code is tested.
• [✔] My code follows the WordPress code style.
• [✔] My code follows the accessibility standards.
• [✔] My code has proper inline documentation.
• [✔] I've included developer documentation if appropriate.
• [✔] I've updated all React Native files affected by any refactorings/renamings in this PR.

[youknowriad]

I believe this filter is needed to run dynamic blocks, shortcuts and other things in the content of the template part, what's the reason behind removing it?

[johnstonphilip]

@youknowriad I attempted to explain the problem in my description. Here's a screenshot that hopefully illustrates it:

https://user-images.githubusercontent.com/7538525/74973542-975e8800-53f1-11ea-9bdf-73a0cabdbea0.png

Imagine those red boxes are Social Sharing buttons, or purchase buttons from an ecommerce plugin (Easy Digital Downloads does this for example). You'll get duplicate output on the screen after every template part.

[youknowriad]

I don't really think that's an issue though, is it? You're just running the hook on all post types (all contents)?

An alternative here would be to have another the_template_content or something like that, that does the same thing as the_content but excludes third-party the_content filters maybe.

i believe if we don't run the filter now, dynamic blocks inside template parts don't work properly right (I didn't test, just a guess) (among other things maybe, embeds, shortcodes...)

[johnstonphilip]

@youknowriad While we definitely can't use the_content here due to many plugins authors using that for custom outputs over the years, I believe you're correct that we do need to do the same processing on the return. We should likely introduce a new filter like the one you described. How about calling it the_template_part to make it specific for this context? If you agree I can add that to this PR.

[johnstonphilip]

It looks like these hook-calls will need to be added to this new filter:
https://github.com/WordPress/wordpress-develop/blob/3fb063f7052bf7427f6c965e493c9b12f2a566c9/src/wp-includes/default-filters.php#L172-L178

[youknowriad]

I think it makes sense but I'd love thoughts from other folks cc @epiqueras @mcsf @azaozz
Also, I'm thinking some of these filters are not needed because they will apply after the fact to the output of the template part too on the parent template get_the_content calls. (depends on the priority I guess)

[johnstonphilip]

@youknowriad You are correct that dynamic blocks and shortcodes do not work in the header.html file with this PR.

I took another shot at this here, where I've added a new/dedicated filter, instead of removing it entirely:
#20344

[epiqueras]

I suggest we manually run the filters from https://github.com/WordPress/wordpress-develop/blob/3fb063f7052bf7427f6c965e493c9b12f2a566c9/src/wp-includes/default-filters.php#L172-L178 and see how we do before introducing a new filter.

[johnstonphilip]

@epiqueras That works as well. Added here: 729a613

[azaozz]

I suggest we manually run the filters

Yeah, should be "good enough" for testing. The prepend_attachment can be removed. It is an ancient hack (from WP 2.0) for when an attachment post is displayed. Doesn't do anything if the current post is not an attachment:

	$post = get_post();

	if ( empty( $post->post_type ) || 'attachment' !== $post->post_type ) {
		return $content;
	}

However this brings another question: most of these "display filters" are intended to run on the front-end for post_content and expect things like $post = get_post(); (i.e. the PHP global $post to be set) in order to work. Even if core doesn't rely on the global, some plugins will. Seems this will need some "special handling" for when the new filter for template parts is added.

[epiqueras]

Thanks for catching this!

[azaozz]

Uh, sorry, a bit late to the party :)

An alternative here would be to have another (filter) the_template_content or something like that, that does the same thing as the_content but excludes third-party the_content filters maybe.

Yeah, thinking that'd be best. The old filter is generally expected to run only on post_content (including when auto-generatig excerpt), and is used in many plugins. It shouldn't be reused in other contexts.

Also, I'm thinking some of these filters are not needed

Big +1. The template parts only support blocks. Things like wpautop and out-of-block/old-style shortcode processing don't seem to make sense. Looking at the default filters:

add_filter( 'the_content', 'do_blocks', 9 );
add_filter( 'the_content', 'wptexturize' );
add_filter( 'the_content', 'convert_smilies', 20 );
add_filter( 'the_content', 'wpautop' );
add_filter( 'the_content', 'shortcode_unautop' );
add_filter( 'the_content', 'prepend_attachment' );
add_filter( 'the_content', 'wp_make_content_images_responsive' );
add_filter( 'the_content', 'do_shortcode', 11 );

only do_blocks, wptexturize, convert_smilies, and wp_make_content_images_responsive would be needed. The rest are handled per-block , and wpautop is (should be?) redundant.

[johnstonphilip]

@azaozz I tested removing each thing you mentioned. Here are the results of each:

Removing wpautop (Results in problem ❌)

Result: Results in broken double-line-to-paragraphs in the header.html output
Conclusion: It should remain here.
Screenshots:

Removing prepend_attachment (Seems to be okay ✅)

Result: Nothing seems to break with this removed, since this is targeted to viewing attachment posts. I can't think of a scenario where you would be setting the global $post to be an attachment ID within a post_template_part, but if anyone can think of a reason that might need to happen, that would be an argument for this staying here.
Conclusion: It can be removed.

Removing do_shortcode (Results in problem ❌)

Result: Results in broken shortcodes in post_template_part(s)
Conclusion: It should remain here.
Screenshots:

In conclusion, I think we can remove prepend_attachment, but none of the other filters. I will submit a PR for that and follow up here.

[epiqueras]

Thanks for the sleuthing!

[azaozz]

I tested removing each thing you mentioned.

Yeah, this is partially a "review" of how things work or don't work in template parts. You're right, here's not the best place. Haven't had the time yet, but will definitely try to find the discussion(s) for these.

Removing wpautop: Results in broken double-line-to-paragraphs in the header.html

If double line breaks have to be converted to paragraph tags, why would you expect that to happen dynamically on the front-end? This is intended for when editing HTML in plain textarea (no JavaScript involved at all). Is it even possible to do in a block inside the editor?

Removing do_shortcode: Results in broken shortcodes in post_template_part(s)

Same thing, why would template parts support this ancient, buggy, inefficient way to bypass dynamic blocks?

Both wpautop and shortcodes have to work in post_content for historical reasons. But they don't make much sense outside of post_content. Supporting them in template HTML doesn't seem good.

[johnstonphilip]

@azaozz Good points. Looking at it again, I agree about wpautop.

But for shortcodes, unless we remove the shortcode block from Gutenberg, people can still use shortcodes, and that will include in their header. Without calling do_shortcode, shortcodes in the shortcode block do not render.

For wpautop, I tested the Classic Editor block to make sure it wouldn't require wpautop, and it does not, as it converted my multi-line breaks to <p> before saving to the db.

I don't think there are any other "legacy" style blocks that would require wpautop either, so I think we should remove wpautop.

[azaozz]

I don't think there are any other "legacy" style blocks that would require wpautop either

Yeah, don't think autop is used anywhere else. (Also thinking it's about time to review the classic block and remove it from there too. The part that strips the <p> tags is used in only one edge case as far as I remember).

But for shortcodes, unless we remove the shortcode block from Gutenberg...

The shortcode block is back-compat for when editing post_content. Don't see a good reason for it to be supported in any other context. Don't think it can be removed from Gutenberg (at least not yet) but thinking it doesn't make sense for template parts. There's a way to filter the available blocks according to context (if I remember right).

[johnstonphilip]

@azaozz after giving it a lot of thought and discussing with a few people, I agree that shortcodes could be removed from block template parts.

I took a shot at that here:
#20452

However, I was not able to find a way to conditionally filter the available blocks based on context like you described.

There's a way to filter the available blocks according to context (if I remember right).

[epiqueras]

blockType.parent lets you define the set of blocks where a block should be allowed, and there were proposals for it to support negatives like !core/group, but nothing concrete.

[youknowriad]

I noticed that this triggers a warning on WordPress trunk

We should probably update to use the latest behavior instead

[johnstonphilip]

@youknowriad PR opened: #21514