Simulate time passage instead of actually waiting in EntityTestMulChanged

I don't thin this works anyway, isn't changed using REQUEST_TIME?

Could we use the new time service and actually fake the current time in ChangedItem instead of all those hacks?

https://www.drupal.org/node/2785211

1. +++ b/core/lib/Drupal/Core/Field/Plugin/Field/FieldType/CreatedItem.php
   @@ -22,8 +22,17 @@ class CreatedItem extends TimestampItem {
   +  protected function getRequestTime() {
   +    return \Drupal::time()->getRequestTime();
   +  }
   

   We are doing this to avoid a BC if anything has CreatedItem (instead of injecting it)? There is also another explcit use of the static service locator, which I assume is for the same reason?

2. +++ b/core/modules/system/tests/modules/entity_test/src/TimeShifter.php
   @@ -0,0 +1,92 @@
   +class TimeShifter extends Time {
   

   Do we explicitly add the interface when we extend something? Don't recall.

Overall I am very happy to see this being done. We added the time service to both abstract this (to allow different time sources), and to allow for predictable testing.

Right now, the class still relies on the initial request time, and then lets you bump it. I think it would be better to add this class to the testing system in general, and have everything be set explicitly to make it deterministic and also allow us to reuse this to set up different time scenarios (like force the time to be between DST fallback).

I'll play with this over the weekend.

Re 1: There is a more obvious reason. typed data/field type plugins currently simply don't properly support dependency injection.

This is what I was thinking about...

Let's regenerate that patch. Don't know why some of the new files are there, but not the new service.

Adding credit to @BramDriesen and @valthebald for their work in #2903549: Replace REQUEST_TIME with time service in CreatedItem.

1. +++ b/core/lib/Drupal/Core/Field/Plugin/Field/FieldType/CreatedItem.php
   @@ -22,8 +22,17 @@ class CreatedItem extends TimestampItem {
   +    return \Drupal::time()->getRequestTime();
   

   it should use injection of $this->timeService

2. +++ b/core/modules/system/tests/modules/entity_test/src/EntityTestServiceProvider.php
   @@ -0,0 +1,18 @@
   +    $timeService->setClass('Drupal\Tests\TimeTestHelper');
   

   TimeTestHelper::class

3. +++ b/core/tests/Drupal/Tests/TimeTestHelper.php
   @@ -0,0 +1,113 @@
   +    else {
   +      return $this->requestTime;
   +    }
   ...
   +    else {
   +      return $this->requestMicroTime;
   +    }
   ...
   +    else {
   +      return $this->currentTime;
   +    }
   ...
   +    else {
   +      return $this->currentMicroTime;
   +    }
   

   The "else" is useless here

Uploading a patch from #3064727: Remove the REQUEST_TIME constant from ChangedItem and CreatedItem and delete ChangedTestItem in favour of a mock programmable time service, which was a duplicate of this issue. Quoting the IS there:

As a follow-on from that, a lot of tests are using a fake ChangedTestItem that extends ChangedItem. This isn't ideal because:

• It extends and changes the thing being tested, less confidence we're actually testing ChangedItem.
• It bakes in it's own logic around moving time forward, instead of putting that control in the test itself.

Personally I prefer controlling time in the test cases themselves, doing so means you could additionally test things that are happening at the same time. I also probably prefer installing the service in the test case as well, instead of assuming it should be installed for entity_test. Also means other test cases can use the same service, and we're mocking less actual implementation when it's not required.

1. +++ b/core/lib/Drupal/Core/Field/Plugin/Field/FieldType/ChangedItem.php
   @@ -30,7 +30,7 @@ public function preSave() {
   -      $this->value = REQUEST_TIME;
   +      $this->value = \Drupal::time()->getRequestTime();
   
   @@ -47,7 +47,7 @@ public function preSave() {
   -          $this->value = REQUEST_TIME;
   +          $this->value = \Drupal::time()->getRequestTime();
   

   I looks weird to see \Drupal accessed for the same service twice, at least protected method makes sense, otoh it could add optional dependency on this service and make it required in 9.x

2. +++ b/core/lib/Drupal/Core/Field/Plugin/Field/FieldType/CreatedItem.php
   @@ -22,7 +22,7 @@ class CreatedItem extends TimestampItem {
   -    $this->setValue(['value' => REQUEST_TIME], $notify);
   +    $this->setValue(['value' => \Drupal::time()->getRequestTime()], $notify);
   

   Same as above, as this class parent the service should be injected here

I thought FieldItems still don't support proper DI (see comments early in thread), or am I forgetting something?

Yes, that's why I'd better add protected getter, so child classes can reuse

Thanks for the review! How about something like this?

1. +++ b/core/tests/Drupal/KernelTests/Core/Action/SaveActionTest.php
   @@ -17,12 +18,21 @@ class SaveActionTest extends KernelTestBase {
   +    $this->time = new TestTime();
   +    $this->container->set('datetime.time', $this->time);
   +    \Drupal::setContainer($this->container);
   

   Here and in the other test classes that use this pattern: we should register the testing time class by overriding \Drupal\KernelTests\KernelTestBase::register(), and then set the local variable with $this->time = $this->container->get('datetime.time');.

2. +++ b/core/tests/Drupal/KernelTests/TestTime.php
   @@ -0,0 +1,49 @@
   + * Provides a dummy time service that can be used for testing.
   

   Maybe we should mention here that the time can be advanced manually for testing purposes?

3. +++ b/core/tests/Drupal/KernelTests/TestTime.php
   @@ -0,0 +1,49 @@
   +   * @see \Drupal\Tests\search_api\Kernel\TestTimeService::getRequestTime()
   

   This should reference \Drupal\Component\Datetime\Time::getRequestTime() I think :)

4. +++ b/core/tests/Drupal/KernelTests/TestTime.php
   @@ -0,0 +1,49 @@
   +   * Constructs a TestTimeService object.
   

   TestTimeService -> TestTime.

5. +++ b/core/tests/Drupal/KernelTests/TestTime.php
   @@ -0,0 +1,49 @@
   +  public function getRequestTime() {
   

   Shouldn't we also provide the same "manual advance" feature for getRequestMicroTime()?

6. +++ b/core/tests/Drupal/KernelTests/TestTime.php
   @@ -0,0 +1,49 @@
   +   *   (optional) Number of seconds by which to advance the reported request
   +   *   time.
   

   Needs a "Defaults to 1 second." at the end.

+++ b/core/tests/Drupal/KernelTests/TestTime.php
@@ -0,0 +1,49 @@
+   * Constructs a TestTimeService object.

TestTimeService -> TestTime.

I can’t find any reference, one way or the other, at the moment, but I think the suggested first line is actually Constructs a new class instance. This ensures that it will also be applicable for child classes.

+++ b/core/tests/Drupal/KernelTests/TestTime.php
@@ -0,0 +1,49 @@
+   *   (optional) Number of seconds by which to advance the reported request
+   *   time.

Needs a "Defaults to 1 second." at the end.

I disagree. I’m pretty sure it’s standard to omit this where the default is clear from the method header. (See here (last sentence).)

There is a contrib module datetime_testing. We use it for integration testing that needs arbitrary time jumps.

Re-rolling for 9.1.x. Also addresses #36 and #37

@raman.b, please first upload a re-rolled patch and then a second one for addressing comments. We do not need a diff that shows the re-roll result, but the changes made when addressing comments. Both patches then could be uploaded in a single comment.

@hchonov here is the files, for me, it looks fine.
There's no need for rerolling for 9.2.x, it's applied fine.

I have just one note, that I can't agree to remove the parameters of the constructor `public function __construct(RequestStack $request_stack)` if somebody uses it as a parent class, they won't be able to populate properly the `$request_stack`. Don't use the Time class as a parent class (because technically you doesn't need anything from it, just the interface), or construct fully the object.

1. Fix failing test on 9.2.x.
2. Change parent object (Time) to the interface (TimeInterface).

Can somebody find a place where the `datetime.time` can be overwritten globally in the tests? Otherwise, it will be a pain to always override the registry... (well if it's needed actually)

Patch no longer applies cleanly to 9.3:

KristenBackupMBP:drupal-9.3.x-dev admin$ patch -p1 < drupal-simulate_time_passage-2809515-45-9.1.patch 
patching file core/lib/Drupal/Core/Field/Plugin/Field/FieldType/ChangedItem.php
patching file core/lib/Drupal/Core/Field/Plugin/Field/FieldType/CreatedItem.php
patching file core/modules/menu_link_content/tests/src/Kernel/MenuLinksTest.php
Hunk #3 FAILED at 152.
Hunk #4 FAILED at 160.
2 out of 4 hunks FAILED -- saving rejects to file core/modules/menu_link_content/tests/src/Kernel/MenuLinksTest.php.rej
patching file core/modules/system/tests/modules/entity_test/src/Entity/EntityTestMulChanged.php
patching file core/modules/system/tests/modules/entity_test/src/Plugin/Field/FieldType/ChangedTestItem.php
patching file core/tests/Drupal/KernelTests/Core/Action/SaveActionTest.php
patching file core/tests/Drupal/KernelTests/Core/Entity/ContentEntityChangedTest.php
Hunk #4 succeeded at 93 with fuzz 1.
Hunk #5 succeeded at 109 (offset -3 lines).
Hunk #6 succeeded at 135 (offset -15 lines).
Hunk #7 succeeded at 155 (offset -15 lines).
Hunk #8 succeeded at 175 with fuzz 1 (offset -21 lines).
Hunk #9 succeeded at 273 with fuzz 1 (offset -51 lines).
Hunk #10 succeeded at 294 (offset -54 lines).
Hunk #11 succeeded at 307 (offset -57 lines).
Hunk #12 succeeded at 324 (offset -57 lines).
Hunk #13 succeeded at 352 (offset -63 lines).
Hunk #14 succeeded at 371 (offset -69 lines).
Hunk #15 succeeded at 397 (offset -75 lines).
Hunk #16 succeeded at 430 (offset -81 lines).
Hunk #17 succeeded at 466 (offset -81 lines).
Hunk #18 succeeded at 490 (offset -81 lines).
patching file core/tests/Drupal/KernelTests/Core/Entity/EntityValidationTest.php
Hunk #4 succeeded at 247 with fuzz 2.
patching file core/tests/Drupal/KernelTests/TestTime.php

Re-rolled against 9.3.x, resolving conflicts from #3131281: Replace assertEqual() with assertEquals(). and pushed as a MR.

MR created w dogit:

dogit convert 2809515 . -vvvrd -e '<=29'

MR(patch) applies with offsets to versions 9.3, 9.4, and 10.

[drupal-9.3.x-dev/9.3.x] [drupal-9.3.x-dev]$ patch -p1 < 1119.diff 
patching file core/lib/Drupal/Core/Field/Plugin/Field/FieldType/ChangedItem.php
patching file core/lib/Drupal/Core/Field/Plugin/Field/FieldType/CreatedItem.php
patching file core/modules/menu_link_content/tests/src/Kernel/MenuLinksTest.php
Hunk #3 succeeded at 154 (offset 2 lines).
Hunk #4 succeeded at 162 (offset 2 lines).
patching file core/modules/system/tests/modules/entity_test/src/Entity/EntityTestMulChanged.php
patching file core/modules/system/tests/modules/entity_test/src/Plugin/Field/FieldType/ChangedTestItem.php
patching file core/tests/Drupal/KernelTests/Core/Action/SaveActionTest.php
patching file core/tests/Drupal/KernelTests/Core/Entity/ContentEntityChangedTest.php
patching file core/tests/Drupal/KernelTests/Core/Entity/EntityValidationTest.php
patching file core/tests/Drupal/KernelTests/TestTime.php

[drupal-9.4.x-dev/9.4.x] [drupal-9.4.x-dev]$ patch -p1 < 1119.diff 
patching file core/lib/Drupal/Core/Field/Plugin/Field/FieldType/ChangedItem.php
patching file core/lib/Drupal/Core/Field/Plugin/Field/FieldType/CreatedItem.php
patching file core/modules/menu_link_content/tests/src/Kernel/MenuLinksTest.php
Hunk #3 succeeded at 154 (offset 2 lines).
Hunk #4 succeeded at 162 (offset 2 lines).
patching file core/modules/system/tests/modules/entity_test/src/Entity/EntityTestMulChanged.php
patching file core/modules/system/tests/modules/entity_test/src/Plugin/Field/FieldType/ChangedTestItem.php
patching file core/tests/Drupal/KernelTests/Core/Action/SaveActionTest.php
patching file core/tests/Drupal/KernelTests/Core/Entity/ContentEntityChangedTest.php
patching file core/tests/Drupal/KernelTests/Core/Entity/EntityValidationTest.php
patching file core/tests/Drupal/KernelTests/TestTime.php

[drupal-10.0.x-dev/10.0.x] [drupal-10.0.x-dev]$ patch -p1 < 1119.diff 
patching file core/lib/Drupal/Core/Field/Plugin/Field/FieldType/ChangedItem.php
patching file core/lib/Drupal/Core/Field/Plugin/Field/FieldType/CreatedItem.php
patching file core/modules/menu_link_content/tests/src/Kernel/MenuLinksTest.php
Hunk #3 succeeded at 154 (offset 2 lines).
Hunk #4 succeeded at 162 (offset 2 lines).
patching file core/modules/system/tests/modules/entity_test/src/Entity/EntityTestMulChanged.php
patching file core/modules/system/tests/modules/entity_test/src/Plugin/Field/FieldType/ChangedTestItem.php
patching file core/tests/Drupal/KernelTests/Core/Action/SaveActionTest.php
patching file core/tests/Drupal/KernelTests/Core/Entity/ContentEntityChangedTest.php
patching file core/tests/Drupal/KernelTests/Core/Entity/EntityValidationTest.php
patching file core/tests/Drupal/KernelTests/TestTime.php

One minor thing... not sure why some asserts are this order:

$this->assertEquals($link->getChangedTime(), $this->time->getRequestTime()...

while others are reversed...

$this->assertEquals(
  $this->time->getRequestTime(),
  $entity->getChangedTime(),
  ...

Since they are assertEquals, it does matter, but it would be nice if things were consistent.

The Needs Review Queue Bot tested this issue. It either no longer applies to Drupal core, or fails the Drupal core commit checks. Therefore, this issue status is now "Needs work".

Apart from a re-roll or rebase, this issue may need more work to address feedback in the issue or MR comments. To progress an issue, incorporate this feedback as part of the process of updating the issue. This helps other contributors to know what is outstanding.

Consult the Drupal Contributor Guide to find step-by-step guides for working with issues.

Updated patch for 9.5.x, we'll see if the test bot can apply it to 10.x.

This change looks great, we need to update phpstan base level though.

Fixed failed commands on #58
Attached patch against Drupal 10.1.x

Patch does not apply. Needs a rebase I think.

Rerolled patch #60, please review it.

Will just repeat #59

> This change looks great, we need to update phpstan base level though.

Noting that the functional changes from the patches in this issue have been incorporated into what looks like all supported versions of core via #3112295: Replace REQUEST_TIME in rest of OO code (except for tests) and #3112283: Replace REQUEST_TIME in non-OO and non-module code. The remainder of the changes are pertinent only to running core's tests, otherwise patching likely is not necessary.