Problem/Motivation

REQUEST_TIME is a deprecated global. There are several issues to remove it's use in core.

This issue is to remove it from just Kernel tests.

There are no usages of REQUEST_TIME in Unit tests.

Proposed resolution

Use \Drupal::time()->getRequestTime() instead of REQUEST_TIME in Kernel tests.

Remaining tasks

Review
Commit

CommentFileSizeAuthor
#52 drupal-core-changed-deprecated-request-time-3112284-52.patch125.45 KBviniciusrp
#50 3112284-50.patch126.91 KBbhanu951
#34 3112284-34.patch80.71 KBquietone
#34 interdiff-32-34.txt9.69 KBquietone
#32 interdiff_3112284_28-32.txt1.15 KBankithashetty
#32 3112284-32.patch80.38 KBankithashetty
#28 reroll_diff_3112284_22-28.txt75.88 KBankithashetty
#28 3112284-28.patch80.4 KBankithashetty
#22 3112284-22.patch80.44 KBadityasingh
#13 interdiff-11-13.txt1.92 KBmpdonadio
#13 3112284-13.patch90.01 KBmpdonadio
#11 3112284-11.patch90.6 KBmpdonadio
#5 interdiff_2-4.txt25.21 KBswatichouhan012
#5 3112284-4.patch72.21 KBswatichouhan012
#2 3112284-02.patch91.31 KBmpdonadio

Issue fork drupal-3112284

Command icon Show commands

Start within a Git clone of the project using the version control instructions.

Or, if you do not have SSH keys set up on git.drupalcode.org:

About issue forks

Comments

mpdonadio created an issue. See original summary.

mpdonadio’s picture

mpdonadio
he/him
Primary language English
Location Philadelphia/PA/USA (UTC-5)
commented
Status: Active » Needs review
StatusFileSize
new 3112284-02.patch91.31 KB

Took the patch from 2902895-43.patch

- Isolated just the test changes (well, I tried)
- Introduced some changes to KTB and BTB, but not sure how we really want to handle this
- REQUEST_TIME -> $this->requestTime

Status: Needs review » Needs work

The last submitted patch, 2: 3112284-02.patch, failed testing. View results
- codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

swatichouhan012’s picture

swatichouhan012 commented
Assigned: mpdonadio » swatichouhan012

I am working on this issue.

swatichouhan012’s picture

swatichouhan012 commented
Status: Needs work » Needs review
StatusFileSize
new 3112284-4.patch72.21 KB
new interdiff_2-4.txt25.21 KB

I have created patch to remove deprecated REQUEST_TIME, Kindly review .

swatichouhan012’s picture

swatichouhan012 commented
Assigned: swatichouhan012 » Unassigned

Status: Needs review » Needs work

The last submitted patch, 5: 3112284-4.patch, failed testing. View results
- codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

hash6’s picture

hash6 commented
Assigned: Unassigned » hash6
mpdonadio’s picture

mpdonadio
he/him
Primary language English
Location Philadelphia/PA/USA (UTC-5)
commented
Issue summary: View changes

@swatichouhan012, thanks for picking up the issue, but #2 really is the starting point. For example,

  1. +++ b/core/lib/Drupal/Core/Batch/BatchStorage.php
@@ -109,7 +109,7 @@ public function cleanup() {
-        ->condition('timestamp', REQUEST_TIME - 864000, '<')
+        ->condition('timestamp', \Drupal::time()->getRequestTime() - 864000, '<')
         ->execute();

    The scope of this issue is just tests. That particular instance is being handled in a different issue.

  2. +++ b/core/modules/block_content/tests/src/Functional/BlockContentRevisionsTest.php
@@ -37,6 +37,7 @@ class BlockContentRevisionsTest extends BlockContentTestBase {
   protected function setUp() {
     parent::setUp();
 
+    $request_time = \Drupal::time()->getRequestTime();
     /** @var \Drupal\user\Entity\UserInterface $user */
     $user = User::load(1);
 
@@ -56,7 +57,7 @@ protected function setUp() {

@@ -56,7 +57,7 @@ protected function setUp() {
       $block->setNewRevision(TRUE);
       $block->setRevisionLogMessage($this->randomMachineName(32));
       $block->setRevisionUser($this->adminUser);
-      $block->setRevisionCreationTime(REQUEST_TIME);
+      $block->setRevisionCreationTime($request_time);
       $logs[] = $block->getRevisionLogMessage();
       $block->save();
       $blocks[] = $block->getRevisionId();
diff --git a/core/modules/block_content/tests/src/Functional/BlockContentSaveTest.php b/core/modules/block_content/tests/src/Functional/BlockContentSaveTest.php

    This change undoes work in #2. Part of the first patch on the issue was to add the time service and request time to the test bases. We want to avoid using the singleton when possible.

I adjusted the IS to outline this better.

hash6’s picture

hash6 commented
Assigned: hash6 » Unassigned
mpdonadio’s picture

mpdonadio
he/him
Primary language English
Location Philadelphia/PA/USA (UTC-5)
commented
Assigned: Unassigned » mpdonadio
Status: Needs work » Needs review
StatusFileSize
new 3112284-11.patch90.6 KB

Reroll of #2 to account for change in UserBlocksTest.

Status: Needs review » Needs work

The last submitted patch, 11: 3112284-11.patch, failed testing. View results
- codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

mpdonadio’s picture

mpdonadio
he/him
Primary language English
Location Philadelphia/PA/USA (UTC-5)
commented
Status: Needs work » Needs review
StatusFileSize
new 3112284-13.patch90.01 KB
new interdiff-11-13.txt1.92 KB

Few fixes.

Status: Needs review » Needs work

The last submitted patch, 13: 3112284-13.patch, failed testing. View results

mpdonadio credited JeroenT.

mpdonadio credited Vlad Bo.

mpdonadio credited pifagor.

mpdonadio credited voleger.

mpdonadio’s picture

mpdonadio
he/him
Primary language English
Location Philadelphia/PA/USA (UTC-5)
commented

Commit credits.

Version: 8.9.x-dev » 9.1.x-dev

Drupal 8.9.0-beta1 was released on March 20, 2020. 8.9.x is the final, long-term support (LTS) minor release of Drupal 8, which means new developments and disruptive changes should now be targeted against the 9.1.x-dev branch. For more information see the Drupal 8 and 9 minor version schedule and the Allowed changes during the Drupal 8 and 9 release cycles.

kristen pol’s picture

kristen pol
she/her
Primary language English
Location Santa Cruz, CA, USA
commented
Related issues: +#3112283: Replace REQUEST_TIME in non-OO and non-module code

See possibly related issues noted here:

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

adityasingh’s picture

adityasingh commented
StatusFileSize
new 3112284-22.patch80.44 KB

reroll for 9.1

adityasingh’s picture

adityasingh commented
Status: Needs work » Needs review

Version: 9.1.x-dev » 9.2.x-dev

Drupal 9.1.0-alpha1 will be released the week of October 19, 2020, which means new developments and disruptive changes should now be targeted for the 9.2.x-dev branch. For more information see the Drupal 9 minor version schedule and the Allowed changes during the Drupal 9 release cycle.

Version: 9.2.x-dev » 9.3.x-dev

Drupal 9.2.0-alpha1 will be released the week of May 3, 2021, which means new developments and disruptive changes should now be targeted for the 9.3.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

Version: 9.3.x-dev » 9.4.x-dev

Drupal 9.3.0-rc1 was released on November 26, 2021, which means new developments and disruptive changes should now be targeted for the 9.4.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

andypost’s picture

andypost
he/him
Primary language Russian
commented
Status: Needs review » Needs work
Issue tags: +Needs reroll
ankithashetty’s picture

ankithashetty
Location Karnataka, India
commented
Status: Needs work » Needs review
Issue tags: -Needs reroll
StatusFileSize
new 3112284-28.patch80.4 KB
new reroll_diff_3112284_22-28.txt75.88 KB

Rerolled the patch in #22, thanks!

andypost’s picture

andypost
he/him
Primary language Russian
commented

@ankithashetty please fix CS report from bot) thank you!

mondrake’s picture

mondrake
Location 🇮🇹
commented
Issue tags: -Drupal 9 +PHPStan-0

We'll also need a D10 patch with the PHPStan baseline updated, but let's have an agreed D9.4 first

mondrake’s picture

mondrake
Location 🇮🇹
commented
Status: Needs review » Needs work
ankithashetty’s picture

ankithashetty
Location Karnataka, India
commented
Status: Needs work » Needs review
StatusFileSize
new 3112284-32.patch80.38 KB
new interdiff_3112284_28-32.txt1.15 KB

Fixed the CS errors, thanks!

andypost’s picture

andypost
he/him
Primary language Russian
commented
Status: Needs review » Needs work

Still not enough)

quietone’s picture

quietone commented
Status: Needs work » Needs review
StatusFileSize
new interdiff-32-34.txt9.69 KB
new 3112284-34.patch80.71 KB

Removed some changes in non test files, added some tests, and fixed assertions so an int was compared to an int.

Status: Needs review » Needs work

The last submitted patch, 34: 3112284-34.patch, failed testing. View results

quietone’s picture

quietone commented
Status: Needs work » Needs review

Just noting that the failing tests are all Kernel tests.

jhedstrom’s picture

jhedstrom
Primary language English
Location Portland, OR
commented
Status: Needs review » Needs work

Moving to NW due to failing tests.

Version: 9.4.x-dev » 9.5.x-dev

Drupal 9.4.0-alpha1 was released on May 6, 2022, which means new developments and disruptive changes should now be targeted for the 9.5.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

spokje’s picture

spokje
Primary language Dutch
commented
Version: 9.5.x-dev » 10.0.x-dev
Assigned: mpdonadio » spokje

Rebasing against 10.0.x since in there we have to deal with core/phpstan-baseline.neon.
Also unassigning mpdonadio since it has been 2 years since he worked on this.

spokje’s picture

spokje
Primary language Dutch
commented

Used 3112284-34.patch as base for the new MR.

Spokje opened merge request !2402

daffie’s picture

daffie commented
Issue tags: +Needs change record

Lets add a CR for adding the class variables $timeService and $requestTime to the class KernelTestBase and the class BrowserTestBase. Other developers that write tests based on those 2 classes need to know about the added class variables.

spokje’s picture

spokje
Primary language Dutch
commented
Assigned: spokje » Unassigned
Status: Needs work » Needs review

Thanks @daffie.

We have a problem here, Houston:

By the looks of it \Drupal::time()->getRequestTime() and $this->requestTime have a difference of 2-3 seconds with REQUEST_TIME.

This causes test-failures when we compare the timestamp with timestamps like "changed".

How to tackle this?

- Add an offset of 3 seconds to make these tests pass? Which would still cause test failures when we compare timestamps to be equal, since it's either 2 or 3 seconds.
- Use $_SERVER['REQUEST_TIME'] for these tests?
- Something completely different?

Putting this on NR (whilst it nowhere nearly ready) to get some eyes/Big Brains/random body parts on this.

spokje’s picture

spokje
Primary language Dutch
commented
Assigned: Unassigned » spokje
Status: Needs review » Needs work
andypost’s picture

andypost
he/him
Primary language Russian
commented

only 5 failures left! great job

spokje’s picture

spokje
Primary language Dutch
commented
Assigned: spokje » Unassigned
mondrake’s picture

mondrake
Location 🇮🇹
commented

I would suggest to split this issue in two - one for Functional/FunctionalJavascript and one for Unit/Kernel.

mondrake’s picture

mondrake
Location 🇮🇹
commented
Status: Needs work » Postponed

Filed #3309104: Replace REQUEST_TIME in Functional and FunctionalJavascript tests.

bhanu951’s picture

bhanu951 commented
Title: Replace REQUEST_TIME in tests » Replace REQUEST_TIME in Unit/Kernel tests

Updated title as we created another issue for Replacing REQUEST_TIME in Functional/FunctionalJavascript tests

bhanu951’s picture

bhanu951 commented
StatusFileSize
new 3112284-50.patch126.91 KB

Uploaded current changes to 10.0.x as patch before rebasing to 11.x branch.

@alexpott made below comment on MR #2753

I think we should use \Drupal::time()->getRequestTime() and be done... it's better than another property on the test class.

So we might need to replace all usages of $this->requestTime with \Drupal::time()->getRequestTime()

bhanu951’s picture

bhanu951 commented
Title: Replace REQUEST_TIME in Unit/Kernel tests » Replace REQUEST_TIME in Unit and Kernel tests
Issue summary: View changes
viniciusrp’s picture

viniciusrp commented
Status: Postponed » Needs review
StatusFileSize
new drupal-core-changed-deprecated-request-time-3112284-52.patch125.45 KB

The patch #50 has error to be applied, I recreated based to version 10.0.x.

smustgrave’s picture

smustgrave commented
Version: 10.0.x-dev » 11.x-dev
Status: Needs review » Postponed

Patch was moved to an MR. Repostponing on #3309104: Replace REQUEST_TIME in Functional and FunctionalJavascript tests

andypost’s picture

andypost
he/him
Primary language Russian
commented

Any reason it postponed on other conversion?

acbramley’s picture

acbramley commented
Status: Postponed » Needs work
Related issues: +#3309104: Replace REQUEST_TIME in Functional and FunctionalJavascript tests

There's still a huge number of changes to non Kernel/Unit test files here. All functional/functionaljs tests are being updated in #3309104: Replace REQUEST_TIME in Functional and FunctionalJavascript tests

acbramley’s picture

acbramley commented
Status: Needs work » Needs review
Issue tags: -Needs change record

Peeled this back to just Kernel tests and removed the class members entirely. Used variables for $requestTime where appropriate. Pipeline is green :)

No need for a CR now since we're not adding any class members.

smustgrave’s picture

smustgrave commented

Could issue summary and title be updated too please

acbramley’s picture

acbramley commented
Title: Replace REQUEST_TIME in Unit and Kernel tests » Replace REQUEST_TIME in Kernel tests
Issue summary: View changes
smustgrave’s picture

smustgrave commented
Status: Needs review » Needs work

modules/dblog/tests/src/Kernel/DbLogTest.php
modules/views/tests/src/Kernel/RenderCacheIntegrationTest.php

Found 2 more.

acbramley’s picture

acbramley commented
Status: Needs work » Needs review

Those 2 come from traits which we're not fixing here (FakeLogEntries and AssertViewsCacheTagsTrait)

If you search the test files you'll see that there are no references to REQUEST_TIME. The traits are fixed in the functional test one I believe (confirmed).

smustgrave’s picture

smustgrave commented
Status: Needs review » Reviewed & tested by the community

Thanks for confirming @acbramley

  • catch committed d5a12841 on 11.x 
    Issue #3112284 by Spokje, Bhanu951, acbramley, mpdonadio, mondrake,...
catch’s picture

catch
he/him
Primary language English
commented
Status: Needs review » Fixed

Committed/pushed to 11.x, thanks!

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.