KeyValueExpirableFactory incorrectly chooses its storage

While I agree that replacing the DEFAULT_SETTING key from 'keyvalue_expirable_default' to 'default' is the logical long-term fix, what are the backwards compatibility constraints on minor Drupal releases?

I mean, there could be existing installations that override factory.keyvalue.expirable by supplying keyvalue_expirable_default - changing the name of that key would "break" these installations (by forcing them to immediately update their services.yml file).

Is that an acceptable breakage in 8.x?

As this key is not/wrongly documented in default.services.yml and misused in core.services.yml. I don't think a lot of people are relying on it.
For consistency reasons, I think we should replace the default key by default instead of keyvalue_expirable_default.
If we really want to maintain BC, we can still override the \Drupal\Core\KeyValueStore\KeyValueFactory::get() method to look for the keyvalue_expirable_default key if there is nothing defined under the default key.

I guess this is RTBC :
- SPECIFIC_PREFIX not found elsewhere in core code
- This is what I get on a page load before and after patch

$collection | $service_id BEFORE PATCH | $service_id AFTER PATCH
______________________________________________________________________________
state | keyvalue.database | keyvalue.database
user.private_tempstore.views_bulk_operations_content_page_1 | keyvalue.expirable.database | keyvalue.expirable.database
config.entity.key_store.block | keyvalue.database | keyvalue.database
update_available_releases | keyvalue.expirable.database | keyvalue.expirable.database
update | keyvalue.expirable.database | keyvalue.expirable.database
update_fetch_task | keyvalue.database | keyvalue.database
config.entity.key_store.tour | keyvalue.database | keyvalue.database

I'd feel more confident if we added the BC layer even if it's not likely to be used anywhere.

I think the fix needs to go a little further, because currently KeyValueExpirableFactory can't be used to create an expirable store - it only creates non-expirable KeyValueStore stores.

For example, in core, if you do this:
drupal-check -a modules/update/src/UpdateManager.php
You will see (among other things):

  101    Property Drupal\update\UpdateManager::$keyValueStore                  
         (Drupal\Core\KeyValueStore\KeyValueStoreExpirableInterface) does not  
         accept Drupal\Core\KeyValueStore\KeyValueStoreInterface.              
  103    Property Drupal\update\UpdateManager::$availableReleasesTempStore     
         (Drupal\Core\KeyValueStore\KeyValueStoreExpirableInterface) does not  
         accept Drupal\Core\KeyValueStore\KeyValueStoreInterface.

I encountered this while working on Honeypot. Honeypot has this code:

\Drupal::service('keyvalue.expirable')->get('honeypot_time_restriction')
  ->setWithExpire($identifier, \Drupal::time()->getCurrentTime(), 3600 * 24);

Which looks reasonable. This code tries to:

1. Get a KeyValueExpirableFactory using \Drupal::service('keyvalue.expirable')
2. Get a store implementing KeyValueStoreExpirableInterface from that factory
3. Call setWithExpire() on that KeyValueStoreExpirableInterface

But this doesn't work because KeyValueExpirableFactory::get does NOT return KeyValueStoreExpirableInterface as expected, it returns a non-expirable KeyValueStoreInterface which does not support the setWithExpire() method.

Note that if you use keyvalue.expirable.database instead (implemented by KeyValueDatabaseExpirableFactory), then this all DOES work, because KeyValueDatabaseExpirableFactory::get returns the expected KeyValueStoreExpirableInterface.

This is definitely working, otherwise core wouldn't function. I think drupal-check is just confused about the trickery that's going on with the subclass and the extra interface that overrides the return type.

This issue is about the fact that you can't change the returned store to use a non-database implementation based on how it is documented.

I linked as a related issue the original issue which removed the use of (but not definition of) SPECIFIC_PREFIX and added factory.keyvalue.expirable as a service parameter.

Yeah you're right that my comments aren't directly related, but they're an indication that this 'trickery' you referred to is fragile and could be broken by this patch.

Specifically, right now the factory.keyvalue.expirable parameter (which is keyvalue.expirable.database) is being ignored, and as long as no-one tries to override that in core.services.yml then things still work as expected because the default value in core.services.yml is the same value as the fallback DEFAULT_SERVICE, which is also keyvalue.expirable.database.

I assume you mean the 'trickery' here is that the keyvalue.expirable.database service will provide an expirable store with an interface KeyValueStoreExpirableInterface, so that calling setWithExpire() will work even though the return type declaration of KeyValueFactory::get is technically the non-expirable KeyValueStoreInterface.

That's all well and good, and will currently always work because any override of that default service parameter is currently ignored.

But if you fix this issue so that the service parameter CAN be overridden, then there's nothing that guarantees that an expirable store will be returned from the overridden factory, so there's no guarantee that the setWithExpire() method will be available. And if it's not available, core breaks.

Static analysis is not really being fooled, it's just pointing out a problem where the return type is not guaranteed to be KeyValueStoreExpirableInterface therefore there is no contract and no promise that setWithExpire() method will be available.

Not sure if it's related but keyvalue storage test is what causes segfault for core+php8.1+sqlite in CI https://dispatcher.drupalci.org/job/drupal8_core_regression_tests/45252/...

https://www.drupal.org/pift-ci-job/2225773

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.

Reading the comments seems there is still work/discussion to be had.

Updated the issue summary to include missing sections of the template.

I am not sure about the comments and the deprecation message.