Problem/Motivation

Time::getRequestTime() and Time::getRequestMicroTime() are meant to return the same value during the life of a request. That is meant to be the time the HTTP request was made to the server.

However, when there is no request object available, and no global REQUEST_TIME variable defined, which can be a case for unit tests, the code falls back to calling ::getCurrentTime(), which differs each time is called - and fails the original purpose.

Steps to reproduce

See test in the MR.

Proposed resolution

  • Add a property to Time that would be stored with a time value when no request info is available, and return that value across any subsequent calls.
  • Make the RequestStack argument to the constructor optional for those cases when a request stack is not available yet or at all.

Remaining tasks

User interface changes

API changes

Data model changes

Release notes snippet

Issue fork drupal-3371840

Command icon Show commands

Start within a Git clone of the project using the version control instructions.

Or, if you do not have SSH keys set up on git.drupalcode.org:

About issue forks

Comments

mondrake created an issue. See original summary.

mondrake opened merge request !4303

mondrake’s picture

mondrake
Location 🇮🇹
commented
Issue summary: View changes
mondrake’s picture

mondrake
Location 🇮🇹
commented
Assigned: Unassigned » mondrake

On this.

mondrake’s picture

mondrake
Location 🇮🇹
commented
Assigned: mondrake » Unassigned
Status: Active » Needs review

IMHO we should be able to instantiate Time also with no request stack. Yes having the request stack available is in the normal path with container injection, but you may want to have a Time instance available also in early bootstrap containers and/or unit tests when request_path service is not a thing.

That is clearly coming up in #3113971: Replace REQUEST_TIME in services.

mondrake’s picture

mondrake
Location 🇮🇹
commented
Issue summary: View changes
mondrake’s picture

mondrake
Location 🇮🇹
commented
Issue summary: View changes
spokje’s picture

spokje
Primary language Dutch
commented
Status: Needs review » Reviewed & tested by the community

Dropped some minor, non-RTBC-blocking nits in the MR.

Not claiming to fully understand this, but the parts I do get make sense, moving to RTBC to lure core-committer eyes 'n brains to it.

mondrake’s picture

mondrake
Location 🇮🇹
commented

@Spokje if you want, share your doubts and I will certainly try to address them - and if I can't then at least we make them shout :)

spokje’s picture

spokje
Primary language Dutch
commented

No doubts, cause for the issue seems legit, also because many moons ago I worked on a related issue where tests went wrong on timing of Time.

Also can't come up with a better or even other way to do this, so I'm getting the popcorn and await the shouting ;)

voleger made their first commit to this issue’s fork.

quietone’s picture

quietone commented

I'm triaging RTBC issues.

After reading the IS and comments I don't see anything left to do. I did a scan of the comments in the patch and they all made sense.

Leaving at RTBC.

  • longwave committed 5427d50c on 11.x 
    Issue #3371840 by mondrake, Spokje: Time::getRequestTime is not...
longwave’s picture

longwave
he/him
Primary language English
Location UK
commented
Status: Reviewed & tested by the community » Fixed

This makes sense to me and I see no better way of doing it. It adds no new API, only changing the fallback return value in an edge case, which will help in a small number of tests.

Also not a fan of sleep() in tests but as we are testing time itself here I also see no better way of doing that!

Not backported to 10.1.x as this is a minor behaviour change and the REQUEST_TIME changes likely won't need to be backported either.

Committed 5427d50 and pushed to 11.x. Thanks!

mondrake closed merge request !4303

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.