Language configuration overrides should have their own storage

Weirdly enough, it all broke down again after last week and I now have now even more errors then on the current patch, so there's no real use to posting it now, sorry for the delay on this :/

+++ b/core/modules/language/language.services.yml
@@ -11,8 +11,21 @@ services:
+      - { name: persist }
...
+    tags:
+      - { name: persist }

persist can be removed now.

Hm... This introduces a much higher coupling between language storage, cached storage and file storage than right now, the fact that we have to extend the cached storage and then inside hardcode the active storage constants and use of FileStorage seems very unfortunate. Need to think if there's a better way.

This should fix the most obvious mistakes, let's see where this puts us. Going to work on improving the storage now.

Trying to untangle the storage stuff, there are now two classes, one that subclasses CachedStorage, which now supports a prefix directly, the old method were not actually called, and one that subclasses FileStorage. Still have to hardcode the active path in the file storage, but I think this is much cleaner already.

Also added the missing return array(), that should fix those php notices.

That was a fun ride, going to explain the fixes with dreditor in a follow-up comment. Also added some basic test coverage for the cache prefix.

1. +++ b/core/lib/Drupal/Core/Config/CachedStorage.php
   @@ -97,8 +97,8 @@ public function readMultiple(array $names) {
    
   -    if (!empty($names)) {
   -      $list = $this->storage->readMultiple($names);
   +    if (!empty($cids)) {
   +      $list = $this->storage->readMultiple(array_keys($cids));
          // Cache configuration objects that were loaded from the storage, cache
   

   Because we now use $cids to pass through to the cache backend, we also need to use that for checking if there is something that we couldn't load from the cache.

2. +++ b/core/lib/Drupal/Core/Config/ConfigInstaller.php
   @@ -138,12 +138,12 @@ public function installDefaultConfig($type, $name) {
    
   -      foreach ($sorted_config as $name) {
   -        $new_config = new Config($name, $this->activeStorage, $this->eventDispatcher, $this->typedConfig);
   -        if ($data[$name] !== FALSE) {
   -          $new_config->setData($data[$name]);
   +      foreach ($sorted_config as $config_name) {
   +        $new_config = new Config($config_name, $this->activeStorage, $this->eventDispatcher, $this->typedConfig);
   +        if ($data[$config_name] !== FALSE) {
   +          $new_config->setData($data[$config_name]);
   

   This fixes LocaleConfigManagerTest, we already pass $name (name of the module) to the method, so overwrote that and passed 'locale_test.translation' to the locale config override which then resulted in obviously not finding and importing any configuration.

3. +++ b/core/modules/language/lib/Drupal/language/Config/LanguageConfigFactoryOverride.php
   @@ -80,7 +80,7 @@ public function loadOverrides($names) {
        $storage = clone $this->storage;
        $data = $storage->setLangcode($langcode)->read($name);
   -    $override = new LanguageConfigOverride($name, $this->storage, $this->typedConfigManager);
   +    $override = new LanguageConfigOverride($name, $storage, $this->typedConfigManager);
        if (!empty($data)) {
   

   We cloned the storage and changed the language but didn't actually pass that storage to the config override class, so we loaded from there, but passed in the storage with the old langcode. Then when saving, we wrote the cache with the wrong prefix.

   See next snippet why it wrote the file correctly...

4. +++ b/core/modules/language/lib/Drupal/language/Config/LanguageOverrideCachedStorage.php
   @@ -43,4 +43,13 @@ public function setLangcode($langcode) {
   +  function __clone() {
   +    // Also clone the inner storage, so that language code changes work as
   +    // expected.
   +    $this->storage = clone $this->storage;
   +  }
   

   There are now two nested storage objects, and we only cloned the cache wrapper, so the file only existed globally and we changed the langcode on it globally.

5. +++ b/core/tests/Drupal/Tests/Core/Config/CachedStorageTest.php
   @@ -116,7 +143,7 @@ public function testGetMultipleOnPartiallyPrimedCache() {
        $storage->expects($this->once())
          ->method('readMultiple')
   -      ->with(array(2 => $configNames[2], 4 => $configNames[4]))
   +      ->with(array($configNames[2], $configNames[4]))
          ->will($this->returnValue($response));
    
   

   Because I'm now passing the config names as array_keys($cids) to the inner storage, they are now re-keyed, which is fine I think but we need to update the test mock.

I was wondering what those inverse methods that I deleted were for :p

Will, implementing unit tests for the wrong behavior is a pretty stupid thing to do ... :)

30: language-override-storage-30.patch queued for re-testing.

Looks like the drupal_set_time_limit() fix wasn't fixing anything after all :)

Trying to see if that works, will re-open that issue.

30: language-override-storage-30.patch queued for re-testing.

30: language-override-storage-30.patch queued for re-testing.

So we need to work out how to get configuration sync to play nice with this approach

Poostponing on #2238069: Create a way to add steps to configuration sync per @alexpott.

Unpostponed!

34: language-override-storage-34.patch queued for re-testing.

was green... but HEAD is moving fast

Rerolled.

Updating the patch with rerolled from #34.

Sorry , Its my mistake. Uploaded at same time with martin107

I think because of same name of patch, martin 107's patch might be updated with last one.

@ Jalandhar no problem, a happy mistake, critical issue will draw lots of attention... if there was any mistake is was myself not assigning it to me to prevent duplication
of your work lets see what testbot thinks of yours.

PS

Just did a diff of the two reroll patches.. virtually identical.. one patch renames a file another deletes files and add a new files .. both are equivalent

@martin107,

Hmmm. I think testbot also thought positively like you...:P. Tests got passed..:)

Thanks @Jalandhar and @martin107. Next we need to update this based on comment #37 following #2238069: Create a way to add steps to configuration sync.

I will try to update this this weekend to subclass DatabaseStorage instead and use a different table with a langcode column and open a follow-up/separate issue to make cached storage get support for cache prefix as we'll no longer need it here until we want to put a cache in front of database storage.

Ok, let's try this.

- Removed cached storage and file storage changes
- Added a new database storage subclass, seems to be working fairly well but have to copy a lot of code because we have to add an additional condition to every query.
- Also added a deleteAllLanguages() which is deleteAll() for all languages, needs a better name but does make uninstall much easier. Not sure if we have test coverage for that?

The null/false fails are because I changed read() to call readMultiple() and return NULL. Can be easily changed to FALSE but we should probably look into changing that to return NULL as we did elsewhere (like loading entities).

Everything else seems to pass, so now we need to solve export/import/sync.

Thanks @Berdir!

Next up, Mr. Pott.

Patch:

• Changes DatabaseStorage::read() to return FALSE - if we want to change this to NULL then let's do that in a much smaller followup
• Protects against changing the language on the override storage to mitigate against strange side effects like have a language configuration override object where the override data is in one language and the storage is set to another language.
• Changes deleteAllLanguages() to deleteAllOverrides() since that is what it is doing.

Now working on the configuration sync part of this.

The patch attached adds the ability to synchronise language overrides. However I think this approach highlights several issues:

• If we are only importing language overrides at the moment this will not work because the config sync pages is only looking for changes in the regular config storage
• The language config sync code has to make assumptions about how staging in structured and that it is in fact a file storage
• If another module was providing overrides through a similar method of storing in pseudo config files it is going to have to write much of the same code
• If the language module is not enabled then it is totally impossible with this approach for the sync process to discover what is going to happen

I attach the patch so people can see how ugly this approach is - I really do not want to go this way.

What I want to do is #2262861: Add concept of collections to config storages

postponing on #2262861: Add concept of collections to config storages

Rolled the patch on top of #2262861: Add concept of collections to config storages to ensure that collections is going to work out.

So

+++ b/core/modules/language/language.install
@@ -0,0 +1,26 @@
+function language_install() {
+  // Get all the existing modules that are enabled and install any language
+  // configuration.
+  $language_config_override = \Drupal::service('language.config_factory_override');
+  foreach (array_keys(\Drupal::moduleHandler()->getModuleList()) as $module) {
+    $language_config_override->install('module', $module);
+  }
+  /** @var \Drupal\Core\Extension\ThemeHandler $theme_handler */
+  $theme_handler = \Drupal::service('theme_handler');
+  foreach ($theme_handler->listInfo() as $theme) {
+    if ($theme->status) {
+      $language_config_override->install('theme', $theme->name);
+    }
+  }
+  \Drupal::configFactory()->reset();
+}

+++ b/core/modules/language/lib/Drupal/language/Config/LanguageConfigFactoryOverride.php
@@ -165,4 +137,38 @@ public function setLanguageFromDefault(LanguageDefault $language_default = NULL)
+  public function install($type, $name) {
+    // Work out if this extension provides default language overrides.
+    $config_dir = drupal_get_path($type, $name) . '/config/install/language';
+    if (is_dir($config_dir)) {
+      // List all the directories.
+      // \DirectoryIterator on Windows requires an absolute path.
+      $it  = new \DirectoryIterator(realpath($config_dir));
+      foreach ($it as $dir) {
+        if (!$dir->isDot() && $dir->isDir() ) {
+          $default_language_config = new FileStorage($dir->getPathname());
+          $storage = $this->getStorage($dir->getFilename());
+          foreach ($default_language_config->listAll() as $config_name) {
+            $data = $default_language_config->read($config_name);
+            $config = new LanguageConfigOverride($config_name, $storage, $this->typedConfigManager);
+            $config->setData($data)->save();
+          }
+        }
+      }
+    }
+  }
...
+  public function uninstall($type, $name) {
+    foreach (\Drupal::languageManager()->getLanguages() as $language) {
+      $storage = $this->getStorage($language->getId());
+      $storage->deleteAll($name . '.');
+    }
+  }

So this code means that all language overrides are always installed regardless of whether the language exists on the site or not. I think this is the wrong approach. We should only be create language overrides for languages that exist. Especially since if you look at the uninstall code we're only uninstalling overrides for languages that exist.

Fixed up test failures due to bug in collections code - new patch posted on issue. Improved install code according to #64. Now using collections in ExtensionInstallStorage which means that if language is enabled and then views is enabled and an another module provides views and translations in languages that are enabled they'll be installed at this point.

Refactored ConfigInstaller to work with collections so that Language can re-use to install overrides.

Testbot why did you not test?

Keeping up with #2262861: Add concept of collections to config storages which no includes code to handle installation, uninstallation and more.

less code++ :)

Now on top of 8.x as collections is in.

Oops forgot to commit the deletes of the old language config overrides...

1. Ahh we should test collections config too in DefaultConfigTest -fixed - although this opens a can of worms - see below.
2. Fixed
3,4. I've implemented a \Drupal\Core\Config\StorableConfigBase::preSave() method which limits the amount of code duplication to just the necessary differences whilst keeping the save and delete methods abstract and their implementations concerned with doing just that. The @todo is a copy and paste. Considering it was added in #1668820: Concept, base class, and interface for Configurables (e.g., node types, image styles, vocabularies, views, etc) I we've had long enough to consider removing and we've not done it so removing. It is difficult to know how to add @see's to ensure that we make the correct changes to both Config and LanguageOverrideConfig.
5. Fixed
6. Not sure I get the question.

Can of worms

--- a/core/modules/tour/tests/tour_test/config/install/language/it/tour.tour.tour-test.yml
+++ b/core/modules/tour/tests/tour_test/config/install/language/it/tour.tour.tour-test.yml
@@ -1,5 +1,6 @@
 label: Tour test italian
 tips:
   tour-test-1:
-    label: La pioggia cade in spagna
-    body: Per lo più in pianura.
+    plugin: text
+    label: 'La pioggia cade in spagna'
+    body: 'Per lo più in pianura.'

In order to get this configuration to validate and use schema correctly I've added the plugin key since the schema is dynamic and determined by the plugin - how are we going to do this in practice?

Okay so @Gábor Hojtsy and I agreed to descope use of schema during language config override saves since through using collections we already get the big wins. So the can of worms described in #77 has been descoped to #2270399: Use configuration schema during language override save

So we're disabling language module to test enabling during the configuration synchronisation - so once the import is complete the language module should be enabled and the overrides working - which is exactly want is tested.

So whilst thinking about how to solve #2270399: Use configuration schema during language override save I realised that we have a problem since during config install and config synchronisation we're not using the new LanguageConfigOverride class to create configuration - we're using the regular Config class which means events are fired. We need to change how collections interact with the ConfigInstaller and ConfigImporter and delegate Config object creation to an overrider if necessary. The patch attached adds a test to show what I'm on about - obviously it is going to fail.

Adding test for configuration synchronisation too... will fail.

Now for a patch that should make the two new tests pass. The event ConfigCollectionNamesEvent is renamed to ConfigCollectionInfoEvent since it now has the ability to get and set config factory overriders that are responsible for each possible collection. If a config factory overrider is responsible for a collection then the installer and importer delegate the responsibility for Config object creation to the overrider thereby allowing the use of LanguageConfigOverride instead of Config.

So the patch in #84 did not have the expected additional failure because the event dispatcher was not refreshed after installing a module.

The will-fail.patch does not tie up the collection to the factory override so this should produce the expected fails.

The patch also moves the collection info getting to the ConfigManager so the ConfigImporter and ConfigInstaller can share code.

+++ b/core/modules/language/lib/Drupal/language/Tests/LanguageConfigOverrideImportTest.php
@@ -0,0 +1,106 @@
+class LanguageConfigOverrideImportTest extends WebTestBase {

+++ b/core/modules/language/lib/Drupal/language/Tests/LanguageConfigOverrideInstallTest.php
@@ -0,0 +1,45 @@
+class LanguageConfigOverrideInstallTest extends KernelTestBase {

Missing class docblocks here btw.

@xjm's review in #89 - fixed

@Gábor Hojtsy's review in #90

1. Fixed
2. Not really - the first test enables the language module and all the other modules that where dependent on it (locale, config_translation) whereas this test only synchronises a single configuration override.
3. Fixed the first missing variable type - the second one is the return so no variable name needed
4. Good point - removed addCollections() - less code++
5. Fixed
6. Fixed
7. Good point - adding an InvalidArgumentException to addCollection() if the default collection is passed in
8. Discussed this with @Gábor Hojtsy in IRC - I can't see how we can get around delegating to something to create the config object - @Gábor Hojtsy has raised concerns about different arguments being passed to Config and $overrider->getConfigObject() but I've pointed out that as a service the overrider is free to use whatever services it likes to create its StorableConfigBase extending configuration object. However to improve things I've created a createConfigObject method on the ConfigManager so that there is no duplicated code on the ConfigImporter and the ConfigInstaller
9. Well the "eventness" of ConfigCollectionInfoEvent is an implementation detail that does not need to be exposed. Therefore renamed it to ConfigCollectionInfo - as the object contains the information and has useful accessors.

Also did some minor clean up of the patch.

+++ b/core/lib/Drupal/Core/Config/ConfigManagerInterface.php
@@ -123,4 +123,25 @@ public function findConfigEntityDependentsAsEntities($type, array $names);
+  /**
+   * Creates a StorableConfigBase object according to the needs of the collection.
+   *
+   * @param string $name
+   *   The configuration object name.
+   * @param string $collection
+   *   The configuration collection.
+   *
+   * @return \Drupal\Core\Config\StorableConfigBase
+   *   The configuration object.
+   */
+  public function createConfigObject($name, $collection);

I'm not happy with this method being called this on the ConfigManager. It is too easy to confuse this with the whole purpose of the ConfigFactory - which is to return Config objects :)

I think it is more like this...

The ConfigManager::createConfigObject really bothers me - removed it and added more documentation.

Haven't got much time this week, so only a partial review.

Overall this looks complex for configuration overriders, but there's not going to be that many of them and it does centralize things a lot.

Couple of minor things:

+++ b/core/lib/Drupal/Core/Config/ConfigFactoryOverrideInterface.php
@@ -32,4 +32,30 @@ public function loadOverrides($names);
+  /**
+   * Creates a configuration object for use during install and synchronization.
+   *
+   * If the overrider stores it's overrides in configuration collections then
+   * it can have its own implementation of
...
+   * themselves to a configuration collection by listening to the
+   * \Drupal\Core\Config\ConfigEvents::COLLECTION_INFO event and adding the
+   * collections they are responsible for. Doing this will allow installation
+   * and synchronization to use the overrider's implementation of
+   * StorableConfigBase.

Any reason why these have to be declared via an event rather than static YAML?

Also extra stray apostrophe in "Configuration overrider's".

If I want to get a list of overriders and collections how do I get that?

• The list of the collections is dynamic - for example the Language module creates on per configured language - an event is used because this is low level config stuff
• Fixed stray apostrophe.
• To get a list of possible collections \Drupal::service('config.manager')->getConfigCollectionInfo()->getCollectionNames() .
• To check if an overrider manages it is configuration in a collection \Drupal::service('config.manager')->getConfigCollectionInfo()->getOverrideService($collection_name)

The list of the collections is dynamic - for example the Language module creates on per configured language - an event is used because this is low level config stuff

Hmm I guess that'd be the same for domain as well (one collection per-domain).

I realised I didn't get to review #2262861: Add concept of collections to config storages before it went in, so I'll go back over to there, then return here where things should make more sense.

Read through #2262861: Add concept of collections to config storages and this patch again. Agreed with Gabor's comments in #97 but the trade-off seems OK and I don't have any better ideas. We've still got some time to try to clarify things a bit more as well.

Committed/pushed to 8.x, thanks!