In `register_core_block_style_handles` store CSS files with relative path in transient #5052

petitphp · 2023-08-22T21:31:44Z

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

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.

…path in transient

mukeshpanchal27

Thanks @petitphp for the PR. Overall look solid to me. Left some feedback for consideration.

src/wp-includes/blocks/index.php

Co-authored-by: Mukesh Panchal <mukeshpanchal27@users.noreply.github.com>

petitphp · 2023-08-23T12:19:56Z

Thanks for the review and the suggestions @mukeshpanchal27 !

felixarntz

Thank you @petitphp, this fix looks great to me!

felixarntz

My approval was a bit too fast :)

The approach here overall is solid, but I found one thing that needs to be addressed and one minor performance recommendation.

src/wp-includes/blocks/index.php

…elative paths

mukeshpanchal27

Thanks @petitphp for the update.

In line no. 31 could you please update code.

$core_blocks_meta = require ABSPATH . WPINC . '/blocks/blocks-json.php';

replace to

$core_blocks_meta = require BLOCKS_PATH . 'blocks-json.php';

felixarntz

@petitphp Looks great now! One minor follow-up, but not a big deal.

src/wp-includes/blocks/index.php

joemcgill

I really like this approach. The main thing we need to consider that has not yet been discussed is that the list of file paths will change between WP versions, which could cause breakage after an update. Furthermore, I think it would be safer for us to add an expiration time to this cache so a transient value that is corrupted for any reason would eventually expire and be rebuilt.

I also think we can restructure this a bit to remove some redundancy in the string replacing logic. Here's an example of what I'm thinking...

Add the WP version to the transient name, so a change in WP versions would result in a cache miss.
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.
Check dev mode before trying to retrieve the files from cache and again before setting the cache to avoid duplicated logic.

	/*
	 * 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_' . $wp_version;
		$files          = get_transient( $transient_name );

		// Ensure cached values use relative paths.
		if ( is_array( $files ) && ! str_starts_with( reset( $files ), 'blocks/' ) ) {
			$files = false;
		}
	}

	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, $files, DAY_IN_SECONDS );
	}

What do you think?

felixarntz · 2023-08-31T21:46:30Z

@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.

joemcgill · 2023-08-31T21:58:08Z

@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.

petitphp · 2023-09-01T07:56:42Z

@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 ( ! isset(  $cached_files['version'] || ! isset( $cached_files['files'] ) {
				$files = false;
			} elseif ( $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
				)
			);
		}
	}

joemcgill · 2023-09-01T14:48:20Z

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.

…WordPress versions

petitphp · 2023-09-04T12:56:56Z

@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.

mukeshpanchal27

Great work, @petitphp! It looks good from my perspective.

mukeshpanchal27 · 2023-09-05T05:53:07Z

src/wp-includes/blocks/index.php

+		$cached_files = get_transient( $transient_name );
+
+		/*
+		 * Check the validity of cached values.


Suggested change

* Check the validity of cached values.

* Validate the transient cached values.

Personally, I think the original reads a bit better here.

joemcgill

Thanks @petitphp the updates look good to me.

felixarntz

@petitphp Tiny bit of follow up feedback, but this is otherwise good to commit.

src/wp-includes/blocks/index.php

Co-authored-by: Felix Arntz <felixarntz@users.noreply.github.com>

joemcgill

Based on more info and testing from David Smith in this comment, it seems we've overlooked one assumption here, and it's that __DIR__ in the glob function will return a path that includes the $includes_path so it can be removed. However, if the files are symlinked, it appears this might not work as expected.

I think a safer, and more consistent thing for us to do is to replace all instances of $includes_path and the __DIR__ in the glob with references to BLOCKS_PATH, which would ensure all of these places are referencing the same path.

The diff would be something like this:

diff --git a/src/wp-includes/blocks/index.php b/src/wp-includes/blocks/index.php
index d4dfde75b3..6aaffef0f6 100644
--- a/src/wp-includes/blocks/index.php
+++ b/src/wp-includes/blocks/index.php
@@ -30,18 +30,17 @@ function register_core_block_style_handles() {
 		return;
 	}
 
-	$includes_url  = includes_url();
-	$includes_path = ABSPATH . WPINC . '/';
-	$suffix        = wp_scripts_get_suffix();
-	$wp_styles     = wp_styles();
-	$style_fields  = array(
+	$includes_url = includes_url();
+	$suffix       = wp_scripts_get_suffix();
+	$wp_styles    = wp_styles();
+	$style_fields = array(
 		'style'       => 'style',
 		'editorStyle' => 'editor',
 	);
 
 	static $core_blocks_meta;
 	if ( ! $core_blocks_meta ) {
-		$core_blocks_meta = require $includes_path . 'blocks/blocks-json.php';
+		$core_blocks_meta = require BLOCKS_PATH . 'blocks-json.php';
 	}
 
 	$files          = false;
@@ -68,10 +67,10 @@ function register_core_block_style_handles() {
 	}
 
 	if ( ! $files ) {
-		$files = glob( wp_normalize_path( __DIR__ . '/**/**.css' ) );
+		$files = glob( wp_normalize_path( BLOCKS_PATH . '**/**.css' ) );
 		$files = array_map(
-			static function ( $file ) use ( $includes_path ) {
-				return str_replace( $includes_path, '', $file );
+			static function ( $file ) {
+				return str_replace( BLOCKS_PATH, '', $file );
 			},
 			$files
 		);
@@ -88,9 +87,9 @@ function register_core_block_style_handles() {
 		}
 	}
 
-	$register_style = static function( $name, $filename, $style_handle ) use ( $includes_path, $includes_url, $suffix, $wp_styles, $files ) {
-		$style_path = "blocks/{$name}/{$filename}{$suffix}.css";
-		$path       = wp_normalize_path( $includes_path . $style_path );
+	$register_style = static function( $name, $filename, $style_handle ) use ( $includes_url, $suffix, $wp_styles, $files ) {
+		$style_path = "{$name}/{$filename}{$suffix}.css";
+		$path       = wp_normalize_path( BLOCKS_PATH . $style_path );
 
 		if ( ! in_array( $style_path, $files, true ) ) {
 			$wp_styles->add(

felixarntz · 2023-09-05T22:08:32Z

@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.

joemcgill · 2023-09-05T22:22:45Z

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

diff --git a/src/wp-includes/blocks/index.php b/src/wp-includes/blocks/index.php
index 6aaffef0f6..270b2bb0a4 100644
--- a/src/wp-includes/blocks/index.php
+++ b/src/wp-includes/blocks/index.php
@@ -99,7 +99,7 @@ function register_core_block_style_handles() {
                        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 );

joemcgill · 2023-09-05T22:59:37Z

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?

felixarntz · 2023-09-05T23:07:39Z

@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.

spacedmonkey · 2023-09-06T07:44:55Z

src/wp-includes/blocks/index.php

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


Suggested change

$files = glob( wp_normalize_path( __DIR__ . '/**/**.css' ) );

$files = array_map(

static function ( $file ) use ( $includes_path ) {

return str_replace( $includes_path, '', $file );

},

$files

);

$raw_files = glob( wp_normalize_path( __DIR__ . '/**/**.css' ) );

$files = array();

foreach( $raw_files as $file ){

$files[] = str_replace( $includes_path, '', $file );

}

This feels a little more readable.

I don't feel strongly about this, but array_map seems totally fine to me. It's used already in several places in core and is a syntax that is fairly common in the broader PHP ecosystem.

spacedmonkey · 2023-09-06T14:51:14Z

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 petitphp#1. Something to explore later, but this is a good change.

joemcgill · 2023-09-06T15:16:32Z

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.

Issue 59111 updates

petitphp · 2023-09-06T17:25:34Z

Thank @joemcgill, I've merged your changes.

joemcgill

Thanks @petitphp. All blockers from my perspective are now resolved. Really appreciate you sticking with us through all the iterations!

joemcgill · 2023-09-06T18:08:19Z

Merged in r56524.

In register_core_block_style_handles store CSS files with relative …

ea48e02

…path in transient

mukeshpanchal27 reviewed Aug 23, 2023

View reviewed changes

src/wp-includes/blocks/index.php Outdated Show resolved Hide resolved

src/wp-includes/blocks/index.php Outdated Show resolved Hide resolved

petitphp and others added 2 commits August 23, 2023 14:19

Update src/wp-includes/blocks/index.php

51c8785

Co-authored-by: Mukesh Panchal <mukeshpanchal27@users.noreply.github.com>

Update src/wp-includes/blocks/index.php

e8c978b

Co-authored-by: Mukesh Panchal <mukeshpanchal27@users.noreply.github.com>

felixarntz approved these changes Aug 30, 2023

View reviewed changes

felixarntz requested changes Aug 30, 2023

View reviewed changes

src/wp-includes/blocks/index.php Outdated Show resolved Hide resolved

src/wp-includes/blocks/index.php Show resolved Hide resolved

petitphp added 2 commits August 31, 2023 00:53

Switch to using relative paths for the $files array

d93cd32

Invalidate wp_core_block_css_files if the stored values don't use r…

f6cbed7

…elative paths

mukeshpanchal27 reviewed Aug 31, 2023

View reviewed changes

Update $core_blocks_meta to use BLOCKS_PATH constant

a6db561

felixarntz reviewed Aug 31, 2023

View reviewed changes

src/wp-includes/blocks/index.php Show resolved Hide resolved

felixarntz approved these changes Aug 31, 2023

View reviewed changes

joemcgill reviewed Aug 31, 2023

View reviewed changes

Refactor core blocks css files cache to handle paths changes between …

2fb7374

…WordPress versions

petitphp requested review from joemcgill and felixarntz September 4, 2023 12:42

mukeshpanchal27 approved these changes Sep 5, 2023

View reviewed changes

joemcgill approved these changes Sep 5, 2023

View reviewed changes

felixarntz reviewed Sep 5, 2023

View reviewed changes

src/wp-includes/blocks/index.php Outdated Show resolved Hide resolved

src/wp-includes/blocks/index.php Outdated Show resolved Hide resolved

src/wp-includes/blocks/index.php Outdated Show resolved Hide resolved

felixarntz reviewed Sep 5, 2023

View reviewed changes

src/wp-includes/blocks/index.php Outdated Show resolved Hide resolved

src/wp-includes/blocks/index.php Outdated Show resolved Hide resolved

petitphp and others added 2 commits September 5, 2023 20:35

CR suggestions

68b1cad

Co-authored-by: Felix Arntz <felixarntz@users.noreply.github.com>

Fix phpcs issue

87f45a0

joemcgill requested changes Sep 5, 2023

View reviewed changes

Test block path constant

bf59f36

Fix URL path during registration

935f3fa

spacedmonkey reviewed Sep 6, 2023

View reviewed changes

Swap $includes_url for $block_url

6879a59

Merge pull request #2 from joemcgill/issue/59111-test

dc69faf

Issue 59111 updates

joemcgill approved these changes Sep 6, 2023

View reviewed changes

joemcgill closed this Sep 6, 2023

This was referenced Sep 6, 2023

Invalidate wp_core_block_css_files transient when files' path and includes path doesn't match #5112

Closed

Cache file sizes. petitphp/wordpress-develop#1

Draft

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

In `register_core_block_style_handles` store CSS files with relative path in transient #5052

In `register_core_block_style_handles` store CSS files with relative path in transient #5052

petitphp commented Aug 22, 2023

mukeshpanchal27 left a comment

petitphp commented Aug 23, 2023

felixarntz left a comment

felixarntz left a comment

mukeshpanchal27 left a comment

felixarntz left a comment

joemcgill left a comment

felixarntz commented Aug 31, 2023

joemcgill commented Aug 31, 2023

petitphp commented Sep 1, 2023 •

edited

Loading

joemcgill commented Sep 1, 2023

petitphp commented Sep 4, 2023

mukeshpanchal27 left a comment

mukeshpanchal27 Sep 5, 2023

joemcgill Sep 5, 2023

joemcgill left a comment

felixarntz left a comment

joemcgill left a comment

felixarntz commented Sep 5, 2023

joemcgill commented Sep 5, 2023

joemcgill commented Sep 5, 2023

felixarntz commented Sep 5, 2023

spacedmonkey Sep 6, 2023

joemcgill Sep 6, 2023

spacedmonkey commented Sep 6, 2023

joemcgill commented Sep 6, 2023

petitphp commented Sep 6, 2023

joemcgill left a comment

joemcgill commented Sep 6, 2023

	* Check the validity of cached values.
	* Validate the transient cached values.

In register_core_block_style_handles store CSS files with relative path in transient #5052

In register_core_block_style_handles store CSS files with relative path in transient #5052

Conversation

petitphp commented Aug 22, 2023

mukeshpanchal27 left a comment

Choose a reason for hiding this comment

petitphp commented Aug 23, 2023

felixarntz left a comment

Choose a reason for hiding this comment

felixarntz left a comment

Choose a reason for hiding this comment

mukeshpanchal27 left a comment

Choose a reason for hiding this comment

felixarntz left a comment

Choose a reason for hiding this comment

joemcgill left a comment

Choose a reason for hiding this comment

felixarntz commented Aug 31, 2023

joemcgill commented Aug 31, 2023

petitphp commented Sep 1, 2023 • edited Loading

joemcgill commented Sep 1, 2023

petitphp commented Sep 4, 2023

mukeshpanchal27 left a comment

Choose a reason for hiding this comment

mukeshpanchal27 Sep 5, 2023

Choose a reason for hiding this comment

joemcgill Sep 5, 2023

Choose a reason for hiding this comment

joemcgill left a comment

Choose a reason for hiding this comment

felixarntz left a comment

Choose a reason for hiding this comment

joemcgill left a comment

Choose a reason for hiding this comment

felixarntz commented Sep 5, 2023

joemcgill commented Sep 5, 2023

joemcgill commented Sep 5, 2023

felixarntz commented Sep 5, 2023

spacedmonkey Sep 6, 2023

Choose a reason for hiding this comment

joemcgill Sep 6, 2023

Choose a reason for hiding this comment

spacedmonkey commented Sep 6, 2023

joemcgill commented Sep 6, 2023

petitphp commented Sep 6, 2023

joemcgill left a comment

Choose a reason for hiding this comment

joemcgill commented Sep 6, 2023

In `register_core_block_style_handles` store CSS files with relative path in transient #5052

In `register_core_block_style_handles` store CSS files with relative path in transient #5052

petitphp commented Sep 1, 2023 •

edited

Loading