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

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 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 📌 Use cache collector for state Fixed 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 📌 Use cache collector for state Fixed . 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!

Automatically closed - issue fixed for 2 weeks with no activity.