Contrib.social

Cookie conditions may add cache contexts too liberally

Open on Drupal.org →
Created on 4 March 2025, about 1 month ago

Problem/Motivation

Presently, the condition plugin adds the cookies:<cookie name> cache context unconditionally. Functionally, this means that the context is added to every page on the site. Sites with multiple block conditions may experience cache sharding that can likely be avoided.

Steps to reproduce

  1. Add a block that should appear on the <front> page when test_cookie equals its a test.
  2. Visit a non-front-page where the block would never render.
  3. Notice the response still varies on cookies:test_cookie.

Proposed resolution

When applying the cache context for a particular cookie, only add it when the block is capable of rendering on the current page.

This can possibly be accomplished through a hook_block_access implementation!

/**
 * Implements hook_block_access().
 */
function psu_cookies_block_access(Block $block, $operation, AccountInterface $account) {
  if ($operation === 'view') {
    $conditions = $block->getVisibilityConditions();
    if ($conditions->has('cookie')) {
      $should_vary = TRUE;
      foreach ($conditions as $condition) {
        // Skip evaluating the cookie condition.
        if ($condition->getPluginId() === 'affiliation') {
          continue;
        }
        if (!$condition->execute()) {
          $should_vary = FALSE;
        }
      }
      // If the block would render without considering the cookie condition, this page has to vary on the cookie,
      // otherwise the block would never have a chance of rendering, so the context doesn't need to be added!
      if ($should_vary) {
        return AccessResult::neutral()->addCacheContexts(['cookies:acquia_a']);
      }
    }
  }
  return AccessResult::neutral();
}

Remaining tasks

  1. Write a test case illuminating the original performance situation.
  2. Improve cache hit rate by adding cache contexts more conservatively.

User interface changes

None

API changes

None

Data model changes

None

📌 Task
Status

Active

Version

2.0

Component

Code

Created by

🇺🇸United States luke.leber Pennsylvania

Live updates comments and jobs are added and updated live.
Sign in to follow issues

Merge Requests

Comments & Activities

  • Issue created by @luke.leber
  • Comment about 1 month ago →
    🇺🇸United States luke.leber Pennsylvania
  • Comment about 1 month ago →
    🇺🇸United States luke.leber Pennsylvania

    Self-assigning to work on a pull request.

  • Merge request !5Vary more conservatively → (Open) created by luke.leber
  • Comment about 1 month ago →
    🇺🇸United States luke.leber Pennsylvania

    Update I.S. - Moving on to some manual testing on the issue fork on a real site :-).

  • Comment about 1 month ago →
    🇬🇷Greece Pavel Ruban

    Hello,

    This module is not about blocks, it's about condition plugin, it's used way wider than just a plain visibility rules, having the patch for blocks users only is fine as long as they don't use conditions in other contexts which will break cache logic.

    There idea is valid that it is preferable to add context only where it's needed, but the resolution breaks other logic.

    I don't recall from the top of my head how contexts work if cache context value doesn't change. I mean whether when you visit a page without cookie condition the context key generates several variants of the data or it adds 1 variant as context cookie value always null. If it doesn't overflow the data yes it's still undesired to have a lot of cache context keys but at gets less critical but still good to have a fix.

    Regarding the other places which may use this, e.g. we use it for conditioned layouts https://www.drupal.org/project/layout_library/issues/3089493 ✨ Library Layouts Auto Selection & Selection Rules Like It Was In Panels (D7) for Page Variants Needs work , as it's a plugin you may basically instansiniate a plugin and use it as bool condition in custom logic. I guess the more correct way is to rely on plugin invocation and add context whenever it's invoked despite of the invocation context, whether it's blocks, layout library, custom code or any other logic.

  • Comment about 1 month ago →
    🇺🇸United States luke.leber Pennsylvania

    Those are really good points!

    I think perhaps our use case (in using a condition plugin to control the presence of global blocks) may be best achieved with a different mechanism -- probably on the block types themselves.

    I don't see an elegant way to hit all of the needs of this module as prescribed in the I.S. I'm leaning towards this being a wontfix.

    Thanks for the swift response.

  • Comment about 1 month ago →
    🇬🇷Greece Pavel Ruban

    Also your resolution even for block case is faulty, if you add 2 cookie plugins or more you basically apply only one context and ignore all the rest

  • Comment about 1 month ago →
    🇺🇸United States luke.leber Pennsylvania

    I wasn't aware that more than one condition of a specific type per block was possible to configure. The core block GUI does not allow for that.

    Perhaps with custom code or lesser popularity contrib though? That should be easy to resolve, but I'm not sure it's worth doing.

contrib.social Blog FAQ Discussions
Production build 0.71.5 2024