Generalize language config override responsibility

Attached patch fixes the language configuration overrides

Here is a fix for test fails...

as discussed with @Gábor Hojtsy on IRC, we can use ConfigFactory instead of trying to get the same from languageManager. Updating patch as per this condition.

We can improve on the deleteLanguageTranslations to only delete things that actually exist.

Forgot to rebase :( - interdiff still valid.

In fact we can get away with even less change from 8.x here

1. +++ b/core/lib/Drupal/Core/Language/LanguageManagerInterface.php
   @@ -168,4 +168,23 @@ public function getFallbackCandidates($langcode = NULL, array $context = array()
   +   *
   +   * @return $this
   +   *   The language manager.
   

   No description needed for $this.

2. +++ b/core/modules/config/lib/Drupal/config/Tests/ConfigSchemaTest.php
   @@ -21,7 +21,7 @@ class ConfigSchemaTest extends DrupalUnitTestBase {
       */
   -  public static $modules = array('system', 'locale', 'field', 'image', 'config_test');
   +  public static $modules = array('system', 'language', ',locale', 'field', 'image', 'config_test');
    
   

   Can you spot the error?

   (Hint: Count the ",").

   And yes, DUBT does not validate this right now.

Looks great, a lot of *awesome* cleanup in tests and config_translation.

Want to do some profiling here, just to understand how this changes single and multi-language sites, will do that asap.

Ok, profiling. A regression has to be expected when languages are enabled as we're doing separate calls now, so let's see what happens:

Notes:
- All xhprof runs were taken by looking at 4 results and going with the fastest one. There were considerable call time differences, so don't rely too much on that, but the function call changes are obviously stable.
- I did test using an empty front page. Real sites will obviously load many config files more, which will increase the difference.

First, comparing a site without language.module. Not surprisingly, no big difference there, this patch is a tiny bit faster but it's almost not measurable. The difference between multiple xhprof runs was considerable larger.

Then, I added a second language and access the site with that. And was seeing a 40% regression of the whole request and 500% in ConfigFactory::loadMultiple():

That's way more than expected, so I started looking into why it's happening. Quickly identifed that we again had a problem with caching the missing language override config files, so we were doing a cache get + file get + cache set for them. Every time. Tracking down why that happens took me a bit more time:

LanguageConfigFactoryOverride passes an array like array('system.something' => 'langcode.config.en.system.something'), to storage, that passes it to the cache backend, that passes it as query arguments to the database. Which uses the keys to generate the query placeholder, resulting in :cids_system.something, which apparently silently doesn't work (I guess the ".") and doesn't return anything, so it never found the cache entries that it wrote before.

Easily fixed by doing an array_values() on the config names passed to the storage in LanguageConfigFactoryOverride. I'm not sure what the correct layer to fix this is, though. Do we even need them to be keyed by the original name? We don't seem to be using it right now? Should the config storage enforced numeric keys, or the cache implementation? (possibly cache?)

Anyway, another profile run, now it looks considerably better and pretty much what was to be expected. 37 additional config loads, resulting in 37 additional cache requests. That's a 7% regression on the whole site (still within the variations of the different runs, I had before xhprof results that were slower than after xhprof results, but also equally slower after runs) and 40% on ConfigFactory::loadMultiple().

I'm not sure if what we can do there. We *need* to make the implementation better anyway, and having it in a separate class makes it much easier to improve that in a follow and doing so here would make this more complicated and it would be kind of cheating (implementing generic improvements that would also have applied to the previous implementation to make it look like this is not a regression. It is.):

Some ideas to improve performance of loading language overrides:
- A simple blacklist perhabs, of config files that are always loaded and can definitely not be translated. Especially of the low-level config files that are always loaded like system.performance, system.module (per-language enabled module list => mind. blown.), timezone settings and stuff like that. Attaching a list of config files that are loaded on an empty site, we can look at what we can exclude from that.
- A more intelligent system that could track a black and/or whitelist of files to load automatically and save it in a cache or state would als be a possibility, but that's much more complicated as then the override would need to ract on config writes
- Preloading will help, if we can load some of these config files early on and together, it will also be a single load for the language overrides for them.

=> Open a major (at least) follow-up, non-beta blocking to discuss this and try out things?

=> Left to do here: Decide if we want to do a different fix than I did for the cache problem. Can we (unit) test this somehow (Verify that loading a config file with enabled but missing language override (and also one that exists) does not result in cache writes/file storage reads?

Ah, forgot the interdiff.

Changing the logic with the default language a bit:

Right now, we always set the default language and then it is eventually set again to the actual language once negotiation happened.

Instead, this patch does only either of those. For monolingual sites (but with language.module enabled), we set the default language through setLanguageFromDefault() directly set on the service definition and for multilingual sites, we set it after language negotiation did run. That means multilingual sites have no config override language set before language negotiation happens as those overrides would be wrong anyway.

This saves us 10 language override reads/cache gets. Monolingual sites with language.module still have them right now but we can still look into the ideas described above and exclude those with a blacklist instead.

Also removed the now unnecessary persist tag.

Ouch.

21: 2219499.20.patch queued for re-testing.

Grr, accidently added my xhprof hacks to the patch :(

Ok, so those tests fail because getConfigOverrideLanguage() no longer always returns a language. Specifically, it doesn't in a multilingual environment within the test method as there's no bootstrap, obviously I *think* that's ok, if we really want to test langage config override loading in there, we could set it manually.

I'm not sure what's the correct way to fix this, though. Should the language be optional there? LanguageConfigFactoryOverrideInterface::setLanguage() already is, should setConfigOverrideLanguage() be consistent with that? Or should getConfigOverrideLanguage() ensure to always return a language? Looking at the code, I think the first would be the expected behavior, as we want to restore the original behavior and that was no language.

Ok, all we need to do then is make that argument optional. Should be an easy task for someone at the sprint tomorrow?

Committed/pushed to 8.x, thanks!