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

Remove the_content filter for block_template_parts #20343

Merged
merged 2 commits into from
Feb 21, 2020
Merged

Remove the_content filter for block_template_parts #20343

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
8a3f02f
Remove the_content filter for block_template_parts
johnstonphilip Feb 20, 2020
729a613
Apply default filters before returning
johnstonphilip Feb 20, 2020
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
13 changes: 12 additions & 1 deletion packages/block-library/src/template-part/index.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,7 +32,18 @@ function render_block_core_template_part( $attributes ) {
if ( is_null( $content ) ) {
return 'Template Part Not Found';
}
return apply_filters( 'the_content', str_replace( ']]>', ']]>', $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 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?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@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.

Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

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)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@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

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

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.


// Run through the actions that are typically taken on the_content.
$content = do_blocks( $content );
$content = wptexturize( $content );
$content = convert_smilies( $content );
$content = wpautop( $content );
$content = shortcode_unautop( $content );
$content = prepend_attachment( $content );
$content = wp_make_content_images_responsive( $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 noticed that this triggers a warning on WordPress trunk

Capture d’écran 2020-04-09 à 5 23 32 PM

We should probably update to use the latest behavior instead

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@youknowriad PR opened: #21514

$content = do_shortcode( $content );

return str_replace( ']]>', ']]>', $content );
}

/**
Expand Down