When using Simple Caching the cache IDs typically use the following format:

  • panels_simple_cache:[display ID]:[pane ID]:[object ID]

When you are building the panel definition, i.e the definition is in the database, the cache ID will look like these:

  • panels_simple_cache:55:123:26
  • panels_simple_cache:55:124:26
  • panels_simple_cache:55:125:26

If the panels definition is exported and the in-database overrides are reverted, you end up with cache IDs as follows:

  • panels_simple_cache:new:new-1:26
  • panels_simple_cache:new:new-2:26
  • panels_simple_cache:new:new-3:26

Note one glaring problem: it doesn't reference what the entity is, so there's a strong chance of e.g. the panel for user ID 26 conflicting with node ID 26.

CommentFileSizeAuthor
#6 panels-n1349118-6.patch1.21 KBDamienMcKenna
#3 panels-n1349118-3.patch513 bytesDamienMcKenna
#2 panels-n1349118.patch486 bytesDamienMcKenna
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

DamienMcKenna’s picture

DamienMcKenna
Location NH, USA
CreditAttribution: DamienMcKenna commented

I think this should be fixed by including the $context->type in the cache ID, e.g. changing panels_simple_cache_get_id() in simple.inc to include the following:

      foreach ($contexts as $context) {
        if (isset($context->argument)) {
          $id .= ':' . $context->type[0] . ':' . $context->argument;
        }
      }

Thoughts?

DamienMcKenna’s picture

DamienMcKenna
Location NH, USA
CreditAttribution: DamienMcKenna commented
Status: Active » Needs review
FileSize
panels-n1349118.patch486 bytes

Basically the same code as #1, just makes sure that the $context->type[0] exists first.

DamienMcKenna’s picture

DamienMcKenna
Location NH, USA
CreditAttribution: DamienMcKenna commented
FileSize
panels-n1349118-3.patch513 bytes

The wrong patch was attached, try this one.

DamienMcKenna’s picture

DamienMcKenna
Location NH, USA
CreditAttribution: DamienMcKenna commented

This isn't sufficient, it's still possible to lead to collisions by having multiple page definitions that have the same context, e.g. user profile definitions. What really needs to happen is have the 'new' string replaced by the panel's name attribute, thus each definition would *always* be unique.

DamienMcKenna’s picture

DamienMcKenna
Location NH, USA
CreditAttribution: DamienMcKenna commented

The exported name of the panel isn't present in any of the data structures available in panels_simple_cache_get_id(), the closest is $display->cache_key but it is in the format "panel_context:[object_type]:[object_name]", e.g. "panel_context:node_view:node_view__blog_post" (where "node_view__blog_post" is the name of the exported panel. Of course this is only present for some objects. Gah.

DamienMcKenna’s picture

DamienMcKenna
Location NH, USA
CreditAttribution: DamienMcKenna commented
FileSize
panels-n1349118-6.patch1.21 KB

The specific panels that didn't have a $display->cache_key were minipanels, which *did* happen to have a $display->css_id, so this patch first uses a numeric did, then checks cache_key, then checks css_id, then fails back to just using did as-is.

Letharion’s picture

Letharion CreditAttribution: Letharion commented
Assigned: Unassigned » merlinofchaos

This will require merlins opinion.

merlinofchaos’s picture

merlinofchaos CreditAttribution: merlinofchaos commented
Status: Needs review » Fixed

Makes sense.

Mini panels probably don't have a cache_key because they don't support the IPE. Maybe someday they will, but it seems unlikely.

Committed, pushed.

Status: Fixed » Closed (fixed)

Automatically closed -- issue fixed for 2 weeks with no activity.

joshuajabbour’s picture

joshuajabbour CreditAttribution: joshuajabbour commented
Version: 7.x-3.x-dev » 6.x-3.x-dev
Status: Closed (fixed) » Patch (to be ported)

The same problem exists in the D6 branch. The patch in #6 applies cleanly, and fixes the issue.

cedarm’s picture

cedarm CreditAttribution: cedarm commented
Status: Patch (to be ported) » Reviewed & tested by the community

The patch in #6 works for 6.x. Tested with term_view system handler, custom page manager pages, mini panels, and panel nodes. Mini panels in code do end up using the display css_id.

Dean Reilly’s picture

Dean Reilly CreditAttribution: Dean Reilly commented

This patch also fixes another bug where mini panels caches wern't being cleared.

Steps to recreate:
1) Create a mini panel and add a node pane to it.
2) Add the mini panel to a larger panel and view it to create a cache entry.
3) Edit the mini panel to show another node and save it.
4) View the panel again and the node won't have changed.

The problem is that when the mini panel is loaded through the parent panel $display->owner->id is set by panels_mini_panels_mini_content_type_render() in panels_mini/plugins/content_types/panels_mini.inc.

/**
 * Render a mini panel called from a panels display.
 */
function panels_mini_panels_mini_content_type_render($subtype, $conf, $panel_args, &$contexts) {
  static $viewing = array();
  $mini = panels_mini_load($subtype);
  if (!$mini) {
    return FALSE;
  }
  if (!empty($viewing[$mini->name])) {
    return FALSE;
  }

  // Load up any contexts we might be using.
  $context = ctools_context_match_required_contexts($mini->requiredcontexts, $contexts);
  $mini->context = $mini->display->context = ctools_context_load_contexts($mini, FALSE, $context);

  if (empty($mini) || !empty($mini->disabled)) {
    return;
  }
  $viewing[$mini->name] = TRUE;

  $mini->display->args = $panel_args;
  $mini->display->css_id = panels_mini_get_id($subtype);
  $mini->display->owner = $mini;
  // unique ID of this mini.
  $mini->display->owner->id = $mini->name;

  $block = new stdClass();
  $block->module  = 'panels_mini';
  $block->delta   = $subtype;
  $block->content = panels_render_display($mini->display);
  $block->title = $mini->display->get_title();

  unset($viewing[$mini->name]);
  return $block;
}

But this doesn't happen when using the mini panel else where. Hence when we generate the cache id:

  $cid = 'panels_simple_cache';

  // This is used in case this is an in-code display, which means did will be something like 'new-1'.
  if (isset($display->owner) && isset($display->owner->id)) {
    $cid .= ':' . $display->owner->id;
  }
  $cid .= ':' . $display->did;

We end up with different cids so the caches remain untouched.

DamienMcKenna’s picture

DamienMcKenna
Location NH, USA
CreditAttribution: DamienMcKenna commented

Related: #2017455: Exported panels won't clear simple cache

  • Commit a28a038 on 7.x-3.x, 7.x-3.x-i18n, 8.x-3.x by merlinofchaos: 
    Issue #1349118 by DamienMcKenna: Improve cache key generation for...
DamienMcKenna’s picture

DamienMcKenna
Location NH, USA
CreditAttribution: DamienMcKenna commented
Issue summary: View changes
Parent issue: » #1774796: Plan for Panels v6.x-3.11 release (PHP 5.4 fix)
japerry’s picture

japerry
Location KVUO
CreditAttribution: japerry at Drupal Association commented
Status: Reviewed & tested by the community » Fixed

Cool. Easy enough. Fixed.

  • japerry committed 9344f8c on 6.x-3.x authored by DamienMcKenna 
    Issue #1349118 by DamienMcKenna: Simple Caching ID scheme will lead to...

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.