Problem/Motivation

This is a sub-issue for #2201437: [META-1] Config overrides and language, now that we refactored configuration classes, events for read and override loading for language. We did not yet move config storage to its own place. That would hep with key length explosion as well as storing files at different places. It needs to handle import/sync/install/uninstall on its own.

Proposed resolution

- Implement separate storage for configuration.

Remaining tasks

- Import and sync have issues, that should still be solved.

User interface changes

- None.

API changes

- Introduce separate storage for configuration
- Add install/uninstall to the config override interface

Files: 
CommentFileSizeAuthor
#99 2224887.99.patch47.19 KBalexpott
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 71,790 pass(es). View
#99 95-99-interdiff.txt807 bytesalexpott
#96 91-95-interdiff.txt5.23 KBalexpott
#96 2224887.95.patch47.2 KBalexpott
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 71,756 pass(es). View

Comments

Gábor Hojtsy’s picture

Title: Language configuration overrides should have their own storae » Language configuration overrides should have their own storage
Gábor Hojtsy’s picture

Assigned: Unassigned » Gábor Hojtsy

Taking a stab at this one :)

xjm’s picture

Issue tags: +Naming things is hard
jessebeach’s picture

Issue tags: +Configuration system
Gábor Hojtsy’s picture

Status: Active » Needs review
FileSize
29.12 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Invalid PHP syntax in core/modules/language/lib/Drupal/language/Config/LanguageConfigFactoryOverride.php. View

Here is a first roll of this. I did not successfully wired up the overrides yet.

Status: Needs review » Needs work

The last submitted patch, 5: language-override-storage.patch, failed testing.

Gábor Hojtsy’s picture

Status: Needs work » Needs review
FileSize
29.12 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 61,524 pass(es), 3,283 fail(s), and 1,933 exception(s). View
784 bytes

Helps to not introduce PHP errors.

Status: Needs review » Needs work

The last submitted patch, 7: language-override-storage-7.patch, failed testing.

Gábor Hojtsy’s picture

Status: Needs work » Needs review
FileSize
29.27 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 56,525 pass(es), 125 fail(s), and 0 exception(s). View
617 bytes

The service definition should use the right storage not the old file storage. This exposes some new issues but shut cut down on fails a lot as a start :D Babysteps.

Status: Needs review » Needs work

The last submitted patch, 9: language-override-storage-9.patch, failed testing.

Gábor Hojtsy’s picture

Assigned: Gábor Hojtsy » swentel

@swentel told me he has fixes to post. Now just need to get him post them :)

swentel’s picture

Assigned: swentel » Unassigned

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 :/

Berdir’s picture

+++ 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.

Berdir’s picture

Status: Needs work » Needs review
FileSize
27.95 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 66,235 pass(es), 2,040 fail(s), and 17,967 exception(s). View
2.67 KB

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

Status: Needs review » Needs work

The last submitted patch, 14: language-override-storage-14.patch, failed testing.

Berdir’s picture

Status: Needs work » Needs review
FileSize
35.16 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 66,890 pass(es), 52 fail(s), and 58 exception(s). View
13.18 KB

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.

Status: Needs review » Needs work

The last submitted patch, 16: language-override-storage-16.patch, failed testing.

Gábor Hojtsy’s picture

Status: Needs work » Needs review
FileSize
2.63 KB
34.77 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 66,948 pass(es), 47 fail(s), and 3 exception(s). View

Debugged this further. The attached patch fixes the following two issues:

- The config translation UI is now getting a config override object and should typehint it as such (fixes some test fails)
- The setLanguage() method has an optional langcode, if no language was passed, NULL should be passed along (fixes lots if not all of the exceptions)

I also debugged why upon saving the translation, it is not showing up as translated through the config translation forms (its clearly exported into files right away), but did not yet get to the bottom of that. That results in several fails in config translation and possibly others. May be too aggressive caching.

Status: Needs review » Needs work

The last submitted patch, 18: language-override-storage-18.patch, failed testing.

Gábor Hojtsy’s picture

Status: Needs work » Needs review
FileSize
38.51 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 66,958 pass(es), 37 fail(s), and 3 exception(s). View
1.93 KB

Somehow the .yml file renames did not make it into my patch.... Doh. This should fix some further fails.

Status: Needs review » Needs work

The last submitted patch, 20: language-override-storage-20.patch, failed testing.

Gábor Hojtsy’s picture

Status: Needs work » Needs review
FileSize
2.36 KB
39.3 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 67,032 pass(es), 24 fail(s), and 0 exception(s). View

- Found one more language.config YAML that should be renamed.
- Found a use of typedConfig where it should be typedConfigManager.
- Invoking setLancgode() would not work :D setLangcode() is better.

Status: Needs review » Needs work

The last submitted patch, 22: language-override-storage-22.patch, failed testing.

Gábor Hojtsy’s picture

All right, I spent considerable time with this and could not figure out an apparent fix. The problem is clear: The code used in the config translation form (ConfigTranslationFormBase::submitForm) does save proper to file storage but does not refresh the language override cache. This is apparent from the UI and is easily reproducible manually:

  • Add a few languages
  • Enable config translation
  • Go translate your site info, 'Add' a translation
  • It will say its saved, will appear in the config active directory under language/XY/* but you'll still see an 'Add' button
  • Using the 'Add' button again, now your translation loads back in proper (its already there)
  • Save it again, now it will be in the cache too
  • If you remove the translation ('Delete' here), it will be both removed from cache and the file system

Ironically even though the bug looks very simple, I could not figure out how does the file storage / cached storage and the override object itself interact properly and where this goes wrong. It probably a simple thing and I got lost in the forest.

AFAIS fixing this would fix all but one of the fails. And there are no more exceptions already :)

I'll likely not have time to fix this anytime soon, so help is needed :) @alexpott, @berdir? :)

Berdir’s picture

Status: Needs work » Needs review
FileSize
41.27 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 67,469 pass(es), 4 fail(s), and 0 exception(s). View
6.41 KB

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.

Berdir’s picture

  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.

Status: Needs review » Needs work

The last submitted patch, 25: language-override-storage-25.patch, failed testing.

Berdir’s picture

Status: Needs work » Needs review
FileSize
41.92 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 67,430 pass(es), 3 fail(s), and 0 exception(s). View
1.13 KB

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

Status: Needs review » Needs work

The last submitted patch, 28: language-override-storage-28.patch, failed testing.

Berdir’s picture

Status: Needs work » Needs review
FileSize
42.07 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 67,568 pass(es). View
1.08 KB

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

Status: Needs review » Needs work

The last submitted patch, 30: language-override-storage-30.patch, failed testing.

Berdir’s picture

The last submitted patch, 30: language-override-storage-30.patch, failed testing.

Berdir’s picture

Status: Needs work » Needs review
FileSize
42.54 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Unable to apply patch language-override-storage-34.patch. Unable to apply patch. See the log in the details link for more information. View
478 bytes

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.

Berdir’s picture

tstoeckler’s picture

alexpott’s picture

Status: Needs review » Needs work

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

xjm’s picture

Title: Language configuration overrides should have their own storage » [PP-1] Language configuration overrides should have their own storage
Status: Needs work » Postponed
Gábor Hojtsy’s picture

Issue tags: +sprint
xjm’s picture

Status: Postponed » Needs work

Unpostponed!

xjm’s picture

Title: [PP-1] Language configuration overrides should have their own storage » Language configuration overrides should have their own storage
martin107’s picture

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

was green... but HEAD is moving fast

The last submitted patch, 34: language-override-storage-34.patch, failed testing.

martin107’s picture

Status: Needs work » Needs review
FileSize
44.22 KB

Rerolled.

Jalandhar’s picture

FileSize
42.06 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Unable to apply patch language-override-storage-44_0.patch. Unable to apply patch. See the log in the details link for more information. View

Updating the patch with rerolled from #34.

Jalandhar’s picture

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.

martin107’s picture

@ 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

Jalandhar’s picture

@martin107,

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

xjm’s picture

Status: Needs review » Needs work

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.

Gábor Hojtsy’s picture

Status: Needs work » Needs review

Status: Needs review » Needs work

The last submitted patch, 45: language-override-storage-44.patch, failed testing.

Gábor Hojtsy’s picture

Status: Needs work » Needs review
FileSize
44.3 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 67,807 pass(es). View

Rerolled to apply again. Not sure how/if this is even going to work with the new database storage :)

Berdir’s picture

Assigned: Unassigned » Berdir
Status: Needs review » Needs work

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.

Berdir’s picture

Status: Needs work » Needs review
FileSize
33.84 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 68,114 pass(es), 7 fail(s), and 0 exception(s). View
25.01 KB

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?

Status: Needs review » Needs work

The last submitted patch, 54: language-override-storage-54.patch, failed testing.

Berdir’s picture

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.

xjm’s picture

Assigned: Berdir » alexpott

Thanks @Berdir!

Next up, Mr. Pott.

alexpott’s picture

Status: Needs work » Needs review
FileSize
15.28 KB
38.93 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 70,511 pass(es). View

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.

Gábor Hojtsy’s picture

Reviewed those changes:

  1. +++ b/core/lib/Drupal/Core/Config/DatabaseStorage.php
    @@ -77,6 +77,9 @@ public function read($name) {
    +    // @todo change to return to be consistent with other parts of Drupal, for
    +    //   example, loading an entity returns NULL if it does not exist.
    +    return FALSE;
    

    Would be important to qualify this with an issue link :) Also fix to make it a proper sentence.

  2. +++ b/core/modules/language/lib/Drupal/language/Config/LanguageOverrideDatabaseStorage.php
    @@ -148,7 +149,7 @@ public function deleteAll($prefix = '') {
    -  public function deleteAllLanguages($prefix) {
    +  public function deleteAllOverrides($prefix) {
    

    Good rename. This does not delete languages :D

  3. +++ b/core/modules/language/lib/Drupal/language/Config/LanguageOverrideDatabaseStorage.php
    @@ -164,6 +165,15 @@ public function deleteAllLanguages($prefix) {
    +    // Prevent side effects from switching changing a storage's langcode. The
    

    switching/changing repetition :)

  4. +++ b/core/modules/language/lib/Drupal/language/Config/LanguageOverrideStorageInterface.php
    @@ -14,11 +14,16 @@
        * Sets the langcode to determine the override configuration directory to use.
        *
    +   * Implementations
    +   *
    

    Incomplete doc.

alexpott’s picture

FileSize
16.47 KB
52.66 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 70,577 pass(es), 1 fail(s), and 0 exception(s). View

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

Status: Needs review » Needs work

The last submitted patch, 60: 2224887.60.patch, failed testing.

alexpott’s picture

Status: Needs work » Postponed
alexpott’s picture

Status: Postponed » Needs review
FileSize
28.36 KB
116.18 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 71,355 pass(es), 4 fail(s), and 0 exception(s). View

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

alexpott’s picture

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.

Status: Needs review » Needs work

The last submitted patch, 63: 2224887-collections.patch, failed testing.

alexpott’s picture

Status: Needs work » Needs review
FileSize
30.6 KB
119.43 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 71,363 pass(es), 3 fail(s), and 0 exception(s). View
6.38 KB

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.

Status: Needs review » Needs work

The last submitted patch, 66: 2224887.66.collections.patch, failed testing.

alexpott’s picture

Status: Needs work » Needs review
FileSize
12.44 KB
38.64 KB
127.47 KB

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

alexpott’s picture

FileSize
127.47 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 71,362 pass(es). View

Testbot why did you not test?

alexpott’s picture

FileSize
15.41 KB
157.35 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Setup environment: Test cancelled by admin prior to completion. View
26.68 KB

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

alexpott’s picture

FileSize
2.9 KB
153.85 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 71,444 pass(es). View
23.17 KB

less code++ :)

Gábor Hojtsy’s picture

Postpoed #2268939: Config overrides not updated when config changes (Critical beta target bug) on this one, since this introduces the infrastructure for it to be implemented sanely.

Gábor Hojtsy’s picture

alexpott’s picture

FileSize
22.95 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Setup environment: Test cancelled by admin prior to completion. View

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

alexpott’s picture

FileSize
23.17 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 71,531 pass(es). View

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

Gábor Hojtsy’s picture

Status: Needs review » Needs work
  1. +++ b/core/modules/config/lib/Drupal/config/Tests/DefaultConfigTest.php
    @@ -51,9 +51,9 @@ public function testDefaultConfig() {
    -      if (strpos($config_name, 'migrate.migration') === 0 || strpos($config_name, 'language.config') === 0) {
    ...
    +      if (strpos($config_name, 'migrate.migration') === 0) {
    

    Yay, I love how this patch makes this possible :)

  2. +++ b/core/modules/language/lib/Drupal/language/Config/LanguageConfigFactoryOverride.php
    @@ -165,4 +139,48 @@ public function setLanguageFromDefault(LanguageDefault $language_default = NULL)
    +   * @param $langcode
    

    Typehint with string.

  3. +++ b/core/modules/language/lib/Drupal/language/Config/LanguageConfigOverride.php
    @@ -0,0 +1,71 @@
    +  public function save() {
    +    // Validate the configuration object name before saving.
    +    static::validateName($this->name);
    

    This method is a close replica of the save() method on Config, except there is no save event fired (good) and no validation delegation if there was no schema (confusing). How is that validation applicable there but not here?

    Also once we do that validation, there is only 1 line of difference between the two methods.

  4. +++ b/core/modules/language/lib/Drupal/language/Config/LanguageConfigOverride.php
    @@ -0,0 +1,71 @@
    +  public function delete() {
    +    // @todo Consider to remove the pruning of data for Config::delete().
    

    Would be important to qualify this todo with an issue link.

    I see this is unqualified the same way in Config.php and that delete is also very similar but not exactly. Of course the differences are THE reason we embarked on this whole process to begin with and PHP does not provide us with an easy way to do cross-branch inheritance like this :) At least we should cross-reference the two implementations as similar then so someone who fixes one may notice there is another similar copy.

    Or alternatively move the differing logic to methods in the base class invoked from save()/delete() with empty implementations in StorableConfigBase and no implementation in LanguageConfigOverride and an actual implementation in Config. Would be nicer in terms of less code copied. :)

  5. +++ b/core/modules/language/lib/Drupal/language/ConfigurableLanguageManagerInterface.php
    @@ -91,12 +91,24 @@ public function updateLockedLanguageWeights();
    +   * Gets a language config override storage object.
    

    I don't think we abbreviate config like that? We write out "configuration" in docs if possible.

  6. +++ b/core/modules/language/lib/Drupal/language/Tests/LanguageConfigOverrideImportTest.php
    @@ -0,0 +1,67 @@
    +    \Drupal::moduleHandler()->uninstall(array('language'));
    

    How would the rest of the code work if language module is not enabled?!

alexpott’s picture

Status: Needs work » Needs review
FileSize
10.4 KB
30.32 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 71,677 pass(es). View

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?

alexpott’s picture

FileSize
3.03 KB
8.25 KB
23.33 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 71,652 pass(es). View

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

Gábor Hojtsy’s picture

Status: Needs review » Needs work

Ok, only 76/6 is left then. I asked if the test uninstalls language module, how would overrides even work afterwards? I don't get how that works. In the test. Maybe worth some added comments at least...

alexpott’s picture

Status: Needs work » Needs review
FileSize
23.48 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 71,668 pass(es). View
1.43 KB

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.

Gábor Hojtsy’s picture

Status: Needs review » Reviewed & tested by the community

Right, the way that happens is the two key lines above where the current copy is moved over to staging before language module is uninstalled, so that the only difference at that point is language module would be enabled with the sync. Then it adds language overrides to staging too, so then the diff is staging has language module enabled and language overrides set. Then the import proves the overrides have been loaded.

That seems to be completely testing what we aim to achieve here, and all other pieces look good now, so let's get this in :)

alexpott’s picture

Status: Reviewed & tested by the community » Needs review
FileSize
2.06 KB
26.24 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 71,714 pass(es), 1 fail(s), and 0 exception(s). View

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.

Status: Needs review » Needs work

The last submitted patch, 82: 2224887.82.patch, failed testing.

alexpott’s picture

Status: Needs work » Needs review
FileSize
2.55 KB
27.69 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 71,761 pass(es), 1 fail(s), and 0 exception(s). View

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

alexpott’s picture

FileSize
15.11 KB
42.63 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 71,710 pass(es). View

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.

The last submitted patch, 84: 2224887.84.patch, failed testing.

alexpott’s picture

FileSize
642 bytes
14.78 KB
46.72 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 71,782 pass(es), 2 fail(s), and 0 exception(s). View
46.72 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 71,697 pass(es). View

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.

The last submitted patch, 87: 2224887.87-will-fail.patch, failed testing.

xjm’s picture

+++ 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.

Gábor Hojtsy’s picture

Status: Needs review » Needs work

I reviewed all the interdiffs individually up until 87. I did review 87 as well, but the renames/doc fixes make sense there, so no comment on those. Just keep this in mind as some notes may not be 100% the updated code. Overall this looks a bit sad as we said storage collections would not be tied to config access APIs but looks like sync/install has no other way and it needs to backtrack that and this patch introduces APIs distinct for accessing "override collections" and "non-override collections" (AKA the default collection), which distinction is not otherwise spelled out. And then the code to do this is special casing the two...

+++ b/core/modules/language/lib/Drupal/language/Tests/LanguageConfigOverrideInstallTest.php
@@ -0,0 +1,42 @@
+    $this->enableModules(array('language_config_override_test'));

The test makes total sense and shows the problem :)

Can we use the existing language override test data to do this? Looks like overkill to me to add one more module for this.

  1. +++ b/core/modules/language/lib/Drupal/language/Tests/LanguageConfigOverrideImportTest.php
    @@ -29,7 +29,7 @@
    +   * Tests that language can be enabled and overrides created during a sync.
    

    minor: "are created" IMHO

  2. +++ b/core/modules/language/lib/Drupal/language/Tests/LanguageConfigOverrideImportTest.php
    @@ -69,2 +69,36 @@
    +  public function testConfigOverrideImportEvents() {
    

    This almost entirely overlaps with the other added test method, so why not test both there?

  3. +++ b/core/modules/language/lib/Drupal/language/Config/LanguageConfigFactoryOverride.php
    @@ -162,17 +171,40 @@
    +   * @param $collection
    +   *   The configuration collection name.
    +   *
    +   * @return string
    +   *   The langcode of the collection.
    

    One has no type, other has no variable name.

  4. +++ b/core/modules/language/lib/Drupal/language/Config/LanguageConfigFactoryOverride.php
    @@ -162,17 +171,40 @@
         foreach (\Drupal::languageManager()->getLanguages() as $language) {
           $collections[] = $this->createConfigCollectionName($language->getId());
         }
    -    $event->addCollectionNames($collections);
    +    $event->addCollectionNames($collections, $this);
    

    Why not use addCollectionName() and do away with this hair splittling array combine business going on in addCollectionNames(). Ie don't have a method to mass-add collections, since we need to assemble that array to do it anyway.

  5. +++ b/core/lib/Drupal/Core/Config/ConfigCollectionInfoEvent.php
    @@ -0,0 +1,82 @@
    + * Wraps a configuration event for event listeners.
    

    Would be good to define what is this event.

  6. +++ b/core/lib/Drupal/Core/Config/ConfigCollectionInfoEvent.php
    @@ -0,0 +1,82 @@
    +   * Configuration collection names.
    

    Its combined with a reference to their implementation now, so need to document that. its keyed by collection name with references or NULLs.

  7. +++ b/core/lib/Drupal/Core/Config/ConfigCollectionInfoEvent.php
    @@ -0,0 +1,82 @@
    +    if ($include_default) {
    +      array_unshift($collection_names, StorageInterface::DEFAULT_COLLECTION);
    +    }
    

    Would the default collection name by no chance be in the list on the event? Ie. no chance of it being duplicate when we add it at the start? I guess that would be because core does not respond to this event or fills in the default...

  8. +++ b/core/lib/Drupal/Core/Config/ConfigImporter.php
    @@ -896,7 +903,13 @@ protected function checkOp($collection, $op, $name) {
    +    if ($overrider) {
    +      $config = $overrider->getConfigObject($name, $collection);
    +    }
    +    else {
    +      $config = new Config($name, $this->storageComparer->getTargetStorage($collection), $this->eventDispatcher, $this->typedConfigManager);
    +    }
    
    +++ b/core/lib/Drupal/Core/Config/ConfigInstaller.php
    @@ -185,7 +187,13 @@ protected function createConfiguration($collection, array $config_to_install) {
    +      if ($overrider) {
    +        $new_config = $overrider->getConfigObject($name, $collection);
    +      }
    +      else {
    +        $new_config = new Config($name, $this->getActiveStorage($collection), $this->eventDispatcher, $this->typedConfig);
    +      }
    

    Its unfortunate we need to special case it here, I hoped we could encapsulate this on the service, but they need very different method arguments :/

  9. +++ b/core/lib/Drupal/Core/Config/ConfigInstaller.php
    @@ -169,8 +169,10 @@ protected function listDefaultConfigCollection($collection, $type, $name, array
    +   * @param \Drupal\Core\Config\ConfigCollectionInfoEvent $event
    +   *   The event which holds information about configuration collections.
    

    It looks strange to me we keep passing around an event with the information and not the information itself. Why would the rest of the API need to be concerned with what API was used to gather this information?

alexpott’s picture

Status: Needs work » Needs review
FileSize
23.3 KB
47.18 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 71,739 pass(es). View

@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.

alexpott’s picture

+++ 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 :)

Gábor Hojtsy’s picture

I wanted to put my instantiation doubts into a visual so it is clear what I have in my head that hurts. I must be missing something and this is much simpler... This is how I understand it with the patch in this issue:

So if I'm a contrib maintainer I think this is what I need to understand to be able to tell when to use what. Now correct all the fallacies of this figure :)

alexpott’s picture

FileSize
43.79 KB

I think it is more like this...

Gábor Hojtsy’s picture

Yeah that is pretty much the same just a little simplified from what I have. My point is this is complex. Much more complex than what we used to have. Of course you either know the implementation to use or you don't. In the former case that will use the right config storage collection in the later case you need to ask the config storage collection. Those may be different enough that no middle ground is possible. In that case I'll just spend some time to mourn the simplicity :D

alexpott’s picture

FileSize
47.2 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 71,756 pass(es). View
5.23 KB

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

Gábor Hojtsy’s picture

Assigned: alexpott » catch
Status: Needs review » Reviewed & tested by the community

Right, so at least we explored how this complicates things and the docs got better. I think the language handling itself is good, the collections changes needed are just based on how collections in combination with sync and install are and that we need to implement instantiation backwards there. I think this is ready for catch's eyes then.

catch’s picture

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?

alexpott’s picture

FileSize
807 bytes
47.19 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 71,790 pass(es). View
  • 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)
catch’s picture

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.

catch’s picture

Status: Reviewed & tested by the community » Fixed

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!

  • Commit 68d6316 on 8.x by catch:
    Issue #2224887 by alexpott, Berdir, Gábor Hojtsy, Jalandhar: Language...
Gábor Hojtsy’s picture

Issue tags: -sprint

Yay, great to see this landed :) Woot!

Gábor Hojtsy’s picture

Added a change notice for this at https://drupal.org/node/2274351, including the instantiation decision tree.

Status: Fixed » Closed (fixed)

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