The cache_get() code seems to never use the "expire" column outside of checking whether it is equal to CACHE_PERMANENT.

Here is the code in question (cache.inc lines 27+):

  if (isset($cache->data)) {
    // If the data is permanent or we're not enforcing a minimum cache lifetime
    // always return the cached data.
    if ($cache->expire == CACHE_PERMANENT || !variable_get('cache_lifetime', 0)) {
      $cache->data = db_decode_blob($cache->data);
      // ...
    }
  }
  ...

If the cache item is not permanent (either temporary or with an expire time) and there is no enforced minimum cache lifetime, shouldn't the code then check to see if the current time is beyond the item's expiration time?

By the time you hit the else clause in line 42 you're guaranteed that the cache item is not permanent and there is a minimum lifetime on the cache. Once it's been determined that the current user's time is past the minimum cache lifetime, shouldn't the code also do another check based on the item's expiration time?

I think the fix to this is simple, but before I submit a patch or anything I would like confirmation that what I'm proposing is expected behavior.

Comments

legion80’s picture

legion80 commented

Despite me saying I didn't want to submit a patch, here's the diff anyway of what I'm proposing:

diff --git a/includes/cache.inc b/includes/cache.inc
index d08a485..9c9fa95 100644
--- a/includes/cache.inc
+++ b/includes/cache.inc
@@ -28,6 +28,9 @@ function cache_get($cid, $table = 'cache') {
     // If the data is permanent or we're not enforcing a minimum cache lifetime
     // always return the cached data.
     if ($cache->expire == CACHE_PERMANENT || !variable_get('cache_lifetime', 0)) {
+      if ( $cache->expire < time() ) {
+        return 0;
+      }
       $cache->data = db_decode_blob($cache->data);
       if ($cache->serialized) {
         $cache->data = unserialize($cache->data);
@@ -43,6 +46,9 @@ function cache_get($cid, $table = 'cache') {
         // This cache data is too old and thus not valid for us, ignore it.
         return 0;
       }
+      else if ( $cache->expire < time() ) {
+        return 0;
+      }
       else {
         $cache->data = db_decode_blob($cache->data);
         if ($cache->serialized) {
Mark Theunissen’s picture

Mark Theunissen commented
Status: Active » Closed (duplicate)

Duplicate of #534092: cache_get() returns expired cache items