TTL handling broken, always permanent

This module now has tests on merge requests ( doesn't show up here), and they are failing, which is expected based on what was discussed in the related github issue.

This patch has been required for Pantheon customers hitting maxmemory limit and is critical to evicting keys as expected using LRU

I can't tell whether the automated test needs to change or if the behavior itself has a bug. I've merged in the most recent release to the 8.1 branch, and the error that I'm seeing from PHPUnit is:

1) Drupal\Tests\redis\Kernel\RedisCacheTest::testSetGet
Failed asserting that false is of type "object".
/builds/issue/redis-3179757/vendor/phpunit/phpunit/src/Framework/Constraint/Constraint.php:122

I believe that the automated test itself needs to be updated, but I'm not certain. Anyone have a firm answer on that?

See the duplicate issue #2877893: Cache Expiration doesn't follow Drupal conventions → where I've explained this about 7 years ago :)

The test is from core. Core expects that the API allows to return expired cache items if you ask for that. Setting a TTL in redis is incompatible with the expectation that core has for a cache backend implementation.

The only way to get tests to pass is to make this behavior configurable. Either a boolean enable/disable, or an idea I just had, is that we add a ttl offset setting. So that can be set to e.g. 300, then any ttl we receive is extended by 300s, which means that for this timeframe, we can still return expired items, allowing certain use cases like returning expired items while one process recalculates it. Then we just need to configure that specific test to have enough ttl offset for the tests to still work.

Any updates?

Rerolled patch for 1.8 version of the module without the change to the line:

$this->permTtl = ($iv->y * 31536000 + $iv->m * 2592000 + $iv->days * 86400 + $iv->h * 3600 + $iv->i * 60 + $iv->s);

This line is conflicting with the issue/patch https://www.drupal.org/node/2944938 → , since we are using both patches on our projects.

Again, per #21, this is incompatible with test expectations that core has for cache backends and it will never pass tests, don't bother debugging that, that's by design. Core expects expired items to be returned when asked for. This changes it so that it doesn't.

🐛 Allow to remove support for invalidateAll() and treat it as deleteAll() Active is an example of what I'm asking for. Add a setting that allows to make this a configurable opt-in behavior, it absolutely makes sense, but I want to have redis by default correspond to core cache backend expecatations as much as possible.

I closed one issue as a duplicate, I think I'd prefer to also close ✨ Default lifetime for permanent items for Drupal 8 Needs review as a duplicate and merge them together.

Also, if this is a behavior that is important for your site, feel free to reach out and sponsor a few hours of my time through MD Systems to finish this up with tests and documentation and I'm happy to commit it.

I agree that that "core compatibility" should be configurable.

But if I understand the issue correctly, "core compatibility" currently only exists because of a bug in the calculation?

I don't think that test would fail, because we correctly handle expiration set on items, you need to do a version with TRUE so that invalid items are returned and then expect it to no longer do that. Only then you could actually make it behave differently. See the expire check in \Drupal\redis\Cache\CacheBase::expandEntry.

What I suggested in #21 is that we do this as an integer instead of a boolean flag. NULL would mean the current behavior, also for BC, and any integer, including 0 is the offset added to the provided expiration time. This would only be added to the TTL set in redis and not the expire flag.

Writing to Redis sometimes takes more than 2s in the tests :-(
Expire is time() + 2, but

    'created' => 1746536865.132
    'expire' => '1746536864'

Redis used request time

Redis used request time

That explains why the tests are still failing.
So I leave it to you improve the test.
For us, the patch is good enough to fix the critical issue in our production environment.