Add Site Health test to verify that static assets are served with far-future expires #1727

b1ink0 · 2024-12-09T17:35:10Z

Summary

Fixes #323

Relevant technical choices

Test are based on the Cache-Control with max-age or an Expires to determine if the static assets are served with far future expires. If Cache-Control and Expires are unavailable then the ETag and Last-Modified are used to do a secondary request to the same asset URL with If-None-Match and If-Modified-Since, respectively. If those return with 304 Not Modified then that could pass the test as well.

plugins/performance-lab/includes/site-health/far-future-headers/helper.php

b1ink0 · 2024-12-09T17:49:47Z

plugins/performance-lab/includes/site-health/far-future-headers/helper.php

+	}
+
+	return $final_status;
+}


Currently even if one asset fails the header check then this test will be show in the site health. Would it be better to show a table with mime type which will specifically tell for which mime type needs to add the Cache-Control headers?

A table makes sense to me.

b1ink0 · 2024-12-09T17:53:22Z

plugins/performance-lab/includes/site-health/far-future-headers/helper.php

+			'<p>%s</p>',
+			esc_html__( 'Far-future Cache-Control or Expires headers can be added or adjusted with a small configuration change by your hosting provider.', 'performance-lab' )
+		);
+	}


There are two different checks which are getting performed one with Cache-Control, Expires and other with Etag, Last-Modified should there be different messages shown based on the checks?

In the new 3db5886 commit a table is used to display reason for different failure cases.

…ate a report table for missing headers

…and updating the extensions table generation

github-actions · 2024-12-10T12:20:01Z

The following accounts have interacted with this PR and/or linked issues. I will continue to update these lists as activity occurs. You can also manually ask me to refresh this list by adding the props-bot label.

If you're merging code through a pull request on GitHub, copy and paste the following into the bottom of the merge commit message.

Co-authored-by: b1ink0 <[email protected]>
Co-authored-by: westonruter <[email protected]>
Co-authored-by: manuelRod <[email protected]>
Co-authored-by: joemcgill <[email protected]>

To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook.

plugins/performance-lab/includes/site-health/far-future-headers/helper.php

westonruter · 2025-01-07T01:32:01Z

plugins/performance-lab/includes/site-health/far-future-headers/helper.php

+ * @param WpOrg\Requests\Utility\CaseInsensitiveDictionary $headers Response headers.
+ * @return array{passed: bool, reason: string}|false Detailed result. If passed=false, reason explains why it failed and false if no headers found.
+ */
+function perflab_ffh_check_headers( WpOrg\Requests\Utility\CaseInsensitiveDictionary $headers ) {


The wp_remote_retrieve_headers() function can return a CaseInsensitiveDictionary object or an array:

Suggested change

* @param WpOrg\Requests\Utility\CaseInsensitiveDictionary $headers Response headers.

* @return array{passed: bool, reason: string}|false Detailed result. If passed=false, reason explains why it failed and false if no headers found.

*/

function perflab_ffh_check_headers( WpOrg\Requests\Utility\CaseInsensitiveDictionary $headers ) {

* @param WpOrg\Requests\Utility\CaseInsensitiveDictionary|array $headers Response headers.

* @return array{passed: bool, reason: string}|false Detailed result. If passed=false, reason explains why it failed and false if no headers found.

*/

function perflab_ffh_check_headers( $headers ) {

This would raise a PHPStan error; it seems we also need to provide the type of array. Below is what I used to solve it:
@param WpOrg\Requests\Utility\CaseInsensitiveDictionary|array<string, string|array<string>>
The array<string> is needed because if multiple same keyed headers are present, it is placed into the array.

Ah yes, good point.

plugins/performance-lab/includes/site-health/far-future-headers/helper.php

westonruter · 2025-01-07T01:35:12Z

plugins/performance-lab/includes/site-health/far-future-headers/helper.php

+		}
+
+		$headers = wp_remote_retrieve_headers( $response );
+		if ( ! is_object( $headers ) ) {


Since wp_remote_retrieve_headers() can return an array:

Suggested change

if ( ! is_object( $headers ) ) {

if ( count( $headers ) === 0 ) {

Fatal error: Uncaught Error: count(): Argument # 1 ($value) must be of type Countable|array, WpOrg\Requests\Utility\CaseInsensitiveDictionary given

This error is encountered when using count, below condition seems to solve this error.
if ( ! is_object( $headers ) && 0 === count( $headers ) )

It's surprising that CaseInsensitiveDictionary is not implemented as a Countable.

westonruter · 2025-01-08T19:06:01Z

plugins/performance-lab/includes/site-health/far-future-headers/helper.php

+		// Expires header exists but not far enough in the future.
+		if ( $max_age > 0 && $max_age < $threshold ) {
+			return array(
+				'passed' => false,
+				'reason' => __( 'max-age below threshold', 'performance-lab' ),
+			);
+		}
+		return array(
+			'passed' => false,
+			'reason' => __( 'expires below threshold', 'performance-lab' ),
+		);
+	}
+
+	// No max-age or expires found at all or max-age < threshold and no expires.
+	if ( 0 === $max_age ) {
+		return false;
+	} else {
+		// max-age was present but below threshold and no expires.
+		return array(
+			'passed' => false,
+			'reason' => __( 'max-age below threshold', 'performance-lab' ),
+		);
+	}


It would seem helpful to indicate the max-age and expires values in the error message so you can see what the actual values are. Likewise, shouldn't the threshold be added to the error message so the user knows the minimum TTL that they should configure the expires and max-age to be?

This makes sense I have added the max-age, expires and minimum threshold.

For the expires , should we display the timestamp like above, or should we show$expires_time - time() seconds?

Additionally, should we convert the remaining time into a human-readable format, such as 1 month or 1 year? Since our threshold filter uses seconds as a parameter, displaying it in seconds might be more appropriate.

I think converting the expires to seconds makes sense. I don't think we need to worry about converting to anything other than seconds, however. Just pass the number of seconds through number_format_i18n() to get some better formatting.

westonruter · 2025-01-08T19:07:07Z

plugins/performance-lab/includes/site-health/far-future-headers/helper.php

+
+		// Extract filename from the URL.
+		$path_info = pathinfo( (string) wp_parse_url( $asset, PHP_URL_PATH ) );
+		$filename  = isset( $path_info['basename'] ) ? $path_info['basename'] : basename( $asset );


Suggested change

$filename = isset( $path_info['basename'] ) ? $path_info['basename'] : basename( $asset );

$filename = $path_info['basename'] ?? basename( $asset );

westonruter · 2025-01-08T19:08:09Z

plugins/performance-lab/includes/site-health/far-future-headers/helper.php

+			$conditional_pass = perflab_ffh_try_conditional_request( $asset, $headers );
+			if ( ! $conditional_pass ) {
+				$final_status   = 'recommended';
+				$fail_details[] = array(
+					'filename' => $filename,
+					'reason'   => __( 'No far-future headers and no conditional caching', 'performance-lab' ),
+				);
+			} else {
+				$final_status   = 'recommended';
+				$fail_details[] = array(
+					'filename' => $filename,
+					'reason'   => __( 'No far-future headers but conditionally cached', 'performance-lab' ),
+				);
+			}


Factoring $final_status out of the if statement.

Suggested change

$conditional_pass = perflab_ffh_try_conditional_request( $asset, $headers );

if ( ! $conditional_pass ) {

$final_status = 'recommended';

$fail_details[] = array(

'filename' => $filename,

'reason' => __( 'No far-future headers and no conditional caching', 'performance-lab' ),

);

} else {

$final_status = 'recommended';

$fail_details[] = array(

'filename' => $filename,

'reason' => __( 'No far-future headers but conditionally cached', 'performance-lab' ),

);

}

$conditional_pass = perflab_ffh_try_conditional_request( $asset, $headers );

$final_status = 'recommended';

if ( ! $conditional_pass ) {

$fail_details[] = array(

'filename' => $filename,

'reason' => __( 'No far-future headers and no conditional caching', 'performance-lab' ),

);

} else {

$fail_details[] = array(

'filename' => $filename,

'reason' => __( 'No far-future headers but conditionally cached', 'performance-lab' ),

);

}

westonruter · 2025-01-08T19:09:32Z

plugins/performance-lab/includes/site-health/far-future-headers/helper.php

+		// There can be multiple cache-control headers, we only care about max-age.
+		$controls = is_array( $cache_control ) ? $cache_control : array( $cache_control );
+		foreach ( $controls as $control ) {
+			if ( (bool) preg_match( '/max-age\s*=\s*(\d+)/', $control, $matches ) ) {


Because PhpStorm complains about the unnecessary cast, but then without it PHPStan complains about "Only booleans are allowed in an if condition, int|false given."

Suggested change

if ( (bool) preg_match( '/max-age\s*=\s*(\d+)/', $control, $matches ) ) {

if ( 1 === preg_match( '/max-age\s*=\s*(\d+)/', $control, $matches ) ) {

westonruter · 2025-01-08T19:11:13Z

plugins/performance-lab/includes/site-health/far-future-headers/helper.php

+	// If max-age is too low or not present, check Expires.
+	if ( is_string( $expires ) && '' !== $expires ) {
+		$expires_time = strtotime( $expires );
+		if ( (bool) $expires_time && ( $expires_time - time() ) >= $threshold ) {


Suggested change

if ( (bool) $expires_time && ( $expires_time - time() ) >= $threshold ) {

if ( is_int( $expires_time ) && ( $expires_time - time() ) >= $threshold ) {

westonruter · 2025-01-08T19:13:56Z

plugins/performance-lab/includes/site-health/far-future-headers/helper.php

+		}
+
+		// Expires header exists but not far enough in the future.
+		if ( $max_age > 0 && $max_age < $threshold ) {


This condition doesn't seem to be necessary:

Suggested change

if ( $max_age > 0 && $max_age < $threshold ) {

if ( $max_age > 0 ) {

Because above we're already doing:

if ( $max_age >= $threshold ) { return array( 'passed' => true, 'reason' => '', ); }

So we already know that $max_age is less than the $threshold.

westonruter · 2025-01-08T19:17:47Z

plugins/performance-lab/includes/site-health/far-future-headers/helper.php

+	$cache_control = isset( $headers['cache-control'] ) ? $headers['cache-control'] : '';
+	$expires       = isset( $headers['expires'] ) ? $headers['expires'] : '';


I believe this works even when $headers is a CaseInsensitiveDictionary:

Suggested change

$cache_control = isset( $headers['cache-control'] ) ? $headers['cache-control'] : '';

$expires = isset( $headers['expires'] ) ? $headers['expires'] : '';

$cache_control = $headers['cache-control'] ?? '';

$expires = $headers['expires'] ?? '';

westonruter · 2025-01-08T19:18:37Z

plugins/performance-lab/includes/site-health/far-future-headers/helper.php

+		$controls = is_array( $cache_control ) ? $cache_control : array( $cache_control );
+		foreach ( $controls as $control ) {


Seems this could be simplified a bit:

Suggested change

$controls = is_array( $cache_control ) ? $cache_control : array( $cache_control );

foreach ( $controls as $control ) {

foreach ( (array) $cache_control as $control ) {

…ssage

codecov · 2025-01-09T18:06:19Z

Codecov Report

Attention: Patch coverage is 78.65169% with 38 lines in your changes missing coverage. Please review.

Project coverage is 58.05%. Comparing base (b6e5c5a) to head (ed28196).

Files with missing lines	Patch %	Lines
...includes/site-health/far-future-headers/helper.php	83.33%	28 Missing ⚠️
.../includes/site-health/far-future-headers/hooks.php	0.00%	8 Missing ⚠️
...gins/performance-lab/includes/site-health/load.php	0.00%	2 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##            trunk    #1727      +/-   ##
==========================================
+ Coverage   57.48%   58.05%   +0.56%     
==========================================
  Files          84       86       +2     
  Lines        6516     6694     +178     
==========================================
+ Hits         3746     3886     +140     
- Misses       2770     2808      +38

Flag	Coverage Δ
multisite	`58.05% <78.65%> (+0.56%)`	⬆️
single	`35.68% <78.65%> (+1.17%)`	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

b1ink0 added 4 commits December 9, 2024 18:05

Add site health checks for far-future expiration headers

dc9e364

Add filter for customizing far-future headers threshold

4cb07aa

Refactor far-future headers checks and improve asset validation logic

ec9a552

Merge branch 'trunk' into add/far-future-expiration-test

dd8e70d

b1ink0 commented Dec 9, 2024

View reviewed changes

plugins/performance-lab/includes/site-health/far-future-headers/helper.php Show resolved Hide resolved

b1ink0 commented Dec 9, 2024

View reviewed changes

b1ink0 added 4 commits December 10, 2024 14:05

Use assets from wp-includes for far-future headers site health check

0078cfc

Add filter to customize assets checked for far-future headers

3e5d958

Enhance far-future headers check to return detailed results and gener…

35e7977

…ate a report table for missing headers

Improve far-future headers checks by adding detailed failure reasons …

3db5886

…and updating the extensions table generation

b1ink0 marked this pull request as ready for review December 10, 2024 12:19

b1ink0 requested review from mukeshpanchal27 and felixarntz as code owners December 10, 2024 12:19

b1ink0 added 5 commits December 11, 2024 23:56

Add test to ensure status is 'good' when all headers are valid

655d4e5

Add test for assets with conditional caching

20099bf

Add test for status messages based on far-future headers results

54f3621

Add tests for filters affecting far-future headers asset checks

0a776c5

Refactor to use filenames instead of extenstions in failure details

3c98337

b1ink0 requested a review from westonruter January 6, 2025 18:10

westonruter reviewed Jan 7, 2025

View reviewed changes

b1ink0 added 3 commits January 8, 2025 20:32

Filter assets to ensure only string URLs are checked

7afd746

Update parameter type hints for far-future headers functions

3fcd641

Merge branch 'trunk' into add/far-future-expiration-test

54c73ec

westonruter added this to the performance-lab n.e.x.t milestone Jan 8, 2025

westonruter added [Type] Enhancement A suggestion for improvement of an existing feature [Plugin] Performance Lab Issue relates to work in the Performance Lab Plugin only labels Jan 8, 2025

westonruter reviewed Jan 8, 2025

View reviewed changes

b1ink0 added 4 commits January 9, 2025 21:08

Include max-age, expires actual value and threshold value in error me…

6a2da47

…ssage

Simplify filename extraction and conditional logic

48166ac

Refactor header checks to use null coalescing and improve conditionals

17d380e

Fix far future headers tests failing due to updated error messages

200b518

b1ink0 added 2 commits January 10, 2025 23:00

Improve error messages for far future headers check

1b4cef4

Merge branch 'trunk' into add/far-future-expiration-test

ed28196

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add Site Health test to verify that static assets are served with far-future expires #1727

Add Site Health test to verify that static assets are served with far-future expires #1727

b1ink0 commented Dec 9, 2024 •

edited

Loading

b1ink0 Dec 9, 2024

westonruter Dec 9, 2024

b1ink0 Dec 9, 2024

b1ink0 Dec 10, 2024 •

edited

Loading

github-actions bot commented Dec 10, 2024

westonruter Jan 7, 2025

b1ink0 Jan 8, 2025

westonruter Jan 8, 2025

westonruter Jan 7, 2025

b1ink0 Jan 8, 2025

westonruter Jan 8, 2025

westonruter Jan 8, 2025

b1ink0 Jan 9, 2025

westonruter Jan 9, 2025

westonruter Jan 8, 2025

westonruter Jan 8, 2025

westonruter Jan 8, 2025 •

edited

Loading

westonruter Jan 8, 2025

westonruter Jan 8, 2025

westonruter Jan 8, 2025

westonruter Jan 8, 2025

codecov bot commented Jan 9, 2025 •

edited

Loading

	if ( ! is_object( $headers ) ) {
	if ( count( $headers ) === 0 ) {

	$filename = isset( $path_info['basename'] ) ? $path_info['basename'] : basename( $asset );
	$filename = $path_info['basename'] ?? basename( $asset );

	if ( (bool) preg_match( '/max-age\s=\s(\d+)/', $control, $matches ) ) {
	if ( 1 === preg_match( '/max-age\s=\s(\d+)/', $control, $matches ) ) {

	if ( (bool) $expires_time && ( $expires_time - time() ) >= $threshold ) {
	if ( is_int( $expires_time ) && ( $expires_time - time() ) >= $threshold ) {

	if ( $max_age > 0 && $max_age < $threshold ) {
	if ( $max_age > 0 ) {

		$cache_control = isset( $headers['cache-control'] ) ? $headers['cache-control'] : '';
		$expires = isset( $headers['expires'] ) ? $headers['expires'] : '';

		$controls = is_array( $cache_control ) ? $cache_control : array( $cache_control );
		foreach ( $controls as $control ) {

	$controls = is_array( $cache_control ) ? $cache_control : array( $cache_control );
	foreach ( $controls as $control ) {
	foreach ( (array) $cache_control as $control ) {

Add Site Health test to verify that static assets are served with far-future expires #1727

Are you sure you want to change the base?

Add Site Health test to verify that static assets are served with far-future expires #1727

Conversation

b1ink0 commented Dec 9, 2024 • edited Loading

Summary

Relevant technical choices

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

b1ink0 Dec 10, 2024 • edited Loading

Choose a reason for hiding this comment

github-actions bot commented Dec 10, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

westonruter Jan 8, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov bot commented Jan 9, 2025 • edited Loading

Codecov Report

b1ink0 commented Dec 9, 2024 •

edited

Loading

b1ink0 Dec 10, 2024 •

edited

Loading

westonruter Jan 8, 2025 •

edited

Loading

codecov bot commented Jan 9, 2025 •

edited

Loading