Skip to content

Patterns: Don't inject theme attr on frontend #5455

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

Closed
wants to merge 10 commits into from
Closed

Patterns: Don't inject theme attr on frontend #5455

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
909ec6b
Patterns: Don't inject theme attr on frontend
ockham Oct 10, 2023
8fb2f79
Remove _inject_theme_attribute_in_template_part_block() and all its u…
felixarntz Oct 11, 2023
9751e1c
Revert "Remove _inject_theme_attribute_in_template_part_block() and a…
felixarntz Oct 11, 2023
9315aa3
Only call _inject_theme_attribute_in_template_part_block() if a REST …
felixarntz Oct 11, 2023
5932fba
Only call _inject_theme_attribute_in_template_part_block() if a REST …
felixarntz Oct 11, 2023
5c27373
Avoid duplicate code.
felixarntz Oct 11, 2023
44d7eab
Remove tests which are no longer applicable.
felixarntz Oct 11, 2023
14f6306
Implement test coverage to ensure template part block works correctly…
felixarntz Oct 11, 2023
2af8248
Use `!== null` equality checks
ockham Oct 12, 2023
17ca37a
Use `!== null` equality checks once more
ockham Oct 12, 2023
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
9 changes: 6 additions & 3 deletions src/wp-includes/block-template-utils.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -549,15 +549,18 @@ function _build_block_template_result_from_file( $template_file, $template_type
$template->area = $template_file['area'];
}

$before_block_visitor = '_inject_theme_attribute_in_template_part_block';
$before_block_visitor = ( defined( 'REST_REQUEST' ) && REST_REQUEST ) ? '_inject_theme_attribute_in_template_part_block' : null;
$after_block_visitor = null;
$hooked_blocks = get_hooked_blocks();
if ( ! empty( $hooked_blocks ) || has_filter( 'hooked_block_types' ) ) {
$before_block_visitor = make_before_block_visitor( $hooked_blocks, $template );
$after_block_visitor = make_after_block_visitor( $hooked_blocks, $template );
}
$blocks = parse_blocks( $template_content );
$template->content = traverse_and_serialize_blocks( $blocks, $before_block_visitor, $after_block_visitor );
if ( null !== $before_block_visitor || null !== $after_block_visitor ) {
$blocks = parse_blocks( $template_content );
$template_content = traverse_and_serialize_blocks( $blocks, $before_block_visitor, $after_block_visitor );
}
$template->content = $template_content;

return $template;
}
Expand Down
4 changes: 3 additions & 1 deletion src/wp-includes/blocks.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -779,7 +779,9 @@ function make_before_block_visitor( $hooked_blocks, $context ) {
* @return string The serialized markup for the given block, with the markup for any hooked blocks prepended to it.
*/
return function ( &$block, $parent_block = null, $prev = null ) use ( $hooked_blocks, $context ) {
_inject_theme_attribute_in_template_part_block( $block );
if ( defined( 'REST_REQUEST' ) && REST_REQUEST ) {
_inject_theme_attribute_in_template_part_block( $block );
}

$markup = '';

Expand Down
8 changes: 5 additions & 3 deletions src/wp-includes/class-wp-block-patterns-registry.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -165,14 +165,16 @@ public function unregister( $pattern_name ) {
private function prepare_content( $pattern, $hooked_blocks ) {
$content = $pattern['content'];

$before_block_visitor = '_inject_theme_attribute_in_template_part_block';
$before_block_visitor = ( defined( 'REST_REQUEST' ) && REST_REQUEST ) ? '_inject_theme_attribute_in_template_part_block' : null;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

We might consider moving the code that injects the theme attr to the patterns controller instead. Probably okay to leave as-is for now, but might be a nice follow-up post-6.4.

(Noting that if we still insert hooked blocks here, we'd be running traverse_and_serialize_blocks twice, which is suboptimal; so we'd also need to move hooked blocks insertion to the controller so we can remove it here; which would then require us to make sure it's run on the frontend. We can probably achieve that through running it in the Pattern block's render_block. This means that we'd need to make quite a few changes, which we most likely don't want to do before 6.4.)

Copy link
Member

Choose a reason for hiding this comment

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

Can you explain why traverse_and_serialize_blocks would get called twice?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Can you explain why traverse_and_serialize_blocks would get called twice?

Assuming that we inject the theme attr in the patterns controller, but keep hooked blocks insertion here in the patterns registry (since it's needed both for the frontend and editor (i.e. REST API)) -- both of these call traverse_and_serialize_blocks 😅

Copy link
Member

Choose a reason for hiding this comment

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

I see what you mean here - If we were to run the theme attribute injection in the REST API controller. Yes, good point that it would get called twice with different processing 👍🏻

$after_block_visitor = null;
if ( ! empty( $hooked_blocks ) || has_filter( 'hooked_block_types' ) ) {
$before_block_visitor = make_before_block_visitor( $hooked_blocks, $pattern );
$after_block_visitor = make_after_block_visitor( $hooked_blocks, $pattern );
}
$blocks = parse_blocks( $content );
$content = traverse_and_serialize_blocks( $blocks, $before_block_visitor, $after_block_visitor );
if ( null !== $before_block_visitor || null !== $after_block_visitor ) {
$blocks = parse_blocks( $content );
$content = traverse_and_serialize_blocks( $blocks, $before_block_visitor, $after_block_visitor );
}

return $content;
}
Expand Down
58 changes: 0 additions & 58 deletions tests/phpunit/tests/block-template-utils.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -161,64 +161,6 @@ public function test_build_block_template_result_from_file() {
$this->assertEmpty( $template_part->modified );
}

/**
* @ticket 59325
*
* @covers ::_build_block_template_result_from_file
*
* @dataProvider data_build_block_template_result_from_file_injects_theme_attribute
*
* @param string $filename The template's filename.
* @param string $expected The expected block markup.
*/
public function test_build_block_template_result_from_file_injects_theme_attribute( $filename, $expected ) {
$template = _build_block_template_result_from_file(
array(
'slug' => 'single',
'path' => DIR_TESTDATA . "/templates/$filename",
),
'wp_template'
);
$this->assertSame( $expected, $template->content );
}

/**
* Data provider.
*
* @return array[]
*/
public function data_build_block_template_result_from_file_injects_theme_attribute() {
$theme = 'block-theme';
return array(
'a template with a template part block' => array(
'filename' => 'template-with-template-part.html',
'expected' => sprintf(
'<!-- wp:template-part {"slug":"header","align":"full","tagName":"header","className":"site-header","theme":"%s"} /-->',
$theme
),
),
'a template with a template part block nested inside another block' => array(
'filename' => 'template-with-nested-template-part.html',
'expected' => sprintf(
'<!-- wp:group -->
<!-- wp:template-part {"slug":"header","align":"full","tagName":"header","className":"site-header","theme":"%s"} /-->
<!-- /wp:group -->',
$theme
),
),
'a template with a template part block with an existing theme attribute' => array(
'filename' => 'template-with-template-part-with-existing-theme-attribute.html',
'expected' => '<!-- wp:template-part {"slug":"header","theme":"fake-theme","align":"full","tagName":"header","className":"site-header"} /-->',
),
'a template with no template part block' => array(
'filename' => 'template.html',
'expected' => '<!-- wp:paragraph -->
<p>Just a paragraph</p>
<!-- /wp:paragraph -->',
),
);
}

/**
* @ticket 59338
*
Expand Down
121 changes: 121 additions & 0 deletions tests/phpunit/tests/blocks/renderBlockCoreTemplatePart.php
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,121 @@
<?php
/**
* Tests for `render_block_core_template_part()`.
*
* @package WordPress
* @subpackage Blocks
*
* @group blocks
*/
class Tests_Blocks_RenderBlockCoreTemplatePart extends WP_UnitTestCase {

/**
* Temporary storage for original block to render.
*
* @var array|null
*/
private $original_block_to_render = null;

/**
* Flag to indicate whether to switch back to the default theme at tear down.
*
* @var bool
*/
private $switch_to_default_theme_at_teardown = false;

public function set_up() {
parent::set_up();

$this->original_block_to_render = WP_Block_Supports::$block_to_render;
}

public function tear_down() {
WP_Block_Supports::$block_to_render = $this->original_block_to_render;
$this->original_block_to_render = null;

if ( $this->switch_to_default_theme_at_teardown ) {
$this->switch_to_default_theme_at_teardown = false;
switch_theme( WP_DEFAULT_THEME );
}

parent::tear_down();
}

/**
* Tests that the core template part block assumes the current theme if no theme attribute provided.
*
* @ticket 59583
*/
public function test_render_block_core_template_part_without_theme_attribute() {
$this->maybe_switch_theme( 'block-theme' );

WP_Block_Supports::$block_to_render = array( 'blockName' => 'core/template-part' );

$content = render_block_core_template_part( array( 'slug' => 'small-header' ) );

$expected = '<header class="wp-block-template-part">' . "\n";
$expected .= '<p>Small Header Template Part</p>' . "\n";
$expected .= '</header>';

$this->assertSame( $expected, $content );
}

/**
* Tests that the core template part block returns the relevant part if current theme attribute provided.
*
* @ticket 59583
*/
public function test_render_block_core_template_part_with_current_theme_attribute() {
$this->maybe_switch_theme( 'block-theme' );

WP_Block_Supports::$block_to_render = array( 'blockName' => 'core/template-part' );

$content = render_block_core_template_part(
array(
'slug' => 'small-header',
'theme' => 'block-theme',
)
);

$expected = '<header class="wp-block-template-part">' . "\n";
$expected .= '<p>Small Header Template Part</p>' . "\n";
$expected .= '</header>';

$this->assertSame( $expected, $content );
}

/**
* Tests that the core template part block returns nothing if theme attribute for a different theme provided.
*
* @ticket 59583
*/
public function test_render_block_core_template_part_with_another_theme_attribute() {
$this->maybe_switch_theme( 'block-theme' );

WP_Block_Supports::$block_to_render = array( 'blockName' => 'core/template-part' );

$content = render_block_core_template_part(
array(
'slug' => 'small-header',
'theme' => WP_DEFAULT_THEME,
)
);

$expected = 'Template part has been deleted or is unavailable: small-header';
$this->assertSame( $expected, $content );
}

/**
* Switches the theme when not the default theme.
*
* @param string $theme Theme name to switch to.
*/
private function maybe_switch_theme( $theme ) {
if ( WP_DEFAULT_THEME === $theme ) {
return;
}

switch_theme( $theme );
$this->switch_to_default_theme_at_teardown = true;
}
}
46 changes: 0 additions & 46 deletions tests/phpunit/tests/blocks/wpBlockPatternsRegistry.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -316,29 +316,6 @@ public function test_get_registered() {
$this->assertSame( $pattern_two, $pattern );
}

/**
* Should insert a theme attribute into Template Part blocks in registered patterns.
*
* @ticket 59583
*
* @covers WP_Block_Patterns_Registry::register
* @covers WP_Block_Patterns_Registry::get_all_registered
*/
public function test_get_all_registered_includes_theme_attribute() {
$test_pattern = array(
'title' => 'Test Pattern',
'content' => '<!-- wp:template-part {"slug":"header","align":"full","tagName":"header","className":"site-header"} /-->',
);
$this->registry->register( 'test/pattern', $test_pattern );

$expected = sprintf(
'<!-- wp:template-part {"slug":"header","align":"full","tagName":"header","className":"site-header","theme":"%s"} /-->',
get_stylesheet()
);
$patterns = $this->registry->get_all_registered();
$this->assertSame( $expected, $patterns[0]['content'] );
}

/**
* Should insert hooked blocks into registered patterns.
*
Expand Down Expand Up @@ -391,29 +368,6 @@ public function test_get_all_registered_includes_hooked_blocks() {
$this->assertSame( $expected, $registered );
}

/**
* Should insert a theme attribute into Template Part blocks in registered patterns.
*
* @ticket 59583
*
* @covers WP_Block_Patterns_Registry::register
* @covers WP_Block_Patterns_Registry::get_registered
*/
public function test_get_registered_includes_theme_attribute() {
$test_pattern = array(
'title' => 'Test Pattern',
'content' => '<!-- wp:template-part {"slug":"header","align":"full","tagName":"header","className":"site-header"} /-->',
);
$this->registry->register( 'test/pattern', $test_pattern );

$expected = sprintf(
'<!-- wp:template-part {"slug":"header","align":"full","tagName":"header","className":"site-header","theme":"%s"} /-->',
get_stylesheet()
);
$pattern = $this->registry->get_registered( 'test/pattern' );
$this->assertSame( $expected, $pattern['content'] );
}

/**
* Should insert hooked blocks into registered patterns.
*
Expand Down