Fix inefficient config factory caching

@berdir asked me to respond on language overrides. Here you go :)

And lastly, I'm wondering why we're even trying to load language overrides on my system at all, because this is not a multilingual installation. We can skip the language overrides completely if languageManager->isMultilingual() is FALSE

Yeah, this is true. Before the built-in overrides system, language overrides were in fact injected from the locale module which would not have been enabled on your site. So yeah, we can skip applying overrides if languageManager->isMultilingual() is FALSE. Indeed.

and we might be able to optimize more there but that is going to be a bit more complicated (config entity objects all have a langcode, we don't need to look for overrides if that matches the current language or if it doesn't have such a key and we are using the default language?)

Yeah theoretically it is possible to have a language override file even in the same language that the entity was originally saved in, but I see no use case for that :D Eg. locale and config translation will never create such files. If the config file has no top level langcode in it, locale and config translation will entirely ignore it for language support, since it has no idea what language the file may be in, so it cannot really provide translations (ie. it cannot tell the source language). So if the file had no langcode, then it is safe to assume that there are no language overrides possible. Yes.

Getting started here:

- Ripping out the whole lazy load code in Config, when we always pre-load things in the ConfigFactory then there is no point in checking that
- Changed the language handling on the config factory, inject the manager so that we can check for multilingual, make setting a manual language for language overrides easier with enforceLanguage() & resetEnforcedLanguage().

I also got started on an (untested) patch, here. It may have ideas to bring over to yours :) Eg. I think you are not checking if the source language equals the target language if I read your patch right. Although you are making way many more changes :D

I thought about that, but the thing is that we load the actual file and an eventual override at the same time, so we don't know yet if we will need it or not. The part that you are changing only affects new config files.

I don't think we can inject the language manager directly, due to circular dependencies...

I think it's not that bad that we can't compare the language, as we can cache the overrides. Assuming that we fix CachedStorage for non-existing config objects.

Also, speaking of caching, I currently can not to see how caching and module overrides could possibly work together, as we can not consider whatever those modules use to decide what overrides to apply?

@Berdir: I believe the idea for module overrides was that it will be the module's responsibility to cache and return from the cache. Exactly because you cannot really tell the override criteria. The prior context system provided *some* of that criteria but could not really be used as authoritative source either, eg. if you have domain based overrides or OG based overrides, etc.

Right, but the ConfigFactory has a static *and* persistent cache based on it's own cache key, a module has no chance to do anything about that?

Edit: I got confused, it is only a static cache, so not that bad. sorry for the noise.

Re-uploading my patch, testbot doesn't seem to be testing it.

This should work better. installer-error-handling--.

Fixing the remaining direct calls to load(), not sure about the changes in ConfigFactory and Importer, but tests seem to pass...

As discussed with alexpott, the multilingual check doesn't work, at least not that easily, as we also want to translate default configuration through language overrides*. Not touching that part yet and there are test failures related to that.

* While doing some tests with that, I noticed that helper functions like the on that creates node body fields needs to force the langcode to en, as they have a hardcoded, non-translatable english label.

Ok, removed the isMultilingual() check for now, added caching for non-existing configuration objects.

Confirmed that there are no calls to FileStorage::read() on a warm cache anymore. I always found the non-arrray checks in CachedStorage extremely weird, it makes no sense that your own cache could be something that we didn't put there, so glad that this can be cleaned up :)

Added the initWithData() call to ConfigImporter.

Note that the language related changes are currently unnecessary as they don't change anything. So unless we can find a way to actually improve something there, I can revert those again.

Added the initWithData() call to ConfigImporter.

Note that the language related changes are currently unnecessary as they don't change anything. So unless we can find a way to actually improve something there, I can revert those again.

@Berdir: yeah, I found the language changes a bit confusing, so if they don't help, maybe best to remove :)

I agree the language changes should be removed - #2108599: Convert language_default to CMI ensures that language is alway set in the configuration factory and I think this is correct. Even if the site is a monolingual site we need to be able to use the language override system otherwise we have no way of translating default configuration.

Added some PHPUnit tests to CachedStorageTest that tests both CachedStorage::read() and CachedStorage::readMultiple().

We never need to do language overrides if language.module is not enabled IMHO. And right now, if that is not there, then nobody does set the language(manager), so no overriding happens. Not sure if actually works like that right now, but I think would make sense.

@Berdir yep you are correct - but I think we should handle the interaction between the config factory and language over in #2108599: Convert language_default to CMI

23: 2177739.23.patch queued for re-testing.

The test failures appear to be unrelated.

Looks good to me. I'm not sure I'm the best one to RTBC though. Would be good to have a once-over from @beejeebus.

All right, since @beejeebus does not seem to be available, I did a once-over again on the patch and the fixes look good. Especially the new test coverage to ensure that the loads are efficient. Looks great.

23: 2177739.23.patch queued for re-testing.

Re-roll. Conflicted with renamed constant and a new line in the dropped load() method.

Today is apparently stupid-typo-day for me.

Removing a new isLoaded() check.

1. +++ b/core/lib/Drupal/Core/Config/CachedStorage.php
   @@ -99,9 +91,10 @@ public function readMultiple(array $names) {
   +        $this->cache->set($name, isset($list[$name]) ? $list[$name] : FALSE, Cache::PERMANENT);
   

   Minor but no need to explicitly set Cache::PERMANENT since it's the default.

2. +++ b/core/lib/Drupal/Core/Config/CachedStorage.php
   @@ -68,22 +68,14 @@ public function exists($name) {
   +    $this->cache->set($name, $data, Cache::PERMANENT);
   

   And here.

Removed Cache::PERMANENT from \Drupal\Core\Config\CachedStorage where possible.

Thanks! Committed/pushed to 8.x.