I'm running into errors when a memcache backend is defined in settings.php, but the module that provides it isn't enabled (yet). I think the fix is to check for the non-existent cache service and default to using the database cache backend by default.
Examples where this is needed is when re-installing the site from scratch with config_installer which we are using for our automated testing. Also if one wanted to deploy memcache module along with it's settings to a site where it wasn't enabled yet.
There doesn't seem to be a way to confirm the existence of a module in settings.php (which makes sense from a performance standpoint), so I think it makes sense to add a graceful fallback when it's not available. Patch forthcoming.
To reproduce or test, set the memcache settings in settings.php and try to use the site.
$settings['memcache']['servers'] = [$memcache_ip .':11211' => 'default'];
$settings['memcache']['bins'] = ['default' => 'default'];
$settings['memcache']['key_prefix'] ='';
$settings['cache']['default'] = 'cache.backend.memcache';
Comment | File | Size | Author |
---|---|---|---|
#74 | 2766509-73.patch | 8.65 KB | julien |
#72 | 2766509-72.patch | 8.6 KB | jofitz |
#72 | interdiff-2766509-67-72.txt | 1.1 KB | jofitz |
#67 | 2766509-67.patch | 8.97 KB | oheller |
#55 | interdiff-2766509.txt | 8.34 KB | dawehner |
Comments
Comment #2
frankcarey CreditAttribution: frankcarey as a volunteer and at DEVINCI commentedComment #3
MiSc CreditAttribution: MiSc at Wunder commentedAgree on that this is a problem, but also a problem that Drupal stores the cache in the database, then it should not. I rather would have preferred something like cache.backend.null at this phase - but that does not exist by default.
Comment #4
frankcarey CreditAttribution: frankcarey as a volunteer and at DEVINCI commentedThe default is currently the database, but I could see changing to null as a backup when it's set to a value that doesn't exist.
Comment #5
cilefen CreditAttribution: cilefen commentedThe config line that matters for the demonstration is
$settings['cache']['default'] = 'cache.backend.memcache';
. The same issue exists in Drupal 7. But to be fair, the memcache documentation explains the right order to do this.Comment #6
MiSc CreditAttribution: MiSc at Wunder commentedFor me, the right way should be that it works without I have to edit the files manually after a deploy. Also, when you have a workflow then you install you site over and over again - this is a real quality blocker.
Comment #8
MiSc CreditAttribution: MiSc at Wunder commentedI have tested this workaround, that seems to work well:
In
settings.php
:Comment #9
azinck CreditAttribution: azinck commentedThanks for the patch, @frankcarey, it's sorely needed. A question: can you clarify the purpose of your change to CacheFactory.php?
MiSc unfortunately your code makes too many assumptions. For instance: it doesn't work if you have an existing site to which you're trying to deploy a new cache backend.
Comment #10
azinck CreditAttribution: azinck commented@cilefen it's not an issue in D7 with the memcache module because the file that contains the cache class is specified right in settings.php and the bootstrap process ensures it's loaded, regardless of whether the module is enabled.
Comment #11
azinck CreditAttribution: azinck commentedThe patch in #1 no longer applies cleanly so here's a re-roll *without* the changes in ConfigFactory.php since I didn't really understand the logic there and it appears to be working for me without that.
Comment #12
azinck CreditAttribution: azinck commentedI spoke too soon. This patch appears to resolve nothing for me. Continuing to look into it.
Comment #13
azinck CreditAttribution: azinck commentedComment #14
azinck CreditAttribution: azinck commentedOk, I think this is all that's needed.
Comment #15
dhansen CreditAttribution: dhansen at Sevaa Group commentedI tried the patch and it wasn't working. The issue seemed to be the the site was calling cache.backend.chainedfast, which in turn reloaded the settings that were incorrect. I've attached a patch that addresses this.
This patch may warrant a test before I would approve it, but it works in context so far.
Comment #16
MiSc CreditAttribution: MiSc at Wunder commentedThis will lead to outdated cache inside the database - if the other service is not available the cache will fallback on outdated cache entries.
Comment #17
dhansen CreditAttribution: dhansen at Sevaa Group commented@MiSc What direction do you think we should take this? I agree that outdated cache entries are no good, but I cannot see trying to wipe the cache every time we need to fall back on the database.
I envision this patch as more of a stop-gap measure. If your cache backend is incorrectly configured, this patch is designed to keep the site from breaking completely. To that end, I think adding an entry to the status report and maybe even an admin warning might be the best plan.
Comment #18
MiSc CreditAttribution: MiSc at Wunder commentedTagging the issue, and see if we could get some more thoughts about a solution for this.
Comment #19
dhansen CreditAttribution: dhansen at Sevaa Group commentedSo there are a handful of backend cache options available to us:
None of these are ideal candidates for fallback because they are actually caching. I think the best plan may actually be to expose NullBackend as a new cache.backend.null service. It is designed to 'short-circuit' the caching system so no actual cache is stored. It is also, by definition, a 'stub' meaning I do not know if it is complete enough to operate in this capacity, though it really needs to be only good enough to deliver the message that the caching configuration is broken.
Comment #20
dhansen CreditAttribution: dhansen at Sevaa Group commentedI hacked together this patch that adds the cache.backend.null service and also brings in a notice on the status report if things are broken. This is rolled against 8.2, so it'll eventually need to be re-rolled on 8.4 to get committed.
Comment #21
dhansen CreditAttribution: dhansen at Sevaa Group commentedComment #22
frankcarey CreditAttribution: frankcarey as a volunteer and at DEVINCI commented+1 to @dhansen's approach to fallback to a new NullBackend that provides a status warning in hook_requirements. An alternative would be to actually log the issue, but I'm not sure it's worth the overhead.
@azinck I don't recall why that change was needed in CacheFactory, sorry (been a while).
Comment #24
MiSc CreditAttribution: MiSc at Wunder commented+1 for dhansens solution, also make sense to have 'null' for caching then the choosen service does not exist. And we do not want random behaviours with outdated caches.
Comment #25
MiSc CreditAttribution: MiSc at Wunder commentedMissing newline in end.
Comment #26
MiSc CreditAttribution: MiSc at Wunder commentedComment #29
Dane Powell CreditAttribution: Dane Powell at Acquia commentedTest failures don't seem related to this patch, maybe try running tests again. I'm also testing this locally.
Comment #30
Dane Powell CreditAttribution: Dane Powell at Acquia commentedComment #32
Dane Powell CreditAttribution: Dane Powell at Acquia commentedDon't know why tests are failing but #25 works for me.
Comment #33
dawehnerMaybe we should still log something so people can fix it.
Comment #34
SerkanB CreditAttribution: SerkanB at Bright Solutions GmbH commentedWorking now with 8.3.x. The patch #25 is almost completly in Core, except for that one line.
Attaching patch for that one line in 8.3.x, as the patch in #25 doesn't work anymore.
Comment #35
SerkanB CreditAttribution: SerkanB at Bright Solutions GmbH commentedShould've tested better...
I do fail even before the ChainedFastBackendFactory, in CacheFactory when getting the service (I guess...)
Attaching a patch which handles that, using the same logic as before in the other file. I'm not sure if that's the way to go, if that's dirty and shouldn't ever be done to Drupal-Core, but it works for me and might help others.
Comment #37
Toche CreditAttribution: Toche as a volunteer commented#35 Not working for me
Error Before:
[23-Aug-2017 18:54:22 America/Denver] Symfony\Component\DependencyInjection\Exception\ServiceNotFoundException: You have requested a non-existent service "cache.backend.memcache". Did you mean one of these: "cache.backend.database", "cache.backend.apcu", "cache.backend.php", "cache.backend.memory"? in .../public_html/core/lib/Drupal/Component/DependencyInjection/Container.php on line 157 #0 .../public_html/core/lib/Drupal/Core/Cache/CacheFactory.php(83): Drupal\Component\DependencyInjection\Container->get('cache.backend.m...')
Error after:
[23-Aug-2017 19:22:08 America/Denver] Symfony\Component\DependencyInjection\Exception\ServiceNotFoundException: You have requested a non-existent service "cache.backend.null". Did you mean one of these: "cache.backend.apcu", "cache.backend.php", "cache.backend.memory"? in .../public_html/core/lib/Drupal/Component/DependencyInjection/Container.php on line 157 #0 .../public_html/core/lib/Drupal/Core/Cache/CacheFactory.php(89): Drupal\Component\DependencyInjection\Container->get('cache.backend.n...')
Comment #38
nikunjkotechaUpdated patch in 35 to use database instead of null. Null backend cache is not defined by default in any Drupal services.
Comment #39
cilefen CreditAttribution: cilefen commentedComment #41
dhansen CreditAttribution: dhansen at Sevaa Group commented@nikunjkotecha as discussed in #16-#22, the patch was previously set up to add the null cache service. The logic was that if you're intentionally trying to point the cache away from the database cache, then having it default to the database cache is obviously not the correct behavior and would be outdated anyways.
I'm not sure when that got taken out or why, probably around #34 where @SerkanB basically seemed to restart the patch from scratch (or was it meant to be an interdiff on the last patch?), but I think it should be added back in and will try to make time to reroll the patch in the near future for 8.x-4.x.
Comment #42
jofitz CreditAttribution: jofitz at ComputerMinds commentedI started this patch before @dhansen added #41, hopefully it still helps, but sorry if I'm just adding to the noise!
Comment #44
nikunjkotecha@dhansen, I missed a lot of discussion here and tried to update patch in a hurry. Apologies for that. For me as of now the priority was to get install working without changing anything in core. Which meant - not adding a service which is not there in Core and still use this for adding memcache support.
Comment #45
legolasboAll patches since #25 seem to be noise deviating from the consensus reached in the discussion up to #25. Attached you'll find a reroll of #25 against 8.4.x.
Comment #46
legolasboComment #48
legolasboTurns out we should default to the database backend, but fallback to null backend if the configured service doesn't exist.
Comment #49
legolasboLet's fix the code standards violations while we're at it.
Comment #50
legolasboWhile working on this I noticed CKEditorLoadingTest could do with some refactoring. Created #2908842: Replace deprecated function calls in CKEditorLoadingTest for that, which also has a patch.
Comment #51
borisson_Can we write a test for this functionality?
Comment #52
LNakamura CreditAttribution: LNakamura at Mediacurrent commented@legolasbo - #49 generates the following for us on /admin/config, /admin/reports/status, and possibly elsewhere:
Notice: Undefined index: default in system_requirements() (line 985 of core/modules/system/system.install)....
and
Notice: Undefined index: default in system_requirements() (line 988 of core/modules/system/system.install)....
Comment #53
michfuer CreditAttribution: michfuer as a volunteer and at Mediacurrent commentedThis should resolve the
Undefined index
and added a test to check the cache factory falls back to null backend if the service doesn't exist.Comment #54
damiankloip CreditAttribution: damiankloip at Acquia commentedI guess this is good to save on memory, as opposed to using the memory backend. However, if this is going to be defined in core.services.yml it should be maybe just be removed from development.services.yml?
The ChainedFastBackendFactory is already container aware, so no need to use \Drupal:: calls? Also, shouldn't this code live outs of the if () block? If a consistent service name that's invalid is passed in, it would still break.
This naming is confusing. The custom one defined in the settings should never be used right? so this mock would be better called $null_backend_factory? In which case would it just be simpler to avoid mocks altogether and just make sure an instance of NullBackend is returned?
Comment #55
dawehnerWe do, because this call happens in a constructor ...
I know this is coming from the other test functions, but well, better be better than consistent :)
Here are a good amount of additional fixes, variable names, documentation improvements etc.
Comment #56
mpdonadioLooks nice.
I think these comments needs to be swapped. Main reason for NW.
Maybe 'Fall back to the no-op cache backend ...' may make more sense?
In scope to alphabetize this?
Worth a comment about why this is needed? Think we also need a blank line before/after the function?
And for the sake of discussion, can/should we use the static cache instead of the null cache? I don't think so, but the thought popped into my head when I was looking at this.
Comment #57
lcatlett CreditAttribution: lcatlett commentedI believe these issues can be addressed without patching core. Falling back to null or database for a non-existent cache service could cause major performance issues during site installs and deploys so a more ideal solution would be to autoload required classes and services in cases where the module is not installed and config exists in settings.php.
Since drupal bootstraps the service container to the database by default, the below configuration overrides this to use memcache as a backend for the container in addition to overriding the default configuration for cache_config, cache_bootstrap, and cache_discovery bins to prevent issues caused by race conditions and stale container and static cache data on site installs / deploys / config updates.
Comment #58
fgmWhat I do for this type of issues is configure memcache in a closure in settings.local.php and only enable it if available, a bit like:
Comment #60
justafishI use this in settings.php for redis:
Comment #62
nplowman CreditAttribution: nplowman at FFW commentedNote that there were some changes to class names and class constructors in Memcache 8.x-2.0-alpha7, and the example in #57 will need some adjustment.
1) 'memcache.config' is now 'memcache.settings'.
2) 'Drupal\memcache\DrupalMemcacheConfig' is now 'Drupal\memcache\MemcacheSettings'.
3) 'Drupal\memcache\DrupalMemcacheFactory' is now 'Drupal\memcache\Driver\MemcacheDriverFactory'.
4) The 'cache.container' service takes fewer arguments now. '@lock.container' and '@memcache.config' can be removed.
Here is a full snippet for context. (This is based on Acquia's version, so there are a few differences from #57)
Comment #63
tunicI've tested patch #55. We are using Acquia Cloud Site Factory hosting, and with this patch applied site didn't install, although already installed sites work ok. We changeds this condition:
to
Not sure if this is the right approach, and not sure if this only impact on ACSF hosting or because something we have in our code, but I guess is good to mention here.
Comment #65
RoSk0What about situations like using memcache replacement for lock service? Following https://git.drupalcode.org/project/memcache/blob/8.x-2.x/README.txt I added :
services:
# Replaces the default lock backend with a memcache implementation.
lock:
class: Drupal\Core\Lock\LockBackendInterface
factory: memcache.lock.factory:get
and now "drush site:install --existing-config --yes" as well as UI installation fails with:
The service "lock" has a dependency on a non-existent service "memcache.lock.factory". in Symfony\Component\DependencyInjection\Compiler\CheckExceptionOnInvalidReferenceBehaviorPass->processValue() (line 31 of /var/www/vendor/symfony/dependency-injection/Compiler/CheckExceptionOnInvalidReferenceBehaviorPass.php).
I think that this issue should be broader then just have one use case cover. Or we need a new issue to deal with non existent services on installation, something similar to #2863986: Allow updating modules with new service dependencies.
Comment #66
fgmThe way I handle this - and I suggest it should be more commonly used - is to recognize a site really has 2 different states: one during "build", where services only rely on the default (database) implementations, and one during "run" where all optional modules have been installed and the container can be rebuilt accordingly.
I do this using settings.build.local.php and settings.run.local.php and the (composer-based) deployment commands switch the symlink settings.local.php from one to the other.
Comment #67
oheller CreditAttribution: oheller at Electric Citizen commentedRe-rolling patch 55 for 8.7.x
Comment #68
weynhamzPatch #67 failed to apply with composer-patches, changes to sites/development.services.yml is out of drupal/core package, any idea how to fix this?
Comment #69
weynhamzFor the moment, remove changes to sites/development.services.yml.
Comment #71
RenrhafPatch #69 seems to work great. thanks.
Comment #72
jofitz CreditAttribution: jofitz commentedCorrect coding standards errors in patch #67.
Comment #74
julien CreditAttribution: julien at Code Enigma commentedRe-rolling patch 55 for 8.7.x
Comment #75
jungle#2429321: Verify that the configured service exists before calling it in CacheFactory was filed on 19 Feb 2015, which is similar to this and created earlier than this, let's mark this one as duplicate?
Comment #78
tyler36 CreditAttribution: tyler36 commentedComment #79
tyler36 CreditAttribution: tyler36 commented#2429321: Verify that the configured service exists before calling it in CacheFactory