Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

DDST-213: Adjust caching for embargo and related access control. #39

Merged
merged 3 commits into from
Jun 10, 2024
Merged

DDST-213: Adjust caching for embargo and related access control. #39

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
63d0178
Adjust caching for embargo and related access control.
adam-vessey May 9, 2024
d8c5b3b
Add in the related entity to the mix.
adam-vessey May 9, 2024
ec7eac6
Merge branch 'main' into fix/caching
adam-vessey May 27, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 4 additions & 0 deletions src/Access/EmbargoAccessCheck.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -74,6 +74,10 @@ public function access(EntityInterface $entity, AccountInterface $user) {
)->render()
);
array_map([$state, 'addCacheableDependency'], $embargoes);

$type = $this->entityTypeManager->getDefinition('embargo');
$state->addCacheTags($type->getListCacheTags())
->addCacheContexts($type->getListCacheContexts());
return $state;
}

Expand Down
17 changes: 16 additions & 1 deletion src/Entity/Embargo.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -43,7 +43,8 @@
* "html" = "Drupal\Core\Entity\Routing\AdminHtmlRouteProvider"
* },
* },
* list_cache_tags = { "node_list", "media_list", "file_list" },
Copy link
Contributor Author

@adam-vessey adam-vessey May 9, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

* list_cache_contexts = { "ip.embargo_range", "user" },
* list_cache_tags = { "embargo_list", "embargo_ip_range_list" },
Comment on lines +46 to +47
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 user context to account for the per-user variability of the response/value

Gonna give it something of a look...

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Have improvements to caching in #40

* base_table = "embargo",
* admin_permission = "administer embargo",
* entity_keys = {
Expand Down Expand Up @@ -451,4 +452,18 @@ public function ipIsExempt(string $ip): bool {
return $exempt_ips && $exempt_ips->withinRanges($ip);
}

/**
* {@inheritdoc}
*/
protected function getListCacheTagsToInvalidate() : array {
return array_merge(
parent::getListCacheTagsToInvalidate(),
[
'node_list',
'media_list',
'file_list',
]
);
}

}
Loading