Cached services can't be used in service providers/modifiers

We could just provide an empty cache_default_bin_backendsinside our core.services.yml file?

Makes sense to me.

What would happen if a cache service defines a default backend wouldn't this mean that potentially we switch backend during a request?

Related, https://github.com/md-systems/file_entity/blob/8.x-2.x/src/FileEntitySer... is my workaround for checking if a module is enabled in a ServiceProvider.

Patch no longer applies.

This fix makes sense to me. I looked briefly at writing a test for this, but couldn't find really any existing service container level tests.

Architecture wise this makes sense to me.

The only question I have - if there was a cache set or delete during the compiler pass, then it'll do that on the null backend and be a no-op. In most cases container rebuild = cache clear anyway, so it won't make any difference. Also doing deletes there would be strange.

We might just need some docs why this doesn't matter though.

So, I realise my opinion might not have great weight on this issue, but this patch makes sense to me ... I've been using it partly as an excuse to learn some of the moving parts involved. I have one question below, which I think largely stems from a lack of understanding on my part:

• - I've checked that the null cache backend setup makes sense (to me at least)
• - as you already noted, the tests need to be re-added and fixed
• - I've checked that the modified language methods are not used in the previous form elsewhere
• - there will be a couple of nitpicks (comment wrapping and caps) but probably makes sense to deal with those once the patch including test fixes is ready for review

Manual testing done:

• - OK enable language module
• - OK apply the patch,
• - OK test out rebuild on an existing site
• - OK rough check site
• - OK test new site install in non-English
• - OK rough check site
• - OK put in debug to sanity check / review befores and afters of $this->services during compilation

So here's the one question: I'm sure this is just me missing background on this but what's our guarantee that it's ok to remove all non-synthetic services in the compile method here? Are there no non-synthetic services that are valid at that point? Is it the case that not being synthetic, they'd get automatically reinstantiated later as needed?

I put in some debugging here too and discovered really quite a lot of definitions that are not flagged as synthetic... seems to make sense.

#33, #34:

Already now only synthetic services are persisted from ContainerBuilder to Container, so this is fine.

The removal of persisted services is only needed for KernelTestBase*, where currently the ContainerBuilder is used for testing the system and not the Container for various reasons.

Many thanks chx and fabianx - that's helpful.

#37: Yes, but because no services that are created during compiling are persisted from one container to the next, we can be very sure that removing those services from the compiled container that are created during compilation is safe as well :).

+++ b/core/lib/Drupal/Core/DependencyInjection/ContainerBuilder.php
@@ -100,6 +108,17 @@ private function synchronize($id) {
+    // Remove all non-synthetic services set by compiler passes. For example,
+    // cache bins are forced to NulLBackend before the ListCacheBinsPass and
+    // so cache services might got stored as NullBackend.

Lets rephrase to something like:

Remove all services constructed during compiler passes. Only keep those that have been explicitly set.

What I meant is to - lets rephrase to:

+    // Remove all services constructed during compiler passes. Only keep those that have been explicitly set. For example,
+    // cache bins are forced to NulLBackend before the ListCacheBinsPass and
+    // so cache services might got stored as NullBackend.

The current text is misleading as:

"Remove all non-synthetic services set by compiler passes."

set is wrong, because those are the ones we keep.

non-synthetic is wrong too, because we don't distinguish here between synthetic and non-synthetic.

Does that make more sense now?

For #42, assuming this is still an issue

Reading this issue makes me realise we are doing a tonne of unnecessary work. Now that config is stored in the db there's no need to duplicate everything in the cache_config table. We can lose all the complexity and have faster rebuilds and sites after rebuild.

I have just bumped into this issue, #39 does not apply on >= 8.9. How we should move forward? Should I try to reroll the patch or @alexpott's comment in #52 change anything? Based on my understanding that describes a different issue.

Okay, so many things have changed since #39 was born 5 years ago, today, it seems only adding this one-liner to core.services.yml fixes the issue. (Test coverage could be still needed to warrant this issue does not occur again.)

My fix in #54 seems to be working but I bumped into another interesting issue with Redis which could be slightly related to this one...

It'd be great to add test coverage here.

This MR essentially contains my patch from #54 and also one important addition, a mitigation for a workaround like you can see here:
https://github.com/amazeeio/drupal-example-simple/blob/46d71e3885dc9fca7...
https://github.com/amazeeio/drupal-example-simple/blob/46d71e3885dc9fca7...

The null cache backend has to be registered because by default it is not available. I do not see a reason why it should not available by default, it does not mean it is in use. Although codes like you can see in the description of #3201355 may trigger calls that try to access to null cache backend as a fallback.

It'd be great to add test coverage here.

I agree although I cannot promise when I am going to have time for that. I could use some help on this.
(First I would like to make sure that the latest changes do not break anything. )

@alexpott Here is a minimal POC test that confirms the missing cache_default_bin_backends issue exists. This can be only reproduced if we remove the cache_factory override from the KernelTestBase because that change removes what is the key to reproduce this issue:

core.services.yml

  cache_factory:
    class: Drupal\Core\Cache\CacheFactory
    arguments: ['@settings', '%cache_default_bin_backends%']
    calls:
      - [setContainer, ['@service_container']]

This test does not prove that cache.backend.null should be always defined, although as I wrote in #59, I do not see why it should not be.

ok, now I need some feedback

This is also blocking #2575105: Use cache collector for state now it seems, which makes state use a cache and as a result, ContainerRebuildTestServiceProvider, which does use state, fails.

Added some comments about the comments, didn't think too much about the tests but the state issue will add more functional test coverage then...

It probably doesn't make sense to have the null backend definition in sites/development.services.yml if its in core after this.

@alexpott I'm curious what complexity you where referring to. Currently we seem to only be adding things so not sure we really removing complexity. Happy to dig in though, removing complexity is my jam.

This issue is difficult to understand, as most things that have to do with the cache system. @neclimdul asked a great question in #65, that should probably be answered. Because as they said, reducing complexity is great but I don't see this issue already doing that.

I think we need to also add a test that the null backend is available without the development services being included?

This issue is being reviewed by the kind folks in Slack, #needs-review-queue-initiative. We are working to keep the size of Needs Review queue [2700+ issues] to around 400 (1 month or less), following Review a patch or merge request as a guide.

As mentioned this is difficult. But what I can gather is that there is still discussion being done.
#65 needs an answer
and possible more tests from #69.

Rebased the MR, this is what I could do quickly. I am also pinging @alexpot on Slack to ask if he can answer #65.

The complexity I'm referring to is the fact that in core we have both the config table and cache_config table. These tables are basically the same. The config cache really should just be fast cache backend - i.e APCu / memcache / redis. So maybe we're not losing complexity but we losing a layer. None of this is in scope here. I think I just realised this when looking at the code in the earlier patches.

I think we need to also add a test that the null backend is available without the development services being included?

@borisson_ Can you please explain what you meant by this? The proposed changed _removes_ cache.backend.null from development.services.yml

--- a/core/assets/scaffold/files/development.services.yml
+++ b/core/assets/scaffold/files/development.services.yml
@@ -4,6 +4,3 @@
 # 'example.settings.local.php' file, which sits next to this file.
 parameters:
   http.response.debug_cacheability_headers: true
-services:
-  cache.backend.null:
-    class: Drupal\Core\Cache\NullBackendFactory

From #65:

It probably doesn't make sense to have the null backend definition in sites/development.services.yml if its in core after this.

I agree, It was removed in the latest MR from development.services so that is fixed now.

From #69:

I think we need to also add a test that the null backend is available without the development services being included?

We don't specifically test that services are available for any other services, so I actually don't think we need to do it here either.

I only have one more question about the comment next to the cache.backend.null definition in the services.yml, I'm not sure we need to specify the intended usecase and that comment probably is ok to be deleted. Not holding this issue up for that though.

> The complexity I'm referring to is the fact that in core we have both the config table and cache_config table. These tables are basically the same. The config cache really should just be fast cache backend - i.e APCu / memcache / redis.

We already use the ChainedFast backend by default for the config bin, it's only if APCu is _not_ available that it's kinda unnecessary, but we don't have a mechanism right now to automatically fall back to the null backend if apcu is not available, we just always fall back to the default bin then. At best we could document that _if_ you can't use APCu or redis/memcache then you could consider to manually the config bin to use the null backend.

We can't _only_ use APCu either, as that does not support multiple multiple servers and I think even on a single server, there's drush to consider?

I'm not sure we need to specify the intended usecase and that comment probably is ok to be deleted.

I have removed the comment to eliminate all roadblocks :)

I think the change to \Drupal\Core\Cache\CacheFactory is unnecessary and should be removed. And if this results in a broken test run then we need to do the work to understand why.

I also don't understand why we need to move the null backend out of development.services to fix this issue. I think that this is a remnant of an earlier attempt at a fix.

I think the change to \Drupal\Core\Cache\CacheFactory is unnecessary

Answered this in an MR comment. TL;DR it is needed, but no Drupal core tests catch the issue that I see on downstream.

I also don't understand why we need to move the null backend out of development.services to fix this issue. I think that this is a remnant of an earlier attempt at a fix.

TBH, I do not see an issue with having a null service/object registered, but you were right, it looks like it is not needed to make Drupal core tests pass and also everything worked fine on my guinea pig downstream project without it.

> how the "field config import fails when the site is installed from configuration" thing can be covered with tests?

Is this really happening on a clean site with just core and and patches? My guess is that this only happens in combination with specific additional modules/and or patches.

Reminder that #2575105: Use cache collector for state is blocked on this issue and is currently failing. Combining those patches should give us a testbed to test the different fixes and which changes are required exactly.

I looks like it needs another review

The new development compiler pass is now triggering this reliably on HEAD, will look into it in the next days.

I'm proposing we remove the snippet from CacheFactory for now. It seems unrelated to the main change here, which has been blocking progress on issues like state caching for a very long time.

Lets open a follow-up issue to see if we can figure out when that's not needed. Could be a drush problem as well, try to reproduce with the drupal core UI installer?

Ok, I overlooked the part that this appears to be a regression introduced by this issue, but still, without a way to reproduce or understand why it's required, I'm not sure about keeping that change here.

Another thing to debug would be a backtrace when exactly that is needed/called. I did this in the same place as the removed part and got nothing during a regular install:

if (InstallerKernel::installationAttempted()) {
  debug_print_backtrace(DEBUG_BACKTRACE_IGNORE_ARGS);
}

Not even with the condition removed, which kind of makes sense as the whole class is meant to be replaced during the installer. What do you get?

Brought this up in #needs-review-queue-initiative channel and lets try and remove the code in question from #83/#84 to a follow up.

Will admit cache is one of my worst components but want to help keep the issue moving where I can. Also can help test where needed.

Patch against 11.x-dev from the MR. The mentioned code is already removed from the MR and it still applies, but can't change the target.

Best way to test this is combined with #2575105: Use cache collector for state. I'll upload a combined patch there next. This was already RTBC before and the reason it was pushed back has been removed, so I think it could go back to that.

I'm unsure about the follow and if that's really needed. I'd suggest @mxr576 or someone else who can reproduce that problem opens it *if* it can be reproduced, because without a way to reproduce, there's nothing we can do.

Understood on the follow up.

And seems the code in question was removed that I could tell so I'll remark this.

After nine years the functional change here is the same as the original patch from 2014. I think we should get this in to unblock the state caching issue, and if we do end up being able to reproduce the config import issues, we can fix in a follow-up or worst case roll back. 10.2 is still some time away.

Committed 28db985 and pushed to 11.x. Thanks!