| FazBrowse GitHub Viewer | Trending | | Home |
| Tools: [Download Repo ZIP] [Original HTTPS Page] |
Sorry, something went wrong.
|
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: Gilmoursa <gilmoursa@git.wordpress.org> Co-authored-by: westonruter <westonruter@git.wordpress.org> Co-authored-by: tejas0306 <suhan2411@git.wordpress.org> To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook. |
Sorry, something went wrong.
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>
Codecov Report❌ Patch coverage is 95.55556% with 2 lines in your changes missing coverage. Please review.
@@ 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.
|
Sorry, something went wrong.
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.
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:
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.
Sorry, something went wrong.
…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 no-cache or max-age=0 would require a browser to do a round-trip to do a conditional request to see if there is a newer version available. Also, what if there is an Expires response header with a past date and there is no Cache-Control header (since Cache-Control supersedes Expires)? So it seems there are additional scenarios that can be included in the test as failures. |
Sorry, something went wrong.
There was a problem hiding this comment.
See #2481 (comment)
Sorry, something went wrong.
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. |
Sorry, something went wrong.
… 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>
Sorry, something went wrong.
| Back | FazBrowse Home | New Git URL |
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, recommended status), 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 of 0 — 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:
no-store forbids caching outright, so the wording is unqualified.
These permit caching but force revalidation on every request, so the wording is softened.
The Cache-Control directives are parsed into a name⇒value map, so max-age is compared numerically. This catches variant forms such as max-age="0" and max-age = 0 (whitespace around the =), while max-age=3600 still passes.
When there is no Cache-Control header, the check falls back to Expires:
When there is neither Cache-Control nor Expires, the browser can only fall back to heuristic caching, which is only viable when a validator is present:
Changes
Test plan