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

Template Parts: Fix loading issue #28088

Merged
merged 14 commits into from
Jan 14, 2021
Merged

Template Parts: Fix loading issue #28088

Show file tree
Hide file tree
Changes from 12 commits
Commits
Show all changes
14 commits
Select commit Hold shift + click to select a range
97f7636
Inject theme attribute in theme provided template content
jeyip Jan 11, 2021
18dd3d5
Add JS doc
jeyip Jan 11, 2021
dd37f48
Ensure that a theme attribute is not already assigned
jeyip Jan 11, 2021
0957e1c
Only serialize updated content
jeyip Jan 12, 2021
b3b4af6
Remove theme attribute from exported templates
jeyip Jan 12, 2021
d7cec12
Fix phpcs spacing issues
jeyip Jan 12, 2021
00dc839
Add _inject_theme_attribute_in_content specs
jeyip Jan 13, 2021
893ca0c
Assert equality between serialized content to ensure no changes to ma…
jeyip Jan 13, 2021
c04828e
Add test comments
jeyip Jan 13, 2021
6c7dc0a
Compare serialized blocks in block templates tests
jeyip Jan 13, 2021
2f50732
Add _remove_theme_attribute_in_content specs
jeyip Jan 14, 2021
3639d46
Check if the theme we're injecting is the current theme
jeyip Jan 14, 2021
a2ef1a3
Fix _remove_theme_attribute_from_content test title
jeyip Jan 14, 2021
52f61bf
Refactor new content serialization
jeyip Jan 14, 2021
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
45 changes: 43 additions & 2 deletions lib/full-site-editing/block-templates.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -105,6 +105,42 @@ function _gutenberg_get_template_files( $template_type ) {
return $template_files;
}

/**
* Parses wp_template content and injects the current theme's
* stylesheet as a theme attribute into each wp_template_part
*
* @param string $template_content serialized wp_template content.
* @param string $theme the active theme's stylesheet.
*
* @return string Updated wp_template content.
*/
function _inject_theme_attribute_in_content( $template_content, $theme ) {
$has_updated_content = false;
$new_content = '';
$template_blocks = parse_blocks( $template_content );

foreach ( $template_blocks as $key => $block ) {
if (
'core/template-part' === $block['blockName'] &&
! isset( $block['attrs']['theme'] ) &&
wp_get_theme()->get_stylesheet() === $theme
Copy link
Member

Choose a reason for hiding this comment

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

Would it make sense to extract this as a bail condition before the for loop? It seems to be independent of any template blocks data being looped over.

Going a bit further, based on this description
@param string $theme the active theme's stylesheet.

Can we drop this parameter from the function, given that we can obtain this with wp_get_theme()->get_stylesheet()? 🤔 It looks to me that if the passed $theme parameter that is different from wp_get_theme()->get_stylesheet() this function will essentially be a no-op.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Would it make sense to extract this as a bail condition before the for loop? It seems to be independent of any template blocks data being looped over.

If we keep $theme as a param, this makes sense.

Can we drop this parameter from the function, given that we can obtain this with wp_get_theme()->get_stylesheet()? 🤔 It looks to me that if the passed $theme parameter that is different from wp_get_theme()->get_stylesheet() this function will essentially be a no-op.

I mentioned something similar here #28088 (comment). It didn't generate a lot of discussion, but I prefer what you're describing.

) {
$template_blocks[ $key ]['attrs']['theme'] = $theme;
$has_updated_content = true;
}
}

if ( $has_updated_content ) {
foreach ( $template_blocks as $block ) {
$new_content = $new_content . serialize_block( $block );
jeyip marked this conversation as resolved.
Show resolved Hide resolved
}

return $new_content;
} else {
Copy link
Member

Choose a reason for hiding this comment

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

Nitpick: we can drop the else block here since if ends with return, and just return $template_content at top level.

Copy link
Contributor Author

@jeyip jeyip Jan 15, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

My mistake if this is a convention! I thought run composer format would catch it. I'm on board.

Copy link
Member

Choose a reason for hiding this comment

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

It might not be a written one but it's less keywords and nesting that achieves the same effect. 😄 Also that way it's immediately obvious that this function has a default return value while quickly scanning the code, at least to me.

return $template_content;
}
}

/**
* Build a unified template object based on a theme file.
*
Expand All @@ -115,12 +151,17 @@ function _gutenberg_get_template_files( $template_type ) {
*/
function _gutenberg_build_template_result_from_file( $template_file, $template_type ) {
$default_template_types = gutenberg_get_default_template_types();
$template_content = file_get_contents( $template_file['path'] );
$theme = wp_get_theme()->get_stylesheet();

if ( 'wp_template' === $template_type ) {
$template_content = _inject_theme_attribute_in_content( $template_content, $theme );
}

$theme = wp_get_theme()->get_stylesheet();
$template = new WP_Block_Template();
$template->id = $theme . '//' . $template_file['slug'];
$template->theme = $theme;
$template->content = file_get_contents( $template_file['path'] );
$template->content = $template_content;
$template->slug = $template_file['slug'];
$template->is_custom = false;
$template->type = $template_type;
Expand Down
35 changes: 35 additions & 0 deletions lib/full-site-editing/edit-site-export.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,38 @@
* @package gutenberg
*/

/**
* Parses wp_template content and injects the current theme's
Copy link
Member

Choose a reason for hiding this comment

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

Would a more accurate description here be that it removes the theme attribute, instead of stating that it injects a new one?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good catch. I agree.

* stylesheet as a theme attribute into each wp_template_part
*
* @param string $template_content serialized wp_template content.
*
* @return string Updated wp_template content.
*/
function _remove_theme_attribute_from_content( $template_content ) {
jeyip marked this conversation as resolved.
Show resolved Hide resolved
$has_updated_content = false;
$new_content = '';
$template_blocks = parse_blocks( $template_content );

foreach ( $template_blocks as $key => $block ) {
if ( 'core/template-part' === $block['blockName'] && isset( $block['attrs']['theme'] ) ) {
jeyip marked this conversation as resolved.
Show resolved Hide resolved
unset( $template_blocks[ $key ]['attrs']['theme'] );
$has_updated_content = true;
}
}

if ( $has_updated_content ) {
foreach ( $template_blocks as $block ) {
$new_content = $new_content . serialize_block( $block );
}

return $new_content;
} else {
Copy link
Member

Choose a reason for hiding this comment

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

Same as above.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yep. Sounds good 👍

return $template_content;
}
}


/**
* Output a ZIP file with an export of the current templates
* and template parts from the site editor, and close the connection.
Expand All @@ -24,6 +56,9 @@ function gutenberg_edit_site_export() {
// Load templates into the zip file.
$templates = gutenberg_get_block_templates();
foreach ( $templates as $template ) {
$updated_content = _remove_theme_attribute_from_content( $template['content'] );
Copy link
Member

Choose a reason for hiding this comment

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

No need for declaring $updated_content variable here since it's not reused below.

$template->content = _remove_theme_attribute_from_content( $template['content'] );

Copy link
Contributor Author

Choose a reason for hiding this comment

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

👍

$template->content = $updated_content;

$zip->addFromString(
'theme/block-templates/' . $template->slug . '.html',
$template->content
Expand Down
30 changes: 30 additions & 0 deletions phpunit/class-block-templates-test.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -74,6 +74,36 @@ function test_gutenberg_build_template_result_from_post() {
$this->assertEquals( 'wp_template', $template->type );
}

function test_inject_theme_attribute_in_content() {
$theme = get_stylesheet();
$content_without_theme_attribute = '<!-- wp:template-part {"slug":"header","align":"full", "tagName":"header","className":"site-header"} /-->';
$template_content = _inject_theme_attribute_in_content(
$content_without_theme_attribute,
$theme
);
$expected = sprintf(
'<!-- wp:template-part {"slug":"header","align":"full","tagName":"header","className":"site-header","theme":"%s"} /-->',
get_stylesheet()
);
$this->assertEquals( $expected, $template_content );

// Does not inject theme when there is an existing theme attribute.
$content_with_existing_theme_attribute = '<!-- wp:template-part {"slug":"header","theme":"fake-theme","align":"full", "tagName":"header","className":"site-header"} /-->';
$template_content = _inject_theme_attribute_in_content(
$content_with_existing_theme_attribute,
$theme
);
$this->assertEquals( $content_with_existing_theme_attribute, $template_content );

// Does not inject theme when there is no template part.
$content_with_no_template_part = '<!-- wp:post-content /-->';
$template_content = _inject_theme_attribute_in_content(
$content_with_no_template_part,
$theme
);
$this->assertEquals( $content_with_no_template_part, $template_content );
}

/**
* Should retrieve the template from the theme files.
*/
Expand Down
26 changes: 26 additions & 0 deletions phpunit/class-edit-site-export-test.php
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,26 @@
<?php
/**
* Test `gutenberg_edit_site_export` and its helper functions.
*
* @package Gutenberg
*/

class Edit_Site_Export_Test extends WP_UnitTestCase {
function test_inject_theme_attribute_in_content() {
jeyip marked this conversation as resolved.
Show resolved Hide resolved
$content_without_theme_attribute = '<!-- wp:template-part {"slug":"header","theme":"tt1-blocks","align":"full","tagName":"header","className":"site-header"} /-->';
$template_content = _remove_theme_attribute_from_content( $content_without_theme_attribute );
$expected = '<!-- wp:template-part {"slug":"header","align":"full","tagName":"header","className":"site-header"} /-->';
$this->assertEquals( $expected, $template_content );

// Does not modify content when there is no existing theme attribute.
$content_with_existing_theme_attribute = '<!-- wp:template-part {"slug":"header","align":"full","tagName":"header","className":"site-header"} /-->';
$template_content = _remove_theme_attribute_from_content( $content_with_existing_theme_attribute );
$this->assertEquals( $content_with_existing_theme_attribute, $template_content );

// Does not remove theme when there is no template part.
$content_with_no_template_part = '<!-- wp:post-content /-->';
$template_content = _remove_theme_attribute_from_content( $content_with_no_template_part );
$this->assertEquals( $content_with_no_template_part, $template_content );
}
}