Use a cache for block templates and block template parts content #5463
Conversation
There was a problem hiding this comment.
This seems like a good thing to do. I wonder if we would be better off in the long run saving these as Post Types rather than transients, to be consistent with the way these files get saved if they are manually edited in the site editor. Definitely out of scope for this fix however.
Ideally, I think we should add some intentional expiry to these transients to ensure they get cleaned up during transient garbage collection. We should also try to set them to not autoload, since we don't need all of this data on every pageload, only the template data that is in play for that pageload.
I'm also wondering if there is a way we can incorporate these enhancements with _get_block_templates_files, which is a previously noted performance botte neck (see https://core.trac.wordpress.org/ticket/58196#comment:26)
| $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 ) ); |
There was a problem hiding this comment.
Could use str_replace() here and below, similarly to how we're doing it in register_core_block_style_handles():
| $relative_path = substr( $template_file_path, strlen( $theme_dir ) ); | |
| $relative_path = str_replace( $theme_dir, '', $template_file_path ); |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
@joemcgill Let me clarify what I mean:
str_starts_with( '/dirname/dirname', '/dirname' )results intruesubstr( '/dirname/dirname', strlen( '/dirname' ) )results in the expected/dirnamestr_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 😂
Good catch! Rather than using an expiry though, which isn't needed given the name won't change (the version isn't part of the transient name), I think we should follow the same approach that is used for the recently introduced theme pattern cache (see https://core.trac.wordpress.org/changeset/56765): When switching the theme, delete the caches for the relevant themes. This will ensure a maximum of 2 of those transients are present (and autoloaded). I have added this in ad18c94.
This PR is actually already benefitting block templates: |
…uld be incorrectly cached.
There was a problem hiding this comment.
Thanks for the ping @felixarntz! A few thoughts for consideration, but no blockers that I see. Nice job with the test coverage! 👍
There was a problem hiding this comment.
I don't like this PR. I think is really problematic.
This would be a breaking change. This would REQUIRE users to enable development mode. This would not allow to upload new files to the server and for those to read.
I agree, we need to cache the file_get_contents, but this change would break a lot of people's sites that use FTP.
I am going to put together a PR with my idea of how we can fix this issue.
|
Just to reflect a conversion on slack. I am strongly against this change being merged into core in the WP 6.4 cycle. Caching the file_get_content has clear performance benefits, but this is not a bug fix, it is an enhancement. This should have it's own ticket and big discussed there. If this is every committed, it needs to be committed early in a release cycle, so it get tested. But at the moment, there is no cache invalidation, for files being uploaded via FTP. Uploading files via FTP is how many developers work on client sites. I didn't always update the theme version number when making changes. We can't break that workflow. |
The behavior here is exactly the same as the pattern caching change that you committed last week after us agreeing on it. I acknowledge that this led to a bug, but the problem of loading non-existing files isn't possible here, since this cache literally caches the file contents. Related to #5462 (comment), we should however ensure that any existing file caches are invalidated as soon as someone sets
Users that are making changes to a theme (except updating to a new version) are developing the theme and therefore should set
Again, this is similar to the pattern caching change that you committed last week. Can you please clarify why you think it is okay to commit that change this late in the cycle, but not the change here, even though they do something similar? I acknowledge that now it is even later in the beta cycle, and maybe it's too late. But the argument of this being an enhancement when the pattern caching change https://core.trac.wordpress.org/ticket/59490 was a bug makes no sense to me. |
|
This change is similar to but not exactly same as. Are filter run, block parsed and strings translated if we cache like this. Didn't we just have an issue where hooks were not applied? @ockham @gziolo can you give context here. Again, I will say, adding caching here is a good idea. But we could see a performance benefit from an in memory cache, like in my PR. If there is no breakage from the in memory cache, we could work at making that in memory cache persistent. Going straight to persistent, will likely bring up errors. Doing this so late in the process, is far to risky. I do think it something we can explore in WP 6.5. |
That is a fair point. We could add an expiration to the transient and thus not autoload it. That would lead to 1-2 more DB queries in the frontend, but those are way faster than all the
This change caches only |
|
@spacedmonkey I pushed two import changes related to your feedback:
|
|
@felixarntz Other than a performance improvement, what bug is this fixing again? Why does this need to go in RC? |
| $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 ); |
There was a problem hiding this comment.
This means this will not be autoloaded, is this by design.
There was a problem hiding this comment.
@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.
| @@ -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'] ); | |||
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
|
In my testing here are the size of autoload options after this cache was added.
I am worried about these cache growing large, for complex these. Autoloading a large option may worse then file operations. |
|
Was it considered, to prime this transient cache on theme activation. Saying by doing a glob on the /templates directory and saving the cache. This way caches would be loaded by a request in admin or wp cli and front end users could have to prime the caches. |
|
Added some people that worked on this function before. Hope we can get some more context. |
The options are no longer autoloaded based on your previous feedback. Please make sure to read my explanations in #5463 (comment).
On first thought, I like this idea. However, it wouldn't actually work because of theme updates. We will need to be able to refresh the caches on demand, to cover the scenario of the theme version being different. We can't hook into the update process for that since new theme versions are often deployed in other ways, e.g. Composer, git, FTP. |
|
Sorry for the delay in responding here yesterday. For the record, I'm still a very big fan of the idea of trying to avoid these types of redundant file parsing lookups during template rendering. That said, I think there are a few different approaches to consider here, like saving these files to a post type, different considerations of how transients get cached and loaded, and exploring methods of ensuring that cache busting could be in play for site owners who are directly editing their theme files on their live site 🙈🤠. To give us time to properly consider and test these scenarios, I think it's best that we consider this along with other template loading enhancements that have been discussed for 6.5 rather than trying to rush this into this release during the final week of the beta period before RC. Ideally, this (and things like it) should benefit from being in place at the start of the beta period. In the meantime, I'm retracting my original review as no longer applicable, since my original blocking concerns have been addressed. |
Using a cache for these file content lookups results in a major performance boost for block themes. Here are some Server-Timing benchmarks, using 100 runs with the
benchmark-server-timingtool (with the transient cache populated):The approach used here has already been established for block patterns (see https://core.trac.wordpress.org/ticket/59490 / https://core.trac.wordpress.org/changeset/56765), and it makes sense to apply it here as well.
Trac ticket: https://core.trac.wordpress.org/ticket/59600
This Pull Request is for code review only. Please keep all other discussion in the Trac ticket. Do not merge this Pull Request. See GitHub Pull Requests for Code Review in the Core Handbook for more details.