Default lifetime for permanent items for Drupal 8

The documentation is not written correctly. $conf['redis_perm_ttl'] = "1 year"; does not work for Drupal 8.

The current version only supports a lifetime for permanent items for specific cache bins e.g. $settings['redis.settings']['perm_ttl_cache_field'] = "3 months";

I have added a patch which adds support for a default lifetime for all permanent items across bins.

I agree with the approach in #2. The only thing IMHO is if, for consistency with Drupal's bin naming, name for the fallback, default ttl for all the bins should be perm_ttl or perm_ttl_default? I have no strong opinions, and with good documentation, which was also provided in #2, both options looks fine.

One last thing I spotted just before posting a comment above - ternary operator `??` was added in php 7 but the module doesn't set minimal php requirements neither via composer.json nor redis.info.yml, hence back to "Needs work".

The patch seems to work quite well; I'm able to configure a TTL for my cache entries when I use it. However, one bug remains; in the part just below what the patch changes, there's a call to DateInterval::createFromDateString() followed by a calculation of the number of seconds. In this, the variable $iv->days is used, which, according to the PHP docs, is only given a value "If the DateInterval object was created by DateTime::diff()". if not, it will be false.

Changing it into $iv->d makes it work: I ran into this because I used the string "1 day", which I've now changed to 86400 to circumvent the bug.

I've created a patch that fixes this. I'm new to this system (I'm used to using GitHub), so I wasn't sure whether my patch needed to include the previous one or not, so I made two.

New patch with # 5 but eliminating the ?? who complains in # 4

I upload # 6 again with the correct path to the module so that the patch is applied correctly

1. +++ src/Cache/CacheBase.php	(date 1582318020000)
   @@ -210,15 +210,16 @@
   +      if (!empty($settings['perm_ttl_' . $this->bin]) || !empty($settings['perm_ttl'])) {
   +        $ttl = isset($settings['perm_ttl_' . $this->bin]) ? $settings['perm_ttl_' . $this->bin] : $settings['perm_ttl'];
   

   I see the php 7 comment earlier which is annoying. That would have been a lot cleaner.

   The first time I read this I thought this was fine and the mix of isset and empty was just awkward to read. I think its actually wrong though. If I'm noodling this correctly, using empty in the outside if statement means you can't set the perm_ttl to 0 which is actually documented in the README as a feature so you can make perm truely non-volatile again.

2. +++ src/Cache/CacheBase.php	(date 1582318020000)
   @@ -210,15 +210,16 @@
            if ($ttl === (int) $ttl) {
              $this->permTtl = $ttl;
            }
            else {
              if ($iv = DateInterval::createFromDateString($ttl)) {
                // http://stackoverflow.com/questions/14277611/convert-dateinterval-object-to-seconds-in-php
   -            $this->permTtl = ($iv->y * 31536000 + $iv->m * 2592000 + $iv->days * 86400 + $iv->h * 3600 + $iv->i * 60 + $iv->s);
   +            $this->permTtl = ($iv->y * 31536000 + $iv->m * 2592000 + $iv->d * 86400 + $iv->h * 3600 + $iv->i * 60 + $iv->s);
              }
              else {
   

   Just an observation because its not part of your changes but its weird this isn't written in a more clear way like:

   if () {
   }
   elseif () {
   }
   else {
   }
   

   Maybe left over from more complicated logic or the author just didn't like elseif :-D

Any updates?

This is not really clear

I've updated #7 for the current version and changed the empty() check to isset() as pointed out by #8

Hi, just realized that this is still in NR.

We have been applying #10 to two products for over a year now and it appears to be working as intended. Hope this helps move things along!

Pretty sure this is a duplicate of #3179757: TTL handling broken, always permanent?

I second #12 - the patch has been applied to the site I'm working on for 2 years and it's been working great on a high volume website.

(Disclaimer: Nick and I work at the same organization but a different site ;-P)

We have been using the patch from #10 for the last 2 years.
The calculation from issue 3179757 is the same as the one done in this issue.
Would be good to have one of them merged into a stable version.
Currently, we are using 7 patches to make this module work.

@Alina Basarabeanu Could you please share the list of patches you use?

1. "2944938: Default lifetime for permanent items for Drupal 8": "https://www.drupal.org/files/issues/2020-09-23/redis-default_permanent_t...",
2. "3163436: Connection refuse in PhpRedis when sentinel/redis are down": "https://www.drupal.org/files/issues/2023-09-13/Add_a_Redis_connection_te...",
3. "3175412: Use item_id instead of qid for Queue item": "https://www.drupal.org/files/issues/2020-10-07/redis-queue-item-id-31754...",
4. "3154093: Add a dedicated permission for accessing redis report": "https://www.drupal.org/files/issues/2020-06-23/3154093-redis-perm.patch",
5. "3192255 & 3173988: Wrong memory percentage on /admin/reports/redis": "patches/Merge_issue_3192255_and_3173988_into_one_patch_.patch",
6. "3004561: Support TLS for Predis": "https://www.drupal.org/files/issues/2021-11-19/redis-support_tls_on_pred..."

@Alina Basarabeanu Thanks! In my case, apart from current #2944938: Default lifetime for permanent items for Drupal 8, I'm using patch on #3216874: Unsupported operand types in CacheTagsChecksumTrait.php.