Cache Expiration doesn't follow Drupal conventions

Yes, noticed this once as well but forgot to fix.

Replaced the tab with spaces and committed. I don't think there was an actual bug here as we still checked the expiration of the cache item but we had to load it and redis isn't able to clean it up.

Ah, but this breaks tests and $allow_invalid because that will not return expired items anymore.

Reverted for now, maybe we should just expliictly document why we do this and remove the check completely.

Have you decided what you want to do with this? :-)

I am asking because we have an issue with a rest response not expring from redis cache as it should (even though we are setting
$cache->setCacheMaxAge(120); on it.

The patch in question does not fix the issue as it only sets the TTL for dynamic_page_cache:response but not page.

I need to look deeper into this.

Yeah, that doesn't have anything to do with redis, especially not if it works with dynamic page cache.

The internal page cache does not support max-age, it ignores that. See #2352009: Bubbling of elements' max-age to the page's headers and the page cache.

The only thing it cares about is a expires header. Either set that directly if you return a response object, or alternatively implement a response subscriber that does it.

Thank you for these very helpful pointers, Berdir!

I wonder if this issue can be closed? It did confuse me, at least.

And then I hope there could be some progress on the page cache issue you mentioned, it apparently confuses many people. But that is another story.

It'd be great to get this in so Redis can expire things automatically, if only a opt in/out feature.

> Ah, but this breaks tests and $allow_invalid because that will not return expired items anymore.

Im also interested on whether its actually a problem if the server expires cache items. Indeed $allow_invalid should return invalid if the cache item exists, but certainly there is no guarantee that Drupal is the one in charge of doing the expiration; its fine for Redis to expire things if it likes. Tests should be updated to expect this.

The existing $expire > $this->permTtl (timestamp > interval) doesn't make sense to me, its guaranteed to be true since all expiration dates are going to be after ~1970. Surely the condition should be ($expire - time()) > permTtl. Or, something like this seems more readable to me:

$expire = $expire == Cache::PERMANENT : $this->permTtl : $expire
return min($expire, $this->permTtl);

> but certainly there is no guarantee that Drupal is the one in charge of doing the expiration; its fine for Redis to expire things if it likes. Tests should be updated to expect this.

No guarantee is one thing, that's true also for core, but setting the expiration would pretty much guarantee that it's *not* returned I think.

And it's not our test, the test is defined in core, we just run it against redis, to ensure that things are working consistently.

Core tests expecting cache items to exist are really testing the underlying infrastructure (DB backend). Arguably those tests could be ignored.

I'm also facing this issue and just wanted to say that I agree with @dpi.
Allowing the server expire the cache correctly with the cost of not being able to retrieve invalid cache items makes more sense than the other way around (don't allow cache to expire to allow retrieval of invalid cache items).

See my proposal on https://www.drupal.org/project/redis/issues/3179757 and https://github.com/md-systems/redis/pull/32.

Closing as duplicate of the other issues.