Add a time service to provide consistent interaction with time()/microtime() and superglobals

Yeah we already converted quite some of the usecases for code that does unit testing, see https://www.drupal.org/node/2463059

Would it be worth to open an upstream issue to sugesting adding $request->getRequestTime() or so? Looking at our code base, knowing that seems like a pretty common requirement?

Well, I think this won't happen, because people will move to PSR-7, that doesn't have it, but you can always try.

/**
 * Time of the current request in seconds elapsed since the Unix Epoch.
 *
 * This differs from $_SERVER['REQUEST_TIME'], which is stored as a float
 * since PHP 5.4.0. Float timestamps confuse most PHP functions
 * (including date_create()).
 *
 * @see http://php.net/manual/reserved.variables.server.php
 * @see http://php.net/manual/function.time.php
 */
define('REQUEST_TIME', (int) $_SERVER['REQUEST_TIME']);

Seems like the was another reason to create the constant.

But that comment actually looks wrong for php5.6 at least... if I dump $_SERVER I see...

     "REQUEST_TIME_FLOAT" => 1462262200.1395,
     "REQUEST_TIME" => 1462262200,

Looking at the PHP docs...

'REQUEST_TIME'
The timestamp of the start of the request. Available since PHP 5.1.0.
'REQUEST_TIME_FLOAT'
The timestamp of the start of the request, with microsecond precision. Available since PHP 5.4.0.

https://secure.php.net/manual/en/reserved.variables.server.php

Yeah, its always an int in our relevant PHP versions, see https://3v4l.org/VKWSU

We have an issue to remove the cast in the define() and some other random casts in #1209470: REQUEST_TIME is a float with microseconds on PHP 5.4.

Some more related issues, and the change record: The REQUEST_TIME constant can no longer be used in PHPUnit tests.

I'm fairly against the proposed resolution. If we went through the trouble of using modern PHP OO techniques in Drupal 8, then is seems counterintuitive to go back to using a superglobal, especially one that changes.

I thought I made an issue about this, but maybe it was just a conversation I had (or the voices in my head). I think we should introduce a time service, to abstract away $_SERVER['REQUEST_TIME'], time(), microtime(), and maybe a few others.

interface TimeInterface {
  public function getRequestTime();
  public function getCurrentTime();
  public function getCurrentMicroTime();
}

Give the time service the request stack to get the request time from the current request, and then provide thin wrappers around the PHP functions for the rest. Then classes that need these can use mocking for sane unit testing. Just not sure how we could provide full test coverage when by definition, some of these functions don't return the same value of successive calls (have never tried the namespace shenanigans for shadowing the system calls).

Something like this.

1. +++ b/core/lib/Drupal/Core/Datetime/Time.php
   diff --git a/core/lib/Drupal/Core/Datetime/TimeInterface.php b/core/lib/Drupal/Core/Datetime/TimeInterface.php
   new file mode 100644
   
   new file mode 100644
   index 0000000..8917b91
   
   index 0000000..8917b91
   --- /dev/null
   
   --- /dev/null
   +++ b/core/lib/Drupal/Core/Datetime/TimeInterface.php
   
   +++ b/core/lib/Drupal/Core/Datetime/TimeInterface.php
   +++ b/core/lib/Drupal/Core/Datetime/TimeInterface.php
   @@ -0,0 +1,28 @@
   
   @@ -0,0 +1,28 @@
   +<?php
   +
   

   Could we put the interface into a component at least?

2. +++ b/core/tests/Drupal/Tests/Core/Datetime/TimeTest.php
   @@ -0,0 +1,101 @@
   +// Set up our shadow namespace so we can mock global system calls.
   +
   +namespace Drupal\Core\Datetime;
   +
   +/**
   + * Shadowed time() system call that lives in our namespace.
   + *
   + * @returns int
   + */
   +function time() {
   +  return 12345678;
   +}
   +
   +/**
   

   Are you sure that not using {} is a good idea for the namespace?

I can see the value of wrapping $this->requestStack->getCurrentRequest()->server->get('REQUEST_TIME') for DX purposes, and would also greatly prefer this over a superglobal. Having written this, would a simple $this->requestStack->getCurrentTime() which does the same not solve the same problem?

I think wrapping time() and microtime() is a nice idea for unit testing, but is a separate issue, no?

#14.1 - Think the whole thing can move to component. Had it in \Core\ because it was exposed as a service, but we expose some Symfony classes directly, so why not this. There is nothing really Drupal specific about it.

#14.2 - Good idea, and makes the file read better. I think this would be the only case in core that does this, so not really sure how to document it.

#15 - It's a tough call, an may lean towards YAGNI. However, the $_SERVER['REQUEST_TIME'] vs REQUEST_TIME vs $this->requestStack->getCurrentRequest()->server->get('REQUEST_TIME') vs time() are all related. I would suggest, we rescope this to introduce the time service and deprecate REQUEST_TIME. Then as followups, we can work on standardizing core to use the time service: 1, replace $_SERVER['REQUEST_TIME'] / REQUEST_TIME usages / $this->requestStack->getCurrentRequest()->server->get('REQUEST_TIME') usages (maybe split out the later, as it may affect a whole bunch of injections). 2, replace time() with the proper service method (there are a lot of places where time() is called when it should be the request time. 3, replace usages of microtime. Issues exist already for some of these.

#17, good idea.

Triggering retest of my last patch. That passes locally

../vendor/phpunit/phpunit/phpunit tests/Drupal/Tests/Component/Datetime/TimeTest.php
PHPUnit 4.8.11 by Sebastian Bergmann and contributors.

...

Time: 215 ms, Memory: 6.00Mb

OK (3 tests, 3 assertions)

Let's try this. Also added @deprecated for REQUEST_TIME, tweaked some comments, and #17.

If we are happy with this general approach and just need polish the patch, I'll write up a change record to announce the new service.

Fixed a few nits when I was looking at this. Think this is ready for full review / comments.

Setting back to Needs Review. Fails in #23 have been from #2724871: Random failure in \Drupal\migrate_drupal_ui\Tests\d7\MigrateUpgrade7Test.

We're implementing date facets and we'd like to be able to use this patch to make our tests somewhat easier. I did find some nitpicks related to coding standards, everything else looks great.

1. +++ b/core/lib/Drupal/Component/Datetime/Time.php
   @@ -0,0 +1,54 @@
   +   * @inheritdoc
   

   /s/@inheritdoc/{@inheritdoc}/

   This applies for several place in the patch, I won't mention all of them.

2. +++ b/core/lib/Drupal/Component/Datetime/Time.php
   @@ -0,0 +1,54 @@
   +}
   \ No newline at end of file
   

   Needs a newline here.

3. +++ b/core/tests/Drupal/Tests/Component/Datetime/TimeTest.php
   @@ -0,0 +1,126 @@
   +<?php
   +
   

   I think we can only omit the @file docblock if the file only contains one class and nothing else, not sure about this though.

4. +++ b/core/tests/Drupal/Tests/Component/Datetime/TimeTest.php
   @@ -0,0 +1,126 @@
   +      $this->requestStack->expects($this->any())
   +                         ->method('getCurrentRequest')
   +                         ->willReturn($request);
   

   I don't think we do this indentation in core, but there's an open issue about this as far as I know. I think it should only be 2 spaces for the method/willReturn lines.

Addresses #27.

+++ b/core/includes/bootstrap.inc
@@ -82,6 +82,9 @@
+ * @deprecated in Drupal 8.0.0, will be removed before Drupal 9.0.0.

Shouldn't this be 8.2.0?

Well, with this service you can mock the time and theoretically be able to go back in time. The actual version, we deprecate, then doesn't matter that much.

You are right though, we can fix that on commit or someone uploads a new patch.

Well, with this service you can mock the time and theoretically be able to go back in time. The actual version, we deprecate, then doesn't matter that much.

Can't tell if this is serious or making time travel references :P

1. +++ b/core/lib/Drupal/Component/Datetime/Time.php
   @@ -0,0 +1,54 @@
   +
   +class Time implements TimeInterface {
   

   No docs. I think this needs some documentation calling on all developers to use this instead of any other way of getting the time so that we can change the time at will during testing. I support this approach completely - the first software product I ever worked on was a billing system and all calls to time had to go through a central time service that could be modified at will - it's a great way to sensibly test things that change over time. Or maybe this should be on the interface.

2. +++ b/core/tests/Drupal/Tests/Component/Datetime/TimeTest.php
   @@ -0,0 +1,139 @@
   + * shadow namespace for overriding PHP global functions within the test.  PHP
   

   Double spaces after a .

3. +++ b/core/tests/Drupal/Tests/Component/Datetime/TimeTest.php
   @@ -0,0 +1,139 @@
   +namespace Drupal\Component\Datetime {
   ...
   +namespace Drupal\Tests\Component\Datetime {
   

   No need for the indenting after this just use a ; instead of { ... }

   Also I'd put the test first.

Plus the change record needs be done.

Thanks @alexpott. In taking a quick look at some other habit-breaking classes/services (Url, LinkGenerator, etc), I'll add some code samples to the docs to the functions on the interface, and a better overview comment to the interface and the concrete class. The CR will pretty much follow these examples.

Should address #33. Was going to ping @jhodgdon for a look, but just realized she stepped down :(

Comments and proofreading greatly appreciated. Once we are happy with these docs, I will adapt for CR.

Every single time.

Banner day. Uploaded wrong files. Look at these. Note to self, always clean up old patch files and interdiffs.

The interwebs have officially beaten me today. Sorry for the noise.

Should address #33. Was going to ping @jhodgdon for a look, but just realized she stepped down :(

Comments and proofreading greatly appreciated. Once we are happy with these docs, I will adapt for CR.

Updated @deprecated to reflect 8.3.0. Added draft CR.

Noticed an unnecessary change that crept in.

1. +++ b/core/lib/Drupal/Component/Datetime/Time.php
   @@ -0,0 +1,57 @@
   + * Provides a class obtaining system time.
   

   What about "Provides a class for obtaining system time".

2. +++ b/core/lib/Drupal/Component/Datetime/TimeInterface.php
   @@ -0,0 +1,149 @@
   +   * Using the time service, rather than other methods, is especially
   +   * important to create tests that run that require predictable timestamps.
   

   What about "Using the time service, rather than other methods, is especially important when creating tests, which require predictable timestamps."

   This sentence is repeated for each of the methods, so I would put it as a documentation for the interface itself.

3. +++ b/core/tests/Drupal/Tests/Component/Datetime/TimeTest.php
   @@ -0,0 +1,135 @@
   +  protected function setUp() {
   

   Why did you remove the inheritdoc?

4. +++ b/core/tests/Drupal/Tests/Component/Datetime/TimeTest.php
   @@ -0,0 +1,135 @@
   +
   

   Not needed empty line.

5. +++ b/core/tests/Drupal/Tests/Component/Datetime/TimeTest.php
   @@ -0,0 +1,135 @@
   +
   +
   

   Two empty lines after a function instead of one.

Beside those nitpicks I would really like to see this in core :)
+1

OK, #43 should be taken care of, but not sure what extra space was meant in (4) and I left the duplicated docs on each method. Similar things happen in other classes (Url comes to mind). Having the docs on each method makes sense when you Cmd-B to a definition and want to read the docs, and also for api.drupal.org so you can see everything in one place when reading the docs for a method on a single webpage.

Found one more nitpick, which could be removed on committing this.

+++ b/core/tests/Drupal/Tests/Component/Datetime/TimeTest.php
@@ -0,0 +1,137 @@
+    // Mocks a the request stack getting the current request.

"Mocks a the request" => "Mocks the request".

+++ b/core/lib/Drupal.php
new file mode 100644
index 0000000..cc6f2a2

index 0000000..cc6f2a2
--- /dev/null

--- /dev/null
+++ b/core/lib/Drupal/Component/Datetime/Time.php

+++ b/core/lib/Drupal/Component/Datetime/Time.php
+++ b/core/lib/Drupal/Component/Datetime/Time.php
@@ -0,0 +1,57 @@

We are adding a new component inside \Drupal\Component so a composer.json file would be nice

@dawehner, we have one already in that component, http://cgit.drupalcode.org/drupal/tree/core/lib/Drupal/Component/Datetim...

Do we want/need to update the description?

Weird, I swear I opened up my IDE to look it up whether there is one. Thank you for pointing it out. Nope, I don't think we really need an update.

#44 should be addressed, other than #2. Not sure what we want to do here. All of those uses are scattered through core (essentially, but not verbatim). Ideally, as a followup (or two or three), we want to replace all of those with the time service.

+++ b/core/tests/Drupal/Tests/Component/Datetime/TimeTest.php
@@ -0,0 +1,134 @@
+ * @file

We do not need this, right?

#51, the docs are a little fuzzy here, https://www.drupal.org/node/1354#file

The @file doc block MUST be present for all PHP files, with one exception: files that contain a namespaced class/interface/trait, whose file name is the class name with a .php extension, and whose file path is closely related to the namespace (under PSR-4 or a similar standard), SHOULD NOT have a @file documentation block.

That particular file has two namespaces in it, one of which doesn't match the filename. One is for the test class, which matches the filename, the other is for the globals that are shadowed for testing purposes.

+++ b/core/tests/Drupal/Tests/Component/Datetime/TimeTest.php
@@ -0,0 +1,134 @@
+/**
+ * @coversDefaultClass \Drupal\Component\Datetime\Time
+ * @group Datetime
+ */
+class TimeTest extends UnitTestCase {

Let's add runTestsInSeparateProcesses to not cause side effects.

Nifty. Didn't realize that existed. Assume we also want a `@preserveGlobalState disabled` as a precaution, too?

Isolated test.

+++ b/core/tests/Drupal/Tests/Component/Datetime/TimeTest.php
@@ -22,6 +22,11 @@
+ * @preserveGlobalState disabled

OH, I always assumes that the separate process disables the global state automatically

#56, the handful of other tests that use `@runTestsInSeparateProcesse`s also use `@preserveGlobalState disabled`.

It doesn't hurt to have more safety involved.

OK, #59 is take care of. phpcs came up clean for me.

Yeah, we have used the shadow technique multiple times already without documenting it in those detaios.

Patch to cherry-pick the service to 8.2.x. Please ignore the comment.