search

colinmollenhour / Cm_Cache_Backend_Redis

A Zend_Cache backend for Redis with full support for tags (works great with Magento)
Other
390 stars 142 forks source link

[FIX] Make redis cache backend work like builtin cache #142

Closed bjornsnoen closed 4 years ago

bjornsnoen commented 4 years ago

Setting lifetime to 0 evaluates to falsey, causing the infinite time flag to be set to true. It also causes the expire to not be set. According to docblock infinity should only be set iff lifetime is null. This makes that work as expected and like magento cache works by default.

Xon commented 4 years ago

Style wise, why not just do $lifetime === null and $lifetime !== null as required?

bjornsnoen commented 4 years ago

Style wise, why not just do $lifetime === null and $lifetime !== null as required?

Not sure what you mean by required as there is no phpcs.xml, no grumphp.yml, or anything like that specifying a code style ruleset. Especially in the ternary using is_null($lifetime) ? 1 : 0 is much more readable than $lifetime === null ? 1 : 0, but if it's a requirement of course I can change it.

colinmollenhour commented 4 years ago

So you're wanting to be able to set an instantly expiring record in cache? What is the use-case for this?

bjornsnoen commented 4 years ago

Fair question, it's just that many modules will simply save cache with a lifetime of 0 as a quick way to bypass cache, instead of actually bypassing cache. In standard magento cache this works, but this module behaves differently, which is the reason for this PR.

colinmollenhour commented 4 years ago

many modules will simply save cache with a lifetime of 0 as a quick way to bypass cache, instead of actually bypassing cache

I've never seen that before, but that sure is an anti-pattern if there ever was one..

bjornsnoen commented 4 years ago

That may be, but is this not supposed to work as a drop-in replacement for the magento cache? And if yes, should it not behave the same way? If not, should it not at least behave in a way consistent with its own internal method documentation?

colinmollenhour commented 4 years ago

Alright, instantly-expiring cache records are now supported.