Verify that the configured service exists before calling it in CacheFactory

patch attached

Should there be unit-test case for this or a simpletest as I am only able to find a place to write test case for this and I am not sure its right or wrong - \Drupal\Tests\Core\Cache\CacheFactoryTest also I am not able to find set a value in settings as settings doesn't have set() method. Please help me on this.

Updated patch with unit test. I think this will only cover first IF condition.

Updated patch with unit test for default bin.

Modified documentation as suggested on #9. Adding interdiff for #8 and #12

Re-uploading patch for the bots.

This works, this does what I want, let's get this in...

Currently it takes two passes to install an alternative cache backed, this should have been merged years ago.

I've been following this issue for ages and I have been using this patch from #14. I forgot to mention before, but it seems to do the job.

Patch does not work against 8.4.x, so it needs some work.

Updated patch against 8.4.x

Looks good to me

Are we sure we want to do this? Doesn't this mean that you might not detect that you are missing the service?

I can imagine this causing quite a few lost hours of wondering why the expected backend is not being used.

Discussed with @Berdir on IRC. We agreed that this "fix" might hide errors. Does anyone have any real use cases for the current code?

@Berdir suggests that maybe an

alternative option would be a clear exception that says something like cache factory X not available for cache bin Y, with some hints on how to fix it?

The critical use case is when enabling a new cache backend, the setting will get read and throw an exception in the CacheFactory before the module has a chance to be enabled. This means its not possible to do one seamless deployment.

If hiding errors is the only issue?? Surely it would be much better for this to be clearly indicated in system_requirements()??

The real use is when we want to use Memcache

Following the instruction on the Readme http://cgit.drupalcode.org/memcache/tree/README.txt?h=8.x-2.x#n37

We enable the module, export the configuration, which goes into core.extension.yml
Then we added

$settings['cache']['default'] = 'cache.backend.memcache';

to settings.php

The problem is that before the configuration is loaded, the backend is switched and causes a fatal error.

This patch helps to temporarily fallback to database until the configuration is imported and the module is effectively loaded.

Also spotted that the ChainedFastBackendFactory was using the default bin without checking. IMO I think this is important not to throw errors if incorrectly configured or if the module isn't yet enabled, the warning should satisfy if this is the only concern...

@berdir fair enough, just to run with this idea though...

But all kinds of things could go wrong if at a later point, memcache is for some reason accidently disabled and it would then again fall back to database caches, which might be very outdated and could break your site.

The same would be true if you removed the cache settings at a 'later point' then. Also disabling a module clears the caches after it's run...

These "includes" via settings.php might do the job, but what happens if for some reason we want to disable the memcache module?
How about if I decide to disable memcache and enable redis instead?
Do we have to make multiple deployments?
In my opinion, its a lot more elegant to fail gracefully with some sort of alert rather than a fatal error

@Berdir for us that works with automated deploys of drupal, this is a big issue. And we would really like to have this solved in core.

My 2c about enabling redis after initial deployment :
In settings.php add comments around the redis settings that contain a placeholder, so the install process use the default bdd backend.

At the end of your deploy script, just remove the comments with a sed command :

//settings.php
/*//TO-REMOVE-AFTER-INITIAL-DEPLOY
  $settings['cache']['default'] = 'cache.backend.redis';
  (...)
//TO-REMOVE-AFTER-INITIAL-DEPLOY*/

#my_deploy_script.sh
(...)
drush site-install config_installer -y 2>&1 | tee -a my_log_file.log
(...)
echo "Post-install : Uncomment cache settings" 2>&1 | tee -a my_log_file.log
sed -i -e 's/.*TO-REMOVE-AFTER-INITIAL-DEPLOY.*/\/\/REMOVED-COMMENTS/g'  "$WEB_DIR/sites/default/settings.php"

Updated for 8.6.x

I applied the patch from #38 It works perfectly. Marking it RTBC. However I haven't gone through the logic or code quality of the patch.

+++ b/core/tests/Drupal/Tests/Core/Cache/CacheFactoryTest.php
@@ -145,4 +145,66 @@ public function testCacheFactoryWithSpecifiedPerBinBackend() {
+ ¶

Space needs to be removed.

#26 and ##2 still have not been addressed. We really need to understand why hiding the missing service is the correct behaviour here.

Not sure I'd say that its "hiding" the service. This is fallback behavior: If the desired cache backend does not exist, use the Drupal default rather than breaking the site. The patch already adds a message to the Drupal Status page like we do for many other requirements.

Without this patch it is very difficult to add Memcache to a site without breaking it. It requires a 2-step deployment (first enable module, then update settings.php). If the module gets disabled accidentally, the site breaks with no way to re-install the module other than via another deployment.

I agree that some sort of test is needed.

Rolled patch for Drupal 8.8.

When re-rolling this, I see two Test functions. So along with those tests and the warning on the status page, I think the issues mentioned in #41 are resolved.

Here this patch might fix failed tests.

Rolled patch for Drupal 8.8.x.

In the reroll patch of the comment #46 there was a problem with duplicatre PluralTranslatableMarkup in the system.install.

Fix the #50 patch.

/var/lib/drupalci/workspace/jenkins-drupal_patches-40654/source/core/tests/Drupal/Tests/Core/Cache/CacheFactoryTest.php
155 Short array syntax must be used to define arrays
156 Short array syntax must be used to define arrays
157 Short array syntax must be used to define arrays
187 Short array syntax must be used to define arrays
188 Short array syntax must be used to define arrays
I found deprecation function
\Drupal\Tests\PhpunitCompatibilityTrait::getMock() is deprecated in drupal:8.5.0 and is removed from drupal:9.0.0. Use \Drupal\Tests\PhpunitCompatibilityTrait::createMock() instead. See https://www.drupal.org/node/2907725

Here i upload a new patch .Please review patch and i resolve coding standard issues and deprecation functions in latest patch

I upload a patch for wrong version Kindly review new patch

I upload a patch with no coding standard issues in core/tests/Drupal/Tests/Core/Cache/CacheFactoryTest.php file

@prabha1997 thank you!

To make reviewing easier, would you attach an interdiff.txt? See https://www.drupal.org/documentation/git/interdiff

Rerolled the patch for 8.9.x

A review of #58

1. +++ b/core/modules/system/system.install
   @@ -316,6 +316,33 @@ function system_requirements($phase) {
   +  if ($phase == 'install' || $phase == 'runtime') {
   

   prefer using === instead of ==

2. +++ b/core/tests/Drupal/Tests/Core/Cache/CacheFactoryTest.php
   @@ -145,4 +145,67 @@ public function testCacheFactoryWithSpecifiedPerBinBackend() {
   +    $render_bin = $this->createMock('\Drupal\Core\Cache\CacheBackendInterface');
   +    $builtin_default_backend_factory->expects($this->once())
   +      ->method('get')
   +      ->with('render')
   +      ->will($this->returnValue($render_bin));
   ...
   +    $render_bin = $this->createMock('\Drupal\Core\Cache\CacheBackendInterface');
   +    $builtin_default_backend_factory->expects($this->once())
   +      ->method('get')
   +      ->with('render')
   +      ->will($this->returnValue($render_bin));
   

   Prefer Mocking with Prophecy in PHPUnit https://www.drupal.org/docs/8/phpunit/using-prophecy

3. +++ b/core/tests/Drupal/Tests/Core/Cache/CacheFactoryTest.php
   @@ -145,4 +145,67 @@ public function testCacheFactoryWithSpecifiedPerBinBackend() {
   +   * Test that the cache factory falls back to the built-in default service if specified bin service does not exist.
   ...
   +   * Test that the cache factory falls back to the built-in default service if
   +   * specified default bin service does not exist.
   

   Better to use "tests that ..." and the first one exceeds 80 chars, the second one, there should have a blank line between the first line and the second line.

   Suggestion:

   • Tests cache factory that specified bin service does not exist.
   • Tests cache factory that specified default bin service does not exist.

Rerolling patch from #62 to apply to 8.8.x.

This issue seems to be similar to the https://www.drupal.org/project/drupal/issues/2766509, can somebody tell me if I'm wrong ? And if the two issues are doing the same, which one choose ?

Thanks, @benroy! Agree with you. This one was filed earlier than that one, so I think that one should be closed. Commented there. Tests did not pass. Setting back to NW.

Personally I think the approach of using the "null" cache service as taken in the other issue is perhaps a bit better for this special case.

Tagging to get the opinion of a cache maintainer.

Small fix to patch from #65 since it had missing parenthesis in method calls.

Trying to re-roll latest patch

We should keep bugs filed against the bugfix branch (currently 8.8.x; soon to be 8.9.x). We don't need to change the version selector to queue tests because the UI also has a selector to queue tests against any branch when uploading the patch. Thanks!

I guess it's more a minor-only fix since there's a new public method.

Added patch for 8.9.x

Added patch for 9.1 branch

Removed "Needs reroll" tag

Been using this very important patch on many sites to prevent problems when a site is initially deployed and the memcache module hasn't yet been enabled (until config-import runs). Using the README instructions in the memcache module for "preloading" the cache service in settings.php results in very bad performance issues, whereas this patch just solves the problem simply by ignoring the cache settings when the module isn't enabled.

Would really like to see this get the subsystem review it needs and get pushed forward. It solves a very real problem on every one of our Drupal projects.

#75 prevents WSOD as intended on 9.1

After 6 years, someone needs to make a call; should it fail gracefully or not? Fixed or Closed(won't fix)?

I'm currently the only maintainer of the cache system in MAINTAINERS.txt, and I'm fine with the overall approach here given we have the system_requirements() check to inform people something is wrong.

1. +++ b/core/lib/Drupal/Core/Cache/CacheFactory.php
   @@ -65,7 +65,7 @@ public function __construct(Settings $settings, array $default_bin_backends = []
   -    if (isset($cache_settings['bins'][$bin])) {
   +    if (isset($cache_settings['bins'][$bin])  && $this->container->has($cache_settings['bins'][$bin])) {
          $service_name = $cache_settings['bins'][$bin];
        }
        // Second, use the default backend specified by the cache bin.
   @@ -73,7 +73,7 @@ public function get($bin) {
   
   @@ -73,7 +73,7 @@ public function get($bin) {
          $service_name = $this->defaultBinBackends[$bin];
        }
        // Third, use configured default backend.
   -    elseif (isset($cache_settings['default'])) {
   +    elseif (isset($cache_settings['default'])  && $this->container->has($cache_settings['default'])) {
          $service_name = $cache_settings['default'];
        }
        else {
   

   I think rather then do this we can be even safer and remove the final else and do..

       if (!isset($service_name) || !$this->container->has($service_name)) {
         // Fall back to the database backend if nothing else is configured.
         $service_name = 'cache.backend.database';
       }
   

2. +++ b/core/lib/Drupal/Core/Cache/ChainedFastBackendFactory.php
   @@ -66,6 +66,17 @@ public function __construct(Settings $settings = NULL, $consistent_service_name
   +  /**
   +   * Getter for the consistent service name.
   +   *
   +   * @return string
   +   *   The service name of the consistent backend factory.
   +   */
   +  public function getConsistentServiceName() {
   +    return ($this->consistentServiceName && $this->container->has($this->consistentServiceName)) ?
   +      $this->consistentServiceName : 'cache.backend.database';
   +  }
   

   This doesn't need to be public API. I think also this logic could be inlined into the ::get() method. Then we wouldn't have to check if $this->consistentServiceName is set because that method assumes the constructor has correctly set it.

3. +++ b/core/lib/Drupal/Core/Cache/ChainedFastBackendFactory.php
   @@ -80,13 +91,13 @@ public function get($bin) {
        if (isset($this->fastServiceName)) {
          return new ChainedFastBackend(
   -        $this->container->get($this->consistentServiceName)->get($bin),
   +        $this->container->get($this->getConsistentServiceName())->get($bin),
            $this->container->get($this->fastServiceName)->get($bin),
            $bin
          );
        }
        else {
   -      return $this->container->get($this->consistentServiceName)->get($bin);
   +      return $this->container->get($this->getConsistentServiceName())->get($bin);
        }
   

   This is not tested.

4. +++ b/core/modules/system/system.install
   @@ -293,7 +293,7 @@ function system_requirements($phase) {
   -  if ($phase == 'install' || $phase == 'runtime') {
   +  if ($phase === 'install' || $phase === 'runtime') {
   

   This is an out-of-scope change.

5. +++ b/core/modules/system/system.install
   @@ -348,6 +348,33 @@ function system_requirements($phase) {
   +  if ($phase === 'install' || $phase === 'runtime') {
   

   Are we sure we want a warning on install for this? This change could break automated tests of install profiles which have the memcache module.

6. +++ b/core/modules/system/system.install
   @@ -348,6 +348,33 @@ function system_requirements($phase) {
   +          'The configured Cache Backend %backend is not available.',
   +          'The configured Cache Backends %backend are not available.',
   

   I don't think Cache Backend needs to be capitalised.

   Also this message could be more helpful and say with bin is incorrectly configured - or if it the default settings.

   It would nice if the message pointing at least to settings.php where it is likely the misconfiguration is.

   Also this requirement is untested.

Not currently a good novice issue so removing the tag.

Here's an attempt to wrap up all the loose ends here. This addresses all of the feedback in #81. It includes a couple new test classes to specifically address 81.3 and 81.6.

It looks like ChainedFastBackendFactory doesn't have any direct test coverage currently. So I added a unit test mostly based on CacheFactoryTest. The final of the five test cases fails without the fix. I can post that as a test only patch if anyone wants.

Hmmm, that test passes locally. I'm going to set this back to NR to get eyes on it maybe.

Here's a screenshot of what the warnings on the status page look like. This is what happens if you install the cache_test module on your site and go to the status report page. (After applying the patch of course.)

PR created from #85

I'm posting a test-only patch to investigate that failing test. ChainedFastBackendFactoryTest is hard to unit test because there are a couple places it doesn't use service injection. So I'm explicitly unsetting some global values that may be different on the test runner than they are for me locally?

I'm hoping that only the fifth test case fails here. Fingers crossed!

I think I had myself confused. Let's try setting the variable instead of unsetting it.

Well I'm stumped with this test fail. Maybe this is why there aren't tests for ChainedFastBackendFactory to begin with.

I'm going to set back to NR to maybe get some eyes on the patch in #85.

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.

What's left here? Getting it passing?

Update MR https://git.drupalcode.org/project/drupal/-/merge_requests/1521 with 10.1.x

Re-roll https://git.drupalcode.org/project/drupal/-/commit/a5117b293edc1211c0e72... via 9.5.x-dev

#102 Patch works well with D9.5.x version.

Patch as per 10.1.x