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

Comments

andypost’s picture

andypost
he/him
Primary language Russian
commented
Component: other » base system
Issue tags: +Quick fix

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

andypost’s picture

andypost
he/him
Primary language Russian
commented

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

droplet’s picture

droplet commented
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

gábor hojtsy
he/him
Primary language Hungarian
Location Hungary
commented

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

andypost’s picture

andypost
he/him
Primary language Russian
commented
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

gábor hojtsy
he/him
Primary language Hungarian
Location Hungary
commented

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

andypost’s picture

andypost
he/him
Primary language Russian
commented

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

gábor hojtsy’s picture

gábor hojtsy
he/him
Primary language Hungarian
Location Hungary
commented

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

andypost’s picture

andypost
he/him
Primary language Russian
commented

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

gábor hojtsy
he/him
Primary language Hungarian
Location Hungary
commented
Status: Reviewed & tested by the community » Fixed

Ok, misread the patch, sorry. Committed!

andypost’s picture

andypost
he/him
Primary language Russian
commented
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

andypost
he/him
Primary language Russian
commented
StatusFileSize
new 698248-cache-notice-d7.patch949 bytes

Patch

thedavidmeister’s picture

thedavidmeister commented
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

thedavidmeister commented
Issue tags: +Needs issue summary update

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

Status: Postponed (maintainer needs more info) » Closed (outdated)

Automatically closed because Drupal 7 security and bugfix support has ended as of 5 January 2025. If the issue verifiably applies to later versions, please reopen with details and update the version.