PluginManagerSetsCacheBackendRule and $misnamedCacheTagWarnings over-grabby?

[bronius]

I am grappling with PluginManagerSetsCacheBackendRule giving the message "<tag> cache tag might be unclear and does not contain the cache key in it"

As an example (and what I'm puzzling through), the contrib module graphql^3.x presents a few plugins, each with:

$this->setCacheBackend($cacheBackend, 'subscriptions', ['graphql']);
$this->setCacheBackend($cacheBackend, 'mutations', ['graphql']);

...etc. (ex. https://github.com/drupal-graphql/graphql/blob/8.x-3.x/src/Plugin/SubscriptionPluginManager.php#L57)

In this case, it looks like the author's intent is to be able to invalidate caches across all the plugins the module provides, and setting such a generic cache tag looks sensible. Analysis with phpstan says, "graphql cache tag might be unclear and does not contain the cache key in it." I see that a few modules in Core do this as well (ex. workflows https://api.drupal.org/api/drupal/core%21modules%21workflows%21src%21WorkflowTypeManager.php/9.2.x which also reports the same phpstan message)

The only way I found to "overcome" this message was to make the cache tag more specific (like graphql_subscriptions or graphql:subscriptions, for instance), which defeats the author's intent, I believe. In fact, it feels like maybe the spirit of the check would be satisfied if instead of checking whether tag contains key, we check that key contains tag (in this case)? Ex. Key could be graphql:subscriptions, and tag remains just graphql.

Please help me understand what I could do to make the world a better place :)

[mglaman]

It could be overzealous, it's old code I first wrote to catch many problems I found in code audits.

In my opinion, GraphQL should allow invalidating individual cache tags alongside the root one.

In the case of core's Workspaces, that cache tag is definitely weird considering the plugin manager. The main idea was to ensure there is a cache tag and its sensical.

Maybe we could have it to a basic pattern matching? So as long as it has a "root" namespace based on breaking apart the _ we can approve?

[bronius]

In my opinion, GraphQL should allow invalidating individual cache tags alongside the root one.

True 🙂 But I think with the cache key alone this is still achievable, ie, "only clear the subscriptions plugin cache," is it not? I understand cache tags are just to augment the ways to identify which cache buckets to manipulate.

Maybe we could have it to a basic pattern matching?

I think, yes, if there is either some way to determine such a pattern or if there is already a "Drupal 9 best practices" standard warranting a change to contrib/custom module code for it. If there's not the possibility of such a pattern and no best practices to strive toward, we might just want to relax the check or change the message to something more easily ignored.

Thanks for your replies here. I found this wrapper module to be easy to use and super helpful. Esp by drush.

[Ambient-Impact]

Just ran into this with a plug-in manager where one of the additional cache tags is library_info which is also triggering this rule. These plug-in definitions are very closely tied to library definitions, hence the tag, so so this makes it super easy to rebuild all the plug-in definitions whenever that tag is invalidated. I'm not sure that there is a foolproof pattern to match against that won't result in a false positives such as these.