Use the exact current time in KV and tempstore.

OK let's see what fails.

Looks good.

1. +++ b/core/lib/Drupal/Core/KeyValueStore/DatabaseStorageExpirable.php
   @@ -25,9 +33,11 @@ class DatabaseStorageExpirable extends DatabaseStorage implements KeyValueStoreE
   +   * @param \Drupal\Component\Datetime\TimeInterface $time
   

   Nit, needs to have a comment on the line below it, something like "The injected Time service."

2. +++ b/core/lib/Drupal/Core/KeyValueStore/DatabaseStorageExpirable.php
   @@ -25,9 +33,11 @@ class DatabaseStorageExpirable extends DatabaseStorage implements KeyValueStoreE
   -  public function __construct($collection, SerializationInterface $serializer, Connection $connection, $table = 'key_value_expire') {
   +  public function __construct($collection, SerializationInterface $serializer, Connection $connection, $table = 'key_value_expire', TimeInterface $time = NULL) {
        parent::__construct($collection, $serializer, $connection, $table);
   +    $this->time = $time ?: \Drupal::service('datetime.time');
   

   This is the only place in the patch where the time service is optional. The patch should be consistent; either do this on all of the constructors or non of them. Of the options, I think it should be mandatory. If it is mandatory, then the patch needs an empty post_update hook to make sure the container gets rebuilt (prob in system.module?).

3. +++ b/core/modules/user/src/PrivateTempStore.php
   @@ -81,12 +89,13 @@ class PrivateTempStore {
   -  public function __construct(KeyValueStoreExpirableInterface $storage, LockBackendInterface $lock_backend, AccountProxyInterface $current_user, RequestStack $request_stack, $expire = 604800) {
   +  public function __construct(KeyValueStoreExpirableInterface $storage, LockBackendInterface $lock_backend, AccountProxyInterface $current_user, RequestStack $request_stack, $expire = 604800, TimeInterface $time) {
   

   Looks like missing a @param?

Thanks @mpdonadio. I'll update the patch.

OK let's test this one.

1. Added the comment.
2. Made $time mandatory. Updated a few other places.
3. Added missing @param.

@dawener, so this is a bug-report but we are only going to add it to 8.4.x and not cherry pick to 8.3.x? That is what I mentioned the post update hook. If that is the case, we should probably change this to a Task.

What kind of tests do we need here?

Do \Drupal\KernelTests\Core\KeyValueStore\DatabaseStorageExpirableTest, \Drupal\Tests\user\Unit\SharedTempStoreTest and \Drupal\Tests\user\Unit\PrivateTempStoreTest cover these?

#11, it looks like all of them are testing this, I think this can be a new test-method in the kernel test.

Setting to needs work to add the tests.

@dawehner, if you have a better idea where the test should go, I would be helpful to provide more direction.

This is a messy reroll b/c the array syntax change and #2935617: Move User module's temp stores to core.

Here is a reroll, more of the re-do variety.

Patch from #22 seems to still apply cleanly on 8.8.x-dev

I updated the patch and made it compatible with version 8.8.x.

Also fixed the issues that I've found.

I fix some issues finded by Drupal phpcs in this patch.

FILE: /home/thallesvf/Desktop/lando/d8x/core/lib/Drupal/Core/KeyValueStore/DatabaseStorageExpirable.php
-------------------------------------------------------------------------------------------------------
FOUND 1 ERROR AND 1 WARNING AFFECTING 2 LINES
-------------------------------------------------------------------------------------------------------
  38 | ERROR   | Arguments with default values must be at the end of the argument list
 120 | WARNING | Possible useless method overriding detected
-------------------------------------------------------------------------------------------------------

Fix Drupal\Tests\ComposerIntegrationTest and 9 coding standards messages

This issue is being reviewed by the kind folks in Slack, #needs-review-queue-initiative. We are working to keep the size of Needs Review queue [2700+ issues] to around 400 (1 month or less), following Review a patch or merge request as a guide.

Tagging for framework managers thoughts as this feels like something that could be disruptive for other sites.
Tagging for IS update for remaining tasks as this will have to be rescoped for D10
Tagging for change record update on if this moves forward it will need one for sure.

Patch for Drupal version 10.1.x

Resolved PHPCS errors from #38

Still needs issue summary update and change record

Trying to resolve CCF corresponding to #39, upload a patch and interdiff with it.

Fixed the custom commands on #41
upload a patch and interdiff with it.

I am on this. CR added. Constructor property promotion will be used.

Added "in short, REQUEST_TIME does not equal to \Drupal::time()->getCurrentTime()." to the IS.

Oops. 2855457-48.patch is empty.

Removed credit for #38, #39, #41, and #42 as it's expected the patch to be tested locally before uploading.

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

Reviewing #49
Changes look good but think we will need a simple kernel test (maybe Unit) for checking the deprecation of each of the constructors when a null time parameter is passed.

Going to post to the slack channel also for the framework feedback.

+++ b/core/lib/Drupal/Core/KeyValueStore/DatabaseStorageExpirable.php
@@ -24,9 +25,15 @@ class DatabaseStorageExpirable extends DatabaseStorage implements KeyValueStoreE
-  public function __construct($collection, SerializationInterface $serializer, Connection $connection, $table = 'key_value_expire') {
+  public function __construct($collection, SerializationInterface $serializer, Connection $connection, $table = 'key_value_expire', protected ?TimeInterface $time = NULL) {

The problem here is that we will have to remove the default for $table in Drupal 11 as you can't have mandatory arguments following optional ones. Is it worth doing the argument dance here so $time comes before $table?