Enqueued Assets audit: treat zero-size and no-store responses as errors#2481
Conversation
|
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 If you're merging code through a pull request on GitHub, copy and paste the following into the bottom of the merge commit message. To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook. |
In perflab_aea_get_asset_size(), two cases that should have been
treated as errors were silently passing the audit:
- A 200 response with an empty body now returns WP_Error('zero_size').
An asset that returns nothing is effectively broken, regardless of the
status code.
- A 200 response with Cache-Control: no-store now returns
WP_Error('not_cacheable'). An asset that browsers are forbidden from
caching is a clear performance problem and should surface in the audit.
Cache-Control: no-cache (revalidate) is intentionally left as passing
since the asset can still be served from cache after revalidation.
Both cases were already noted as TODOs in the original code. Removes
those TODO comments and adds tests covering zero-size, no-store,
no-cache (should pass), and normal cacheable responses. Also updates
the mock class to pass response headers through so header-dependent
tests can work end-to-end.
Fixes WordPress#2417.
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
7168784 to
4bcfe31
Compare
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## trunk #2481 +/- ##
==========================================
+ Coverage 69.72% 69.87% +0.14%
==========================================
Files 90 90
Lines 7753 7797 +44
==========================================
+ Hits 5406 5448 +42
- Misses 2347 2349 +2
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
wp_remote_retrieve_header() returns string|array (array when the same header appears multiple times in the response). Joining array values with implode() before passing to strtolower() resolves the PHPStan error and also correctly handles multi-value Cache-Control headers. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
Pull request overview
This PR updates the Enqueued Assets Site Health audit so empty asset responses and Cache-Control: no-store responses are surfaced as asset errors instead of passing as successful zero-byte/cacheable assets.
Changes:
- Returns
WP_Errorfor zero-size asset bodies andCache-Control: no-store. - Adds tests for zero-size and cache-control handling.
- Updates the audit asset test mock to pass response headers through.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
plugins/performance-lab/includes/site-health/audit-enqueued-assets/helper.php |
Adds empty-body and no-store error handling in asset size retrieval. |
plugins/performance-lab/tests/includes/site-health/audit-enqueued-assets/test-audit-enqueued-assets-helper.php |
Updates and expands coverage for the new asset-size error behavior. |
plugins/performance-lab/tests/data/class-audit-assets-mock-assets.php |
Includes mocked headers in HTTP responses for header-dependent tests. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
…ze check Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
|
Sorry for the delay in following up on this.
Should it? A directive of Also, what if there is an So it seems there are additional scenarios that can be included in the test as failures. |
Per reviewer feedback: - no-cache and max-age=0 force revalidation on every request, which is a performance problem for static assets. Flag these as not_cacheable alongside no-store. - When no Cache-Control header is present, check the Expires header. A past date means the browser treats the asset as uncacheable. - Update error message to be directive-agnostic. - Add test cases for no-cache, max-age=0, past Expires, and future Expires. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
|
Thanks for the feedback. I've updated the PR to address both points:
Test cases for all new scenarios are included. |
… fix/2417-enqueued-assets-zero-size-no-store
Make the malformed Expires handling explicit rather than relying on strtotime() returning false coercing to 0 in the comparison. An unparseable Expires value is now deliberately treated as already expired (and thus not cacheable) per RFC 7234. Add test coverage for an invalid Expires value. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Parse Cache-Control into a name => value map so max-age is compared numerically rather than matched against the exact "max-age=0" token. This now also catches variant forms like max-age="0" and max-age = 0 (whitespace around the equals sign). Add test coverage for the quoted and whitespace-padded max-age forms. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
no-store truly forbids caching, so its message stays as "cannot be cached by browsers." But no-cache and max-age=0 permit caching while forcing revalidation, and a past Expires means the asset is merely stale, so those cases now report "may not be fully cacheable by browsers" to avoid overstating the problem. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…dator When a response has neither Cache-Control nor Expires, browsers can only apply heuristic caching, and only when a validator (Last-Modified or ETag) is present. Without any validator the asset effectively cannot be cached, so flag it as not_cacheable with the soft message. Responses that carry a validator still pass, since heuristic caching is viable. Update the default good-asset mock to send Cache-Control: max-age=3600 so existing audit tests continue to represent cacheable assets, and add coverage for the no-headers, Last-Modified-only, and ETag-only cases. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>

Summary
Fixes #2417.
perflab_aea_get_asset_size()had two TODOs in the code for cases that silently passed the audit when they should be flagged as errors:Behaviour change
Both new failure modes return a
WP_Error, which feeds into the existing error display path in the audit table (red row, error message shown,recommendedstatus), so no additional UI changes are needed. An asset with an error also fails the test even when the count/byte thresholds are not exceeded.Zero-size
Returns
WP_Error( 'zero_size' )instead of0— message: "The asset returned an empty response body."Cache policy
Returns
WP_Error( 'not_cacheable' )when the response is not cacheable or only weakly cacheable. The error message is tailored to the severity of the directive:Cache-Control: no-store→ "The asset response cannot be cached by browsers."no-storeforbids caching outright, so the wording is unqualified.Cache-Control: no-cacheandCache-Control: max-age=0→ "The asset response may not be fully cacheable by browsers."These permit caching but force revalidation on every request, so the wording is softened.
The
Cache-Controldirectives are parsed into a name⇒value map, somax-ageis compared numerically. This catches variant forms such asmax-age="0"andmax-age = 0(whitespace around the=), whilemax-age=3600still passes.When there is no
Cache-Controlheader, the check falls back toExpires:Expireswith a past date (and noCache-Control) → flagged (soft message). The asset is already expired.Expiresvalue → treated as already expired per RFC 7234, and flagged.Expireswith a future date → passes.When there is neither
Cache-ControlnorExpires, the browser can only fall back to heuristic caching, which is only viable when a validator is present:Cache-Control, noExpires, and noLast-Modified/ETag→ flagged (soft message). The asset effectively cannot be cached.Cache-Control, noExpires, but aLast-ModifiedorETagis present → passes. Heuristic caching is viable, and most servers sendLast-Modifiedfor static files by default, so this avoids false positives.Changes
plugins/performance-lab/includes/site-health/audit-enqueued-assets/helper.php: implement the zero-size and cache-policy checks, remove the TODO comments.plugins/performance-lab/tests/includes/site-health/audit-enqueued-assets/test-audit-enqueued-assets-helper.php: update the zero-size assertion to expectWP_Error; addtest_perflab_aea_get_asset_size_not_cacheable()coveringno-store,no-cache,max-age=0,max-age="0",max-age = 0, array-valuedCache-Control, past/future/invalidExpires, no-headers (with and without aLast-Modified/ETagvalidator), and a normal cacheable response.plugins/performance-lab/tests/data/class-audit-assets-mock-assets.php: passheadersfrom mocked responses through thepre_http_requestfilter so header-dependent tests work end-to-end, and give the default good-asset mock aCache-Control: max-age=3600header so existing audit tests continue to represent cacheable assets.Test plan
composer test:integration -- --filter test_perflab_aea_get_asset_sizecomposer test:integration -- --filter test_perflab_aea_get_asset_size_not_cacheableCache-Control: no-storeshows as an error row in the tableCache-Control: no-cacheshows as an error row in the tableCache-Control: max-age=0shows as an error row in the tableExpiresand noCache-Controlshows as an error row in the tableCache-Control, noExpires, and noLast-Modified/ETagshows as an error row in the tableCache-Control/Expiresbut aLast-ModifiedorETagstill passes the auditCache-Control: max-age=3600still passes the audit