The PHP function time() is used directly in core in lots of places (see report.txt for core/ usages). During long requests, this value can change, which can lead to testing difficulties (sporadic and unpredictable results).
Most uses of time() should be replaced with $requestStack->getCurrentRequest()->server->get('REQUEST_TIME') or $request->server->get('REQUEST_TIME') to allow for better testing. When using the current request isn't feasible, REQUEST_TIME can be used instead.
time() should remain in cases where the difference between REQUEST_TIME and the current time is explicitly needed, like in the batch functions.
| Comment | File | Size | Author |
|---|---|---|---|
| report.txt | 11.69 KB | mpdonadio |
Comments
Comment #2
tr commentedThis has been the policy/recommendation since 2008 (Drupal 7), so if usage of time() has crept back into core, or perhaps never fully removed from core, then it's a bug...
https://www.drupal.org/update/modules/6/7#time
Comment #3
chi commentedLooks too verbose.
Comment #4
mpdonadioStarted on this yesterday, so assigning myself until I post a first pass.
Re #3, there is an issue about this (I'll try to dig it up). Using the injected requeststack / request is the only way to reliable test certain scenarios. Using the REQUEST_TIME global is problematic in testing situations.
Comment #5
chi commented@mpdonadio is it possible to create some shortcut public method for this?
Comment #6
mpdonadio#5, the request stack is already injected all around core, and so is the current request in a lot of places. Both some from Symfony HTTP Foundation, so a shortcut would mean subclassing and a lot of refactoring. The other option would be to create a new service for getting the current date/time, but I think this would be overkill.
Comment #11
joachim commented> The other option would be to create a new service for getting the current date/time, but I think this would be overkill.
That has since been done, which makes this issue obsolete.
Comment #12
mpdonadio