Replace REQUEST_TIME in classes with direct container access

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 don't have container access

Postponing on 📌 Replace REQUEST_TIME in rest of OO code (except for tests) Fixed so there is better starting point. Also thinking that this may really need to two issues to wrangle impact

- container access b/c inheritance
- container access b/c DI

Smaller starting point.

Yeah, I know it is postponed...

These don't have create/constructors yet:

core/modules/toolbar/src/Controller/ToolbarController.php
core/modules/history/src/Plugin/views/filter/HistoryUserTimestamp.php
core/modules/aggregator/src/FeedStorage.php

Let the fails fly!

Commit credits.

mpdonadio → credited voleger → .

mpdonadio → credited pifagor → .

mpdonadio → credited Vlad Bo → .

mpdonadio → credited JeroenT → .

The last submitted patch, 8: 3112298-08.patch, failed testing. View results →

Fixed the new constructs/creates and added a update hook to force rebuild the container.

All the changes in this patch look good. Just one remark:

+++ b/core/modules/system/system.install
@@ -2579,3 +2579,7 @@ function system_update_8901() {
+
+function system_update_8902() {
+  // Nop for testing purposes to force a container rebuild.
+}

Why is this change necessary for this patch?

I have added a patch but not sure if it is enough or not.

Here I have removed the method system_update_8902() because I think it doesn't do anything.

Here again, we need to review the new patch @daffie.

All the code changes look good.
For me it is RTBC.

+++ b/core/modules/aggregator/src/Controller/AggregatorController.php
@@ -21,14 +22,24 @@ class AggregatorController extends ControllerBase {
+    $this->time = $time ?: \Drupal::service('datetime.time');

Do we need to trigger deprecation notices where constructor arguments have been added?

This needs to be done in D9 first. The patch doesn't apply there.

And the answer to #19 is yes which makes this issue 9.1.x really. Especially as REQUEST_TIME is hanging about till D10.

Here I have added patches for Drupal 8.9.x and Drupal 9.0.x

I have added drupal's version in patch name.

@ravi.shankar: Could you post an interdiff.txt file for the changes you have made (for the 9.0 branch as 8.9 branch will never be committed). Thank you for working on this issue.

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 → .

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 → .

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 → .

See possibly related issues noted here:

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

+++ b/core/modules/aggregator/src/Controller/AggregatorController.php
@@ -21,14 +22,24 @@ class AggregatorController extends ControllerBase {
+    $this->time = $time ?: \Drupal::service('datetime.time');

this kind of changes require deprecation error about passing null and to be cleared in next major

I think part of this changes could be reverted to use request time from current request

+++ b/core/modules/aggregator/src/Controller/AggregatorController.php
@@ -21,14 +22,24 @@ class AggregatorController extends ControllerBase {
+    $this->time = $time ?: \Drupal::service('datetime.time');

+++ b/core/modules/content_translation/src/ContentTranslationHandler.php
@@ -127,6 +137,7 @@ public function __construct(EntityTypeInterface $entity_type, LanguageManagerInt
+    $this->time = $time ?: \Drupal::service('datetime.time');

+++ b/core/modules/system/src/DateFormatListBuilder.php
@@ -32,11 +40,14 @@ class DateFormatListBuilder extends ConfigEntityListBuilder {
+    $this->time = $time ?: \Drupal::service('datetime.time');

+++ b/core/modules/user/src/Controller/UserController.php
@@ -70,13 +78,16 @@ class UserController extends ControllerBase {
+    $this->time = $time ?: \Drupal::service('datetime.time');

+++ b/core/modules/views/src/Plugin/views/argument/Date.php
@@ -70,12 +78,15 @@ class Date extends Formula implements ContainerFactoryPluginInterface {
+    $this->time = $time ?: \Drupal::service('datetime.time');

+++ b/core/modules/views/src/Plugin/views/cache/Time.php
@@ -53,10 +61,13 @@ class Time extends CachePluginBase {
+    $this->time = $time ?: \Drupal::service('datetime.time');

+++ b/core/modules/views/src/Plugin/views/field/Date.php
@@ -44,12 +52,15 @@ class Date extends FieldPluginBase {
+    $this->time = $time ?: \Drupal::service('datetime.time');

should use injection deprecation to explain that service will be required for injection

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 → .

Rerolled patch against 9.5

Rerolled patch against 9.5

I'd suggest to target D10 first since this impacts the PHPStan baseline, and then backport.

Uploading a rerolled patch to 10.0.x version, thanks!

The testbot is not happy. See: https://www.drupal.org/pift-ci-job/2393088 →

Confirmed with @catch what branch this should be against: https://drupal.slack.com/archives/C014CT1CN1M/p1659679942164229

10.1.x it is.

How painful, it’s way more deprecation tests than actual changes… some comments in MR

some digging in the MR

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 → .

One consideration on whether we can update the property definition with readonly and remove the @var tag - that repeats several times. I see there are alredy some cases where this was committed but would not like to get into coding standards discussions.

Three points I think need more work.

- Updated IS
- Updated CR
- Addressed threads from review by @mondrake (thanks!)
- Rebased MR on 11.x

All good for me, thanks. There are also deprecation tests for missing constructor arguments, which I think are no longer required, but it's a plus.

I'm triaging RTBC issues → . I read the IS and the comments. I didn't find any unanswered questions.

I skimmed the patch and found quite a few out of scope formatting changes which I think these are limited to changing the _construct methods to be multi line. And this change is inconsistent. I then looked at the size of the change and determined it is 65K and, 29 files + 681 − 116 lines. That is well above the recommended patch size → of 10-40K. This likely needs to be split into two issues.

I am setting to NW and assigning to myself to remove the out of scope changes to see how much that helps. I will get back to this after a short break.

Oh, I also updated credit.

This needed a rebase which I did. I then had to restore a .js file, core/modules/ckeditor5/js/build/drupalMedia.js from 11.x, which I somehow got wrong in the rebase.

Removing the out of scope changes is not sufficient. I am going to look at splitting this into two issues while tests run for the rebase.

It is still rather large but I hope this is a bit easier to review now without the plugins. I have updated the title and the CR.

Do we need a followup for the plugins?

Also would it be worth it to create a trait for time? If not changes in scope seem good.

Discussed all the new tests added here with @catch. There's no need to add the constructor tests here. They don't really test anything.

I've updated the MR to deprecate for 10.3.0 and to use autowiring instead of create where this issue was adding a create.

Committed a9b4000 and pushed to 11.x. Thanks!

After discussion with @catch I decided to delete the change record because it's not adding anything beyond the deprecation message and the constructor documentation.

Spokje → closed merge request !2607

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