Register Assets: cache bust without filemtime #29775

hellofromtonya · 2021-03-11T13:32:48Z

Description

#18227 reports times when outdated assets are being served when updating the plugin. filemtime is suggested as the root cause. Though I was unable to reproduce, it is conceivable that the filemtime could cause this problem. Why?

The asset version strategy in the registration uses filemtime to generate a unique version based on the asset file's modification time. The return of filemtime is cached in the realpath cache with an expiration defined by the realpath_cache_ttl option in php.ini.

This PR replaces filemtime with 2 different strategies:

For production, it uses GUTENBERG_VERSION as the asset version.
For development, it uses current time in seconds via time() as the asset version.

Note:
time() is not cached in the realpath cache.

Benefits:

Simplifies the code
Avoids an extra hit to the filesystem (teeny tiny performance boost though user will never see it as it's in microseconds)
Mitigates the realpath cache scenario

How has this been tested?

Validated new version of the stylesheet loads when updating the plugin.

Steps:

Installed/activated version 9.9.3
In stylesheet build/build-directory/style/css, changed :root{--wp-admin-theme-color:#00ba3e;
Noted asset's version ...gutenberg/build/block-directory/style.css?ver=1615418702
Updated to new version with this PR's changes

Results:
The updated stylesheet was served to the browser.

The color changed to #007cba and rendered
Version changed ..gutenberg/build/block-directory/style.css?ver=1615469843

Checklist:

My code is tested.
My code follows the WordPress code style.
My code follows the accessibility standards.
I've tested my changes with keyboard and screen readers.
My code has proper inline documentation.
I've included developer documentation if appropriate.
I've updated all React Native files affected by any refactorings/renamings in this PR.

For production, uses GUTENBERG_VERSION. For dev, uses microtime().

github-actions · 2021-03-11T13:33:13Z

👋 Thanks for your first Pull Request and for helping build the future of Gutenberg and WordPress, @hellofromtonya! In case you missed it, we'd love to have you join us in our Slack community, where we hold regularly weekly meetings open to anyone to coordinate with each other.

If you want to learn more about WordPress development in general, check out the Core Handbook full of helpful information.

mkaz

The change looks good, tested and works fine.

The one caveat might be for Gutenberg developers who do not set the WP_DEBUG constant, we probably will want to confirm that is mentioned in the docs somewhere. This can be a separate PR.

hellofromtonya · 2021-03-11T16:15:13Z

The one caveat might be for Gutenberg developers who do not set the WP_DEBUG constant, we probably will want to confirm that is mentioned in the docs somewhere. This can be a separate PR.

What constant are Gutenberg devs more inclined to set, if any? The PR can check for either WP_DEBUG or ____.

mkaz · 2021-03-11T16:19:53Z

What constant are Gutenberg devs more inclined to set, if any? The PR can check for either WP_DEBUG or ____.

I'm not sure, my guess is none on the PHP side. The expectation is more setting in at build using something like:

$ NODE_ENV=development npm run build

Here is the dev setup document:
https://github.com/WordPress/gutenberg/blob/trunk/docs/contributors/code/getting-started-with-code-contribution.md

hellofromtonya · 2021-03-11T16:39:51Z

That makes.

It's a reasonable expectation when working in npm run dev that dev versions will be served up. The WP_DEBUG constant is set when using wp-env https://github.com/WordPress/gutenberg/tree/trunk/packages/env#default-wp-config-values. In a separate PR we could add default set up for those not using wp-env.

aristath · 2021-03-12T12:19:21Z

What constant are Gutenberg devs more inclined to set, if any? The PR can check for either WP_DEBUG or ____.

Usually I have SCRIPT_DEBUG enabled... People working on Gutenberg do mostly JS work, so frequently that is set to true in order to get better debug info in the browser console

hellofromtonya · 2021-03-12T13:58:48Z

Usually I have SCRIPT_DEBUG enabled... People working on Gutenberg do mostly JS work, so frequently that is set to true in order to get better debug info in the browser console

Thanks @aristath.

In looking at constants predefined when running wp-env, both are set to true:

WP_DEBUG: true,
SCRIPT_DEBUG: true,

SCRIPT_DEBUG is already being used as development flag in the same file.

Switching to use SCRIPT_DEBUG for consistency.

gziolo · 2021-03-16T16:51:52Z

When I tested it locally I saw the following:

Warning: Use of undefined constant GUTENBERG_VERSION - assumed 'GUTENBERG_VERSION' (this will throw an Error in a future version of PHP) in /var/www/html/wp-content/plugins/gutenberg/gutenberg.php on line 245
| GUTENBERG_VERSION

It looks like GUTENBERG_VERSION is only set on production when using the build version of the plugin. You should be able to reproduce it by setting SCRIPT_DEBUG to false.

This is also why PHP unit tests fail:

https://github.com/WordPress/gutenberg/pull/29775/checks?check_run_id=2095591189#step:9:46

Overall, I like the idea. Thank you for working on it. We just need to find a way to make it work in the case when GUTENBERG_VERSION is udnefined.

gziolo

See my previous comment.

hellofromtonya · 2021-03-16T16:57:10Z

@gziolo I was looking at that earlier today, but then got sidetracked. I'm thinking about altering the decision to include checking if GUTENBERG_VERSION is not defined, then it's also in a development or test mode. Why? As you pointed out, that constant is for production, i.e. meaning it's defined in the production version of the plugin.

Switches the check around to determine if in production mode: - GUTENBERG_VERSION is defined - SCRIPT_DEBUG is not defined || is not true. Else, defaults to time().

hellofromtonya · 2021-03-16T18:22:51Z

@gziolo Commit 352e06a flips the check around by determining if in production mode, else it defaults to use time() as the asset version. For prod mode, it checks:

Is GUTENBERG_VERSION defined? If yes, do the next check; else use time()
Is in not in debug/dev mode, where debug/dev is determined by SCRIPT_DEBUG defined and true? If not in debug/dev mode, then use GUTENBERG_VERSION; else use time().

Testing locally

When the above conditions are met:

<link rel="stylesheet" id="wp-edit-blocks-css" href="http://wpcore.local/wp-content/plugins/gutenberg/build/block-library/editor.css?ver=10.1" media="all">

When adding define( 'SCRIPT_DEBUG', false ); to wp-config.php:

<link rel="stylesheet" id="wp-edit-blocks-css" href="http://wpcore.local/wp-content/plugins/gutenberg/build/block-library/editor.css?ver=10.1" media="all">

When commenting out define( 'GUTENBERG_VERSION', '10.1.1' ) in the plugin:

<link rel="stylesheet" id="wp-edit-blocks-css" href="http://wpcore.local/wp-content/plugins/gutenberg/build/block-library/editor.css?ver=1615918791" media="all">

When restoring the constant and then changing define( 'SCRIPT_DEBUG', true );:

<link rel="stylesheet" id="wp-edit-blocks-css" href="http://wpcore.local/wp-content/plugins/gutenberg/build/block-library/editor.css?ver=1615918820" media="all">

gziolo

Awesome work polishing the logic 💯

hellofromtonya added 3 commits March 11, 2021 06:57

Replaces filemtime when registering package styles.

37c80a7

For production, uses GUTENBERG_VERSION. For dev, uses microtime().

Replaces filemtime when registering package scripts.

a53fafe

For production, uses GUTENBERG_VERSION. For dev, uses microtime().

Improves comments.

30eaa13

hellofromtonya requested review from spacedmonkey and TimothyBJacobs as code owners March 11, 2021 13:32

github-actions bot added the First-time Contributor Pull request opened by a first-time contributor to Gutenberg repository label Mar 11, 2021

gziolo added the Gutenberg Plugin Issues or PRs related to Gutenberg Plugin management related efforts label Mar 11, 2021

Use time() instead of microtime().

e415075

aristath mentioned this pull request Mar 11, 2021

Using filemtime to cache bust editor assets is not reliable #18227

Closed

mkaz approved these changes Mar 11, 2021

View reviewed changes

For consistency, use SCRIPT_DEBUG as dev/prod flag.

f31f27b

aristath approved these changes Mar 12, 2021

View reviewed changes

gziolo requested changes Mar 16, 2021

View reviewed changes

hellofromtonya added 2 commits March 16, 2021 12:27

Check if GUTENBERG_VERSION is defined.

352e06a

Switches the check around to determine if in production mode: - GUTENBERG_VERSION is defined - SCRIPT_DEBUG is not defined || is not true. Else, defaults to time().

Removes extra empty line (for phpcs).

49695e1

hellofromtonya requested a review from gziolo March 16, 2021 18:01

gziolo approved these changes Mar 17, 2021

View reviewed changes

gziolo merged commit 3b2b118 into WordPress:trunk Mar 17, 2021

github-actions bot added this to the Gutenberg 10.3 milestone Mar 17, 2021

This was referenced Mar 17, 2021

Bump gutenberg from 6be7320 to 3b2b118 wordpress-mobile/gutenberg-mobile#3271

Closed

Bump gutenberg from e4eb4be to 8e25520 dcalhoun/gutenberg-mobile#186

Closed

dependabot bot mentioned this pull request Mar 17, 2021

Bump gutenberg from 7f7910f to 5c108bc fluiddot/gutenberg-mobile#125

Closed

hellofromtonya deleted the fix/18227/cache-bust-without-filemtime branch March 17, 2021 12:47

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Register Assets: cache bust without filemtime #29775

Register Assets: cache bust without filemtime #29775

hellofromtonya commented Mar 11, 2021 •

edited

Loading

github-actions bot commented Mar 11, 2021

mkaz left a comment

hellofromtonya commented Mar 11, 2021

mkaz commented Mar 11, 2021

hellofromtonya commented Mar 11, 2021 •

edited

Loading

aristath commented Mar 12, 2021

hellofromtonya commented Mar 12, 2021 •

edited

Loading

gziolo commented Mar 16, 2021 •

edited

Loading

gziolo left a comment

hellofromtonya commented Mar 16, 2021

hellofromtonya commented Mar 16, 2021 •

edited

Loading

gziolo left a comment

Register Assets: cache bust without filemtime #29775

Register Assets: cache bust without filemtime #29775

Conversation

hellofromtonya commented Mar 11, 2021 • edited Loading

Description

How has this been tested?

Checklist:

github-actions bot commented Mar 11, 2021

mkaz left a comment

Choose a reason for hiding this comment

hellofromtonya commented Mar 11, 2021

mkaz commented Mar 11, 2021

hellofromtonya commented Mar 11, 2021 • edited Loading

aristath commented Mar 12, 2021

hellofromtonya commented Mar 12, 2021 • edited Loading

gziolo commented Mar 16, 2021 • edited Loading

gziolo left a comment

Choose a reason for hiding this comment

hellofromtonya commented Mar 16, 2021

hellofromtonya commented Mar 16, 2021 • edited Loading

gziolo left a comment

Choose a reason for hiding this comment

hellofromtonya commented Mar 11, 2021 •

edited

Loading

hellofromtonya commented Mar 11, 2021 •

edited

Loading

hellofromtonya commented Mar 12, 2021 •

edited

Loading

gziolo commented Mar 16, 2021 •

edited

Loading

hellofromtonya commented Mar 16, 2021 •

edited

Loading