Server Timing - Allow description and make duration optional#2379
Server Timing - Allow description and make duration optional#2379the-hercules wants to merge 6 commits intoWordPress:trunkfrom
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. |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## trunk #2379 +/- ##
==========================================
+ Coverage 69.17% 69.34% +0.17%
==========================================
Files 90 90
Lines 7708 7765 +57
==========================================
+ Hits 5332 5385 +53
- Misses 2376 2380 +4
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
plugins/performance-lab/tests/includes/server-timing/test-perflab-server-timing-metric.php
Show resolved
Hide resolved
plugins/performance-lab/includes/server-timing/class-perflab-server-timing-metric.php
Outdated
Show resolved
Hide resolved
plugins/performance-lab/includes/server-timing/class-perflab-server-timing-metric.php
Outdated
Show resolved
Hide resolved
plugins/performance-lab/includes/server-timing/class-perflab-server-timing.php
Outdated
Show resolved
Hide resolved
plugins/performance-lab/tests/includes/server-timing/test-perflab-server-timing.php
Outdated
Show resolved
Hide resolved
plugins/performance-lab/tests/includes/server-timing/test-perflab-server-timing.php
Outdated
Show resolved
Hide resolved
plugins/performance-lab/tests/includes/server-timing/test-perflab-server-timing.php
Show resolved
Hide resolved
plugins/performance-lab/tests/includes/server-timing/test-perflab-server-timing.php
Outdated
Show resolved
Hide resolved
…and improve type definitions - Replace string|mixed param with native string type hint in set_description(), removing manual type check - Explicitly initialize $description = null - Combine addcslashes() calls into single expression - Add @Covers annotations to new description test methods - Import MetricArguments phpstan type and replace mixed with array<string, MetricArguments> in test param annotations
plugins/performance-lab/includes/server-timing/class-perflab-server-timing.php
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Pull request overview
Updates the Performance Lab Server-Timing API to better match the Server-Timing header spec by allowing metrics to include an optional description (desc) and by making duration (dur) optional when formatting header segments.
Changes:
- Add
descriptionsupport toPerflab_Server_Timing_Metricvia new setter/getter. - Update Server-Timing header formatting to conditionally include
dur=and/ordesc=and sanitizedescfor safe header output. - Extend unit tests to cover duration+description combinations and escaping/CRLF stripping edge cases.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| plugins/performance-lab/includes/server-timing/class-perflab-server-timing.php | Formats metric segments with optional dur and desc, including description sanitization. |
| plugins/performance-lab/includes/server-timing/class-perflab-server-timing-metric.php | Adds a new $description field plus set_description() / get_description(). |
| plugins/performance-lab/tests/includes/server-timing/test-perflab-server-timing.php | Adds tests for header output with descriptions and special-character edge cases. |
| plugins/performance-lab/tests/includes/server-timing/test-perflab-server-timing-metric.php | Adds unit tests for description getter/setter behavior. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| // If neither value nor description is set, skip this metric. | ||
| if ( null === $value && null === $description ) { | ||
| return null; | ||
| } |
There was a problem hiding this comment.
The PR description mentions supporting a “name only” header format (e.g. wp-metric), but this implementation still skips metrics when both duration and description are null (returns null), so name-only metrics will never be emitted. If MDN compliance is the goal, consider returning the metric name when both are null and adding a corresponding test case.
There was a problem hiding this comment.
I'll update it to remove the "name only" format since that was inaccurate.Metric with neither duration nor description provides no useful information in the header. This PR only adds description as a new optional field alongside the existing optional duration.
There was a problem hiding this comment.
Metric with neither duration nor description provides no useful information in the header.
Is this so? In the MDN docs, this example is provided:
// A metric with a name only
Server-Timing: missedCache
There was a problem hiding this comment.
@westonruter Added support for name only.
| * @param non-empty-string $description The metric description. | ||
| */ | ||
| public function set_description( string $description ): void { | ||
| if ( 0 !== did_action( 'perflab_server_timing_send_header' ) && ! doing_action( 'perflab_server_timing_send_header' ) ) { | ||
| _doing_it_wrong( | ||
| __METHOD__, | ||
| /* translators: %s: WordPress action name */ | ||
| sprintf( esc_html__( 'The method must be called before or during the %s action.', 'performance-lab' ), 'perflab_server_timing_send_header' ), | ||
| '' | ||
| ); | ||
| return; | ||
| } | ||
|
|
||
| $this->description = $description; | ||
| } |
There was a problem hiding this comment.
The PHPDoc declares $description as non-empty-string, but the method currently accepts and stores empty strings without any validation. Either enforce non-empty input (e.g. _doing_it_wrong() on empty string) or relax the docblock to string so the contract matches runtime behavior.
There was a problem hiding this comment.
Leaving it as it is, relying on the type hint for the developer.
| public function set_description( string $description ): void { | ||
| if ( 0 !== did_action( 'perflab_server_timing_send_header' ) && ! doing_action( 'perflab_server_timing_send_header' ) ) { | ||
| _doing_it_wrong( | ||
| __METHOD__, | ||
| /* translators: %s: WordPress action name */ | ||
| sprintf( esc_html__( 'The method must be called before or during the %s action.', 'performance-lab' ), 'perflab_server_timing_send_header' ), | ||
| '' | ||
| ); | ||
| return; | ||
| } |
There was a problem hiding this comment.
set_description() adds a new “must be called before or during perflab_server_timing_send_header” guard (via _doing_it_wrong() and early return), but there is no test covering the late-call behavior (similar to test_set_value_prevents_late_measurement). Adding a unit test for the late-call path would help prevent regressions.
There was a problem hiding this comment.
The late-call guard in set_description() uses the exact same
did_action() / doing_action() pattern as the existing set_value() method
(lines 91–99).
Adding a test for this would require calling:
do_action( 'perflab_server_timing_send_header' );However, this triggers the global singleton to re-register default metrics
(e.g., before-template), which causes test isolation failures.
…n and improved performance.
|
@westonruter I’ve added support for name-only metrics as suggested. When you get a chance, could you please take a look? |
Summary
Fixes #879
Relevant technical choices
Enhance Server-Timing API for MDN Specification Compliance
Enhances the Server-Timing API to be fully compliant with the MDN Server-Timing specification by making
durationoptional and adding support for adesc(description) field.Changes
class-perflab-server-timing-metric.php$descriptionproperty toPerflab_Server_Timing_Metricset_description()andget_description()methodsstringtype hint forset_description()instead ofstring|mixedwith manual type checking$description = nullfor clarityclass-perflab-server-timing.phpformat_metric_header_value()to conditionally includedur=anddesc=parameters\and"addcslashes()callTests
@coversannotations to all new test methods",\,\n,\r)@phpstan-param array<string, mixed>with:@param array<string, MetricArguments>@phpstan-import-typefor accurate type definitionsSupported Header Formats
wp-metric;dur=45.23wp-metric;desc="Cache hit"wp-metric;dur=45.23;desc="Database queries"Testing
Fixes
Fixes #879
Use of AI Tools
Github Copilot CLI was used to write tests. Reviewed the code afterwards.