Replace REQUEST_TIME in services

Testbot is not happy

See possibly related issues noted here:

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

Patch re-rolled for 9.1.x Please review.

Tested #12, Changes are visible in the above mentioned files

This patch is really straightforward and clean. I did find a few small things...

1. +++ b/core/lib/Drupal/Core/Asset/CssCollectionOptimizer.php
   @@ -58,13 +66,16 @@ class CssCollectionOptimizer implements AssetCollectionOptimizerInterface {
   +    $this->time = $time ?: \Drupal::service('datetime.time');
   

   Do we need deprecation notices in all of these, as per existing instances of this pattern in core? I suspect we do, but this is already a long patch...

2. +++ b/core/lib/Drupal/Core/Asset/JsCollectionRenderer.php
   @@ -39,12 +50,12 @@ public function __construct(StateInterface $state) {
   -    $default_query_string = $this->state->get('system.css_js_query_string', '0');
   +    // browser-caching. The string changes on every update or full cache flush,
   +    // forcing browsers to load a new copy of the files, as the URL changed.
   +    // Files that should not be cached get
   +    // $this->time->getRequestTime() as query-string instead,
   +    // to enforce reload on every page request.
   +    $default_query_string = $this->state->get('system.css_js_query_string') ?: '0';
   

   This is changing the previous call to $this->state->get(). Out of scope?

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

   Why isn't the time service injected here?

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

   Same question here.

5. +++ b/core/lib/Drupal/Core/Cache/MemoryBackend.php
   @@ -201,7 +201,7 @@ public function removeBin() {
   +    return defined('REQUEST_TIME') ? \Drupal::time()->getRequestTime() : (int) $_SERVER['REQUEST_TIME'];
   

   This seems a bit weird, honestly, but the weirdness doesn't originate in this patch, and refactoring it is probably out of scope. That said, shouldn't the time service be injected?

6. +++ b/core/lib/Drupal/Core/Flood/MemoryBackend.php
   @@ -38,7 +38,7 @@ public function register($name, $window = 3600, $identifier = NULL) {
   +    // We can't use \Drupal::time()->getRequestTime() here, because that would not guarantee
   

   Let's say TimeInterface::getRequestTime() here, so that we're referring to an actual core API instead of a specific calling pattern.

7. +++ b/core/lib/Drupal/Core/KeyValueStore/KeyValueDatabaseExpirableFactory.php
   @@ -60,7 +60,7 @@ public function get($collection) {
   +        ->condition('expire', \Drupal::time()->getRequestTime(), '<')
   

   Why isn't the time service injected here?

8. +++ b/core/modules/comment/src/CommentStatistics.php
   @@ -129,8 +140,8 @@ public function create(FieldableEntityInterface $entity, $fields) {
   -      // Default to REQUEST_TIME when entity does not have a changed property.
   -      $last_comment_timestamp = REQUEST_TIME;
   +      // Default to \Drupal::time()->getRequestTime() when entity does not have a changed property.
   +      $last_comment_timestamp = \Drupal::time()->getRequestTime();
   

   Can't we use the injected time service here?

9. +++ b/core/modules/comment/src/CommentStatistics.php
   @@ -251,8 +262,8 @@ public function update(CommentInterface $comment) {
   -          // REQUEST_TIME.
   -          'last_comment_timestamp' => ($entity instanceof EntityChangedInterface) ? $entity->getChangedTimeAcrossTranslations() : REQUEST_TIME,
   +          // \Drupal::time()->getRequestTime().
   

   Again, it's probably preferable to refer to TimeInterface::getRequestTime().

10. +++ b/core/modules/comment/tests/src/Unit/CommentStatisticsUnitTest.php
    @@ -83,7 +84,7 @@ protected function setUp(): void {
    +    $this->commentStatistics = new CommentStatistics($this->database, $this->createMock('Drupal\Core\Session\AccountInterface'), $this->createMock(EntityTypeManagerInterface::class), $this->createMock('Drupal\Core\State\StateInterface'), $this->database,$this->createMock(TimeInterface::class));
    

    Nit: Missing a comma after $this->database.

11. +++ b/core/modules/search/src/SearchIndex.php
    @@ -60,8 +68,10 @@ class SearchIndex implements SearchIndexInterface {
    +   * @param \Drupal\Component\Datetime\TimeInterface $time
    +   *   The time service
    

    The description must end with a period.

Switched us over to an issue fork: https://git.drupalcode.org/issue/drupal-3113971. Pushed patch #12 to it, fixing a merge conflict in the process. :) I'll try to address some of my own feedback in #15; it seems inefficient to wait for a new patch to be posted when so many of those complaints are relatively minor.

+++ b/core/lib/Drupal/Core/Asset/CssCollectionOptimizer.php
@@ -58,13 +66,16 @@ class CssCollectionOptimizer implements AssetCollectionOptimizerInterface {
@@ -188,7 +199,7 @@ public function deleteAll() {

+++ b/core/lib/Drupal/Core/Asset/JsCollectionOptimizer.php
@@ -58,13 +66,16 @@ class JsCollectionOptimizer implements AssetCollectionOptimizerInterface {
+    $this->time = $time ?: \Drupal::service('datetime.time');

it needs deprecation message to be cleared in next major

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

Working on #12 and #20, I think @mpdonadio forgot to remove assign when he created this issue

Hey, I made this new patch, it's becoming huge and I tried to address all the points from #15 and #20, this patch still needs work, at least fix the failing tests and decide some stuff before move on.

Regarding #15:

1. Do we need deprecation notices in all of these, as per existing instances of this pattern in core? I suspect we do, but this is already a long patch...
   • Done! I added the deprecation messages in all places injecting the $time. We still need to cover all with tests, I've already created some tests, but there are more tests to add here in order to cover all the deprecation messages. So adding "Needs tests" tag here
2. This is changing the previous call to $this->state->get(). Out of scope?
   • Done! This was really out of the scope, I rollback the changes on this new patch
3. Why isn't the time service injected here?
   

   Ok this is something that I think we have to discuss, the following classes are not really services:

   • core/lib/Drupal/Core/Cache/ApcuBackend.php
   • core/lib/Drupal/Core/Cache/DatabaseBackend.php
   • core/lib/Drupal/Core/Cache/MemoryBackend.php
   • core/lib/Drupal/Core/Cache/PhpBackend.php

   In fact the factories are actually services and the idea is to construct the backend object, something like:

   • ApcuBackendFactory.php constructs ApcuBackend.php
   • DatabaseBackendFactory.php constructs DatabaseBackend.php
   • MemoryBackendFactory.php constructs MemoryBackend.php
   • PhpBackendFactory.php constructs PhpBackend.php

   So, to get things right, I have some ideas:

   1. Inject the time service into the factories(set a deprecated message, and create a test) and then pass the service at moment we instantiate the respective backend class(see the implementations of CacheFactoryInterface::get for more details).
   2. Inject the time service directly into the backends classes and ignore the factories
   3. Using the first idea, we inject the service into the factories, set a deprecated message, create a test. And inside the backend classes, we do the same, receive the service(from the factories), set a deprecated message and add a test

   It would be nice to see your thoughts here, IMHO the backend classes are not part of the scope for this issue, I mean the idea here is to work on services, and that's not the case, just the factories are services, it would be nice to have an issue to address this.

4. Same question here.
   • See the explanation on 3
5. This seems a bit weird, honestly, but the weirdness doesn't originate in this patch, and refactoring it is probably out of scope. That said, shouldn't the time service be injected?
   • I agree with you, refactoring it is probably out of scope, about time service, see the explanation on 3
6. Let's say TimeInterface::getRequestTime() here, so that we're referring to an actual core API instead of a specific calling pattern.
   • Done! I updated the documentation.
7. Why isn't the time service injected here?
   • Done! The service was injected and I add a deprecated message, but it still need tests.
8. Can't we use the injected time service here?
   • Sure! It's injected and I set a deprecated message, but it still need tests.
9. Again, it's probably preferable to refer to TimeInterface::getRequestTime().
   • Done! The documentation has been updated on this new patch
10. Nit: Missing a comma after $this->database.
    • Done!
11. The description must end with a period.
    • Done! Now all the descriptions ends with a period

Now moving to #20, I added a deprecated message and a new test to validated it.

So I guess that's it, there are some stuff that we need to do here, I'll leave this as need works.

FYI: Regarding the intediff, when I executed, I got a message, interdiff: hunk-splitting is required in this case, but is not yet implemented, I don't know what happened, so I'm upload a diff using diff -u 21 24. Maybe it's a good idea to move this into the branch created by @phenaproxima to avoid this type of error.

Thanks!

Rolling back core/lib/Drupal/Core/Cache/ApcuBackend.php deprecated message.

Cleaned-up a lot, mostly changed usage of time service to request-server-time for places where request stack already present.

Still fails few tests and I also added todos - makes sense to clean-up micro-optimizations from early days in follow-up or blocker issues.

Also removed unrelated changes - code style and function arguments clean-up

Removing tests tag as all coverage added for deprecated arguments

1. +++ b/core/lib/Drupal/Core/Asset/CssCollectionOptimizer.php
   @@ -205,9 +205,11 @@ public function getAll() {
   -    $delete_stale = function ($uri) {
   +    $request_time = $this->time->getRequestTime();
   +    $threshold = \Drupal::config('system.performance')->get('stale_file_threshold');
   +    $delete_stale = function ($uri) use ($request_time, $threshold) {
          // Default stale file threshold is 30 days.
   -      if ($this->time->getRequestTime() - filemtime($uri) > \Drupal::config('system.performance')->get('stale_file_threshold')) {
   +      if ($request_time - filemtime($uri) > $threshold) {
   
   +++ b/core/lib/Drupal/Core/Asset/JsCollectionOptimizer.php
   @@ -203,9 +203,11 @@ public function getAll() {
   -    $delete_stale = function ($uri) {
   +    $request_time = $this->time->getRequestTime();
   +    $threshold = \Drupal::config('system.performance')->get('stale_file_threshold');
   +    $delete_stale = function ($uri) use ($request_time, $threshold) {
          // Default stale file threshold is 30 days.
   -      if ($this->time->getRequestTime() - filemtime($uri) > \Drupal::config('system.performance')->get('stale_file_threshold')) {
   +      if ($request_time - filemtime($uri) > $threshold) {
   

   needs follow-up to prevent getting config in callback, it changes behavior so should get split

2. +++ b/core/lib/Drupal/Core/Cache/MemoryBackend.php
   @@ -201,7 +201,14 @@ public function removeBin() {
      protected function getRequestTime() {
   -    return defined('REQUEST_TIME') ? \Drupal::time()->getRequestTime() : (int) $_SERVER['REQUEST_TIME'];
   +    // @todo Replace calls to this method in loops with a variable.
   +    if (\Drupal::hasRequest()) {
   +      $request_time = \Drupal::request()->server->getInt('REQUEST_TIME');
   +    }
   +    else {
   +      $request_time = defined('REQUEST_TIME') ? REQUEST_TIME : (int) $_SERVER['REQUEST_TIME'];
   +    }
   +    return $request_time;
   

   That's most PITA as this method is used a lot inside of loops and smells, needs separate issue to fix. I bet it could bring speed-up to runtime and tests

3. +++ b/core/lib/Drupal/Core/Cache/PhpBackend.php
   @@ -126,7 +126,8 @@ protected function prepareItem($cache, $allow_invalid) {
   -    $cache->valid = $cache->expire == Cache::PERMANENT || $cache->expire >= REQUEST_TIME;
   +    // @todo Properly inject time service as this method used in loops.
   +    $cache->valid = $cache->expire == Cache::PERMANENT || $cache->expire >= \Drupal::time()->getRequestTime();
   
   @@ -195,7 +196,8 @@ public function invalidate($cid) {
   -      $item->expire = REQUEST_TIME - 1;
   +      // @todo Properly inject time service as this method used in loops.
   +      $item->expire = \Drupal::time()->getRequestTime() - 1;
   

   needs work

Hey @andypost, thanks for the response and the clean up, regarding #28:

1. I created a new issue to address this: #3261415: Remove static config call from CssCollectionOptimizer.php and JsCollectionOptimizer.php
2. I created a new issue to address this: #3261422: Refactor getRequestTime from MemoryBackend
3. About this point I injected the time service at the factories and now each factory pass the time service as argument for the object when constructing it

FYI: The interdiff is actually a diff -u, I'm having some issues locally to generate it, see #24.

I'll leave this as NR, thanks!

Thank you but this approach causes tons tests to fail, that's why I suggest to move backends changes to separate issue

Exception: Drupal\Core\DependencyInjection\ContainerNotInitializedException: \Drupal::$container is not initialized yet. \Drupal::setContainer() must be called with a real container.

1. +++ b/core/modules/search/src/SearchIndex.php
   @@ -58,10 +66,12 @@ class SearchIndex implements SearchIndexInterface {
   -   * @param \Drupal\search\SearchTextProcessorInterface $text_processor
   +   * @param \Drupal\search\SearchTextProcessorInterface|null $text_processor
   

   I think that this change is a mistake.

2. I do not think that we need to add deprecation message testing for new parameters of the method __construct().

Edit: I somehow removed the patch files from comment #29 and I have no idea how to put them back.

With autowiring, maybe this is now simpler? On it.

I think we should limit the scope of this issue to core services only, and address module services separately.

Not arguing, but curious: In the current MR there are only 3 modules addressed, are these not all of them, because three seems a low number to split an issue on.

Just because there are a few more core ones to address and the MR is getting fat. But yeah let’s see at the end.

Postponing on #3371840: Time::getRequestTime is not immutable when there is no request.

This will need some simplification once #3371840: Time::getRequestTime is not immutable when there is no request is in, and understanding why converting UpdateProcessor fails for UpdateSemverCoreTest.

blocker commited

I am working on this

just rebase for now

Calling Drupal\Core\Cache\MemoryCache\MemoryCache::__construct() without the $time argument is deprecated in drupal:9.4.0 and $time argument will be required in drupal:10.0.0. See https://www.drupal.org/node/3113971

Unassigning, I will be afk for a few days.

Doing some grunt work.

Grunt work done:

- Updated IS.
- Added CR.
- Updated deprecation message and pointed to new CR.
- Used constructor property promotion.
- Made sure new TimeInterface $time constructor parameter is not after optional constructor parameters.

Thanks @voleger for the review.

Personal takeaways:

- There seems to be no mention nor method in our deprecation policy about adding constructor parameters needing to preceed optional constructor parameters.
- Service autowiring _really_ doesn't like union typehints.
- Unsure why when adding autowire: true to services we don't remove any existing arguments: ['@foo', '@bar']. They seem to be ignored when autowire is set to true, and (at least for me) add more confusion than help.

Back to NR, so somebody who actually understands all of this can have a look :)

@Spokje thanks a lot for driving this to green again first of all.

I think we should not do the autowire: true, at least not here. Better leave it to a dedicated issue. When I started using it it was more to see the effect, and I agree that given your findings, there are shadows with autowiring when there are optional parameters or deprecated ones, so let's not have that headache here.

NW to remove autowire: true from all the yml files and replace with explicit '@datetime.time' argument.

Thanks @mondrake, I misunderstood your afk-message as you being on holiday for a long(er) time and took the chance to mess your MR up ;)

Removed the autowiring, back to NR

No worries at all - thanks @Spokje!

Massive work!

I'd RTBC but cannot really. Will drop a thread in Slack to see if anyone's up for a review.

All green and seems @mondrake has done a review so I'll lean on that.

Don't mind marking for yall.

Sorry for being late to the party, but I have started before this issue was marked RTBC

Thanks for the review @Taran2L, applied 2 of your proposed changes, answered 2 others.
Back to NR.

left a comment on MR.

All points since previous RTBC were addressed, back to RTBC.

I am afraid if we don’t start getting these issues solved, we’d have to bring the deprecated define into D11.

rebased

Will sort this out (my) tomorrow morning, about 12hrs from now.

Rebased and updated CR and deprecation errors to mention drupal:10.3.0.

Nice to see this getting fixed.

I looked at this a few days ago and was put off by the size of the MR., 64 files + 518 − 192. This is well above the recommended patch size. And thus there is a greater risk of introducing errors. However, since it more or less a lot of boilerplate I checked with the other release managers before replying.

There is agreement that this needs to be re-scoped into smaller, manageable sizes. I will leave it up to the people working on this issue to create child issues that are easy to review.

Would a split between \Drupal\Core\Cache\* and the rest work?

Still probably (well) above the ideal patch size, but as said, it's mostly boilerplate code.

Attempted a rebase, the services.yml conflicts with cache factories may have been incorrect.

Re #66 - is there an opportunity to bypass that requirement here? IMO the MR is not that difficult to review, it touches a lot of files but it is more than manageable. Splitting would require a bunch more busy work, conflict resolution, etc. These issues are already split into half a dozen issues from the meta and are all going to run into similar issue. As stated in #67 how would we even split it? There's no logical grouping here really.

Seems all threads have been addressed

Tests are still green and deprecation links seem fine. Not sure I've seen a generic CR cover so many classes before but also don't want to see a dozen CRs for the same.

LGTM! Going to mark as this appears to have gone through a number of reviews before.

The MR has multiple merge conflicts so not committable.

Additionally I found multiple issues when reviewing - mostly nits but more than would have been fixable on commit.

@acbramley I'm agnostic about whether this is big enough to be split up, but definitely if it had been smaller in the first place it might have been easier to get in by now. While the changes are mostly boilerplate, they're not scannable find and replace stuff and I found things when reviewing. Whether it would be less work to split at this point is a different question (although if we did, then cache/non-cache seems fine as a split point).

Thanks @catch - my hope was to avoid more round trips and nasty conflicts again but this has bitten me badly now and we have some horrendous stuff to deal with now that #839444: Make serializer customizable for Cache\DatabaseBackend is in. I'm not even quite sure how to handle the now 2 new optional params in DatabaseBackend. That issue didn't take into account string $max_rows either (discussed in the MR on this issue).

Only one test failed!

Added 2 comments to help with #73

This is good to go again

Added fix for all constructors
https://git.drupalcode.org/project/drupal/-/merge_requests/4302/diffs?co...

PHPCS is now failing... I don't understand why those variables had to change. They did not need to be fixed, proven by a green pipeline and my manual testing :)

reverted useless change, only improved comments in my commit

Needs a manual rebase now.

Rebased

Appears all feedback has been addressed.

I'm going to go ahead and commit this despite #66 because I've already reviewed it in its current state once and it's looking good now. However the re-roll hell this has been in since December validates that it probably would have been easier in smaller chunks - e.g. splitting cache changes out to their own issue, or core/modules changes or both. It's always hard to balance between too many changes in one issue vs. too many issues for one change, and can be hard to tell when you start working on an issue how big the eventual MR is going to end up, so very tricky to figure out.

Committed/pushed to 11.x, thanks everyone!

Thank you, CR looks updated, so could be published and MR removed

Thanks very much @catch