DDST-213: Adjust caching for embargo and related access control. #39
Conversation
| @@ -42,7 +42,8 @@ | |||
| * "html" = "Drupal\Core\Entity\Routing\AdminHtmlRouteProvider" | |||
| * }, | |||
| * }, | |||
| * list_cache_tags = { "node_list", "media_list", "file_list" }, | |||
There was a problem hiding this comment.
As per Drupal's EntityTypeInterface::getListCacheTags(), which should be:
The list cache tags associated with this entity type.
Enables code listing entities of this type to ensure that newly created entities show up immediately.
Really could leave unset, as the default is embargo_list, but whatever, we can set explicitly.
Thinking this was originally set as it was in a misguided attempt to have the effects of the embargo applied immediately, but this fits more inline with ::getListCacheTagsToInvalidate(), which is used when invalidating cache tags on entity save/delete, instead of providing the tags/contexts to set where listing of the given embargo entities are used, such as when calculating the access control results.
adam-vessey
changed the title
| * list_cache_contexts = { "ip.embargo_range", "user" }, | ||
| * list_cache_tags = { "embargo_list", "embargo_ip_range_list" }, |
There was a problem hiding this comment.
Have come to realize: Except for embargo_list under list_cache_tags (which would be the default value, anyhow), none of this should be here, especially user under the contexts.
Having the user context specified in anything effectively prevents any caching, as per Drupal's default configuration... thinking the inclusion here was intended to be used in some manner to try to deal with user-specific exemptions (and without awareness at the time of poisoning cacheability), but such isn't really relevant to the listing of the embargo entities proper that these keys are meant for. Instead, we're somewhat in the situation with wanting to set cache contexts/tags from our query tagging/altering, which isn't directly possible. There are some other approaches that might make sense to "bubble" cache metadata via render contexts; however, it's not obvious how to nicely account for specific user exemptions. In a sense, it might make sense to add the role-exemption support to allow for "base line" exemptions (for things that would be widely set without completely granting the "bypass" permission to whatever given roles), which should in theory be able to account for more larger swaths of cacheability, allowing caching in the user.roles context, instead of the problematically (excessively) granular user context.
To deal with specific user exemptions, might get into the situation of:
- producing another context which indicates if a given user is specifically exempt to any embargo
- if not, can cache in such a context
- if true, then applying the
usercontext to account for the per-user variability of the response/value
Gonna give it something of a look...
There was a problem hiding this comment.
Have improvements to caching in #40
No description provided.