Cache bin names should be set from service tags, not the service name

The Needs Review Queue Bot → tested this issue. It either no longer applies to Drupal core, or fails the Drupal core commit checks. Therefore, this issue status is now "Needs work".

Apart from a re-roll or rebase, this issue may need more work to address feedback in the issue or MR comments. To progress an issue, incorporate this feedback as part of the process of updating the issue. This helps other contributors to know what is outstanding.

Consult the Drupal Contributor Guide → to find step-by-step guides for working with issues.

Rebased this into an MR.

Variationcache went into core since this was first rolled so that affected a few things, but nothing too bad. It seems like we're collecting memory caches into a new container parameter now for... some reason that is not really clear to me other than parity with prior behavior.

It seemed to me that we also need to explicitly guard against name conflicts in cache bins because we can no longer rely on the service container doing so. This is actually an improvement in DX, I think, since these will now be explicitly caught instead of silently kludged. (Since later service definitions overwrite earlier ones during builds.) I don't think this is a BC issue per se but it might be worth a CR.

Improved the test coverage of this as well.

https://www.drupal.org/project/drupal/issues/3272093#comment-14642054 🐛 Cache bin names should be set from service tags, not the service name Needs work was RTBC but there was a complaint about how it always instantiated the service. It's possible this was not solvable at the time but now we have ServiceClosureArgument and I am 99% it is a perfect replacement for Reference here. So $factory->addMethodCall('offsetSet', [$bin, new ServiceClosureArgument($id)]); and then return $this->offsetGet($bin)();.

Updated this per the feedback in #60.

bradjones1 → closed merge request !8201

The Needs Review Queue Bot → tested this issue. It no longer applies to Drupal core. Therefore, this issue status is now "Needs work".

This does not mean that the patch necessarily needs to be re-rolled or the MR rebased. Read the Issue Summary, the issue tags and the latest discussion here to determine what needs to be done.

Consult the Drupal Contributor Guide → to find step-by-step guides for working with issues.

bradjones1 → changed the visibility of the branch 3272093-remove-hardwired-assumptions-10.x to hidden.

I think the bot was confused by my uploading a 10.2.x branch for comparison (and for anyone wanting to apply this on to a 10.x project...) I rebased the 11.x branch and it still applies cleanly so this is back to NR.

The Needs Review Queue Bot → tested this issue. It no longer applies to Drupal core. Therefore, this issue status is now "Needs work".

This does not mean that the patch necessarily needs to be re-rolled or the MR rebased. Read the Issue Summary, the issue tags and the latest discussion here to determine what needs to be done.

Consult the Drupal Contributor Guide → to find step-by-step guides for working with issues.

Should we be aiming to use the service locator pattern for the final solution here?

Heavy +1 from me. Recently had a presentation on VariationCache and felt stupid for having to emphasize that people had to name their caches "cache.foo" because reasons.

Re: #70 I'm not sure if a service locator is currently supported in the Drupal DI container? Even if it is, @chx recommended service closures which I think are pretty elegant and the Symfony docs suggest that as a valid way to do lazy-ish instantiation which is what we're doing here. I don't personally see a reason to change it.

We are already using service locators in core:

• RegisterAccessChecksPass
• RegisterStreamWrappersPass

Not picking sides, just stating facts.

Noticed that the MR for 11.x seems to need manual rebase.

 * cache.name_of_bin:
 *   class: Drupal\Core\Cache\CacheBackendInterface
 *   tags:
 *     - { name: cache.bin, bin: name_of_bin }
 *   factory: ['@cache_factory', 'get']
 *   arguments: [name_of_bin]

So now we have to specify the bin name twice, in the tag and as the factory argument? Can we avoid this?

Re: #75 I recall asking about this... somewhere... (can't find it in this thread, maybe Slack) and the answer is, not really.

I _suppose_ you could do some container-building magic and configure the factory and arguments there from the tag (or vice-versa), however I think that's too auto-magical. In theory you need not use the cache factory, right, so we don't want to hard-wire that.

I think the issue is one of context - the cache factory doesn't have access to the tags to pull a default name from there.

How about this?

no_longer_has_pattern_so_name_as_you_wish:
  class: Drupal\Core\Cache\CacheBackendInterface
  tags:
    - { name: cache.bin, bin: name_of_bin }

Where we use a comiler pass to set the factory and arguments, but only when the 'bin' tag is set and the 'no_factory' (or whatever) tag is not set.

Removes a ton of boilerplate from core cache bins, is completely BC and for those rarer cases that don't want a factory, they can opt out of using the factory.

Could even run a check if "class" is set and if not, polyfill it with: class: Drupal\Core\Cache\CacheBackendInterface. Then the standard declaration becomes:

no_longer_has_pattern_so_name_as_you_wish:
  tags:
    - { name: cache.bin, bin: name_of_bin }

I think ##7 is an interesting compromise, however after spending some time working on DB backend-overrides (which are kinda broken in HEAD) that also depend on a bit of heavy definition-adjustment in compiler passes, I do have some concern over how much magic we do behind the scenes. The YAML definitions themselves are not complete, as it were, and you have to "know" it's being fleshed out elsewhere. That's not to say we shouldn't do this, but it would be the most significant case of Drupal-y DI autoconfiguration we have I think.

Tags often run magic, that's the whole point of the cache.bin tag: We collect these in a compiler pass and set some container parameters with this information. So what I'm suggesting wouldn't be 'arcane voodoo" because the tags suggest there is something doing some stuff to whatever is tagged this way.

I'm not sure this would be a Drupalism or rather a Symfonyism. I do believe this could reduce a lot of boilerplate when it comes to declaring your own cache backend.

Yeah to be clear I'm not throwing cold water on the idea so much as playing devil's advocate. I think the sensible defaults you propose are sensible-enough to also allow people to do a definition long-hand, as it were. You can be very explicit or just set the tag and let the builder do the rest.