[META-1] Config overrides and language

Note that I already made very good progress over in:

#2190723: Add a KeyValueStore\FileStorage to replace e.g. ConfigStorage

In essence, it replaces the custom storage engines of the configuration system with regular key/value store implementations.

In the key/value architecture, a "subdirectory" is just simply a "collection".

The most important aspect: Stop coupling the config storage to the filesystem or a specific serialization format.

All of these will work to instantiate storage service for language-specific overrides:

$language_store = new FileStorage('langcode=de', new Yaml(), config_get_config_directory(CONFIG_ACTIVE_DIRECTORY));
$language_store = new FileStorage('langcode=de', new Json(), config_get_config_directory(CONFIG_ACTIVE_DIRECTORY));
$language_store = new DatabaseStorage('langcode=de', new PhpSerialize(), Database::getConnection(), 'config');
$language_store = new DatabaseStorage('langcode=de', new Xml(), Database::getConnection(), 'config');

# And quite possibly in tests:
$language_store = new MemoryStorage('langcode=de');

All right, elevating to critical beta blocker in light of #2168609: Move config translations (language.config.[langcode]) in to new location. The sheer extent of API changes suggested is clearly beta blocking. Will close down #2168609: Move config translations (language.config.[langcode]) in to new location as duplicate.

Interesting stuff. Here are some code review comments. I'll keep digesting the changes for a while, there are lots of changes and I don't fully understand yet how all pieces fit.

1. +++ b/core/lib/Drupal/Core/Config/ConfigFactory.php
   @@ -392,4 +312,30 @@ static function getSubscribedEvents() {
   +  public function getSortedConfigFactoryOverrides() {
   +    if (!isset($this->sortedConfigFactoryOverrides)) {
   +      // Sort the negotiators according to priority.
   +      krsort($this->configFactoryOverrides);
   +      // Merge nested negotiators from $this->configFactoryOverrides into
   +      // $this->sortedConfigFactoryOverrides.
   +      $this->sortedConfigFactoryOverrides = array();
   +      foreach ($this->configFactoryOverrides as $overrides) {
   +        $this->sortedConfigFactoryOverrides = array_merge($this->sortedConfigFactoryOverrides, $overrides);
   +      }
   +    }
   +    return $this->sortedConfigFactoryOverrides;
   

   Negotiators?

2. +++ b/core/lib/Drupal/Core/Config/ConfigFactoryOverridePass.php
   @@ -0,0 +1,33 @@
   +  public function process(ContainerBuilder $container) {
   +    $manager = $container->getDefinition('config.factory');
   +    foreach ($container->findTaggedServiceIds('config.factory.override') as $id => $attributes) {
   +      $priority = isset($attributes[0]['priority']) ? $attributes[0]['priority'] : 0;
   +      $manager->addMethodCall('addOverride', array(new Reference($id), $priority));
   +    }
   +  }
   

   Woah, so these would be now services instead of event listeners? (That at least answers my puzzled mind from above where I've seen you removed the module override event).

3. +++ b/core/lib/Drupal/Core/Config/ConfigInstaller.php
   @@ -72,14 +72,14 @@ public function __construct(ConfigFactoryInterface $config_factory, StorageInter
   +  public function installDefaultConfig($type, $extension_name) {
   
   @@ -131,6 +131,11 @@ public function installDefaultConfig($type, $name) {
   +      $config_override->install($type, $extension_name);
   
   +++ b/core/lib/Drupal/Core/Config/ConfigManager.php
   @@ -128,6 +128,11 @@ function uninstall($type, $name) {
   +      $config_override->uninstall($type, $name);
   

   I see why you changed $name to $extension_name to avoid confusion with config names. But this is not consistently done in the uninstall code. And nonetheless seems unrelated?

4. +++ b/core/lib/Drupal/Core/Config/FileStorage.php
   @@ -241,4 +241,15 @@ public function deleteAll($prefix = '') {
   +   * Sets rhe filesystem path for configuration objects.
   

   typo

5. +++ b/core/lib/Drupal/Core/CoreServiceProvider.php
   @@ -75,6 +76,9 @@ public function register(ContainerBuilder $container) {
   +    // Add the compiler pass that will process the tagged config factory
   +    // override services.
   +    $container->addCompilerPass(new ConfigFactoryOverridePass());
   

   So why do these need to be services now compiled to the container?

6. +++ b/core/lib/Drupal/Core/Datetime/Date.php
   @@ -215,11 +215,14 @@ protected function t($string, array $args = array(), array $options = array()) {
   -      // Enter a language specific context so the right date format is loaded.
   -      $original_language = $this->configFactory->getLanguage();
   -      $this->configFactory->setLanguage(new Language(array('id' => $langcode)));
   +      if ($this->languageManager->isConfigurable()) {
   +        $original_language = $this->languageManager->getConfigOverrideLanguage();
   +        $this->languageManager->setConfigOverrideLanguage(new Language(array('id' => $langcode)));
   +      }
          $this->dateFormats[$format][$langcode] = $this->dateFormatStorage->load($format);
   -      $this->configFactory->setLanguage($original_language);
   +      if ($this->languageManager->isConfigurable()) {
   +        $this->languageManager->setConfigOverrideLanguage($original_language);
   +      }
   

   This is distinctively harder to do / more ugly looking.

7. +++ b/core/lib/Drupal/Core/Language/LanguageManagerInterface.php
   @@ -168,4 +168,12 @@ public function getFallbackCandidates($langcode = NULL, array $context = array()
   +   * Determines if thr language manager is configurable or not.
   

   typo

8. +++ b/core/modules/config_translation/lib/Drupal/config_translation/Form/ConfigTranslationDeleteForm.php
   @@ -141,9 +136,9 @@ public function buildForm(array $form, array &$form_state, Request $request = NU
   +    $this->configStorage->setLangcode($this->language->id);
   

   Woo, its own storage, fancy :)

9. +++ b/core/modules/config_translation/lib/Drupal/config_translation/Tests/ConfigTranslationUiTest.php
   @@ -285,13 +281,12 @@ public function testContactConfigEntityTranslation() {
   -      $language_config_name = \Drupal::configFactory()->getLanguageConfigName($langcode, 'contact.category.feedback');
   -      $config_parsed = $file_storage->read($language_config_name);
   +      $config = \Drupal::languageManager()->config('contact.category.feedback', language_load($langcode));
   

   Fancy! This actually looks more understandable.

10. +++ b/core/modules/language/lib/Drupal/language/Config/LanguageConfigOverride.php
    @@ -0,0 +1,131 @@
    +   * @todo maybe this should be done somewhere else?
    +   */
    +  public function uninstall($type, $name) {
    

    Not sure where else would be possible?

11. +++ b/core/modules/language/lib/Drupal/language/ConfigurableLanguageManager.php
    @@ -372,4 +408,39 @@ public function getLanguageSwitchLinks($type, $path) {
    +  /**
    +   * {@inheritdoc}
    +   */
    +  public function isConfigurable() {
    +    return TRUE;
    +  }
    
    +++ b/core/modules/language/lib/Drupal/language/ConfigurableLanguageManagerInterface.php
    @@ -17,6 +17,11 @@
       /**
    +   * TRUE if the LanguageManager is configurable. Default implementation is FALSE.
    +   */
    +  const CONFIGURABLE = TRUE;
    +
    

    Why both a const and a method?!

It would be great to see a before/after architecture comparison (IMHO would be great as figures). If nobody else comes around to this, I may be able to do this late this week (I'll hopefully be able to sync up in person with Alex Pott too in London in the meantime).

I'm not that bothered about the extra cache i/o from language overrides. The main problem we have with cache i/o in CMI is pre-loading of configuration objects. So at the moment this might mean 2 x 25 cache hits which is pretty bad. With pre-loading implemented we're looking at more like 2 x 1 or 2 x 5 or whatever, which is going to be better than the monolingual case now.

It'll also be better than i18n language overrides in Drupal 7 where there's a parallel variables table iirc + locale/t() integration for configuration entities now - or at least not worse.

It is definitely better than Drupal 7 and also more flexible than current Drupal 8 with support for swapping out by eg. language domain or other alternate implementations as needed.

Just had a discussion about this at DrupalCamp London. Here are the raw notes:

Currently

• Config overrides baked into config factory
• Prefixed with language.config.$langcode.*
• Manage with config translation and locale module (write)
• We enable to do config translation reading with language module
• In config translation we use config() (fire config events), and sometimes access through storage

Breaks

• Config length 250, any config file which is close there will not be translatable
• We cannot apply schema because they get a different name, values should match in type
• Language override is not a complete representation (but not REAL config)
• In itself cannot be overridden although we treat it as config, its a proto-config, does not exist on its own

Current patch

• 1. Config in language is not special
• 2. Instead of event listeners, the overrides are a compiler pass
• no special handling needed for language in config factory (no NULL language)
• we would invoke the event all the time, this way it needs to explicitly register
• reading config fires no events anymore, performance critical to read quicker
• because the override is a service, it can be overriden (eg. domain language module, group language module)
• we could do this without changing the language storage (could be separate patch)
• 3. Introduces use of the override event in core, good example for contrib
• 4. Question is should we store in config? => store in subdirectory
• Solves the number of files stored, which would slow down listings, it would be uncontrollable growth
• Nicer to work with
• Could apply schema
• Problem: we still use the config object to read it, but should not
• 5. Performance concern is this will need to access config once more on the page, but changing storage (eg. non MySQL cache backend) would speed it up

Results

• Simpler config factory
• Replaceable language overrides
• Cleaner config directories
• Full testing / real implementation of the config override system
• Only one way to do overrides
• Schema checking is now natively possible

TODO

• Cache keys are still 255 chars, and we still prefix the cache key with langcode, has the same issue. Langcode may be longer than 5 chars. => Question is how much, eg. domains may be longer, but you don’t need to use the same cache.
• Importing does not work at the moment. The import method is there, not implemented.
• Implement a LanguageConfigOverride class that is similar to Config separate, so it can provide CRUD but no language override for example :D Config would be more complex.

Also updated issue summary with better versions of the summaries (not just copied the raw notes :D).

Here is a visual summary to help us move forward. Also adding to the issue summary. Editable at https://docs.google.com/drawings/d/1TiSqEVGUFoi3zMtK7fHop_wfyLJZ_ddl-vXe...

I think this starts to reach into way too many areas. We should maybe start by breaking out the compiler pass and the wrapping of overrides into its own issue, get that in and then continue forward... The extent of changes in this patch and the reach it has far and wide makes it hard to believe people would agree on all fronts at once :/

We agreed on breaking this up. Here is one of the items: #2215503: Use services instead of event subscribers for config module overrides. I believe Eli-T is working on another part.

Part 1 split in [#8561409-24] has passed tests and is awaiting review under #2215413: Split the Config class into StorableConfigBase and ConfigBase. Make ThemeSettings inherit from ConfigBase.

- #2215503: Use services instead of event subscribers for config module overrides is green and ready for review.
- #2215413: Split the Config class into StorableConfigBase and ConfigBase. Make ThemeSettings inherit from ConfigBase. only has a minor review feedback to fix but otherwise looks good :)

Since this is a meta, let's make it active so we can get continued architectural input here.

I'm hard at work on #2219499: Generalize language config override responsibility which will break out the language override features. It will NOT introduce its own storage, so diffs, deployment/install/uninstall/sync are still not affected. There is considerable code here still to handle that, so this issue will still not be fully solved, but that is a well rounded problem space we can cover.

#2219499: Generalize language config override responsibility now has the config language refactoring and its only a 70k patch. Much nicer to review :) Feedback welcome there.

The final(?) sub-issue then is #2224887: Language configuration overrides should have their own storage :) We still have some code from @alexpott above to put in there :)

#2224887: Language configuration overrides should have their own storage has all the remainder of this one, so closing this down as duplicate. Will update the issue summary there to carry over all remaining info that applies.

Yay for only one issue left.