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.
See test in the MR.
Time that would be stored with a time value when no request info is available, and return that value across any subsequent calls.
Show commandsStart 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:
changes, plain diff MR !4303
Comments
Comment #3
mondrakeComment #4
mondrakeOn this.
Comment #5
mondrakeIMHO we should be able to instantiate
Timealso with no request stack. Yes having the request stack available is in the normal path with container injection, but you may want to have aTimeinstance available also in early bootstrap containers and/or unit tests whenrequest_pathservice is not a thing.That is clearly coming up in #3113971: Replace REQUEST_TIME in services.
Comment #6
mondrakeComment #7
mondrakeComment #8
spokjeDropped 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.
Comment #9
mondrake@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 :)
Comment #10
spokjeNo 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 ;)
Comment #12
quietone commentedI'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.
Comment #14
longwaveThis 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!