Taxonomy views filters should not have user context

Probably duplicate of #1947270: Add test coverage for the term_access query tag. I added some tests over there.

This is blocking #2677234: Views Page display with filter "Has Taxonomy Term" adds unnecessary 'user' cache context, and is causing cacheability problems.

#2677234: Views Page display with filter "Has Taxonomy Term" adds unnecessary 'user' cache context was closed as a duplicate of that issue as @jibran pointed out. Note that while that issue was marked major, @catch, @alexpott, @effulgentsia, @Cottser and I actually agreed that this was better as a normal-priority issue. There is no risk of cache poisoning, just a possibility that the cache invalidation might happen more than it needs to, and the scenario is uncommon as well as not really supported by core out of the box. So we agreed "normal task" was correct for this issue.

Thanks @jibran and @Wim Leers!

Let's also incorporate the additional information from that issue as well as @jibran's tests here.

Two things are not correct about this sentence:

> that the cache invalidation might happen more than it needs to, and the scenario is uncommon as well as not really supported by core out of the box

* It's not about invalidation, it's about contexts/variations. This means that views with a filter like this creates caches for every single user. This is also interesting because exposed forms inherit the views cache contexts, so they too end up being per-user and then they are placeholdered.

* This cache context is *always* added, not just for the rare scenarios where people actually have term access. Not doing it unless it is really needed is the point of this issue.

This currently even affects search-api based views with term filters, where it is even more unlikely that it needs to be cached by-user.

Simple patch to disable that for sites that know they don't need it. Also wondering if we have any tests that actually rely on it, which I doubt.

I encountered this with a view that simply grouped Country nodes by a Region taxonomy, rendering the view uncacheable.

I strongly agree with @Berdir: The rare scenario requires the cache context; the common scenarios that would benefit from caching won't be cached until the cache context is only added when it's needed, or until there's a way to opt out.

#8's corrections to #4 are correct.

This challenge with cacheability metadata not being optimal for all use cases is not unique to this particular @ViewsFilter plugin.

Yikes - I traced down this pesky cache context when trying to figure out why a seemingly innocent views block on a page was making the entire page uncacheable in dynamic page cache*! This is a very commonly used views filter - I hope no one minds bumping this to major. It was a major time sink tracking this down as I never would have thought it was a taxonomy term filter that was adding the context.

*I'd love if someone with more expertise in dynamic page cache can help explain why this happens. I know the "user" context is by default in the auto_placeholder_conditions list, which I thought just meant that whatever render data declared this cache context would be placeholdered and then rendered later, but still allow dynamic page cache to create a cache entry. But that's not what happens. Dynamic page cache will just not cache anything at all if the response has the "user" cache tag.

Yeah, you're not the first to find this :)

The auto placeholder stuff can only happen if the problematic element is in an area that can be placeholdered, specifically a regular block and not the main content region.

I still think we should just remove this, optionally combined with checking for a term_access specific hook implementation maybe? So that at least only sites that use that are affected. Which is probably very, very few.

I believe that the user cache context is now always required, because #3101738: Exposed term filters should not show term options that the user does not have access to added a check for the 'administer taxonomy terms' permission.

Possibly this means that this issue should be closed, as there is no straightforward route to changing the cache contexts without reworking altogether how access is handled in this taxonomy exposed filter.

Checking for a permission does _not_ require the user cache context, just user.permissions, which is already there by default.

A really odd thing about this that had me stumped for a little is that \Drupal\taxonomy\Plugin\views\filter\TaxonomyIndexTidDepth adds the user context but \Drupal\taxonomy\Plugin\views\argument\IndexTidDepth which is very very similar code - does not. I'm glad it doesn't because the site I'm performance profiling would have worse performance but it is odd.

I think the mean reason the filter has it is exposed filter forms, which lists all terms of a vocabulary. We could as a minimal first step move the context just to the form structure and not the plugin.

Just found out a site which was burnt by it, having a Views block using that filter in the footer of the view building their main page, killing the cacheability.

Bumping the version top current core. Of course as @Wim Leers says, this fixes the issue, but this can not be introduced in core in its current #9 state: the rare case where it is needs still needs to be catered for.

Accordingly, resetting to CNW.

After I applied the patch, I thought it would immediately remove the user cache context on the view. But you have to resave the view after applying the patch (and export to config). I thought this was dynamically computed. The cache context is re-computed on view save.

Hope this helps.

Well this was an interesting one to XDebug :D

I agree this should just be removed. The user.permissions context should cover variations here and it doesn't appear to provide any additional benefit that wouldn't be provided by #1947270: Add test coverage for the term_access query tag

This is quite a large issue when a view is a page display or is embedded as part of the main content region and thus completely uncacheable for dynamic page cache.

Welp, it didn't break any tests, so I'd be in favor of getting this in and dealing with test coverage in the other ticket. Marking for review.

Views' default cache plugin uses the generated query as the cache key - see https://api.drupal.org/api/drupal/core%21modules%21views%21src%21Plugin%...

hook_query_alter() implementations are also able to add cache contexts, see for example: https://api.drupal.org/api/drupal/core%21modules%21node%21node.module/fu...

So we should definitely remove this, however we need a post update + presave here to resave all views so that the cache contexts get recalculated. Other view updates + ViewsConfigUpdater should have examples.

Agreed it doesn't need test coverage, we'd just be testing that the code doesn't do what it used to at a particular point in time, there wouldn't really be positive assertions to make.

Yup, makes sense. Normally just a view re-save would've done here, but it appears the re-calculating of cache meta is only done via UI. MR has been updated.

Was previously tagged for IS so moving to NW for that.

But with a post_update hook will this require test coverage?

Also new functions may need a change record, could be wrong there.

Updated issue summary and removed tag. I agree with @catch that there is no test coverage needed here as we're removing code, not adding. Tests for what this originally existed for should go into #1947270. I don't really see any reason for a change record here either, back to Needs Review.

I don't see the changes to the issue summary? Maybe they didn't save?

Not sure what happened there, it change the title but not summary.. trying again

Thanks much better!

Looks good to me too! this just bit me again so I'd be happy to get this committed

Committed/pushed to 11.x.

MR has conflicts with 10.1.x, and there's a post update in here, so leaving in that branch - it'll be released with 10.2.0

For anyone looking for a fix, exporting the view to config/sync as yml, removing the cache_metadata.contexts.user and importing the config back to site fixed the caching issue for me.