[meta][no patch] Replace uses of REQUEST_TIME and time() with time service

So I see 3 active child issues where all 3 were blocked by @alexpott. I agree with Alex that we must consider a plan how we replace usage of REMOTE_TIME/time()/etc with datetime.time service. I also agree that we should think about usage scope rather than module/file/class.

IMO one of the ways of scoping this meta issue is to split them by the way how we use the service - either through static service container wrapper or dependency injection.

Not for the issues I've been working on. These were grouped by coding standard rule, rather than by the module, class or file.

offtopic
Honestly - it sometimes takes a lot of time to reach the RTBC on an issue and then we are told this is all for nothing because we should do it in a different way from the beginning. It is what it is, though I agree with #5 that we could improve that process - give an early feedback to issues that go the wrong way at very early stage instead of waiting until RTBC before giving such a comment. Not sure how this can be done, though.

previous attempt https://www.drupal.org/project/drupal/issues/2858572

I added patch

1. +++ b/core/lib/Drupal/Component/Datetime/TimeInterface.php
   @@ -17,13 +17,13 @@
       * This method can replace instances of
       * @code
       * $request_time = $_SERVER['REQUEST_TIME'];
   -   * $request_time = REQUEST_TIME;
   +   * $request_time = \Drupal::time()->getRequestTime();
       * $request_time = $requestStack->getCurrentRequest()->server->get('REQUEST_TIME');
   

   this is actually the documentation of the method itself, so this is kind a recursion now.

   I'm not quite sure what to make of it, likely just remove that line. The can replace instances of part is a bit strange, but that's nor for this issue to improve (strange because it *is* the only proper way to access the time, not those others).

2. +++ b/core/lib/Drupal/Component/Datetime/TimeInterface.php
   @@ -17,13 +17,13 @@
       * @code
   -   * $time = time();
   +   * $time = \Drupal::time()->getCurrentTime();
       * @endcode
   

   this shouldn't be replaced because time() is a php function, we can't deprecate that, just say that in most cases, this shoudl be used instead (which is what it does)

3. +++ b/core/lib/Drupal/Core/Asset/JsCollectionRenderer.php
   @@ -42,8 +42,9 @@ public function render(array $js_assets) {
   -    // URL changed. Files that should not be cached get REQUEST_TIME as
   -    // query-string instead, to enforce reload on every page request.
   +    // URL changed. Files that should not be cached get
   +    // \Drupal::time()->getRequestTime() as query-string instead,
   

   maybe documentation like this should say "the current time as query strang instead", aka focus on the concept, not the specific method call.

Dear @berdir

I changed items 1 and 2.

As for item 3, I didn't quite understand how I need to change documentation, so I did not make any changes.

Fixed small issue

The requests stack is empty in some cases so getCurrentTime goes to fail.
Also, there are a few places where time service called before container build.
Here introduced some changes, I hope this will fix a lot of failures.

Interdiff

Replacements should not be performed in the Unit tests as of the reason of container unavailability.
Here are reverts and some updates. See interdiff.

Fixes for the last failures.

Replacements still required:
core/modules/comment/comment.module:69
core/modules/history/history.module:24

Hello @voleger

Here we can replace them only at $_SERVER ['REQUEST_TIME']. Since the change to \Drupal::time()->getRequestTime() will cause a critical error.

In my opinion, we also have to replace the define define('COMMENT_NEW_LIMIT', REQUEST_TIME - 30 * 24 * 60 * 60) and define('HISTORY_READ_LIMIT', REQUEST_TIME - 30 * 24 * 60 * 60) with the method. But to do this in other issues, since the replacement in one patch HISTORY_READ_LIMIT, COMMENT_NEW_LIMIT, time(), REQUEST_TIME is not entirely the right solution.

I like the idea that each module should introduce own static method instead of the constant. This will allow using a service for the module needs.

COMMENT_NEW_LIMIT can be replaced by static method, but HISTORY_READ_LIMIT probably should be totally removed in these issues:
#2006632: Make HISTORY_READ_LIMIT configurable
#2355527: Make constant HISTORY_READ_LIMIT changeable

Bumping priority and tagging.

Patch needed reroll.

Removed some unrelated changes.

And replaced another REQUEST_TIME call in \Drupal\search\SearchIndex.

Few comments. Didn't look at whole patch yet.

1. +++ b/core/includes/bootstrap.inc
   @@ -400,7 +400,7 @@ function drupal_valid_test_ua($new_prefix = NULL) {
   -    $time_diff = REQUEST_TIME - $time;
   +    $time_diff = $_SERVER['REQUEST_TIME'] - $time;
        $test_hmac = Crypt::hmacBase64($check_string, $key);
   

   Should use the (int) cast to avoid floating point issues.

2. +++ b/core/includes/bootstrap.inc
   @@ -400,7 +400,7 @@ function drupal_valid_test_ua($new_prefix = NULL) {
   -    $time_diff = REQUEST_TIME - $time;
   +    $time_diff = $_SERVER['REQUEST_TIME'] - $time;
        $test_hmac = Crypt::hmacBase64($check_string, $key);
   

   Should use the (int) cast to avoid floating point issues.

3. +++ b/core/lib/Drupal/Component/Datetime/Time.php
   @@ -30,7 +30,11 @@ public function __construct(RequestStack $request_stack) {
   +    return $_SERVER['REQUEST_TIME'];
   

   Should use the (int) cast to avoid floating point issues.

4. +++ b/core/lib/Drupal/Core/Path/AliasManager.php
   @@ -332,12 +332,12 @@ protected function pathAliasWhitelistRebuild($path = NULL) {
   -    return defined('REQUEST_TIME') ? REQUEST_TIME : (int) $_SERVER['REQUEST_TIME'];
   +    return \Drupal::hasContainer() ? \Drupal::time()->getRequestTime() : $_SERVER['REQUEST_TIME'];
      }
   

   Should use the (int) cast to avoid floating point issues.

https://dispatcher.drupalci.org/job/drupal_patches/28837/console isn't showing anything useful for why #36 timed out, but it is possible that there is a test in an infinite loop now. @Mixlogic is here w/ me and I'll see if he has any thoughts.

PhpStorm gives this as the usage report after #36,

Constant
    REQUEST_TIME
Found usages  (14 usages found)
    Value read  (14 usages found)
        drupal-8.8.x  (14 usages found)
            core/modules/comment  (1 usage found)
                comment.module  (1 usage found)
                    70 define('COMMENT_NEW_LIMIT', REQUEST_TIME - 30 * 24 * 60 * 60);
            core/modules/content_translation/src/Tests  (1 usage found)
                ContentTranslationUITestBase.php  (1 usage found)
                    328 'created' => REQUEST_TIME - mt_rand(0, 1000),
            core/modules/field/tests/src/Functional/Update  (1 usage found)
                FieldUpdateTest.php  (1 usage found)
                    183 $this->state->set('system.cron_last', REQUEST_TIME + 100);
            core/modules/history  (1 usage found)
                history.module  (1 usage found)
                    25 define('HISTORY_READ_LIMIT', REQUEST_TIME - 30 * 24 * 60 * 60);
            core/modules/history/tests/src/Kernel  (1 usage found)
                HistoryLegacyTest.php  (1 usage found)
                    57 $this->assertEquals(REQUEST_TIME, $render['#attached']['drupalSettings']['history']['lastReadTimestamps'][1]);
            core/modules/system/tests/fixtures/update  (3 usages found)
                drupal-8.taxonomy-parent-multilingual-3066439.php  (2 usages found)
                    28 ->values(['tid' => $tid, 'vid' => 'tags', 'langcode' => 'en', 'name' => $name, 'weight' => 0, 'changed' => REQUEST_TIME, 'default_langcode' => 1])
                    33 ->values(['tid' => $tid, 'vid' => 'tags', 'langcode' => 'es', 'name' => $name . ' es', 'weight' => 0, 'changed' => REQUEST_TIME, 'default_langcode' => 0])
                drupal-8.views-taxonomy-parent-2543726.php  (1 usage found)
                    42 ->values(['tid' => $tid, 'vid' => 'tags', 'langcode' => 'en', 'name' => $name, 'weight' => 0, 'changed' => REQUEST_TIME, 'default_langcode' => 1])
            core/modules/user/tests/src/Kernel  (1 usage found)
                TempStoreDatabaseTest.php  (1 usage found)
                    142 ->fields(['expire' => REQUEST_TIME - 1])

The history.module usage looks like it has the related issue.

The comment.module is used to define a new global constant, that isn't used any was slated to be removed (but no@deprecated; it was an old 8.0.x comment). See #2081585: [META] Modernize the History module

I think the remaining uses can be fixed in this issue.

Went back to #34 for this.

PhpStorm just reports the history.module and comment.module uses, and some update test fixtures (which are removed in 9).

Starting to look at the rest of the patch.

It has some conflicts with related

Reverted a bunch on out of scope changes.

1. +++ b/core/lib/Drupal/Core/Asset/JsCollectionRenderer.php
   @@ -40,11 +40,12 @@ public function render(array $js_assets) {
   -    $default_query_string = $this->state->get('system.css_js_query_string', '0');
   ...
   +    $default_query_string = $this->state->get('system.css_js_query_string') ?: '0';
   

   This change seems out of scope.

2. +++ b/core/lib/Drupal/Core/EventSubscriber/FinishResponseSubscriber.php
   @@ -248,8 +248,8 @@ protected function setResponseCacheable(Response $response, Request $request) {
   +      $timestamp = \Drupal::time()->getRequestTime();
   +      $response->setLastModified(new \DateTime(gmdate(DateTimePlus::RFC7231, \Drupal::time()->getRequestTime())));
   

   Here we could reuse $timestamp on the second line to avoid two method calls.

3. +++ b/core/modules/comment/src/CommentStatistics.php
   @@ -136,8 +136,9 @@ public function create(FieldableEntityInterface $entity, $fields) {
   +      // Default to \Drupal::time()->getRequestTime() when entity does not have
   

   This would probably just read better as "Default to request time when..."

4. +++ b/core/modules/comment/src/CommentStatistics.php
   @@ -258,8 +259,8 @@ public function update(CommentInterface $comment) {
              // Use the changed date of the entity if it's set, or default to
   -          // REQUEST_TIME.
   

   Same here

5. +++ b/core/modules/search/src/SearchIndex.php
   @@ -52,12 +60,15 @@ class SearchIndex implements SearchIndexInterface {
   +  public function __construct(ConfigFactoryInterface $config_factory, Connection $connection, Connection $replica, CacheTagsInvalidatorInterface $cache_tags_invalidator, TimeInterface $time) {
   

   I think we need to add this in a backward-compatible way?

6. +++ b/core/modules/statistics/tests/src/Functional/StatisticsAdminTest.php
   @@ -160,8 +160,9 @@ public function testExpiredLogs() {
   +    // from \Drupal::time()->getRequestTime() in the delete query,
   

   "from the request time" is more simple to understand

7. +++ b/core/modules/system/tests/src/Kernel/System/CronQueueTest.php
   @@ -65,14 +65,16 @@ public function testExceptions() {
   +    // Expire the queue item manually. system_cron() relies in
   +    // \Drupal::time()->getRequestTime() to find queue items
   +    // whose expire field needs to be reset to 0. This is a
   +    // Kernel test, so \Drupal::time()->getRequestTime() won't change
   +    // when cron runs.
   

   This is a bit repetitive, I think we can just say "so the time won't change when cron runs".

8. +++ b/core/modules/views/src/Tests/AssertViewsCacheTagsTrait.php
   @@ -45,7 +45,7 @@ protected function assertViewsCacheTags(ViewExecutable $view, $expected_results_
   -    $request->server->set('REQUEST_TIME', REQUEST_TIME);
   +    $request->server->set('REQUEST_TIME', \Drupal::time()->getRequestTime());
   

   Not sure this one should change, as we are passing REQUEST_TIME on?

9. +++ b/core/modules/views/src/Tests/AssertViewsCacheTagsTrait.php
   @@ -128,7 +128,7 @@ protected function assertViewsCacheTagsFromStaticRenderArray(ViewExecutable $vie
   -    $request->server->set('REQUEST_TIME', REQUEST_TIME);
   +    $request->server->set('REQUEST_TIME', \Drupal::time()->getRequestTime());
   

   Same as above

10. +++ b/core/modules/views/tests/src/Functional/Wizard/ItemsPerPageTest.php
    @@ -29,14 +29,14 @@ public function testItemsPerPage() {
    +    $node1 = $this->drupalCreateNode(['type' => 'article', 'created' => \Drupal::time()->getRequestTime()]);
    +    $node2 = $this->drupalCreateNode(['type' => 'article', 'created' => \Drupal::time()->getRequestTime() + 1]);
    +    $node3 = $this->drupalCreateNode(['type' => 'article', 'created' => \Drupal::time()->getRequestTime() + 2]);
    +    $node4 = $this->drupalCreateNode(['type' => 'article', 'created' => \Drupal::time()->getRequestTime() + 3]);
    +    $node5 = $this->drupalCreateNode(['type' => 'article', 'created' => \Drupal::time()->getRequestTime() + 4]);
    

    Would be simpler to get the time once and perhaps use $timestamp++ on each line?

#44, thanks for taking a look at this; everything you said makes sense. Few quick comments.

#5, that probably depends on whether we land in 8 or 9 with this (since this isn't marked as a critical). We can def do the BC way of adding a new service. That raises a bigger question.

I jumped into this a little late, but now recall why we didn't do this when we made the time service and deprecated that constant. It looks like the first version of the patch was a mostly search/replace (which valid approach for something this size to get to a passing patch). But, nearly all of the uses are using the \Drupal singleton and not DI. We need to make the decision about whether this is appropriate. I am wondering, even fore review sake, whether we need to split into three (or even four patches).

1. include files that need the $_SERVER thing.
2. tests; adjust KTB, BTB, and JSTB to add the service as a protected method and possible request time as a protected variable; this avoid a lot of calls to the singleton.
3. services/plugins should do proper DI
4. classes that don't have access to the container can use the singleton

That will make this easier to review, which will make it easier to get in. And, first one really is the important one to unblock progress on killing the includes issue. Then we can also do a f/up to nuke the calls to global time() and microtime().

?

@mpdonadio I think your suggestion of splitting this up makes more sense; the order you suggested sounds good. I also think we might get stuck on part 3, but we can deal with that when we come to it; if we are going to do proper DI everywhere I don't think that will be viable in a single issue if we also have to do BC in constructors and add deprecation tests, there is just too much to change and review in one go.

Ok, child issues are created w/ starting point patches, esstentially taking the #43 patch and then looking at results of `git diff --name-only 8.9.x` and then doing `git checkout 8.9.x ...` to revert changes unrelated to the child issue.

Assigned to datetime.module as we have historically used that for the greater date/time API and not just the module. Also assigned to myself as I hope to see all of these home.

Will remember to carryover credits later today.

+++ b/core/lib/Drupal/Core/Cache/DatabaseBackend.php
@@ -151,7 +151,7 @@ protected function prepareItem($cache, $allow_invalid) {
 
     // Check expire time.
-    $cache->valid = $cache->expire == Cache::PERMANENT || $cache->expire >= REQUEST_TIME;
+    $cache->valid = $cache->expire == Cache::PERMANENT || $cache->expire >= \Drupal::time()->getRequestTime();
 

This one could be nasty.

We did the same in redis, but got some bug reports about errors: #3112364: Call to a member function get() on null in Drupal\\Component\\Datetime\\Time->getRequestTime().

I suspect right now that this happens for the internal page cache on pages that have an expiration time set, then the request time is not yet set up.

This isn't failing in core because nothing in core sets an expiration time on the internal page cache yet.

@catch and @alexpott agreed in Slack to postpone removing the constant to D10 as it seems like a lot of work is left that close to the first beta deadline. We can keep working on its removal in core but should also do an issue to change the deprecation message. And I guess remove it again from the hardcoded list in phpstan-drupal?

Created #3113284: Move deprecation of REQUEST_TIME to Drupal 10 to address #49. Totally on board with this approach. If we end up using Symfony autowiring, then #3112298: Replace REQUEST_TIME in classes with direct container access may be easier. I also would't be surprised some of the fails in #3112284: Replace REQUEST_TIME in Kernel tests are from similar things (that issue is in my queue for this weekend).

Will also cleanup the IS to reflect the latest approach.

++ to undeprecate it, moreover I think that remaining conversions slightly depends on request usage in controllers which still broken in many places starting from related, which could cause side-effects like in redis

#3113284: Move deprecation of REQUEST_TIME to Drupal 10 was committed.

Not entirely sure what this means for issue metadata … I think this?

So we should still work on removing usages in 8.9.x, but since that's all happening in child issues it doesn't matter too much what version this is against.

This then becomes a Drupal 10 beta blocker eventually.

looking at the problem with redis and page cache and middleware... I think the problem is happening because the request is only pushed onto the stack during \Drupal\Core\DrupalKernel::preHandle() - this means that any middleware that runs prior to \Drupal\Core\DrupalKernel::preHandle() will fail.

I think there are two options here...
1. Implement a fallback in \Drupal\Component\Datetime\Time() if the request is not available.
2. Add a very early middleware that sets a request on the Time service and use that instead.

#3113476: Fallback when request is not available on the stack in Time service addresses #55 and would allow all conversions to proceed without much bother.

Put a big dent into splitting this up into manageable chunks.

#3113971: Replace REQUEST_TIME in services
#3112298: Replace REQUEST_TIME in classes with direct container access
#3112295: Replace REQUEST_TIME in rest of OO code (except for tests)
#3112283: Replace REQUEST_TIME in non-OO and non-module code

are all ready for prelim review, even if there are fails.

#3112284: Replace REQUEST_TIME in Kernel tests

has fails, but would appreciate some thoughts on the approach and changes to KTB and BTB. This should be last to go in, anyway.

So we've hit something interesting in [##3112295-18]

Here's the comment in full...

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.

#58, I am thinking that it could be as simple as

diff --git a/core/tests/Drupal/KernelTests/KernelTestBase.php b/core/tests/Drupal/KernelTests/KernelTestBase.php
index 4b258588c0..182345b9d2 100644
--- a/core/tests/Drupal/KernelTests/KernelTestBase.php
+++ b/core/tests/Drupal/KernelTests/KernelTestBase.php
@@ -374,6 +374,7 @@ private function bootKernel() {
     $kernel->boot();
     $request->attributes->set(RouteObjectInterface::ROUTE_OBJECT, new Route('<none>'));
     $request->attributes->set(RouteObjectInterface::ROUTE_NAME, '<none>');
+    $request->server->set('REQUEST_TIME', $_SERVER['REQUEST_TIME']);
     $kernel->preHandle($request);
 
     $this->container = $kernel->getContainer();

may do the trick, but I haven't tried this across all the child issues yet. I does consistently fix some of my local fails.

I think that all of these child issues
- need to be against the same branch; I think you were thinking 9.1 b/c the service definitions?
- need to sit at RTBC for a while to double check for spurious fails, and then commit at same time

Updating IS, changing the title a little bit and setting the status to Active. Unsigned @mpdonadio as no patch won't be accepted to inline with the title.

#57 posted/splitted this 5 child issues from my understanding, but it's still unclear to me where is the child issue for modules/components. Or Do it here? I did visit the child issues mentioned in #57, two of them have an empty IS. Putting back the Needs issue summary update tag.

The background is I just closed #3111923: Deprecated REQUEST_TIME in Path Alias module in Drupal 9 as duplicate, But can not give an exact comment of which child issue they should continue and put their efforts on

I found no views related issue, but Time cache plugin could use #3109110-6: Deprecate request argument for view time cache plugin

There are few occurrences of REQUEST_TIME in install.core.inc file - https://git.drupalcode.org/project/drupal/-/blob/9.3.x/core/includes/ins... & https://git.drupalcode.org/project/drupal/-/blob/9.3.x/core/includes/ins...
not sure in which issue we are targeting this, didn't find the exact issue so commenting here.

Looks like it is in #3112283: Replace REQUEST_TIME in non-OO and non-module code.

Added #3112290: Replace REQUEST_TIME in procedural code to the issue summary.

it still needs work for child issues, remaining deprecations will be removed in #3260778: Remove deprecated code from bootstrap.inc

#3112283: Replace REQUEST_TIME in non-OO and non-module code has landed.

Small edits to issue summary to make keeping it up to date easier.

#3112283: Replace REQUEST_TIME in non-OO and non-module code and #3112290: Replace REQUEST_TIME in procedural code landed, so removing from remaining tasks.

Reparenting to #3213895: [META] Remove deprecated classes, methods, procedural functions and code paths outside of deprecated modules on the Drupal 10 branch.

Moving this under #3301204: Properly deprecate ajax.js methods since it's an incomplete deprecation rather than just removing deprecated code.

This also means that unless the subissues are resolved in time for Drupal 10 beta, the deprecation will be bumped to Drupal 11 as per discussion with @catch, so this is not a Drupal 10 beta requirement.

There seems to be an issue with the datetime.time service as well, as we cannot seem to access it when customers are defining their own cache containers. See #3302086: After upgrade to 2.4, Drupal 9.4.5 and PHP 8.0.21, \Drupal::$container is not initialized yet. \Drupal::setContainer() must be called with a real container. and #3113971: Replace REQUEST_TIME in services -- both of which would require releases in 9.5 and 10. So my vote would be punt this to 11.

Referenced new issue #3309104: Replace REQUEST_TIME in Functional and FunctionalJavascript tests in summary.

Hiding patches since this is now a meta.

Services fixed #3113971: Replace REQUEST_TIME in services

#3112295: Replace REQUEST_TIME in rest of OO code (except for tests) is the last of the issues! Once that's in I'll double check if there's any left and create a final cleanup if necessary. Great work everyone

checked - only mentions in comments are left

Last issue fixed

Closing it in favor of follow-up to fix remaining mentions in comments #3422973: Replace remaining mentions of REQUEST_TIME in comments

Removed/re-titled a child issue as not a part of it #2908210: FIx TODO in Drupal\entity_test\Plugin\Field\FieldType\ChangedTestItem

Congrats everyone. Massive effort 💪

Closed as duplicate #3030738: [meta] Replace usages of the deprecated REQUEST_TIME constant