Make WordPress Core

Opened 17 months ago

Closed 6 months ago

Last modified 6 weeks ago

#58433 closed enhancement (fixed)

Improve Etag-based cache busting for styles and scripts

Reported by: dav4's profile dav4 Owned by: peterwilsoncc's profile peterwilsoncc
Milestone: 6.6 Priority: normal
Severity: normal Version:
Component: Script Loader Keywords: has-patch needs-testing has-testing-info has-screenshots commit has-unit-tests
Focuses: Cc:

Description (last modified by ironprogrammer)

Improve how Etag headers are generated for styles and scripts to reflect changes in underlying script versions, rather than relying only on the WP version. This would address unexpected caching (HTTP 304 responses) encountered during development in instances where a script's version has changed but the WP version has not.

See comment:2.

Updated to match direction this ticket has taken. Original description follows:


Hi

I know there are plenty of plugins to hide WP version. You also decided to hide WP version from readme.html some time ago. When the user uses all possible methods to hide WP version, current version still can be checked by load-styles.php and load-scripts.php files. It is visible in eTag header.

I know, this is not a big issue but I think is worth considering to use here some hash or checksum.

Regards
Dawid

Attachments (2)

58433-poc.diff (1.5 KB) - added by peterwilsoncc 16 months ago.
58433-poc-2.diff (1.5 KB) - added by ironprogrammer 16 months ago.

Download all attachments as: .zip

Change History (34)

#1 in reply to: ↑ description @SergeyBiryukov
16 months ago

Hi there, welcome back to WordPress Trac!

Replying to dav4:

You also decided to hide WP version from readme.html some time ago.

Just to clarify a bit for anyone looking at the ticket, the version number was removed from readme.html not for security reasons, but rather as part of a general focus on de-emphasizing the version in the admin, except for a few places where it's relevant: the About page, the Updates screen, Site Health, etc. See [39583] / #35554 and #42386.

Indeed, there are other ways to determine the version number.

#2 @peterwilsoncc
16 months ago

It might be worth changing how the ETAG header is generated, not to hide the WordPress version but in order to better calculate a value that changes with the scripts' versions rather than the WordPress version.

As is, a 304 HTTP response may be returned despite the scripts' version changing. I've attached a POC to demonstrate a more accurate response. This helps explain some caching issues I've had working on WordPress-Develop.

While the POC does hide the WP version behind a hash, it won't help hide the WordPress version from any bots as the ETAG header would continue to provide a method for bots to determine the current version of WP if they store the hashes.

This ticket was mentioned in Slack in #core-test by ironprogrammer. View the logs.


16 months ago

#5 @monzuralam
16 months ago

  • Keywords needs-testing-info added
  • Type changed from defect (bug) to enhancement

@dav4 Thanks for your report.
As per POC, we can leave the request as an enhancement.

Not all contributors will be clear on how to test ETAG headers, so enhancement testing instructions would be helpfull.

#6 @ironprogrammer
16 months ago

  • Keywords has-patch needs-testing added

As discussed during bi-weekly Test Team triage, adding a couple additional keywords to help move this forward.

#7 @peterwilsoncc
16 months ago

In the recent test meeting in slack, there was a question about the poc suffix on 58433-poc.diff. It's to indicate it's a proof of concept.

I've designated it as such as an equivalent change would be required in wp-includes/load-styles.php prior to commit. If the concept is proven as workable for loading scripts, then I will follow up with a patch that covers loading styles too.

Steps to test:

  1. Apply the patch to your development environment
  2. Visit the dashboard of your development environment.
  3. Open the developer tools in your browser, switch to the network tab.
  4. Look for a URL that contains load-scripts.php
  5. Click on the URL, this should open a panel with further details
  6. The panel will show a section called "Response Headers" or similar
  7. Look for the header ETAG, it should show an apparently random string made up of 0-9A-F
  8. Look for a second URL that contains load-scripts.php
  9. Follow the steps above to look for the ETAG header
  10. Confirm the ETAG header differs from the first URL's

Advanced testing

  1. Look for a URL containing both load-scripts.php and jquery
  2. Observe the ETAG header following the steps anove
  3. Change the version number of jQuery as registered on line 825 of script-loader.php
  4. Reload the page in your browser
  5. Look for the same URL
  6. Confirm the ETAG header has changed from the initial version

@ironprogrammer let me know if these steps help or you think they need clarifying.

#8 @ironprogrammer
16 months ago

  • Keywords needs-testing-info removed

Thanks, @peterwilsoncc! In running through these steps, it appears the current Etags emitted by load-scripts.php and load-styles.php are not double-quoted, which prevents them from being emitted (at least in Nginx).

The tweak in 58433-poc-2.diff should emit the tag as expected (see the Etag header in `ms-files.php` for comparison).

The steps provided are great 👍🏻 The only addition would be to ensure that your environment has define( 'SCRIPT_DEBUG', false );, as that allows both load-scripts.php and load-styles.php to be used (or just apply the patch to a vanilla WP install).

#9 @ironprogrammer
16 months ago

  • Keywords has-testing-info has-screenshots added

Test Report - POC

Patch tested: attachment:58433-poc-2.diff. This only covers load-scripts.php, as noted in comment:7.

Environment

  • Hardware: MacBook Pro Apple M1 Pro
  • OS: macOS 13.4
  • Browser: Safari 16.5, Google Chrome 114.0.5735.106
  • Server: nginx/1.25.0
  • PHP: 7.4.33
  • WordPress: 6.3-alpha-55505-src
  • In wp-config.php: define( 'SCRIPT_DEBUG', false );

Actual Results

  • ✅ The Etag emitted differs between the separate calls to load-scripts.php.
  • ✅ [Advanced] The Etag changes when a loaded script version changes, e.g. for jquery-core. (See the Inception-inspired screenshot in Figure 1.)

Additional Notes

Supplemental Artifacts

Figure 1: A) jquery-core set to version 3.7.0, B) 3.7.1, and C) back to 3.7.0 (same as A):

https://cldup.com/YWOLZAc_-F.png

#11 @peterwilsoncc
16 months ago

In the linked pull request:

  • included quotes around etag per @ironprogrammer's comment
  • modified where the quotes were added so they are detected in time for the HTTP_IF_NONE_MATCH header/304 response.
  • always set the comparison type to weak (again allows for the 304 header to be hit)
  • duplicated it all in the load-styles.php file for CSS
  • added a little code to account for $wp_[ASSET]->registered[ $handle ]->ver === false

#12 @monzuralam
16 months ago

Test Report

Environment

  • Hardware: HP 11th Gen Intel(R) Core(TM) i5-1155G7 @ 2.50GHz 2.50 GHz
  • OS: Windows 11
  • Browser: Firefox 114.0.1 (64-bit)
  • Server: Apache/2.4.56
  • PHP: 8.0.29
  • WordPress: 6.3-alpha-55905
  • Theme: twentytwentythree v1.1
  • Active Plugins: gutenberg v16.0.0-rc.4

Steps to Reproduce

  1. Start Docker Container by command
  2. Open WordPress files on VS Code.
  3. Change Patch Code
  4. WordPress Dashboard > Reload > Inspect > Network Tab

Expected Results

  • ✅ Etag is the same. ( simple test )

Supplemental Artifacts

Image - https://www.awesomescreenshot.com/image/40766965?key=bf8c7cb678c471e77f07028b14ddeede
Video ( simple test )- https://www.awesomescreenshot.com/video/18344738?key=e31f0aa401bca7e237d881d692ebcd6a

#13 @ironprogrammer
16 months ago

Thanks for the PR, @peterwilsoncc 🙌🏻

Test Report

Patch tested: https://github.com/WordPress/wordpress-develop/pull/4618. This covers both load-scripts.php and load-styles.php, reflecting updates noted in comment:11.

Environment

  • Hardware: MacBook Pro Apple M1 Pro
  • OS: macOS 13.4
  • Browser: Safari 16.5, Google Chrome 114.0.5735.106
  • Server: nginx/1.25.0
  • PHP: 7.4.33
  • WordPress: 6.3-alpha-55505-src
  • Also, Postman v10.15.1 (see Additional Notes)

Actual Results

  • Simple Test:
    • ✅ Etag is now emitted and differs between separate calls to load-scripts.php.
    • ✅ Etag is now emitted for load-styles.php.
  • Advanced Test:
    • ✅ Etag changes for the load-scripts.php response when a script version changes, e.g. bumping jquery-core to 3.7.1.
    • ✅ Etag changes for the load-styles.php response when a script version changes, e.g. adding a version to common, a la $styles->add( 'common', "/wp-admin/css/common$suffix.css", array(), '1.0.0' );.
  • Testing HTTP 304 Not Modified:
    • ✅ When If-None-Match is sent, the server responds with the expected 304.
    • ✅ When If-None-Match is sent with an old/different Etag, the server responds with 200, returning the latest resource.

Additional Notes

For the 304 response test using Postman:

  1. Configure a GET request, such as http://YOUR-SERVER/wp-admin/load-scripts.php?c=0&load%5Bchunk_0%5D=jquery-core,jquery-migrate,utils&ver=6.3-alpha-55505-src.
  2. Copy the Etag value returned.
  3. Add the If-None-Match header to the request, setting its value to the Etag.
  4. Resend the request, and a 304 should return.
Last edited 16 months ago by ironprogrammer (previous) (diff)

#14 @peterwilsoncc
16 months ago

Thanks for testing the patch Brian.

As we move toward Beta, I'm tempted to see if we can put this in for WP 6.3.

As I alluded too in comment #2, failing to account for the asset versions in the generated etag can lead to unusual behavior when developing on WordPress-Develop.

As external libraries and block-editor packages are updated, I sometimes get a 304 header even when the files have been modified. It was only once this ticket was logged that I realised why.

#15 follow-up: @azaozz
16 months ago

It might be worth changing how the ETAG header is generated, not to hide the WordPress version but in order to better calculate a value that changes with the scripts' versions rather than the WordPress version.

The way this has been working in alpha/beta (for WP development) is by appending a timestamp to $wp_version every time WP is built, see https://core.trac.wordpress.org/browser/tags/6.2.2/Gruntfile.js#L414.

a 304 HTTP response may be returned despite the scripts' version changing

As far as I see the only way the current ETAG (the $wp_version that includes the last built timestamp) can become outdated is if some core scripts or stylesheets were updated but WP was not build. Not sure if that's possible?

The patch in PR #4618 looks good however I'm not sure if it will fix the problem (edge case) of updating WP scripts and not building WP if that is the actual problem here. $wp_version is used to bust caching for many WP scripts and stylesheets, so the same problem will happen when not concatenating them.

Version 0, edited 16 months ago by azaozz (next)

#16 in reply to: ↑ 15 ; follow-up: @peterwilsoncc
16 months ago

Replying to azaozz:

The way this has been working in alpha/beta (for WP development) is by appending a timestamp to $wp_version every time WP is built, see https://core.trac.wordpress.org/browser/tags/6.2.2/Gruntfile.js#L414.

It looks like this only works if running WordPress-Develop from the build directory. As most people work in the src directory (anecdotal but src is the default in WordPress-Develop's Docker environment) the cache remains unbroken.

Thus the etag doesn't change when the editor packages are updated which can cause the caching issues I've seen.

It may be worth adding something like this in version.php but it's probably best done on another ticket:

<?php
/*
 * When running from the source directory of the WordPress-Develop repo, the
 * WordPress version needs to be modified for the purpose of cache-busting
 * JavaScript and CSS files. The following code will modify the version number
 * to include the current timestamp if the version number is not a release.
 */
if ( preg_match( '/-.*-\d{5,}-src$/', $wp_version ) ) {
        $wp_version = substr_replace( $wp_version, (string) time(), -3 );
}

#17 @kkmuffme
6 months ago

  • the ETAG should be changed to a weak etag "W/" as otherwise transforming by an proxy isn't allowed (e.g. gzip/brotli compressing) which leads to higher than necessary load times
  • use crc32 instead of md5 - it's just as insecure, but at least faster

#18 @ironprogrammer
6 months ago

  • Description modified (diff)
  • Summary changed from WP version in Etag header in load-styles.php file to Improve Etag-based cache busting for styles and scripts

#19 @ironprogrammer
6 months ago

  • Keywords needs-refresh added
  • Milestone changed from Awaiting Review to 6.6

I've updated this ticket's title/description to better match the direction it has taken (since comment:2), namely to improve Etag-based cache busting for scripts and styles.

Since we're now early in the 6.6 cycle, let's move this into the milestone. I believe the following next steps are need to move forward:

  • Refresh/rebase off trunk 🙏.
  • Smoke test refreshed PR.

Then, separately:

  • Address unbroken cache issues when working from src in a separate ticket (comment:15 and comment:16).
  • Consider opening a new performance-focused ticket for using crc32() vs md5() (comment:17).

#20 @peterwilsoncc
6 months ago

I've updated the linked pull request to merge in trunk.

The PR uses weak etags, ie W\ per comment #17.

#21 @ironprogrammer
6 months ago

  • Keywords commit added; needs-refresh removed

Thanks for the refresh, @peterwilsoncc! Still looks good, so marking for commit consideration.

Test Report

Confidence check: Refresh of https://github.com/WordPress/wordpress-develop/pull/4618 (comment:20). LGTM 👍🏻

Environment

  • Hardware: MacBook Pro Apple M1 Pro
  • OS: macOS 14.4.1
  • Browser: Safari 17.4.1
  • Server: nginx/1.25.4
  • PHP: 8.2.17
  • MySQL: 8.0.27
  • WordPress: 6.6-alpha-57778-src

Actual Results

  • ✅ Etag is now emitted and differs between separate calls to load-scripts.php.
  • ✅ Etag is now emitted for load-styles.php.
  • ✅ Etag changes for the load-scripts.php response when a script version changes, e.g. bumping jquery-core back down to 3.7.0 in script-loader.php (see supplemental section for details).
  • ✅ When If-None-Match is sent with matching Etag, the server responds with the expected 304.
  • ✅ When If-None-Match is sent with non-matching Etag, the server responds with 200, returning the latest resource.
  • ✅ Confirmed that Etags are marked with weak indicator: W/.

Supplemental Assets

Example header info demonstrating script version change:

URL: http://wp-src.test/wp-admin/load-scripts.php?c=0&load%5Bchunk_0%5D=jquery-core,jquery-migrate,utils&ver=6.6-alpha-57778-src

ETag: W/"15ed1817ff7302e64fb57cf8f1033f0e" <-- jquery-core 3.7.1
ETag: W/"5dc93d2f474f0d8d7d397ed510e8dcd8" <-- jquery-core 3.7.0 (modified)
ETag: W/"15ed1817ff7302e64fb57cf8f1033f0e" <-- jquery-core 3.7.1 (restored)

#22 @peterwilsoncc
6 months ago

@ironprogrammer Do you think it's worth including a check in these files for -src versions and including a timestamp in the etag generation?

This is a lightweight approach for comment #16 to fix caching issues while testing without the need to account for the use of WP versions throughout core.

#23 in reply to: ↑ 16 @azaozz
6 months ago

Replying to peterwilsoncc:

It looks like this only works if running WordPress-Develop from the build directory.
...
Thus the etag doesn't change when the editor packages are updated which can cause the caching issues I've seen.

Yes, there seems to be an edge case when:

  • WP runs from /src.
  • SCRIPT_DEBUG is (manually) set to false.
  • Editing scripts or stylesheets, or updating scripts added from the npm packages.

Note that if SCRIPT_DEBUG is not set, it is set in wp_scripts_get_suffix() in Script Loader: https://developer.wordpress.org/reference/functions/wp_scripts_get_suffix/. The default is to have SCRIPT_DEBUG set to true when running from /src. That disables concatenation of scripts and stylesheets and ensures caches are always busted.

Generally testing after editing or updating of any JS or CSS files should be done with SCRIPT_DEBUG enabled to help catch any possible errors. This of course includes testing after updating scripts from npm packages.

So it seems that setting SCRIPT_DEBUG to false when running from /src and then updating npm packages is more like a deliberate action, and the result should be expected? In any case, seems a better fix for this may be to add a random string to the URLs when outputting the tags for concatenated scripts and stylesheets while running from /src. Changing (or disabling) the etag may not be enough, and generally doesn't work as well.

It may be worth adding something like this...

Hmm, this will attempt to bust the cache on every page load in wp-admin when running from /src? I.e. it is pretty much the same as the current functionality in Script Loader, only the latter is a bit more refined and can be controlled with constants.

Don't think it is a good idea to change $wp_version as that will duplicate and interfere with the existing functionality in Script Loader.

Last edited 6 months ago by azaozz (previous) (diff)

#24 @peterwilsoncc
6 months ago

Thanks @azaozz, I think you're right, it's best to allow contributors to test the etags in the source generation. I hadn't noticed the defaults.

I'll commit as is.

#25 @peterwilsoncc
6 months ago

  • Component changed from General to Script Loader

#26 @peterwilsoncc
6 months ago

  • Owner set to peterwilsoncc
  • Resolution set to fixed
  • Status changed from new to closed

In 57943:

Script Loader: Improve asset concatenation Etags.

Include the asset version of JavaScript and CSS files when generating the ETag for concatenated assets in load-scripts.php and load-styles.php. This ensures the ETag is updated as script versions change (for example editor package updates) rather than only when the WordPress version changes.

The W\ prefix is added to the generated ETag to allow for CDNs and proxy servers modifying the script to add or improve the compression algorithm.

Props azaozz, dav4, ironprogrammer, johnbillion, kkmuffme, monzuralam, peterwilsoncc, sergeybiryukov.
Fixes #58433.

This ticket was mentioned in PR #6820 on WordPress/wordpress-develop by @vrajadas.


4 months ago
#28

  • Keywords has-unit-tests added

Added tests to change make in PR for 58433

Moved the logic to a separate method under wp-dependencies class and added unit tests.

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

@peterwilsoncc commented on PR #6820:


4 months ago
#29

@vraja-pro The trac ticket you've linked this pull request to is closed on a previous milestone. If you'd like to suggest some follow up changes, are you able to open a new ticket on the Script Loader component.

Please include a brief explanation on the ticket as to why you'd like to make the follow up changes.

Thanks

@vrajadas commented on PR #6820:


4 months ago
#30

Hey @peterwilsoncc I created a new trac ticket and updated the PR description with it's new number.

@martin.krcho commented on PR #6820:


3 months ago
#31

@mukeshpanchal27 would you please re-review this task?

Note: See TracTickets for help on using tickets.