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.

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

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

Closed
retlehs opened this issue Oct 31, 2019 · 10 comments
Closed

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

retlehs opened this issue Oct 31, 2019 · 10 comments
Labels
Good First Issue An issue that's suitable for someone looking to contribute for the first time Needs Dev Ready for, and needs developer efforts [Type] Bug An existing feature does not function as intended

Comments

@retlehs
Copy link
Contributor

retlehs commented Oct 31, 2019

Describe the bug
Gutenberg is using filemtime in several areas when enqueueing assets, such as the editor CSS

When updating Gutenberg, you can run into a scenario where outdated assets are being served because filemtime is cached

To reproduce
Steps to reproduce the behavior:

  1. Note the version being attached to Gutenberg assets, such as build/editor/style.css, on the ver query variable seen via dev tools or the source:
    <link rel='stylesheet' id='wp-editor-css'  href='.../plugins/gutenberg/build/editor/style.css?ver=[filemtime generated version is output here]' type='text/css' media='all' />
    
  2. Switch or update Gutenberg versions
  3. See that the version is not updated until after clearing cache

Expected behavior
The latest version of assets should always be served

In #440 it was suggested to introduce a GUTENBERG_VERSION constant to avoid this problem

Screenshots
filemtime

Notice how the new breadcrumb bar (which is an awesome new feature ❤️) isn't styled correctly because the cache wasn't busted after updating from 6.7 to 6.8

@jorgefilipecosta jorgefilipecosta added the [Type] Bug An existing feature does not function as intended label Nov 21, 2019
@paaljoachim
Copy link
Contributor

Hey @retlehs Ben

Thank you for creating this issue!
A long time has passed.
Is this issue still valid?

@retlehs
Copy link
Contributor Author

retlehs commented Jan 20, 2021

@paaljoachim
Copy link
Contributor

I believe that Ari @aristath and @gziolo might want to know about this issue.

@gziolo
Copy link
Member

gziolo commented Jan 21, 2021

Thank you for the report. GUTENBERG_VERSION might be good for production, for development we could as well disable caching by using a timestamp of something similar.

In the WordPress core we skip the part that sets version, letting WordPress handle it with its current version:

https://github.com/WordPress/wordpress-develop/blob/c145ff4a1a5a45bdbef775b9b47400fcb3311b85/src/wp-includes/script-loader.php#L1493-L1524

@gziolo gziolo added Good First Issue An issue that's suitable for someone looking to contribute for the first time Needs Dev Ready for, and needs developer efforts labels Jan 21, 2021
@hellofromtonya
Copy link
Contributor

Using GUTENBERG_VERSION for production makes sense as it's clean, concise, and aligned with how the assets are versioned in core (i.e. editor assets use the WordPress version).

What about for development?

It depends upon whether the goal is for asset specific versioning or global versioning for all assets.

  • For asset specific versioning: use clearstatcache with filemtime to clear the realpath cache for the specific file while using the asset file's modification time as its version.

  • For global asset version: use time() or microtime() for all like assets.

Notes about filemtime and the realpath cache:

  • filemtime hits the filesystem to get the file's modification time
  • filemtime is cached for performance in realpath cache
  • Realpath cache lifecycle is configurable through php.ini's realpath_cache_ttl
  • Realpath cache for a specific file is clearable via clearstatcache( true, [path to the asset file] );

👉 Which versioning is best for development: asset specific versioning or global versioning for all assets?

@hellofromtonya
Copy link
Contributor

I'm having trouble reproducing the problem.

Env:

  • Chrome Version 89.0.4389.72
  • WordPress 5.7
  • Docker wp-env
  • Using released versions of the plugin

Steps:

Using GB plugin version 9.9.3:

...gutenberg/build/editor/style.css?ver=1612535758

Screen Shot 2021-03-10 at 4 11 22 PM

Updated the plugin to version 10.1.1:

...gutenberg/build/editor/style.css?ver=1615414301

Screen Shot 2021-03-10 at 4 11 59 PM

@retlehs can you share more information on how you were able to reproduce this problem?

@hellofromtonya
Copy link
Contributor

Test 2

Using: build/build-directory/style/css
Versions: 9.9.3 vs. 10.1.1

In 9.9.3, I made the following change to the CSS:

:root{--wp-admin-theme-color:#00ba3e;

whereas in 10.1.1 the color:

:root{--wp-admin-theme-color:#007cba;

Objective: Though the version numbers change as noted above, does each plugin's file get served up or is it the cached one?

Start with 9.9.3

Steps:

  1. Install version 9.9.3
  2. Refreshed the edit post page

Results:

<link rel="stylesheet" id="wp-block-directory-css" href="http://wpcore.local/wp-content/plugins/gutenberg/build/block-directory/style.css?ver=1615418702" media="all">
  • the #00ba3e color did render
  • the right CSS file was served to the browser

9.9.3

Update to 10.1.1

Steps:

  1. Updated to v 10.1.1 through the Plugins screen
  2. Refreshed the edit post page

Results:

<link rel="stylesheet" id="wp-block-library-theme-css" href="http://wpcore.local/wp-content/plugins/gutenberg/build/block-library/theme.css?ver=1615418787" media="all">
  • Version number is different
  • #007cba color renders
  • Correct CSS file is served to the browser

10 1 1

Findings

When updating from one version of the plugin to another:

  • The version number changed
  • The correct stylesheet file was served to the browser (not served from cache)
  • filemtime worked as expected without a realpath caching issue

@gziolo
Copy link
Member

gziolo commented Mar 11, 2021

I checked how we handle assets registered from block.json with register_block_type_from_metadata.

We use filemtime in WP core for styles registered from block.json:
https://github.com/WordPress/wordpress-develop/blob/e20b23d81030a9c57cd6b3a81cf4b01a08679e2e/src/wp-includes/blocks.php#L169-L174

For JS, we let webpack do its thing or the developer has to add an asset file with the required details:
https://github.com/WordPress/wordpress-develop/blob/e20b23d81030a9c57cd6b3a81cf4b01a08679e2e/src/wp-includes/blocks.php#L127-L132

Once we migrate to webpack 5, we definitely should look into outputting a similar asset file for CSS and stop using filemtime.

@hellofromtonya
Copy link
Contributor

hellofromtonya commented Mar 11, 2021

Opened PR #29775 which proposes replacing filemtime with the production and development version strategies. While I was unable to reproduce the reported problem, it is conceivable that the realpath cache could produce this problem.

There are other benefits to switching the approach such as simplifying the code and a teeny teeny tiny microseconds execution time boost.

I think it's a good strategy to move forward with the fix. And then as @gziolo notes above, once we migrate to webpack 5, then the stylesheet versioning can also be used to further simplify the package style registrations and make it the same as the scripts registration for consistency.

@retlehs
Copy link
Contributor Author

retlehs commented Mar 17, 2021

closed by #29775 - thank you @hellofromtonya and everyone else who helped!

@retlehs retlehs closed this as completed Mar 17, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Good First Issue An issue that's suitable for someone looking to contribute for the first time Needs Dev Ready for, and needs developer efforts [Type] Bug An existing feature does not function as intended
Projects
None yet
Development

No branches or pull requests

5 participants