\Drupal\group\Entity\GroupContent::getCacheTagsToInvalidate defines a whole bunch of list cache tags for various use cases. They are useful, but they are in the wrong place.
getCacheTagsToInvalidate() is also called from getCacheTags(), which mean that if you do ->addCacheableDependency($group), then all these list cache tags are also added to that element, but you didn't ask for cache tags for what happens when any content in that group changes, just that specific group.
The naming of those methods is a bit unfortunate and this isn't clear. This also caused a lot of confusing in the recently committed bundle cache tag issue in core: #2145751: Introduce ENTITY_TYPE_list:BUNDLE cache tag and add it to single bundle listing.
Luckily, that core issue also gives us exactly what we need here, a new method getListCacheTagsToInvalidate() that we can override. Unlike the other method, this one is _only_ used for invalidation, and it is up to each implementation of a list to pick the correct cache tag. Which is most likely only one, for example you only want to invalidate content of group A XOR everything of the plugin X. Not all possible things that the group module supports to invalidate.
This also means that this patch requires #3088226: Remove deprecations, compatibility with Drupal 9 and won't work on 8.7.
| Comment | File | Size | Author |
|---|---|---|---|
| #2 | group-list-cache-tags-3110773-2.patch | 1.49 KB | berdir |
Comments
Comment #2
berdirThe test still passes because you only test invaidation not how cache tags are added. Might want to do that as well somewhere in a functional test when looking at group/1, but these things also tend to break easily if core adds/removes some cache tags on the default output due to core changes.
Comment #4
berdirFailed as expected on 8.7.
Comment #5
berdirAlso failed on 8.8, passed now on 8.9, forgot that the bundle list cache tag issue is only in 8.9.
You might want to hold out a bit with this, but this is a pretty severe performance issue.
Comment #6
kristiaanvandeneyndeI'll add it to the 8.1 release, at which time it should be safe to bump up the core version requirement.
Comment #7
kristiaanvandeneyndeComment #9
kristiaanvandeneyndeCommitted, thanks!
Re #3: Might need to do a follow-up for that. Currently a bit stretched on time.
Will bump core version to 8.9, but doing so in a separate ticket so it makes it into the release notes.