Skip to content

Add REQUEST_TIME and REQUEST_TIME_FLOAT to server bag#114

Merged
s2x merged 2 commits intomasterfrom
feature/67-request-time-float
Apr 5, 2026
Merged

Add REQUEST_TIME and REQUEST_TIME_FLOAT to server bag#114
s2x merged 2 commits intomasterfrom
feature/67-request-time-float

Conversation

@s2x
Copy link
Copy Markdown
Collaborator

@s2x s2x commented Apr 5, 2026

Summary

  • Adds REQUEST_TIME (unix timestamp) and REQUEST_TIME_FLOAT (unix timestamp with microseconds) to the server bag in RequestConverter
  • Enables Symfony profiler and debug toolbar to show request duration
  • Values are set using time() and microtime(true) at request conversion time

Changes

  • src/DTO/RequestConverter.php: Added REQUEST_TIME and REQUEST_TIME_FLOAT to the server array
  • tests/RequestConverterTest.php: Added unit test testRequestTimeAndRequestTimeFloatAreSet to verify the values are correctly set
  • CHANGELOG.md: Added entry under [Unreleased] section

Closes #67

@s2x
Copy link
Copy Markdown
Collaborator Author

s2x commented Apr 5, 2026

Code Review

Strengths

  • Clean, minimal implementation matching the issue spec exactly
  • Test covers type assertions and temporal proximity as required
  • CHANGELOG entry is clear and links to the issue
  • Placement in the server array is logical

Issues (all must be addressed before merge)

Important

  1. tests/RequestConverterTest.php:353 — The assertEqualsWithDelta assertion compares microtime(true) at assertion time against the value captured at conversion time. The 1.0 second delta is generous enough to pass, but it does not verify that REQUEST_TIME and REQUEST_TIME_FLOAT are consistent with each other (i.e., that they represent the same moment). A tighter assertion or capturing microtime(true) before the call would be more rigorous.
    • Why it matters: If someone later changes the order of time() and microtime(true) calls or inserts logic between them, the test would not catch a meaningful drift.
    • How to fix: Capture $before = microtime(true) before the call and assert $symfonyRequest->server->get('REQUEST_TIME_FLOAT') is within a small delta (e.g., 0.1s) of $before.

Minor

  1. src/DTO/RequestConverter.php:46-47time() and microtime(true) are called separately, which means they could theoretically span a second boundary (e.g., REQUEST_TIME = 1000, REQUEST_TIME_FLOAT = 1001.0001). In practice this is extremely unlikely, but for correctness you could derive REQUEST_TIME from (int) $requestTimeFloat.
    • Why it matters: Edge case correctness; Symfony itself derives REQUEST_TIME from casting REQUEST_TIME_FLOAT.
    • How to fix:
      $requestTimeFloat = microtime(true);
      // ...
      'REQUEST_TIME' => (int) $requestTimeFloat,
      'REQUEST_TIME_FLOAT' => $requestTimeFloat,

All issues above must be addressed before this PR can be merged.

Piotr Hałas added 2 commits April 5, 2026 22:26
- Enables Symfony profiler and debug toolbar to show request duration
- Sets values using time() and microtime(true) at request conversion time
- Adds unit test for the new values

Closes #67
- Capture microtime(true) once and derive REQUEST_TIME from it
- Tighten test assertions to 0.1s delta
- Add assertion verifying REQUEST_TIME equals cast REQUEST_TIME_FLOAT
@s2x s2x force-pushed the feature/67-request-time-float branch from 37f1c4c to 1b44dd1 Compare April 5, 2026 20:26
@s2x s2x merged commit e2e2d0c into master Apr 5, 2026
21 checks passed
@s2x s2x deleted the feature/67-request-time-float branch April 5, 2026 20:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

RequestConverter — Missing REQUEST_TIME_FLOAT breaks profiler and debug toolbar

1 participant