All plugins are being added in cache tags when it's not needed

Your permissions are calculated based on your memberships (and other factors). So if your membership changes, it might change your permissions, rendering the cached page stale. This is why we add all of your memberships' cache tags.

Furthermore, any plugin that defines entity access over nodes could be used to group the node the group content is for. When that happens, we invalidate the group_content_list:plugin:group_node:NODE_TYPE cache tag, which therefore must be added so that any new group content for the node is taken into account when calculating permissions.

This is currently not bundle-specific and could be optimized to only find those plugins that deal with the entire entity type (node) or specifically cares about a bundle (node type) as all group_node plugins do. The reason we add them all right now is because parts of this code are copied from the query access logic and there we can't know what bundle we're dealing with.

So the only actionable item I can see right now is to reduce the amount of list cache tags, but that would require some more refactoring on $plugin_manager->getPluginIdsByEntityTypeAccess($entity->getEntityTypeId()); so that we can only retrieve plugins for a given bundle.

Closing as a support request, but feel free to open a feature request that only focuses on that part.

Hi @kristiaanvandeneynde,

Apologies for reopening, but it would be useful to conclude the discussion on this issue before opening others and loosing context.

Your permissions are calculated based on your memberships (and other factors). So if your membership changes, it might change your permissions, rendering the cached page stale. This is why we add all of your memberships' cache tags

This makes complete sense, however it doesn't explain why I need membership cache tags for all memberships in all groups. If I belong to 100 groups, I don't see what the justification to add 100 membership cache tags for a piece of content from Group 1. Changing memberships on Group 1 will have cache implications yes, but not the other 99.

So the only actionable item I can see right now is to reduce the amount of list cache tags, but that would require some more refactoring on $plugin_manager->getPluginIdsByEntityTypeAccess($entity->getEntityTypeId()); so that we can only retrieve plugins for a given bundle.

Yes this is exactly what I was thinking when looking at that method, would you be willing to accept a patch here? The way it is now means any change on any given content will invalidate all lists across all groups and content types, which will have huge implications on caching ratio.

Let me know what you think!

Changing memberships on Group 1 will have cache implications yes, but not the other 99.

That's true, if we are adding totally unrelated memberships, that might be a problem.

But I think it's because we are adding the user.group_permissions cache context to something that might also have the user cache context. In that case, the entire set of calculated permissions is added as a dependency, adding all of your membership's cache tags because user.group_permissions is being optimized away.

See: https://www.drupal.org/docs/8/modules/group/turning-off-caching-when-it-...

As to your second point: I already agree to the idea of finetuning the method to filter plugins on bundles. I just don't have this at the top of my list right now as people are experiencing issues with permissions.

Thanks for the detailed report!

With regard to your findings as to the refactor, I already indicated that it is "copied from the query access logic and there we can't know what bundle we're dealing with".

But focusing on the problem at hand, I really am starting to think we are seeing the user.permissions cache context being optimized away in favor of the user cache context. When that happens, all of the cacheable metadata of your calculated permissions get added to the response.

The reason I'm pretty sure about this is because we are seeing the 'group_permissions' cache tag, which is only added in ChainGroupPermissionCalculator. And believe it or not, most of what you're seeing in the list above might be added because it indeed affects your permissions. A reason why group 2 might be added could be that you're using Subgroup (or ggroup).

Having said that, there is currently a bug (with a fix) where we are adding more group_content_list:x:y cache tags than necessary under certain circumstances. Please see #3110773: List cache tags in getCacheTagsToInvalidate() result in bad cacheability and report back after having tried out that patch.

So, the calculator does add cache tag related to group 2 (group_content:3779) to the list. I think this is not correct, right? Since I'm viewing content on group 6.

This is actually doing things correctly. Your permissions are only calculated once so they need to contain all of your group permissions. So if you are a member of Group A, B and C, it wouldn't matter which page you're on, you'll always get the permissions for all 3 groups from the calculated permissions cache.

The problem is that these cache tags are leaking into the headers, probably when the user.group_permissions cache context is optimized away in favor of the user cache context. If you turn off caching for user.group_permissions, the DynamicPageCacheSubscriber will no longer cache the page (because of auto_placeholder_conditions), so that might get rid of the extra cache tags.

In any case, the goal now is to figure out where user.group_permissions is being optimized away. Setting breakpoints in the CacheContextsManager::optimizeTokens() might help a great deal here.

The list cache tags we can solve for individual access checks, but not for query access checks. So it's quite low on my priority list as we'd still have to deal with these cache tags on pages that show entity lists. (We'd be spending time fixing something that cannot be fixed everywhere.)

I just want to confirm what we are investigating here is: Checking why a group 2 cache tags (group_content:3779) is being added to the header when I'm viewing group content that belongs to group 6. Correct?

Not really, as I'm 99% sure it's being added because 'user.group_permissions' is somehow being optimized away into 'user'. The thing we want to find is where that optimization happens.

Either that, or something else is adding your calculated permissions as a dependency.

set a condition breakpoint on return $optimized_content_tokens; where $context_id = "user.group_permissions" and when viewing a group content page, it never hits this breakpoint.

Hmm, that contradicts what I wrote above :/ Maybe you're hitting a cached response?

I know that this is quite low priority for you, just wanted to check if you have any idea on how to fix and then I can put some effort providing a patch. Maybe you can show me the ropes without actually implementing it? :)

Not off the top of my head, sorry :( Fixing this for query access will be nearly impossible without adding all of the plugin tags.

Here is a patch that simply filters out plugins that are not installed.

What this does

.
* Any plugins that are not enabled will not litter our cache-tag lists.

What this doesn't do

* This doesn't check the bundle type of the $entity in hook_entity_access. I think we could apply this logic for bundle filtering in hook_entity_access, after we retrieve the list of plugin ids.

@carolpettirossi, @kristiaanvandeneynde,
In terms of this issue itself, I agree that we should eventually check the bundle type for any entities we check create cache tags against.
As mentioned before, this should happen in 2 places,

1. group_entity_access
2. EntityQueryAlter

group_entity_access, I think the logic can stay in this function.
EntityQueryAlter, Because of the complexity of checking the logic in Conditions, perhaps it would be better to do it not in a query alter but a in the hook_views_post_render()?
(We would still keep the access check logic in the EntityQueryAlter, Just pull the caching related concerns out of this Class).

thoughts?

If you read this piece of code you will see why we can't only add enabled plugins:

    // Find all of the group content plugins that define access.
    $plugin_ids = $this->pluginManager->getPluginIdsByEntityTypeAccess($entity_type_id);
    if (empty($plugin_ids)) {
      return;
    }

    // Load all of the group content types that define access.
    /** @var \Drupal\group\Entity\Storage\GroupContentTypeStorageInterface $gct_storage */
    $gct_storage = $this->entityTypeManager->getStorage('group_content_type');
    $group_content_types = $gct_storage->loadByContentPluginId($plugin_ids);

    // If any new group content entity is added using any of the retrieved
    // plugins, it might change access.
    $cache_tags = [];
    foreach ($plugin_ids as $plugin_id) {
      $cache_tags[] = "group_content_list:plugin:$plugin_id";
    }
    $this->cacheableMetadata->addCacheTags($cache_tags);

    if (empty($group_content_types)) {
      // Because we add cache tags checking for new group content above, we can
      // simply bail out here without adding any group content type related
      // cache tags because a new group content type does not change the
      // permissions until a group content is created using said group content
      // type, at which point the cache tags above kick in.
      return;
    }

If we do so, that large block of text becomes invalid. So we could try to only add installed plugins, but then we'd have to add the group_content_type list cache tag saying: "We only checked for installed plugins before, so when a new GCT is created, the cached results based on those checks might no loner be valid".

Although ideally we do not clear the cache when a GCT is created for an unrelated entity type. So we'd need to introduce a GCT list cache tag per target entity type and clear that one when a new one is created.

I think this might be a duplicate of #3538509: Try to remove group_relationship list cache tags in group_entity_access() at this point.

Indeed it would seem so.