Working on tests for cacherouter I found that sometimes $user->cache is not initialized for example in case fast-page-cache

here is a simple fix

CommentFileSizeAuthor
#13 698248-cache-notice-d7.patch949 bytesandypost
cache_notice.patch714 bytesandypost
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

andypost’s picture

Component: other » base system
Issue tags: +Quick fix

D7 version is different but notice in in caching is really bad, tagged

andypost’s picture

Most of time $user->cache is not initialized so it seems isset() check is less resource intensive then current implementation

droplet’s picture

Status: Needs review » Reviewed & tested by the community

Status: Reviewed & tested by the community » Needs work

The last submitted patch, cache_notice.patch, failed testing.

Gábor Hojtsy’s picture

Why not couple that into a !empty($user->cache)?

andypost’s picture

Status: Needs work » Reviewed & tested by the community

@Gábor Hojtsy, just because isset() faster. We just need to check if this property set

Gábor Hojtsy’s picture

If you just need to see if it is set, why do you have "&& $user->cache" after that?

andypost’s picture

because I need to check if this property isset() and if so to continue comparison

Gábor Hojtsy’s picture

Where does core use or our coding guidelines suggest this pattern to "improve performance"?

andypost’s picture

As I posted in original issue - $user->cache is not avaliable in page_cache_fastpath()

Edit So this lead to notices in bootstrap when using custom cache implementation

+++ includes/cache.inc	28 Jan 2010 19:25:32 -0000
@@ -39,7 +39,7 @@ function cache_get($cid, $table = 'cache
-      if ($user->cache > $cache->created) {
+      if (isset($user->cache) && $user->cache > $cache->created) {
         // This cache data is too old and thus not valid for us, ignore it.
Gábor Hojtsy’s picture

Status: Reviewed & tested by the community » Fixed

Ok, misread the patch, sorry. Committed!

andypost’s picture

Version: 6.x-dev » 7.x-dev
Status: Fixed » Needs review

I found that D7 affected too, but not sure about page_cache_fastpath() in D7 so need review

andypost’s picture

FileSize
949 bytes

Patch

thedavidmeister’s picture

Assigned: andypost » Unassigned
Status: Needs review » Postponed (maintainer needs more info)
Issue tags: +Needs tests

I don't think would be an issue any more, I couldn't find the code patched in #13 at all. I guess it's been refactored significantly since. The closest I could find was:

  /**
   * Prepares a cached item.
   *
   * Checks that items are either permanent or did not expire, and unserializes
   * data as appropriate.
   *
   * @param $cache
   *   An item loaded from cache_get() or cache_get_multiple().
   *
   * @return
   *   The item with data unserialized as appropriate or FALSE if there is no
   *   valid item to load.
   */
  protected function prepareItem($cache) {
    global $user;

    if (!isset($cache->data)) {
      return FALSE;
    }
    // If the cached data is temporary and subject to a per-user minimum
    // lifetime, compare the cache entry timestamp with the user session
    // cache_expiration timestamp. If the cache entry is too old, ignore it.
    if ($cache->expire != CACHE_PERMANENT && variable_get('cache_lifetime', 0) && isset($_SESSION['cache_expiration'][$this->bin]) && $_SESSION['cache_expiration'][$this->bin] > $cache->created) {
      // Ignore cache data that is too old and thus not valid for this user.
      return FALSE;
    }

    // If the data is permanent or not subject to a minimum cache lifetime,
    // unserialize and return the cached data.
    if ($cache->serialized) {
      $cache->data = unserialize($cache->data);
    }

    return $cache;
  }

Which looks like it already has an isset() check, albeit for a different variable.

If this is a bug report for D7, we should write a test to show that it exists, but I think maybe we should just confirm manually that it does before going that far.

#2094585: [policy, no patch] Core review bonus for #2025699: Consolidate 'datetime'/'datetime_form' and 'datelist'/'datelist_form' #type and #theme names for consistency.

thedavidmeister’s picture

Tagging for an issue summary update so we can get clear steps to reproduce the issue.