Drupal >=8.2.x doesn't allow to override all cache bins with $settings['cache']['default'] anymore, documentation says otherwise

This really reminds me of #2751847: Throw exception if the same service is injected twice in ChainedFastBackend

First, note that the Acquia Cloud apcu_store() errors are a separate matter, one we cannot fix here. See #2800605: Warn/inform users when the hosting environment has a too low limit of APCU cache + #2765271: Rationalize use of the 'discovery' cache bin, since it's stored in the limited size APCu by default for that.

AFAICT this is because the cache.bootstrap service definition looks like this:

  cache.bootstrap:
    class: Drupal\Core\Cache\CacheBackendInterface
    tags:
      - { name: cache.bin, default_backend: cache.backend.chainedfast }
    factory: cache_factory:get
    arguments: [bootstrap]

Specifically, this part: default_backend: cache.backend.chainedfast. However, $settings['cache']['bins']… should definitely override this.

So, let's look at \Drupal\Core\Cache\CacheFactory. We see this:

  /**
   * A map of cache bin to default cache backend service name.
   *
   * All mappings in $settings takes precedence over this, but this can be used
   * to optimize cache storage for a Drupal installation without cache
   * customizations in settings.php. For example, this can be used to map the
   * 'bootstrap' bin to 'cache.backend.chainedfast', while allowing other bins
   * to fall back to the global default of 'cache.backend.database'.
   *
   * @var array
   */
  protected $defaultBinBackends;

(Note that this is computed by \Drupal\Core\Cache\ListCacheBinsPass.)

Specifically, this part: All mappings in $settings takes precedence over this, but this can be used to…. This confirms that $settings takes precedence over default_backend: ….

But if we then look at the code that should make that happen:

  public function get($bin) {
    $cache_settings = $this->settings->get('cache');
    // First, look for a cache bin specific setting.
    if (isset($cache_settings['bins'][$bin])) {
      $service_name = $cache_settings['bins'][$bin];
    }
    // Second, use the default backend specified by the cache bin.
    elseif (isset($this->defaultBinBackends[$bin])) {
      $service_name = $this->defaultBinBackends[$bin];
    }
    // Third, use configured default backend.
    elseif (isset($cache_settings['default'])) {
      $service_name = $cache_settings['default'];
    }
    else {
      // Fall back to the database backend if nothing else is configured.
      $service_name = 'cache.backend.database';
    }
    return $this->container->get($service_name)->get($bin);
  }

We first look for a bin-specific override in $settings. If that is not set, we look at default_backend: … in service definitions. If that is also not set, we use the default backend from $settings.

So, this is a slightly different order compared to what you expect. And also compared to what the documentation for $defaultBinBackends suggests.

This was changed in #2753989: CacheFactory::get() should not use default cache backend before cache bin defaults, and it intentionally introduced this behavior. See the CR at https://www.drupal.org/node/2754947 for the full explanation. It's indeed now necessary to explicitly override a certain cache bin.

So we should update the documentation, both the docs for \Drupal\Core\Cache\CacheFactory::$defaultBinBackends and those at https://api.drupal.org/api/drupal/core!core.api.php/group/cache/8.2.x#co....

Patch attached.

Docs are OK I think.

Only question is if we need a better way to override the used fast backend or some other way to disable that completely even if you have apcu theoretically available? That seems pretty hard right now, the only way to do so is the constructor argument for \Drupal\Core\Cache\ChainedFastBackendFactory::__construct, for which you'd have to alter the service definition?

better way to override the used fast backend

This is needed so rarely, I think those people can alter the service definitions. In any case, separate issue.

or some other way to disable that completely even if you have apcu theoretically available

Overriding individual cache bins' backends seems good enough for this case.

Docs look good to me.

There are a few typos:

1. +++ b/core/core.api.php
   @@ -588,9 +588,11 @@
   + * backend for individual cache bins. For example APCu or Memcache.
   

   "For example APCu or Memcache." is not a sentence.

2. +++ b/core/lib/Drupal/Core/Cache/CacheFactory.php
   @@ -23,11 +23,11 @@ class CacheFactory implements CacheFactoryInterface, ContainerAwareInterface {
   +   * All bin-specific mappings in $settings takes precedence over this, but this
   

   There is a plurality mismatch with "mappings" and "takes".

Making text changes.

I could apply the patch, so no reroll is needed.
I have applied the patch that includes the suggested changes in the above comment.

Reroll against 8.4.x

+++ b/core/core.api.php
@@ -588,9 +588,11 @@
  *
- * By default cached data is stored in the database. This can be configured
- * though so that all cached data, or that of an individual cache bin, uses a
- * different cache backend, such as APCu or Memcache, for storage.
+ * By default, cached data is stored in the database. Some cache bins use a
+ * different default cache backend (specified in their service definition). This
+ * can be configured though: both a different default cache backend (to change
+ * all of those using the database backend), as well as a different cache
+ * backend for individual cache bins. E.g. APCu or Memcache.

This is a bit hard to understand, was a long sentence before and now it is twice as long.

Why don't we just keep this, and add a Note: below the example that sets the default, something like:

Note: The default does not override a default backend set on the service definition, only a bin-specific configuration overrides that. This is used to use the chainedfast backend for the boostrap, discovery and config bins.

Tried to simplify the text as suggested in #15.

Note: The default does not override a default backend set on the service definition, only a bin-specific configuration overrides that. This is used to use the chainedfast backend for the boostrap, discovery and config bins.

Sorry, didn't really know what to do with this.

+++ b/core/core.api.php
@@ -588,9 +588,9 @@
+ * By default, cached data is stored in the database. This can be configured
+ * such as an individual cache bin or all cached data use a different cache
+ * backend (specified in their service definition), e.g. APCu or Memcache.

Suggest

By default cached data is stored in the database. However Drupal can be configured to use a different backend e.g. APCu or Memcache. 
This configuration can nominate a different backend for all cached data or for specific cache bins. 

The text in the current patch is still a bit disjointed.

Oh no, stuck on wording. C'est la vie.

Tried to incorporate the feedback from #19 but IDK which part of the patch is still disjointed. Maybe add your suggestions directly?

Hmm.

Feedback from #19 appears to be addressed in #23. Patch #23 also still applies

Wording in #23 seems clear to me. +1 RTBC

Not sure what's going on with the test failure though.

Shoot, actually it does look like the patch failed to apply, see https://www.drupal.org/pift-ci-job/1007356. Needs a re-roll.

Yes @smustgrave is correct that the patch still applies. Sorry, I misunderstood the test results page. I've added a re-test for Drupal 9.5 so that we can get a passing test. Let's see how that goes.

The comment changes look good, we just need passing tests. Setting back to RTBC which hopefully will stick after the test run.

Crediting @Berdir and @cilefen for feedback that changed the patch.

Not crediting myself for a wording nit-pick that put this back 5 years. Tempted to credit @moshe weitzman for his prediction of said outcome 🧙🔮😉.

Not crediting @smustgrave and @Amber Himes Matz even though their efforts here revived a dead issue, but I do love your work fellow #bugsmashers. I have however credited you both on this fortnight's bugsmash triage meta for said efforts.

Committed to 10.1.x and backported all the way to 9.4.x to keep things consistent as there's no risk of disruption with a docs only patch.