Support from Acquia helps fund testing for Drupal Acquia logo

Comments

mpdonadio created an issue. See original summary.

mpdonadio’s picture

Issue summary: View changes
Status: Active » Needs review
FileSize
52.29 KB

OK, took the patch from 2902895-43.patch

- Isolated just the OO code (well, I tried); this includes classes w/ and w/o access to the container as a starting point

At this point will start going through `git diff --name-only 8.9.x` to isolate by path/namespace revert changes to classes that def have container access

mpdonadio’s picture

OK, I think these are all of the usages of classes that need to use the \Drupal singleton.

mpdonadio credited JeroenT.

mpdonadio credited Vlad Bo.

mpdonadio credited pifagor.

mpdonadio credited voleger.

mpdonadio’s picture

Commit credits.

daffie’s picture

Status: Needs review » Reviewed & tested by the community

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.

alexpott’s picture

Status: Reviewed & tested by the community » Needs work
+++ 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.

Hardik_Patel_12’s picture

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

Hardik_Patel_12’s picture

Status: Needs work » Needs review
daffie’s picture

Status: Needs review » Reviewed & tested by the community

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

alexpott’s picture

Priority: Major » Normal
Status: Reviewed & tested by the community » Fixed

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.

  • alexpott committed d23b4cc on 9.0.x
    Issue #3112295 by mpdonadio, Hardik_Patel_12, daffie, JeroenT, Vlad Bo,...

  • alexpott committed 7359583 on 9.0.x
    Revert "Issue #3112295 by mpdonadio, Hardik_Patel_12, daffie, JeroenT,...
alexpott’s picture

Status: Fixed » Needs work

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

alexpott’s picture

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.

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

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

+++ 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

mondrake’s picture

Issue tags: +PHPStan-0
ravi.shankar’s picture

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

cliddell made their first commit to this issue’s fork.

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.

AkashKumar07’s picture

Added reroll for 9.5.x-dev version.

mondrake’s picture

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

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

mondrake’s picture

Version: 10.1.x-dev » 10.0.x-dev

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

mondrake’s picture

mondrake’s picture

Status: Needs work » Needs review

Addressed #24.

mondrake’s picture

Addressed #9 plus another crufty comment.

smustgrave’s picture

Version: 10.0.x-dev » 10.1.x-dev
Status: Needs review » Needs work

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

mondrake’s picture

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

smustgrave’s picture

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

andypost’s picture

  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()

andypost’s picture

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

smustgrave’s picture

FileSize
2.54 KB
17.38 KB

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?

DanielVeza’s picture

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

_pratik_’s picture

Status: Needs work » Needs review
FileSize
17.62 KB

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

Status: Needs review » Needs work

The last submitted patch, 45: 3112295-45.patch, failed testing. View results

Version: 10.1.x-dev » 11.x-dev

Drupal core is moving towards using a “main” branch. As an interim step, a new 11.x branch has been opened, as Drupal.org infrastructure cannot currently fully support a branch named main. New developments and disruptive changes should now be targeted for the 11.x branch, which currently accepts only minor-version allowed changes. For more information, see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

acbramley made their first commit to this issue’s fork.

acbramley’s picture

Issue summary: View changes

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

acbramley changed the visibility of the branch 3112295-replace-requesttime-in to hidden.

acbramley changed the visibility of the branch 11.x to hidden.

acbramley’s picture

Status: Needs work » Needs review

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

acbramley’s picture

Status: Needs review » Needs work

Another similar random failure this time in testEntityChangedConstraintOnConcurrentMultilingualEditing

Taran2L’s picture

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

Taran2L’s picture

Assigned: mpdonadio » Taran2L
andypost’s picture

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.  
Taran2L’s picture

Assigned: Taran2L » Unassigned
Status: Needs work » Needs review

Should be good to go now

acbramley’s picture

Status: Needs review » Needs work

Found one instance that can use DI

Taran2L’s picture

Assigned: Unassigned » Taran2L
Taran2L’s picture

Title: Replace REQUEST_TIME in OO code w/o access to the container » Replace REQUEST_TIME in rest of OO code (except for tests)
Assigned: Taran2L » Unassigned
Status: Needs work » Needs review

@acbramley, something like this ?

acbramley’s picture

Status: Needs review » Reviewed & tested by the community

Looks perfect! Nice one.

  • catch committed 05737686 on 10.3.x
    Issue #3112295 by mondrake, Taran2L, acbramley, cliddell, mpdonadio,...

  • catch committed ddb29450 on 11.x
    Issue #3112295 by mondrake, Taran2L, acbramley, cliddell, mpdonadio,...
catch’s picture

Version: 11.x-dev » 10.3.x-dev
Status: Reviewed & tested by the community » Fixed

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

andypost’s picture

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

PS: please close MR

Status: Fixed » Closed (fixed)

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