Replace non-test usages of \Drupal::time() with IoC injection

Reroll the patch

Thanks for working on this.

It's not just replacing \Drupal::time() with \Drupal::service('datetime.time') , but

It's to replace \Drupal::time() and \Drupal::service('datetime.time') with IoC injection where possible.

Kindly see the Parent issue for more info.

If \Drupal::time() is not being removed, then why replace it with \Drupal::service('datetime.time')? Aren't these two expressions functionally equivalent?

I'd assume that the issue is referring to adding the service via dependency injection where possible.

@jungle According to me I have found only two places where I have done with the changes(Service via dependency injection), Please have a look and advise.

Fixed the test, Please review.

Adding changed record and deprecation error message.

Please note that Drupal\Core\Queue\DatabaseQueue and Drupal\Core\Queue\MemoryQueue are currently using time(), but will be switched to \Drupal::time() after #3116478: Add a way to silently keep an item locked when processing a queue via cron is committed.

The reason that we cannot use dependency injection right away is explained in #54.

The related issue in my last comment has been committed (see 0f10d21).

The scope of this issue will need to be adjusted accordingly to accommodate the changes therein.

Re-rolled patch, updated summary.

MR !910 simply adds IoC injection to all non-static, non-test references to \Drupal::time() without consideration for whether it makes sense or not; I figure code review can be used to determine where we consider this change to actually be useful.

Two areas worth highlighting:

1. _batch_queue($batch_set)

   In order to use IoC for the queue subsystem, we're going to have to solidify some potentially-shoddy API surrounding the queue subsystem's usage in the Batch API. I went ahead and made this change, but I fully expect that we'll either want to improve the Batch API's invocation of the queue subsystem, or ignore this change for now.

2. \Drupal\update\ProjectSecurityRequirement

   Using IoC for this class felt a bit silly while I was implementing it, but I went ahead with it anyway for completeness. I figure this will be another area that will require a bit of discussion to line out.

Tagging with "Needs framework manager review" since it doesn't seem practical to set the component for each subsystem. Affected subsystems (as best as I can tell):

• batch system
• entity system
• cron system (queue)
• comment.module
• system.module
• update.module

Hiding the patches as this appears to be worked on in merge requests.

Can we please get the MR updated to point to 10.1.x

it needs new MR or patch for 10.1.x

@clayfreeman please change MR branch if it's doable

Commit check failures
The last patch doesn't pass commit checks, could you make sure to run ./core/scripts/dev/commit-code-check.sh before uploading a patch to make sure there are no issues with code formatting. see https://www.drupal.org/docs/develop/development-tools/running-core-devel...

@rpayanm please check your patches before uploading..

Seems there were some errors

Added new 10.1.x MR, the old one had no comments/threads, so no need to c/p them.

unsure if this actually needs a framework manager review.

Pushed one commit to fix own feedback

Changed messages to 10.1.0, added types and cleaned up
andypost authored 4 minutes ago

- message is for 10.1.x (formalized)
- properties should be typed to interface
- static method also needs injection
- testing could mock less
- use interfaces in mocks

also fixed CR

Thanks @andypost, got a bit too sloppy in my haste there.

I've already tried removing the always-NULL parameter Connection $connection = NULL from \Drupal\Core\Queue\Memory::construct, but that didn't end well: https://www.drupal.org/pift-ci-job/2536193

By the looks of the current TestBot run, we'll end up with the same result.

oh, missed that, thanks for pointer!

I'm thinking to file new blocker bug - properly pass arguments to queue constructor in _batch_queue

Makes sense to me, at least far more sense than keeping an always-NULL parameter in the middle of a constructor...

Filed #3325158: Properly instantiate a queue in _batch_queue()

It's green after random failure, just not sure about form.inc changes

Probably better re-purpose #3325158 as follow-up for 11.0.x to remove the bridge code

Reviewing the MR looks like the deprecation is correct. The MR will have to be rebased though.

Should this be postponed until framework manager reviews.

#57 appears to be a rebase click.

Reviewing the MR I see all the instances have been addressed.

When I checked locally SqlContentEntityStorageSchema::getTemporaryTableMappingPrefix() I see one more instance I think should be replaced.

$prefix_parts[] = \Drupal::time()->getRequestTime();

This method is static so it can't use DI for service

Maybe it could use follow-up to add argument to inject service but better not

Added fix for #58 https://git.drupalcode.org/project/drupal/-/merge_requests/3062/diffs?co...

Confirmed the point in #58 is addressed. That was my only note. Good to mark this one.

I think we can add a deprecation to allow us to remove the \Drupal::time() from \Drupal\Core\Entity\Sql\SqlContentEntityStorageSchema::getTemporaryTableMappingPrefix

Added some review comments to the MR.

Thanks @alexpott, left one thread open since PHPCS didn't like it

Thanks @mondrake for the (per usual) excellent threads in the MR. Answered them inline over there.

Added a comment to the MR.

Thanks @mondrake, added readonly everywhere.

One significant point in the MR, then, alas, I think we need deprecation tests.

I'm gonna need some help with this test failure:

There was 1 error:

1) Drupal\Tests\Core\Entity\Sql\SqlContentEntityStorageTest::testSqlContentEntityStorageConstructorDeprecation
TypeError: Drupal\Core\Entity\Sql\SqlContentEntityStorage::getCustomTableMapping(): Argument #1 ($entity_type) must be of type Drupal\Core\Entity\ContentEntityTypeInterface, null given, called in /var/www/html/core/lib/Drupal/Core/Entity/Sql/SqlContentEntityStorage.php on line 370

/var/www/html/core/lib/Drupal/Core/Entity/Sql/SqlContentEntityStorage.php:393
/var/www/html/core/lib/Drupal/Core/Entity/Sql/SqlContentEntityStorage.php:370
/var/www/html/core/lib/Drupal/Core/Entity/Sql/SqlContentEntityStorage.php:217
/var/www/html/core/lib/Drupal/Core/Entity/Sql/SqlContentEntityStorage.php:202
/var/www/html/core/tests/Drupal/Tests/Core/Entity/Sql/SqlContentEntityStorageTest.php:1487
/var/www/html/vendor/phpunit/phpunit/src/Framework/TestResult.php:728
/var/www/html/vendor/phpunit/phpunit/src/Framework/TestSuite.php:673
/var/www/html/vendor/phpunit/phpunit/src/TextUI/TestRunner.php:661
/var/www/html/vendor/phpunit/phpunit/src/TextUI/Command.php:144
/var/www/html/vendor/phpunit/phpunit/src/TextUI/Command.php:97

No clue why calling the constructor of SqlContentEntityStorage causes this?

Was able to fix locally by adding

  public function testSqlContentEntityStorageConstructorDeprecation(): void {
    $this->setUpEntityStorage();

Also had to update the deprecation message expected.

Didn't push the changes so I can stay in the reviewer side.

THanks @smustgrave, that was indeed the missing piece of the puzzle.

Left some comments, removing the tag for now, but will keep an eye on the issue/MR comments

Thanks @larowlan for the review.

Back to NW for the open threads in the MR.
I'm going to let somebody else step in, because I've ran out of time/steam/joy working on this one.