Contrib.social

Explore adding an opt-in workbench access user.sections cache context

Open on Drupal.org →
Created on 10 October 2023, about 1 year ago
Updated 20 October 2023, about 1 year ago

Problem/Motivation

In 🐛 workbench_access_entity_access should cache per user, or a new WBA specific cache context (per section) Fixed we made access results cache per-user, which effectively prevents caching in Dynamic page cache (UNCACHEABLE).

This is a follow up to explore using a cache context instead

From my comment on that issue

+++ b/workbench_access.module
@@ -63,7 +63,7 @@ function workbench_access_entity_access(EntityInterface $entity, $op, AccountInt
-    $carry->addCacheableDependency($scheme)->cachePerPermissions()->addCacheableDependency($entity);
+    $carry->addCacheableDependency($scheme)->cachePerUser()->addCacheableDependency($entity);
doesn't this mean anything that checks access with WBA is now UNCACHEABLE?

I would still like to explore a WBA specific cache context, something less granular than user but more granular than user.permissions. We can do that in a follow-up though.

Perhaps user.sections and we could just make it a hash of the user's section IDs.

We can use something like \Drupal\Core\Session\PermissionsHashGenerator::hash over the output from \Drupal\workbench_access\UserSectionStorageInterface::getUserSections

But I wonder how common it is for users to be part of the exact same sections?

I guess if you think of a scenario where you have an org split into departments. The bulk of users would be in a single department, so it would be common for them to have distinct groups of users with the same sections.

Worth exploring?

We could make it a configuration option, so that sites could decide if 'per user' (i.e no caching) was better than using a context with a lot of variations that would effectively grow their cache tables but could also have a poor hit rate anyway

Steps to reproduce

Proposed resolution

Add a cache context and see how that goes

Remaining tasks

User interface changes

API changes

Data model changes

📌 Task
Status

Needs work

Version

2.0

Component

Code

Created by

🇦🇺Australia larowlan 🇦🇺🏝.au GMT+10

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

Comments & Activities

  • Issue created by @larowlan
  • Open in Jenkins → Open on Drupal.org →
    Core: 10.1.x + Environment: PHP 8.1 & MySQL 5.7
    last update about 1 year ago
    42 pass, 2 fail
  • @larowlan opened merge request.
  • Status changed to Needs review about 1 year ago
  • Comment about 1 year ago →
    🇦🇺Australia larowlan 🇦🇺🏝.au GMT+10
  • First commit to issue fork.
  • Open in Jenkins → Open on Drupal.org →
    Core: 10.1.x + Environment: PHP 8.1 & MySQL 5.7
    last update about 1 year ago
    44 pass
  • Status changed to Needs work about 1 year ago
  • Comment about 1 year ago →
    🇺🇸United States agentrickard Georgia (US)

    I don't see this buying us a big performance boost -- but I can see use-cases where having the context might be helpful for developers.

    I also wonder if we should cachePerPermission() as well on the access check? That might actually give a perfornace improvement? Though I suppose users without sections would all have the same cache key.

    Code-wise, the docblock for UserSectionsCacheContext doesn't name the class, though.

contrib.social Blog FAQ Discussions
Production build 0.71.5 2024