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 review 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.