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

Files: 
CommentFileSizeAuthor
#13 698248-cache-notice-d7.patch949 bytesandypost
PASSED: [[SimpleTest]]: [MySQL] 26,746 pass(es).
[ View ]
cache_notice.patch714 bytesandypost
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch cache_notice.patch.
[ View ]

Comments

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

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

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

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.

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

Status:Needs work» Reviewed & tested by the community

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

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

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

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

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.

Status:Reviewed & tested by the community» Fixed

Ok, misread the patch, sorry. Committed!

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

StatusFileSize
new949 bytes
PASSED: [[SimpleTest]]: [MySQL] 26,746 pass(es).
[ View ]

Patch

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: [meta] [experimental] Core review bonus for #2025699: Consolidate 'datetime'/'datetime_form' and 'datelist'/'datelist_form' #type and #theme names for consistency.

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