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: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: felixarntz <[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-17T20:33:04Z

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

+	$etag          = isset( $headers['etag'] ) ? $headers['etag'] : '';
+	$last_modified = isset( $headers['last-modified'] ) ? $headers['last-modified'] : '';


Minor improvement.

Suggested change

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

$last_modified = isset( $headers['last-modified'] ) ? $headers['last-modified'] : '';

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

$last_modified = $headers['last-modified'] ?? '';

Done in 12d0b71

westonruter · 2025-01-17T20:49:58Z

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

+		$check = perflab_ffh_check_headers( $headers );
+		if ( isset( $check['passed'] ) && $check['passed'] ) {
+			// This asset passed far-future headers test, no action needed.
+			continue;
+		}
+
+		// If not passed, decide whether to try conditional request.
+		if ( false === $check ) {


The conditions here seem a bit confusing to me, related to how perflab_ffh_check_headers() returns either an array or false. I think it would be preferable if perflab_ffh_check_headers always returned an array. Maybe it could always return an array key for missing_max_age which is a boolean. And then this could be changed to:

Suggested change

$check = perflab_ffh_check_headers( $headers );

if ( isset( $check['passed'] ) && $check['passed'] ) {

// This asset passed far-future headers test, no action needed.

continue;

}

// If not passed, decide whether to try conditional request.

if ( false === $check ) {

$check = perflab_ffh_check_headers( $headers );

if ( $check['passed'] ) {

// This asset passed far-future headers test, no action needed.

continue;

}

// If not passed, decide whether to try conditional request.

if ( $check['missing_max_age'] ) {

Done in 12d0b71

westonruter · 2025-01-17T21:41:47Z

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

+		$conditional_headers['If-None-Match'] = $etag;
+	}
+	if ( '' !== $last_modified ) {
+		$conditional_headers['If-Modified-Since'] = $last_modified;


Looks like test coverage for this line is warranted.

westonruter · 2025-01-17T21:42:49Z

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

+			return array(
+				'passed' => false,
+				'reason' => sprintf(
+					/* translators: 1: actual max-age value in seconds, 2: threshold in seconds */
+					__( 'max-age below threshold (actual: %1$s seconds, threshold: %2$s seconds)', 'performance-lab' ),
+					number_format_i18n( $max_age ),
+					number_format_i18n( $threshold )
+				),
+			);


Looks like a test is missing for the max-age being present but it not being high enough.

westonruter · 2025-01-17T21:43:47Z

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

+function perflab_ffh_add_test( array $tests ): array {
+	$tests['direct']['far_future_headers'] = array(
+		'label' => __( 'Effective Caching Headers', 'performance-lab' ),
+		'test'  => 'perflab_ffh_assets_test',
+	);
+	return $tests;
+}
+add_filter( 'site_status_tests', 'perflab_ffh_add_test' );


A trivial function to add a test for, but it would ensure test coverage.

Done in aec6f80

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

westonruter · 2025-01-17T21:45:24Z

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

+		if ( is_wp_error( $response ) ) {
+			// Can't determine headers if request failed, consider it a fail.
+			$final_status   = 'recommended';
+			$fail_details[] = array(
+				'filename' => $filename,
+				'reason'   => __( 'Could not retrieve headers', 'performance-lab' ),
+			);
+			continue;
+		}
+
+		$headers = wp_remote_retrieve_headers( $response );
+		if ( ! is_object( $headers ) && 0 === count( $headers ) ) {
+			// No valid headers retrieved.
+			$final_status   = 'recommended';
+			$fail_details[] = array(
+				'filename' => $filename,
+				'reason'   => __( 'No valid headers retrieved', 'performance-lab' ),
+			);
+			continue;
+		}


These two if statements lack tests, but I don't think they are critical.

Done in aec6f80

…l logic

felixarntz

@b1ink0 Technically LGTM, though I would suggest we rename the check to something more purpose-driven and less technical.

felixarntz · 2025-01-21T17:41:58Z

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

+			)
+		),
+		'actions'     => '',
+		'test'        => 'is_far_future_headers_enabled',


Can we maybe use effective_cache_headers for the name / identifier of this entire functionality? I just find this far_future_headers oddly specific, and it sounds strange to me. The purpose for this Site Health is to have "good" caching headers, and I think we should frame the feature as such, rather than the more technical and IMO too specific "far future" terminology.

This applies to all the identifiers using far_future_headers, like this function, the Site Health check itself, the directory name etc. I think we should use effective_cache_headers.

I think we should use asset somewhere in the name because this should be differentiated from the effective Cache-Control header for page responses so as to not break bfcache (#1807).

So maybe effective_asset_cache_headers?

@westonruter That works for me!

felixarntz · 2025-01-21T17:42:32Z

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

+	 *
+	 * @param int $threshold Threshold in seconds.
+	 */
+	$threshold = apply_filters( 'perflab_far_future_headers_threshold', YEAR_IN_SECONDS );


Maybe better as:

Suggested change

$threshold = apply_filters( 'perflab_far_future_headers_threshold', YEAR_IN_SECONDS );

$threshold = apply_filters( 'perflab_effective_cache_headers_expiration_threshold', YEAR_IN_SECONDS );

Per above:

Suggested change

$threshold = apply_filters( 'perflab_far_future_headers_threshold', YEAR_IN_SECONDS );

$threshold = apply_filters( 'perflab_effective_asset_cache_headers_expiration_threshold', YEAR_IN_SECONDS );

felixarntz · 2025-01-21T17:43:01Z

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

+ * @return array{direct: array<string, array{label: string, test: string}>} Amended tests.
+ */
+function perflab_ffh_add_test( array $tests ): array {
+	$tests['direct']['far_future_headers'] = array(


See above:

Suggested change

$tests['direct']['far_future_headers'] = array(

$tests['direct']['effective_cache_headers'] = array(

Per above:

Suggested change

$tests['direct']['far_future_headers'] = array(

$tests['direct']['effective_asset_cache_headers'] = array(

westonruter · 2025-01-21T18:34:06Z

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

+if ( ! defined( 'ABSPATH' ) ) {
+	exit; // Exit if accessed directly.
+}


Per #1815:

Suggested change

if ( ! defined( 'ABSPATH' ) ) {

exit; // Exit if accessed directly.

}

// @codeCoverageIgnoreStart

if ( ! defined( 'ABSPATH' ) ) {

exit; // Exit if accessed directly.

}

// @codeCoverageIgnoreEnd

westonruter · 2025-01-21T18:34:22Z

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

+if ( ! defined( 'ABSPATH' ) ) {
+	exit; // Exit if accessed directly.
+}


Per #1815:

Suggested change

if ( ! defined( 'ABSPATH' ) ) {

exit; // Exit if accessed directly.

}

// @codeCoverageIgnoreStart

if ( ! defined( 'ABSPATH' ) ) {

exit; // Exit if accessed directly.

}

// @codeCoverageIgnoreEnd

…asset-cache-headers'

…ders

…th checks

felixarntz

@b1ink0 Thank you for the updates, LGTM!

Just one minor suggestion to fix the last remaining reference to the old names.

...ts/includes/site-health/effective-asset-cache-headers/test-effective-asset-cache-headers.php

Co-authored-by: Felix Arntz <[email protected]>

westonruter

Attention: Patch coverage is 98.36066%

🎉

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 Outdated Show resolved Hide resolved

b1ink0 commented Dec 9, 2024

View reviewed changes

plugins/performance-lab/includes/site-health/far-future-headers/helper.php Outdated 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 3 commits January 16, 2025 01:35

Update labels to make them less technical

ee3a57d

Add cover annotations for tests and test for when no assets are passed

f6ba392

Mark functions as private

0620840

b1ink0 requested review from felixarntz and westonruter January 15, 2025 20:38

westonruter mentioned this pull request Jan 16, 2025

Use of Cache-Control should be flagged by Site Health for unauthenticated frontend responses due to preventing bfcache #1692

Closed

b1ink0 and others added 2 commits January 16, 2025 22:36

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

7ebdc87

Add covers tags

49ca83f

westonruter reviewed Jan 17, 2025

View reviewed changes

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

westonruter reviewed Jan 17, 2025

View reviewed changes

b1ink0 added 2 commits January 20, 2025 16:11

Refactor to only return array to improve clarity, Simplify conditiona…

12d0b71

…l logic

Add additional tests to improve code coverage

aec6f80

b1ink0 requested a review from westonruter January 20, 2025 19:19

felixarntz reviewed Jan 21, 2025

View reviewed changes

westonruter reviewed Jan 21, 2025

View reviewed changes

b1ink0 added 4 commits January 23, 2025 00:34

Rename directories and files from 'far-furure-headers' to 'effective-…

2190d60

…asset-cache-headers'

Rename functions and update comments to reflect effective caching hea…

4f1a00d

…ders

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

2d08732

Update comments to reflect effective asset cache headers in site heal…

a048e9f

…th checks

b1ink0 requested review from westonruter and felixarntz January 22, 2025 20:22

felixarntz approved these changes Jan 22, 2025

View reviewed changes

...ts/includes/site-health/effective-asset-cache-headers/test-effective-asset-cache-headers.php Outdated Show resolved Hide resolved

Update comment

2c70bd0

Co-authored-by: Felix Arntz <[email protected]>

westonruter approved these changes Jan 22, 2025

View reviewed changes

westonruter merged commit d44b4c1 into WordPress:trunk Jan 22, 2025
16 checks passed

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

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

		$etag = isset( $headers['etag'] ) ? $headers['etag'] : '';
		$last_modified = isset( $headers['last-modified'] ) ? $headers['last-modified'] : '';

	$threshold = apply_filters( 'perflab_far_future_headers_threshold', YEAR_IN_SECONDS );
	$threshold = apply_filters( 'perflab_effective_cache_headers_expiration_threshold', YEAR_IN_SECONDS );

	$tests['direct']['far_future_headers'] = array(
	$tests['direct']['effective_cache_headers'] = array(

	$tests['direct']['far_future_headers'] = array(
	$tests['direct']['effective_asset_cache_headers'] = array(

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

Conversation

b1ink0 commented Dec 9, 2024 • edited Loading

Summary

Relevant technical choices

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 • 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

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

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

felixarntz left a 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

felixarntz left a comment

Choose a reason for hiding this comment

westonruter left a comment

Choose a reason for hiding this comment

b1ink0 commented Dec 9, 2024 •

edited

Loading

b1ink0 Dec 10, 2024 •

edited

Loading

github-actions bot commented Dec 10, 2024 •

edited

Loading