Replace REQUEST_TIME in rest of OO code (except for tests)

All code changes look good to me.
The patch is for me RTBC.
There is one small documentation nitpick that can be changed on commit.

+++ b/core/lib/Drupal/Core/Field/Plugin/Field/FieldType/ChangedItem.php
+++ b/core/lib/Drupal/Core/Field/Plugin/Field/FieldType/ChangedItem.php
@@ -30,7 +30,7 @@ public function preSave() {

+++ b/core/lib/Drupal/Core/Queue/DatabaseQueue.php
@@ -87,8 +87,9 @@ protected function doCreateItem($data) {
-        // We cannot rely on REQUEST_TIME because many items might be created
-        // by a single request which takes longer than 1 second.
+        // We cannot rely on \Drupal::time()->getRequestTime()
+        // because many items might be created by a single request which takes
+        // longer than 1 second.

Documentation lines are not filled out to the max. Can be changed on commit.

+++ b/core/lib/Drupal/Core/Queue/DatabaseQueue.php
@@ -87,8 +87,9 @@ protected function doCreateItem($data) {
         'created' => time(),

+++ b/core/modules/migrate_drupal_ui/src/Batch/MigrateUpgradeImportBatch.php
@@ -286,7 +286,7 @@ public static function finished($success, $results, $operations, $elapsed) {
-    if ((time() - REQUEST_TIME) > static::$maxExecTime) {
+    if ((time() - \Drupal::time()->getRequestTime()) > static::$maxExecTime) {

@@ -317,7 +317,7 @@ public static function onPostImport(MigrateImportEvent $event) {
-    if ((time() - REQUEST_TIME) > static::$maxExecTime) {
+    if ((time() - \Drupal::time()->getRequestTime()) > static::$maxExecTime) {

It feels odd given that we're replacing REQUEST_TIME to not also replace time() in these three instances with the \Drupal::time() equivalent.

Kindly review a new patch, where time() is replaced with \Drupal::time()->getCurrentTime().

The remark from @alexpott from comment #10 has been addressed.
All the code changes look good to me.
Back to RTBC.

Committed d23b4cc and pushed to 9.0.x. Thanks!

+++ b/core/modules/views/src/ViewExecutable.php
@@ -1686,7 +1686,7 @@ public function preExecute($args = []) {
-    $this->dom_id = !empty($this->dom_id) ? $this->dom_id : hash('sha256', $this->storage->id() . REQUEST_TIME . mt_rand());
+    $this->dom_id = !empty($this->dom_id) ? $this->dom_id : hash('sha256', $this->storage->id() . \Drupal::time()->getRequestTime() . mt_rand());

We could have a follow-up to simplify this a bit. The whole point is to generate a unique random identifier. This feels overly complex. But no need to address here.

@Hardik_Patel_12 it would have been great if you could have addressed #9 in #11 as well.

diff --git a/core/lib/Drupal/Core/Queue/DatabaseQueue.php b/core/lib/Drupal/Core/Queue/DatabaseQueue.php
index c2b76c97e3..8e577f8c54 100644
--- a/core/lib/Drupal/Core/Queue/DatabaseQueue.php
+++ b/core/lib/Drupal/Core/Queue/DatabaseQueue.php
@@ -87,9 +87,9 @@ protected function doCreateItem($data) {
       ->fields([
         'name' => $this->name,
         'data' => serialize($data),
-        // We cannot rely on \Drupal::time()->getRequestTime()
-        // because many items might be created by a single request which takes
-        // longer than 1 second.
+        // We cannot rely on \Drupal::time()->getRequestTime() because many
+        // items might be created by a single request which takes longer than 1
+        // second.
         'created' => \Drupal::time()->getCurrentTime(),
       ]);
     // Return the new serial ID, or FALSE on failure.

Fixed on commit.

Need to work out why but this patch has caused instability in the test system. Locally I can't get core/tests/Drupal/KernelTests/Core/Entity/ContentEntityChangedTest.php to pass. And we're seeing other fails too - https://www.drupal.org/pift-ci-job/1586930

Oh fun... so at least the fail in core/tests/Drupal/KernelTests/Core/Entity/ContentEntityChangedTest.php happens because in KernelTestBase we do:

    // DrupalKernel::boot() is not sufficient as it does not invoke preHandle(),
    // which is required to initialize legacy global variables.
    $request = Request::create('/');
    $kernel->boot();
    $request->attributes->set(RouteObjectInterface::ROUTE_OBJECT, new Route('<none>'));
    $request->attributes->set(RouteObjectInterface::ROUTE_NAME, '<none>');
    $kernel->preHandle($request);

Because it does Request::create('/'); this ends up having a different time than $_SERVER['REQUEST_TIME'] (and therefore the legacy REQUEST_TIME) because this request is created using the result of time() not the server globals.

I think this means efforts to do conversions bit by bit are fraught and that we might need to do everything at once.

Changing Request::create('/'); to Request::createFromGlobals(); fixes the test but I don't think this change is correct.

See possibly related issues noted here:

https://www.drupal.org/project/drupal/issues/3112283#comment-13605217

+++ b/core/lib/Drupal/Core/Field/Plugin/Field/FieldFormatter/TimestampFormatter.php
@@ -106,7 +106,7 @@ public function settingsForm(array $form, FormStateInterface $form_state) {
     foreach ($this->dateFormatStorage->loadMultiple() as $machine_name => $value) {
-      $date_formats[$machine_name] = $this->t('@name format: @date', ['@name' => $value->label(), '@date' => $this->dateFormatter->format(REQUEST_TIME, $machine_name)]);
+      $date_formats[$machine_name] = $this->t('@name format: @date', ['@name' => $value->label(), '@date' => $this->dateFormatter->format(\Drupal::time()->getRequestTime(), $machine_name)]);

should move request time out of loop

Added reroll of patch #11 on Drupal 9.4.x.

Added reroll for 9.5.x-dev version.

This impacts PHPStan baseline so better target 10.1.x first. Changing to MR worflow.

Can't create a MR against 10.1.x for some reasons, fallback to 10.0.x

Addressed #24.

Addressed #9 plus another crufty comment.

Believe this missed the 10.0 boat but can land in 10.1 hopefully.

Moving to NW for the MR to be updated to point to 10.1 please

I closed the MR2467 to avoid becoming a bottleneck. Reverting to patch workflow, rerolled patch.

Not sure why but when I tried to run ./core/scripts/dev/commit-code-check.sh to check this patch it now locks up.

1. +++ b/core/lib/Drupal/Core/Datetime/DateHelper.php
   @@ -291,10 +291,10 @@ public static function weekDaysOrdered($weekdays) {
        if (empty($min)) {
   -      $min = intval(date('Y', REQUEST_TIME) - 3);
   +      $min = intval(date('Y', \Drupal::time()->getRequestTime()) - 3);
        }
        if (empty($max)) {
   -      $max = intval(date('Y', REQUEST_TIME) + 3);
   +      $max = intval(date('Y', \Drupal::time()->getRequestTime()) + 3);
   

   could use variable to prevent calling twice if both are 0

2. +++ b/core/modules/migrate_drupal_ui/src/Batch/MigrateUpgradeImportBatch.php
   @@ -293,7 +293,7 @@ public static function finished($success, $results, $operations, $elapsed) {
   -    if ((time() - REQUEST_TIME) > static::$maxExecTime) {
   +    if ((\Drupal::time()->getCurrentTime() - \Drupal::time()->getRequestTime()) > static::$maxExecTime) {
   
   @@ -324,7 +324,7 @@ public static function onPostImport(MigrateImportEvent $event) {
   -    if ((time() - REQUEST_TIME) > static::$maxExecTime) {
   +    if ((\Drupal::time()->getCurrentTime() - \Drupal::time()->getRequestTime()) > static::$maxExecTime) {
   

   Could use variable $time = \Drupal::time()

Btw is there a reason to replace time() with service call? I think only for unit-testing reasons

Addressed points in #41

For #42 I believe it was made for cleaner code? But can't be sure.

Also when I try to run the code check it locks up. Is that normal when making changes to phpstan files?

This needs a reroll for 10.1.x, patch no longer applies.

Ran the test fails on the latest 10.1.x with this patch with --repeat=20. SaveActionTest passed, but EntityValidationTest is now flaky. It fails about 30% of the time

#43 patch not applied for me also, Please try this new patch.
Thanks

Moving back to an MR workflow, rebased #45 on to a new branch off 11.x in a new MR

The MR is green but we did have another failure earlier which I couldn't reproduce locally https://git.drupalcode.org/issue/drupal-3112295/-/pipelines/85141/test_r...

Drupal\KernelTests\Core\Action\SaveActionTest::testSaveAction
Failed asserting that 1706669556 is not identical to 1706669556.

I'm not too sure how this could happen when we're sleeping

Another similar random failure this time in testEntityChangedConstraintOnConcurrentMultilingualEditing

Was looking at this a bit, and I'm not quite sure some of the examples actually do not have access to the container

Baseline needs clean-up

  Line   core/lib/Drupal/Core/KeyValueStore/DatabaseStorageExpirable.php                               
 ------ ---------------------------------------------------------------------------------------------- 
         Ignored error pattern #^Call to deprecated constant REQUEST_TIME\:                            
         Deprecated in drupal\:8\.3\.0 and is removed from drupal\:11\.0\.0\.                          
         Use \\Drupal\:\:time\(\)\->getRequestTime\(\); $# in path                                     
         /builds/issue/drupal-3112295/core/lib/Drupal/Core/KeyValueStore/DatabaseStorageExpirable.php  
         was not matched in reported errors.  

Should be good to go now

Found one instance that can use DI

@acbramley, something like this ?

Looks perfect! Nice one.

Committed/pushed to 11.x and cherry-picked to 10.3.x, thanks!

It was the last in list, so META can be closed as well!

PS: please close MR