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.

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

Using the current_user data producer makes response uncacheable #1351

Open
dulnan opened this issue Aug 25, 2023 · 1 comment · May be fixed by #1417
Open

Using the current_user data producer makes response uncacheable #1351

dulnan opened this issue Aug 25, 2023 · 1 comment · May be fixed by #1417

Comments

@dulnan
Copy link
Contributor

dulnan commented Aug 25, 2023

When using the current_user data producer the response becomes uncacheable. This is because of this line:

$field_context->addCacheableDependency($this->currentUser);

As $this->currentUser is not an instance of CacheableDependencyInterface, the addCacheableDependency() method in RefinableCacheableDependencyTrait will fall back to setting the max age to 0:

https://git.drupalcode.org/project/drupal/-/blob/11.x/core/lib/Drupal/Core/Cache/RefinableCacheableDependencyTrait.php#L23

One option to fix this would be to not set any cacheability at all and leave this up to the consumers of the producer. But that would introduce quite a breaking change, as queries would suddenly become cacheable.

An alternative fix would be to implement a default behaviour that adds the user context (or sets the max age to 0), but introduces a new argument on the producer to prevent this from being done. That way existing implementation still work as before, while making it possible to customise the cacheability when needed.

Happy to do a merge request once the best approach has been found!

@pfrenssen
Copy link

pfrenssen commented Aug 5, 2024

I also hit this when using the CurrentUser data producer.

The documentation is clear on what is the intention: the data should be cached per user so that previously logged in users do not leak to newly logged in users. The bug causes the result to be always uncacheable which also effectively prevents data from leaking.

I think the fix is to simply add the user cache context. This will achieve the intended result.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants