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

Use a cache for block templates and block template parts content #5463

Open
wants to merge 12 commits into
base: trunk
Choose a base branch
from
+366 −1
Open

Use a cache for block templates and block template parts content #5463

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
12 commits
Select commit Hold shift + click to select a range
d805e9c
Introduce function to get cached theme template file contents.
felixarntz Oct 11, 2023
b2408fc
Rename function to be specific to block templates.
felixarntz Oct 11, 2023
d718931
Merge branch 'trunk' into performance/theme-template-file-contents
felixarntz Oct 11, 2023
ad18c94
Clear template content caches when switching theme.
felixarntz Oct 11, 2023
fd3671f
Fix bug where theme slugs starting with same slug as another theme wo…
felixarntz Oct 11, 2023
af0de47
Add test coverage.
felixarntz Oct 11, 2023
0adf093
Merge branch 'trunk' into performance/theme-template-file-contents
felixarntz Oct 12, 2023
fa387c8
Add covers annotations.
felixarntz Oct 12, 2023
a262dab
Eliminate useless variable.
felixarntz Oct 12, 2023
ed704e6
Remove unnecessary detail from function description.
felixarntz Oct 12, 2023
4414d8b
Clear existing cache while in theme development mode.
felixarntz Oct 12, 2023
d0f971f
Add expiration to transient to prevent it from being autoloaded.
felixarntz 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
69 changes: 68 additions & 1 deletion src/wp-includes/block-template-utils.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -507,6 +507,73 @@ function _remove_theme_attribute_from_template_part_block( &$block ) {
}
}

/**
* Gets the contents for the given template file path.
*
* @since 6.4.0
* @access private
*
* @param string $template_file_path Absolute path to a theme template file.
* @return string The template file contents.
*/
function _get_block_template_file_content( $template_file_path ) {
$theme = wp_get_theme();

$template_file_path = wp_normalize_path( $template_file_path );
$theme_dir = wp_normalize_path( $theme->get_stylesheet_directory() ) . '/';

if ( str_starts_with( $template_file_path, $theme_dir ) ) {
$relative_path = substr( $template_file_path, strlen( $theme_dir ) );
Copy link
Member

Choose a reason for hiding this comment

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

Could use str_replace() here and below, similarly to how we're doing it in register_core_block_style_handles():

Suggested change
$relative_path = substr( $template_file_path, strlen( $theme_dir ) );
$relative_path = str_replace( $theme_dir, '', $template_file_path );

Copy link
Member Author

Choose a reason for hiding this comment

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

For this purpose, str_replace() is less precise than substr(), since we want to replace the beginning of the string only, not the partial path wherever it is included in the string.

This is obviously an edge case consideration, but has been pointed out to me in the past, and it makes sense. We shouldn't use str_replace() if we want to only replace a single specific instance of the substring.

Copy link
Member

Choose a reason for hiding this comment

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

Hmmm...even so, if $theme_dir is not at the beginning of the string, then the substr() function wouldn't remove what we intend to, which could be more confusing. You already have this inside an if statement that assures that $template_file_path starts with $theme_dir, so it really seems like both approaches are the same, with no possibility for edge cases to occur.

Copy link
Member Author

Choose a reason for hiding this comment

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

@joemcgill Let me clarify what I mean:

  • str_starts_with( '/dirname/dirname', '/dirname' ) results in true
  • substr( '/dirname/dirname', strlen( '/dirname' ) ) results in the expected /dirname
  • str_replace( '/dirname', '', '/dirname/dirname' ) results in '', which would be incorrect

Obviously this is not realistic to happen here since a file wouldn't be within a directory within the theme that has the same folder path as the overall theme directory. But if anything, it's safer to use substr() here over str_replace() to prevent it. IMO str_replace() is less precise since the intention here is to replace the instance of $theme that the string starts with, not any instance of it within the string.

Anyway, this is a nit-pick discussion 😂

} elseif ( $theme->parent() ) {
$theme = $theme->parent();
$theme_dir = wp_normalize_path( $theme->get_stylesheet_directory() ) . '/';
if ( str_starts_with( $template_file_path, $theme_dir ) ) {
$relative_path = substr( $template_file_path, strlen( $theme_dir ) );
}
}

// Bypass cache if the file is not within the theme directory.
if ( ! isset( $relative_path ) ) {
return file_get_contents( $template_file_path );
}

/*
* Bypass cache while developing a theme.
* If there is an existing cache, it should be deleted.
* This ensures that no stale cache values can be served when temporarily
* enabling "theme" development mode and then disabling it again.
*/
if ( wp_is_development_mode( 'theme' ) ) {
$template_data = get_transient( 'wp_theme_template_contents_' . $theme->get_stylesheet() );
if ( false !== $template_data ) {
delete_transient( 'wp_theme_template_contents_' . $theme->get_stylesheet() );
}

return file_get_contents( $template_file_path );
}

// Check theme template cache first (if cached version matches the current theme version).
$template_data = get_transient( 'wp_theme_template_contents_' . $theme->get_stylesheet() );
if ( is_array( $template_data ) && $template_data['version'] === $theme->get( 'Version' ) ) {
if ( isset( $template_data['template_content'][ $relative_path ] ) ) {
return $template_data['template_content'][ $relative_path ];
}
felixarntz marked this conversation as resolved.
Show resolved Hide resolved
} else {
$template_data = array(
'version' => $theme->get( 'Version' ),
'template_content' => array(),
);
}

// Retrieve fresh file contents if not found in cache.
$template_data['template_content'][ $relative_path ] = file_get_contents( $template_file_path );

// Update the cache.
set_transient( 'wp_theme_template_contents_' . $theme->get_stylesheet(), $template_data, WEEK_IN_SECONDS );
Copy link
Member

Choose a reason for hiding this comment

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

This means this will not be autoloaded, is this by design.

Copy link
Member Author

Choose a reason for hiding this comment

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

@spacedmonkey I explained this here: #5463 (comment)

One of your concerns was that these transients may become large and therefore shouldn't be autoloaded. I made this change in response to your feedback.


return $template_data['template_content'][ $relative_path ];
}

/**
* Builds a unified template object based on a theme file.
*
Expand All @@ -520,7 +587,7 @@ function _remove_theme_attribute_from_template_part_block( &$block ) {
*/
function _build_block_template_result_from_file( $template_file, $template_type ) {
$default_template_types = get_default_block_template_types();
$template_content = file_get_contents( $template_file['path'] );
$template_content = _get_block_template_file_content( $template_file['path'] );
Copy link
Member

Choose a reason for hiding this comment

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

Instead of adding caching to to the file get, why don't we just save this data to the post table. This is how block themes work. You have files stored and once you edit them and save changes, it copies the changes to a post. Why not just copy the data to post on activation or first use.

Copy link
Member Author

Choose a reason for hiding this comment

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

I'm not sure that that is possible.

Only modified templates and template parts are saved in the posts table. If we do that for every template file in the theme, it will break existing functionality, as it will look like every template has been modified by the user. So that would not be feasible.

$theme = get_stylesheet();

$template = new WP_Block_Template();
Expand Down
6 changes: 6 additions & 0 deletions src/wp-includes/theme.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -877,6 +877,12 @@ function switch_theme( $stylesheet ) {
$new_theme->delete_pattern_cache();
$old_theme->delete_pattern_cache();

// Clear template content caches.
delete_transient( 'wp_theme_template_contents_' . $new_theme->get_stylesheet() );
delete_transient( 'wp_theme_template_contents_' . $new_theme->get_template() );
delete_transient( 'wp_theme_template_contents_' . $old_theme->get_stylesheet() );
delete_transient( 'wp_theme_template_contents_' . $old_theme->get_template() );

/**
* Fires after the theme is switched.
*
Expand Down
292 changes: 292 additions & 0 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 @@ -86,6 +86,12 @@ public function set_up() {
switch_theme( self::TEST_THEME );
}

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

unset( $GLOBALS['_wp_tests_development_mode'] );
}

public function test_build_block_template_result_from_post() {
$template = _build_block_template_result_from_post(
self::$template_post,
Expand Down Expand Up @@ -523,4 +529,290 @@ public function test_wp_generate_block_templates_export_file() {
}
$this->assertTrue( $has_html_files, 'contains at least one html file' );
}

/**
* Tests `_get_block_template_file_content()` with a file that is part of the current theme.
*
* This should store the file content in cache.
*
* @ticket 59600
felixarntz marked this conversation as resolved.
Show resolved Hide resolved
*
* @covers ::_get_block_template_file_content
*/
public function test_get_block_template_file_content_with_current_theme_file() {
switch_theme( 'block-theme' );

$template_file = DIR_TESTDATA . '/themedir1/block-theme/parts/small-header.html';

$content = _get_block_template_file_content( $template_file );
$this->assertSame( file_get_contents( $template_file ), $content, 'Unexpected file content' );

$cache_result = get_transient( 'wp_theme_template_contents_block-theme' );
$this->assertArrayHasKey( 'template_content', $cache_result, 'Invalid cache value' );
$this->assertArrayHasKey( 'parts/small-header.html', $cache_result['template_content'], 'File not set in cache' );
$this->assertSame( $content, $cache_result['template_content']['parts/small-header.html'], 'File has incorrect content in cache' );
}

/**
* Tests `_get_block_template_file_content()` with a file that is part of the current parent theme.
*
* This should store the file content in cache.
*
* @ticket 59600
*
* @covers ::_get_block_template_file_content
*/
public function test_get_block_template_file_content_with_current_parent_theme_file() {
switch_theme( 'block-theme-child' );

$template_file = DIR_TESTDATA . '/themedir1/block-theme/templates/index.html';

$content = _get_block_template_file_content( $template_file );
$this->assertSame( file_get_contents( $template_file ), $content, 'Unexpected file content' );

$cache_result = get_transient( 'wp_theme_template_contents_block-theme' );
$this->assertArrayHasKey( 'template_content', $cache_result, 'Invalid cache value' );
$this->assertArrayHasKey( 'templates/index.html', $cache_result['template_content'], 'File not set in cache' );
$this->assertSame( $content, $cache_result['template_content']['templates/index.html'], 'File has incorrect content in cache' );
}

/**
* Tests `_get_block_template_file_content()` with a file that is not part of the current theme.
*
* This should not set any cache.
*
* @ticket 59600
*
* @covers ::_get_block_template_file_content
*/
public function test_get_block_template_file_content_with_another_theme_file() {
switch_theme( 'block-theme-patterns' );

$template_file = DIR_TESTDATA . '/themedir1/block-theme-child/templates/page-1.html';

$content = _get_block_template_file_content( $template_file );
$this->assertSame( file_get_contents( $template_file ), $content, 'Unexpected file content' );

$cache_result = get_transient( 'wp_theme_template_contents_block-theme-patterns' );
$this->assertFalse( $cache_result, 'Cache unexpectedly set for current theme' );

$cache_result = get_transient( 'wp_theme_template_contents_block-theme' );
$this->assertFalse( $cache_result, 'Cache unexpectedly set for current parent theme' );

$cache_result = get_transient( 'wp_theme_template_contents_block-theme-child' );
$this->assertFalse( $cache_result, 'Cache unexpectedly set for non-current theme' );
}

/**
* Tests `_get_block_template_file_content()` with a file that is part of the current theme while using 'theme' development mode.
*
* This should not set any cache.
*
* @ticket 59600
*
* @covers ::_get_block_template_file_content
*/
public function test_get_block_template_file_content_with_current_theme_file_and_theme_development_mode() {
global $_wp_tests_development_mode;

$_wp_tests_development_mode = 'theme';

switch_theme( 'block-theme' );

$template_file = DIR_TESTDATA . '/themedir1/block-theme/parts/small-header.html';

$content = _get_block_template_file_content( $template_file );
$this->assertSame( file_get_contents( $template_file ), $content, 'Unexpected file content' );

$cache_result = get_transient( 'wp_theme_template_contents_block-theme' );
$this->assertFalse( $cache_result, 'Cache unexpectedly set despite theme development mode' );
}

/**
* Tests `_get_block_template_file_content()` with files that are part of the current theme expands the existing cache.
*
* @ticket 59600
*
* @covers ::_get_block_template_file_content
*/
public function test_get_block_template_file_content_expands_existing_cache() {
switch_theme( 'block-theme' );

$template_file1 = DIR_TESTDATA . '/themedir1/block-theme/parts/small-header.html';
$template_file2 = DIR_TESTDATA . '/themedir1/block-theme/templates/index.html';
$template_file3 = DIR_TESTDATA . '/themedir1/block-theme/templates/page-home.html';

$content1 = _get_block_template_file_content( $template_file1 );
$content2 = _get_block_template_file_content( $template_file2 );
$content3 = _get_block_template_file_content( $template_file3 );

$cache_result = get_transient( 'wp_theme_template_contents_block-theme' );
$this->assertSame(
array(
'version' => '1.0.0',
'template_content' => array(
'parts/small-header.html' => $content1,
'templates/index.html' => $content2,
'templates/page-home.html' => $content3,
),
),
$cache_result
);
}

/**
* Tests `_get_block_template_file_content()` with a file that is part of the current theme relies on cached values.
*
* @ticket 59600
*
* @covers ::_get_block_template_file_content
*/
public function test_get_block_template_file_content_with_current_theme_file_relies_on_cache() {
switch_theme( 'block-theme' );

$template_file = DIR_TESTDATA . '/themedir1/block-theme/parts/small-header.html';
$forced_content = '<div>Some cache content that is not actually the file content.</div>';
set_transient(
'wp_theme_template_contents_block-theme',
array(
'version' => '1.0.0',
'template_content' => array(
'parts/small-header.html' => $forced_content,
),
),
WEEK_IN_SECONDS
);

$content = _get_block_template_file_content( $template_file );
$this->assertSame( $forced_content, $content );
}

/**
* Tests `_get_block_template_file_content()` with a file that is part of the another theme ignores cached values.
*
* @ticket 59600
*
* @covers ::_get_block_template_file_content
*/
public function test_get_block_template_file_content_with_another_theme_file_ignores_cache() {
switch_theme( 'block-theme' );

$template_file = DIR_TESTDATA . '/themedir1/block-theme-child/templates/page-1.html';
$forced_content = '<div>Some cache content that is not actually the file content.</div>';
set_transient(
'wp_theme_template_contents_block-theme-child',
array(
'version' => '1.0.0',
'template_content' => array(
'templates/page-1.html' => $forced_content,
),
),
WEEK_IN_SECONDS
);

$content = _get_block_template_file_content( $template_file );
$this->assertSame( file_get_contents( $template_file ), $content );
}

/**
* Tests `_get_block_template_file_content()` with a file that is part of the current theme refreshes existing cache when version is outdated.
*
* @ticket 59600
*
* @covers ::_get_block_template_file_content
*/
public function test_get_block_template_file_content_with_current_theme_file_refreshes_cache_when_version_outdated() {
switch_theme( 'block-theme' );

$template_file = DIR_TESTDATA . '/themedir1/block-theme/parts/small-header.html';
$forced_content = '<div>Some cache content that is not actually the file content.</div>';
set_transient(
'wp_theme_template_contents_block-theme',
array(
'version' => '0.9.0',
'template_content' => array(
'parts/small-header.html' => $forced_content,
),
),
WEEK_IN_SECONDS
);

$content = _get_block_template_file_content( $template_file );
$this->assertSame( file_get_contents( $template_file ), $content, 'Cached file content unexpectedly returned' );

$this->assertSame(
array(
'version' => '1.0.0',
'template_content' => array(
'parts/small-header.html' => $content,
),
),
get_transient( 'wp_theme_template_contents_block-theme' ),
'Cached transient was not updated'
);
}

/**
* Tests `_get_block_template_file_content()` with a file that is part of the current theme ignores cached values while using 'theme' development mode.
*
* @ticket 59600
*
* @covers ::_get_block_template_file_content
*/
public function test_get_block_template_file_content_with_current_theme_file_and_theme_development_mode_ignores_cache() {
global $_wp_tests_development_mode;

$_wp_tests_development_mode = 'theme';

switch_theme( 'block-theme' );

$template_file = DIR_TESTDATA . '/themedir1/block-theme/parts/small-header.html';
$forced_content = '<div>Some cache content that is not actually the file content.</div>';
set_transient(
'wp_theme_template_contents_block-theme',
array(
'version' => '1.0.0',
'template_content' => array(
'parts/small-header.html' => $forced_content,
),
),
WEEK_IN_SECONDS
);

$content = _get_block_template_file_content( $template_file );
$this->assertSame( file_get_contents( $template_file ), $content );
}

/**
* Tests `_get_block_template_file_content()` while using 'theme' development mode clears the existing cache for the current theme.
*
* @ticket 59600
*
* @covers ::_get_block_template_file_content
*/
public function test_get_block_template_file_content_with_theme_development_mode_clears_existing_cache() {
global $_wp_tests_development_mode;

$_wp_tests_development_mode = 'theme';

switch_theme( 'block-theme' );

$template_file = DIR_TESTDATA . '/themedir1/block-theme/parts/small-header.html';
set_transient(
'wp_theme_template_contents_block-theme',
array(
'version' => '1.0.0',
'template_content' => array(
'parts/small-header.html' => '<div>Some content.</div>',
),
),
WEEK_IN_SECONDS
);

// We don't care about the value here as it is already covered by the test above.
_get_block_template_file_content( $template_file );

// Ensure the relevant transient was deleted.
$this->assertFalse( get_transient( 'wp_theme_template_contents_block-theme' ) );
}
}