A stale register_core_block_style_handles cache can cause styles not to load

[joemcgill]

In [56044], a transient caching strategy was added to register_core_block_style_handles to avoid expensive file operations for core block styles. However, this can cause styles for blocks to not get registered if the normalized path for those assets change after the transient is saved.

To reproduce with the Twenty Twenty-three theme:

1. Make sure the transient is deleted: npm run env:cli wp transient delete wp_core_block_css_files
2. Make sure WP_DEVELOPMENT_MODE is not set to 'core'
3. Set LOCAL_DIR=src in .env
4. Build and start the environment npm run build:dev && npm run env:start
5. Load the homepage in your browser (styles should all be loaded)
6. Set LOCAL_DIR=build in env to switch to the build folder
7. Build and restart the environment npm run build && npm run env:restart
8. Refresh the homepage in your browser and see styles for the navigation block is not loaded
9. Set WP_DEVELOPMENT_MODE to core to see that the problem goes away.

A few options to consider here:

1. We could skip the transient if we determine the site is running in any development mode, not just core.
2. Rewrite the in_array check for relative paths based on the blocks directory, rather than the full normalized file path

[joemcgill]

This has also been reported in the support forums. See: ​https://wordpress.org/support/topic/website-breaks-when-changing-directory/

[lhe2012]

This sounds like the issue described in here https://core.trac.wordpress.org/ticket/59056.

○ Examples are shown in the PDF attached to the trac ticket
○ Here is the problem showing up at WPEngine's LOCAL Community ​https://community.localwp.com/t/importing-a-wp-6-3-exported-site-with-block-theme-results-in-css-problems-with-solution/38552

[coreyw]

This issue also occurs with "atomic" deployment methods that utilize symlinks to update the current document root (/srv/site/current -> /srv/site/releases/3, etc.). Since WordPress is saving the full absolute path in the database it causes this issue for every deployment.

I realize this is not standard WordPress install, but it is common, and best practice for applications. And I can't think of any reason where absolute paths would ever need be saved to the database.

For now we can run wp transient delete wp_core_block_css_files after each deployment.

[colorful tones]

This seems to be affecting quite a few migrations and copying of staging and production for Local and WP Engine customers. A temporary solution is to delete the wp_core_block_css_file options, or DELETE FROM wp_options WHERE option_name LIKE '%_transient_wp_core_block_css_files%';

A couple more details:

• When the transient is set ​in core, a duration is not passed which means the transient never expires.
• When the WordPress version is upgraded ​in core, the transient gets deleted.

This is relevant because it means the issue won’t resolve itself until WordPress core is upgraded or we take steps to manually delete the option. (Co-authored by @kevinwhoffman)

I'm hoping this may get addressed in the next release.

This PR propose a solution to fix the ticket 59111.

This is loosely based on the second option suggested, this PR will do additional change to the CSS files paths before storing and after retrieving them in the transient. Paths stored in the transient doesn't contain the path to the wp-includes folder in the server.

Trac ticket: https://core.trac.wordpress.org/ticket/59111

[mukesh27]

Thank you, @petitphp, for the PR. It appears quite robust to me, and it seems like it will effectively address the style issue, assuming I follow the steps outlined by @joemcgill. I've left some feedback on the PR for your consideration.

[mukesh27]

Performance benchmark shows some regression for wp-template.

	Before PR	After PR
wp-before-template (p10)	165.33	166.52
wp-before-template (p25)	167.94	167.76
wp-before-template (p50)	170.58	169.7
wp-before-template (p75)	173.52	171.82
wp-before-template (p90)	175.79	175.01
wp-template (p10)	85.19	110.62
wp-template (p25)	86.61	111.79
wp-template (p50)	88.34	113.63
wp-template (p75)	89.42	115.78
wp-template (p90)	91.07	117.64
wp-total (p10)	253.66	278.96
wp-total (p25)	256	281.16
wp-total (p50)	258.83	283.66
wp-total (p75)	261.91	286.78
wp-total (p90)	264.47	289.96

Thanks for the review and the suggestions @mukeshpanchal27 !

[petitphp]

Not super familiar with the performance benchmark.

I can look into the regression. Where should I look for information about the benchmark and can it run locally ?

[boogah]

Given that the new code is having to dynamically rebuild the full file path from the stored relative paths — in order to avoid the reported bug — a performance regression makes sense to me.

I'm willing to be wrong here, but I feel like a comparison of the benchmarks for WP 6.2.2 and WP 6.3.x (with this PR) is perhaps a fairer indication of if anything is actually regressing.

[petitphp]

#59223 was marked as a duplicate.

[mukesh27]

@petitphp Check ​Performance handbook for how you can measuring the performance.

@boogah Can't do that as register_core_block_style_handles function introduce in 6.3.

[spacedmonkey]

We could add a check in to see if path starts with includes_path and if not, mark the cache as invalid and force it generate again.

Alternative solution for 59111 based on the comment 17 from spacedmonkey.

This PR add a check after getting the value for the wp_core_block_css_files transient, to verify that the start of the files' path match the current includes path.

Trac ticket: https://core.trac.wordpress.org/ticket/59111

[petitphp]

@spacedmonkey Thanks for the input, I've made an alternative PR following your idea of checking the start of the file's path to know if the value is still valid or if it should be regenerated.

[ocean90]

This looks worth to get included in the next maintenance release.

[joemcgill]

Thanks for the PRs @petitphp! I think I slightly prefer the newest approach, but let me review in depth today and we can get this sorted.

[flixos90]

@joemcgill @petitphp I just reviewed the two PRs, I think ​https://github.com/WordPress/wordpress-develop/pull/5052 is a cleaner solution as it solves the problem at its root: The bug here is caused by the cache storing full paths. What it should store is only the paths starting below the WordPress includes directory, e.g. starting after wp-includes.

​https://github.com/WordPress/wordpress-develop/pull/5052 actually solves that while ​https://github.com/WordPress/wordpress-develop/pull/5112 is a workaround.

[joemcgill]

Thanks for reviewing, @flixos90. I generally agree with you. I was leaning towards ​PR5112 because even though it's a workaround, it avoids doing string manipulation twice on a large array of file paths, but I don't feel strongly about it. It would be nice to get some benchmarks on the two approaches but I suspect the difference is negligible.

[flixos90]

@joemcgill I left some feedback in regards to the string concatenation on the PR ​https://github.com/WordPress/wordpress-develop/pull/5052. I think we can reduce the number of times this happens, which should make the difference more negligible :)

[mukesh27]

I did some benchmarks and share the results for both the current trunk and the two PRs we've been working on. After conducting the benchmarks, it's evident that both PRs exhibit only slight differences in performance, and both approaches seem promising.

Here's a summary of the benchmark results:

	Trunk	​PR 5112	​PR 5052
wp-before-template (median)	157.4	155.56	155.43
wp-template (median)	103.6	102.83	103.07
wp-total (median)	261.13	258.16	258.28

As you can see, there are only marginal differences in the numbers. Both PRs appear to be viable options.

Additionally, I've also left an additional suggestion for PR 5052.

[joemcgill]

#59258 was marked as a duplicate.

[joemcgill]

There's a new detail to consider from #59258. @mmcalister reported that he was experiencing this issue after upgrading to 6.3.1, even though reverting to 6.3.0 fixed the issue. I'm wondering if some version specific info is being added to these file paths that we need to account for in our fix?

Also, thanks for running those benchmarks, @mukesh27. Given the similarity, I agree with @flixos90 that ​PR5052 is likely the best approach here. Let's keep working on that path.

[joemcgill]

@petitphp I've left an idea on your PR to address the issue @mmcalister raised in the other ticket, and also to make the caching strategy a bit more resilient overall. @flixos90 I'd appreciate your thoughts on what I proposed as well. I think this is very close to being ready to go in.

@joemcgill Your additional feedback makes sense to me, except for:

Add an expiry time of DAY_IN_SECONDS, which will allow this value to expire naturally. Setting an expiration will also result in this option not being autoload, which is probably ok.

I think we _should_ autoload this transient because it's going to be used on pretty much every page load. Introducing a new database request for this on every page load will likely nullify, or certainly reduce the benefit of using a cache in the first place.

By including the WordPress version in the cache key, we should have enough protections in place to not run into stale values, I think we should avoid an expiration.

@felixarntz

I think we should autoload this transient because it's going to be used on pretty much every page load.

Ideally, I agree. I'm fine with omitting the expiration time for now.

In the future, we could combine setting an expiration with also manually setting the autoload value once https://core.trac.wordpress.org/ticket/58964 is committed.

@joemcgill I agree with your feedback, I completely overlooked the case where the paths changed.

As @felixarntz said, It would be good to keep autoloading this transient. My main concern is that if we make the transient's name custom but don't add an expiration to the transient, this mean we'll keep autoloading all the previous versions of the transient in the future too since they will never be deleted.

To have the best of both (correctly handle change of files between versions but don't clutter autoload options), we can store the WordPress's version alongside the paths to be able to invalidate the value when WordPress is updated :

/*
         * Ignore transient cache when the development mode is set to 'core'. Why? To avoid interfering with
         * the core developer's workflow.
         */
        $files = false;
        if ( ! wp_is_development_mode( 'core' ) ) {
                $transient_name = 'wp_core_block_css_files';
                $cached_files      = get_transient( $transient_name );

                /*
                 * Check the validity of cached values.
                 *  - Should match the current WordPress version
                 *  - Should use relative paths for the files
                 */
                if ( is_array( $cached_files ) ) {
                        if ( $cached_files['version'] !== $wp_version ) {
                                $files = false;
                        } elseif ( ! str_starts_with( reset( $cached_files['files'] ), 'blocks/' ) ) {
                                $files = false;
                        } else {
                                $files = $cached_files['files'];
                        }
                }
        }

        if ( ! $files ) {
                $files = glob( wp_normalize_path( __DIR__ . '/**/**.css' ) );
                $files = array_map(
                        static function ( $file ) use ( $includes_path ) {
                                return str_replace( $includes_path, '', $file );
                        },
                        $files
                );

                // Save core block style paths in cache when not in development mode.
                if ( ! wp_is_development_mode( 'core' ) ) {
                        set_transient(
                                $transient_name,
                                array(
                                        'version' => $wp_version,
                                        'files' => $files
                                )
                        );
                }
        }

My main concern is that if we make the transient's name custom but don't add an expiration to the transient, this mean we'll keep autoloading all the previous versions of the transient in the future too since they will never be deleted.

Completely agree, @petitphp. My original suggestion assumed these transients would expire. Saving the version along with the files makes a lot of sense to me. Alternately, we could save the core_blocks_version to a separate transient that could be checked any place where we're caching block file data.

In your example above, I think you could combine all of the elseif clauses into the first if by chaining on additional || conditions, but this should address the issues we're aware of.

[kimannwall]

Facing a similar issue here, not related to staging and dev, but rather a production environment hosted by Pagely.

The transient has been deleted via CLI and continues to rebuild with paths to CSS files like:

/wordpress-versions/6.3.1/wp-includes/blocks/archives/editor.css

Which doesn't exist within the context of the site and cannot be used to build the final inline CSS needed.

[joemcgill]

Replying to kimannwall:

Facing a similar issue here, not related to staging and dev, but rather a production environment hosted by Pagely.

The transient has been deleted via CLI and continues to rebuild with paths to CSS files like:

/wordpress-versions/6.3.1/wp-includes/blocks/archives/editor.css

Which doesn't exist within the context of the site and cannot be used to build the final inline CSS needed.

Thanks for the context, @kimannwall. I'm not sure how the wp-includes location is being manipulated on your hosting platform, but the additional use case is helpful in terms of things to test against. I think the approach being worked on in ​PR5052 should fix this.

Since you're unable to set a development mode as a workaround, perhaps adding something like this filter as a plugin or your theme's functions.php file might work until a minor release is released with this fix:

<?php
add_filter( 'pre_transient_wp_core_block_css_files', '__return_empty_array' );

[desmith]

In Pagely's hosting containers, the web root has a bunch of symlinks to the "real" WP core files. The code is running in one place, but the core files are actually in the path @kimannwall referenced. Maybe something in this isn't properly dereferencing symbolic links and so is referring to the physical path of the WP core files? (I have a couple test sites and am seeing exactly the same thing that others are seeing.)

@joemcgill: As a short-term thing, what kind of performance problems or other negatives are there to defining the filter you described? (I wonder if this is something I should deploy for all of Pagely's customers, who would presumably have similar issues, presumably wrapped in a version check that only adds the filter for 6.3/6.3.1. But if a fix is imminent it may not be worth the effort, as opposed to waiting for a core fix.)

@joemcgill @felixarntz I've push an update which integrate your feedbacks :

• keep the generic transient name with no expiration to benefit from autoloading,
• handle transient invalidation after a WordPress update,
• fix original issue with the transient storing absolute path to the core blocks CSS paths,

I kept the str_starts_with check for the CSS paths, but since the array shape changed it might not be necessary anymore since existing transient will be invalidated.

[joemcgill]

Hi @desmith! Thanks for the additional context. I'm wondering if the fact that the function is referencing the __DIR__ constant, expecting it to be in the same path as the WPINC constant that is causing problems here? I'm not sure how quickly the next minor version will be released, so it would probably be a good idea to mitigate this problem on your platform in the meantime. There is a bit of a performance hit to not have those file paths cached, but that's a lesser problem than having missing styles, in my opinion.

If you have the ability to test out the latest proposed PR and confirm whether this fixes the issue, I'd appreciate it.

[desmith]

• The filter in comment 36 doesn't seem to address the problem - the transient is continually recreated, with the wrong values. (I did it as an mu-plugin, rather than adding it to the theme, but I wouldn't expect it to make a difference. To fix it for Pagely's customers it would have be part of an mu-plugin, since we're not generally interested in modifying customers' themes.)

• Patch 5052 doesn't seem to address the above. I did a manual DELETE FROM wp_options WHERE option_name LIKE '%_transient_wp_core_block_css_files%' to the DB, reloaded the page, and the transient was recreated with the physical path again (i.e. /wordpress-versions/6.3.1/wp-includes/whatever).

I can go into more detail on what life is like inside our containers if you think it'd be helpful, but the __DIR__ versus WPINC thing feels like a good path (pun totally intended) to go down.

[joemcgill]

Thanks for the follow-up, @desmith. Sounds to me like there may be some potential discrepancy between the path returned by the glob function and the path being removed from each file path prior to saving the transient due to symlinks. I posted a follow-up comment on the PR with a suggestion for how to use consistent paths everywhere these files are being referenced. I'd be grateful if you were able to confirm that this would fix the issue for you as well.

@joemcgill Thank you, I agree with your proposed path.

Regarding the concrete diff, I think there's one problem with it which is that it still relies on $includes_url in a way that in the register callback will lead to URLs like $includes_url . "{$name}/{$filename}{$suffix}.css", i.e. missing the "blocks" directory.

This should be trivial to fix though. I would suggest we get rid of the $includes_url variable and change it to something like $blocks_dir_url so that we have that set up equivalently to the BLOCKS_PATH constant. I think that would reduce the cognitive overhead needed to understand the logic. We could even have $blocks_dir_path = BLOCKS_PATH; to make it even more obvious.

Good catch. Yeah, we'll also need to update line 102, like so:

src/wp-includes/blocks/index.php

@@ -99 +99 @@
                        return;
                }
 
-               $wp_styles->add( $style_handle, $includes_url . $style_path );
+               $wp_styles->add( $style_handle, $includes_url . '/blocks/' . $style_path );
                $wp_styles->add_data( $style_handle, 'path', $path );
 
                $rtl_file = str_replace( "{$suffix}.css", "-rtl{$suffix}.css", $path );

I would suggest we get rid of the $includes_url variable and change it to something like $blocks_dir_url so that we have that set up equivalently to the BLOCKS_PATH constant. I think that would reduce the cognitive overhead needed to understand the logic. We could even have $blocks_dir_path = BLOCKS_PATH; to make it even more obvious.

Sorry @felixarntz. I missed this part of your suggestion. I think it would be fine to swap $includes_url for something like $blocks_url = includes_url() . 'blocks/'; and replace all other instances in the function. I don't think I would store the BLOCKS_PATH constant to another variable. We could define BLOCKS_URL at the top of the file if we really want to keep consistency?

@joemcgill I'm wary of defining a new global constant BLOCKS_URL for this purpose here, so if you prefer not to put BLOCKS_PATH into a variable, then I'd say let's go with $blocks_url and BLOCKS_PATH.

[desmith]

I've been working with @joemcgill on this (well, he's been doing the work, I'm just testing stuff in the Pagely environment), and the latest version of Joe's code appears to address this - the transient has the correct values (i.e. not full paths), and the graphic glitches that were present on Twenty Twenty-Three are no longer there.

One nice side effect of this change is that we can also add file sizes as in the cache. This saves an expensive file_size check. I put together a POC ​https://github.com/petitphp/wordpress-develop/pull/1. Something to explore later, but this is a good change.

I've applied the suggestions from yesterday to ​a new PR here, which consistently uses BLOCKS_PATH everywhere we are referencing a file path, rather than constructing a separate includes path, and swaps out $includes_url for a similarly named $blocks_url variable. Hoping to get this wrapped up today, if we can.

Thank @joemcgill, I've merged your changes.

[joemcgill]

In 56524:

Themes: Avoid stale caches for core block styles.

This fixes an issue introduced in [56044] in which the path to core block styles are cached to avoid expensive file operations on every page load. The original caching strategy is now modified so that only the path relative to the blocks location are stored, rather than the full path, since the path to the wp-includes folder can change after the value is generated. The new cached value also includes the current WordPress version value to ensure it is rebuilt when the version changes.

Props lhe2012, coreyw, colorful tones, petitphp, mukesh27, spacedmonkey, joemcgill, flixos90, kimannwall, desmith.
Fixes #59111.

Merged in r56524.

Closing in favor of #5052 (already merged).

[flixos90]

In 56528:

Themes: Avoid stale caches for core block styles.

This fixes an issue introduced in [56044] in which the path to core block styles are cached to avoid expensive file operations on every page load. The original caching strategy is now modified so that only the path relative to the blocks location are stored, rather than the full path, since the path to the wp-includes` folder can change after the value is generated. The new cached value also includes the current WordPress version value to ensure it is rebuilt when the version changes.

Props lhe2012, coreyw, colorful tones, petitphp, mukesh27, spacedmonkey, joemcgill, flixos90, kimannwall, desmith.
Merges [56524] to the 6.3 branch.
Fixes #59111.

[joemcgill]

In 56785:

Themes: Fix core block style paths on Windows.

This is a follow-up to [56528], which normalizes the BLOCKS_PATH for Windows prior to making paths relative for caches during the registration process. Prior to this change, incorrect file paths would lead to broken styles for core blocks on Windows.

Props wildworks, pbiron, flixos90, joemcgill.
Fixes #59489. See #59111.

[audrasjb]

In 56789:

Themes: Fix core block style paths on Windows.

This is a follow-up to [56528], which normalizes the BLOCKS_PATH for Windows prior to making paths relative for caches during the registration process. Prior to
this change, incorrect file paths would lead to broken styles for core blocks on Windows.

Props wildworks, pbiron, flixos90, joemcgill.
Merges [56785] to the 6.3 branch.
Fixes #59489. See #59111.