Postponed on #2457551: Regression: optional default configuration is not translatable anymore in locale.

Problem/Motivation

Currently the locale module compares the install storage version of configuration with active config version of configuration to tell if there are changes and translates things that are not changed. However, the install storage may change independently with module updates. As the site gets modules updated, the same config may not exist anymore of be dramatically different. When translating the active storage copy configuration later (eg. adding one more language to the site or updating translations), comparing to the new install storage will end up making it impossible to translate any number of things automatically which should still be possible if we'd have the original copy of the install storage data.

This is a problem with the current implementation but it will be a bigger problem once/if #2212069: Non-English Drupal sites get default configuration in English, edited in English, originals not actually used if translated is resolved.

Proposed resolution

The configuration system should store the original copy of the configuration data in a collection for use by the locale and config translation modules (possibly later on). This is not the responsibility of locale or config translation or language modules because we need the original install storage files even if configuration translation is enabled one year later.

Remaining tasks

Implement. Add tests. Review. Commit.

User interface changes

None.

API changes

Possible.

Files: 
CommentFileSizeAuthor
#17 2428045-copy-storage-17.patch6.93 KBGábor Hojtsy
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 95,201 pass(es), 0 fail(s), and 1 exception(s). View
#15 2428045-copy-storage-12.patch6.91 KBGábor Hojtsy
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Unable to apply patch 2428045-copy-storage-12_0.patch. Unable to apply patch. See the log in the details link for more information. View
#12 2428045-copy-storage-12.patch6.91 KBGábor Hojtsy
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 90,580 pass(es). View
#7 interdiff.txt1.07 KBGábor Hojtsy
#7 2428045-copy-storage-7.patch6.8 KBGábor Hojtsy
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 87,093 pass(es). View
#5 2428045-copy-storage-5.patch6.44 KBGábor Hojtsy
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 86,981 pass(es), 1 fail(s), and 0 exception(s). View
#5 interdiff.txt5.62 KBGábor Hojtsy
#3 2428045-copy-storage.patch2.42 KBGábor Hojtsy
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 86,930 pass(es), 11 fail(s), and 0 exception(s). View

Comments

cpj’s picture

Is the version number (8.1.x) correct ? If this is a beta-upgrade blocker, I assume we need this before 8.0.x is released

Gábor Hojtsy’s picture

Version: 8.1.x-dev » 8.0.x-dev

Good point.

Gábor Hojtsy’s picture

Status: Active » Needs review
FileSize
2.42 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 86,930 pass(es), 11 fail(s), and 0 exception(s). View

Quick start to test the waters. I did not even attempt to make locale use this new collection yet instead of installstorage.

Status: Needs review » Needs work

The last submitted patch, 3: 2428045-copy-storage.patch, failed testing.

Gábor Hojtsy’s picture

Status: Needs work » Needs review
FileSize
5.62 KB
6.44 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 86,981 pass(es), 1 fail(s), and 0 exception(s). View

Fixing those fails by the following:

1. FileStorage should check for its directory for the collection before attempting to delete it.
2. ConfigExportUITest tests config which were all imported with projects, so we can easily fix the expectation for exported files.
3. ConfigInstallTest now needs to know about the config copy collection too.
4. The config import test event subscriber should only record events within the default collection.
5. TestBase::copyConfig() should copy all collections. (Note that getAllCollectionNames() DOES NOT return all collection names, it does not return the default collection, so we need to add that manually).
6. To avoid special casing that elsewhere, remove the exception on adding the config copy collection. This way it will be created when needed first.

Status: Needs review » Needs work

The last submitted patch, 5: 2428045-copy-storage-5.patch, failed testing.

Gábor Hojtsy’s picture

Status: Needs work » Needs review
FileSize
6.8 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 87,093 pass(es). View
1.07 KB

Test the config export more sophisticatedly.

Gábor Hojtsy’s picture

Issue summary: View changes
Status: Needs review » Postponed

Ok, now that works, I was trying to integrate this into LocaleConfigManager. However, LocaleConfigManager attempts to do various things based on component names, and only the install storage can segment config based on component names. One of the two things it does is removing config overrides, which is pointless, that already happens. So opened #2429385: locale_system_remove() should not attempt to remove strings from the locale DB for that. Resolving that would only leave one use of component name based locale config manager work, which we can work around here. Postponing on #2429385: locale_system_remove() should not attempt to remove strings from the locale DB.

Gábor Hojtsy’s picture

andypost’s picture

+++ b/core/lib/Drupal/Core/Config/ConfigInstaller.php
@@ -203,9 +203,16 @@ protected function createConfiguration($collection, array $config_to_install) {
+        // Create a copy of the original configuration for later reference.
+        if ($collection == StorageInterface::DEFAULT_COLLECTION) {
+          $config_copy = new Config($name, $this->getActiveStorages(StorageInterface::ORIGINAL_COPY_COLLECTION), $this->eventDispatcher, $this->typedConfig);
...
+        if (isset($config_copy)) {
+          $config_copy->setData($data[$name])->save();
+        }

+1 on making a copy of installed config

+++ b/core/lib/Drupal/Core/Config/FileStorage.php
@@ -231,7 +231,7 @@ public function deleteAll($prefix = '') {
-      if (!(new \FilesystemIterator($this->getCollectionDirectory()))->valid()) {
+      if (file_exists($this->getCollectionDirectory()) && !(new \FilesystemIterator($this->getCollectionDirectory()))->valid()) {

Nice catch for UnexpectedValueException
so bug

Gábor Hojtsy’s picture

Gábor Hojtsy’s picture

Status: Needs work » Needs review
FileSize
6.91 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 90,580 pass(es). View

Rerolled.

Gábor Hojtsy’s picture

Issue summary: View changes
Status: Needs review » Postponed

As per discussion on CMI meeting, this should be looked at after #2457551: Regression: optional default configuration is not translatable anymore in locale which is a direct regression.

Gábor Hojtsy’s picture

Status: Postponed » Needs work
Gábor Hojtsy’s picture

Status: Needs work » Needs review
FileSize
6.91 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Unable to apply patch 2428045-copy-storage-12_0.patch. Unable to apply patch. See the log in the details link for more information. View

Reuploading the patch for testing again.

Status: Needs review » Needs work

The last submitted patch, 15: 2428045-copy-storage-12.patch, failed testing.

Gábor Hojtsy’s picture

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

Straight reroll.

Status: Needs review » Needs work

The last submitted patch, 17: 2428045-copy-storage-17.patch, failed testing.

Gábor Hojtsy’s picture

Assigned: Unassigned » alexpott

We discussed this on the CMI meeting last Thu and @alexpott suggested a different approach which he was about to write up.

Gábor Hojtsy’s picture

@alexpott: do you think you will have some time to explain what you had in mind so we can get feedback on it from people?

alexpott’s picture

Discussed with @Gábor Hojtsy, @xjm, @mtift on a recent CMI call.

This issue introduces a requirement to know how default schema is changing over time. The current proposal stores a copy of the configuration during installation. It stores it in configuration using a special configuration collection. It does this regardless of whether your site is currently using config translation because as some point in the future you might enable it and at that point we need to know the state of the configuration at the point you did the install to know how to possibly translate it. I don't particularly like the current solution because it duplicates everything in active configuration, the install time copy is not really active configuration and it will pollute configuration synchronisation with it's duplicates.

The alternate solutions discussed were:

To never change default configuration once we reach 8.0.0.

This might work for core but it is unlikely to work for contrib which moves at a greater pace and has less controls.

Store previous configuration defaults in the extension

The idea is to store previous versions in the extension's configuration directories. If an extension's default configuration changes then a copy of the old configuration is made. The current folder structure is:

 |-config
 |---install
 |---optional
 |---schema

As updates are made we add the special update directory. Say something changes in the install directory...

 |-config
 |---install
 |---optional
 |---schema
 |---update
 |-----8000

We add a new config storage directory of update and a sub directory of 8000. When this extension was installed before the lack of any configuration update directories in the install or optional directories meant that we stored (in configuration - probably core.extension) that the module was on config version 8000. Now if the extension is stored on a new site we store a config version of 8001.

If we then update something in the optional directory...

 |-config
 |---install
 |---optional
 |---schema
 |---update
 |-----8000
 |-----8001

We a new sub directory of 8001. Optional and mandatory configuration can share an update folder because clashes are impossible. Now if the extension is stored on a new site we store a config version of 8002.

Then if the config translation module is installed it can know the configuration version used at install time and find the original configuration in the extension's update directory.

alexpott’s picture

Issue tags: +D8 upgrade path

This change (whichever way it goes) will have upgrade path implications for currently live sites.

Gábor Hojtsy’s picture

Assigned: alexpott » Unassigned

Unassigning from @alexpott. I'll try to take a first stab at this this week if possible.

Gábor Hojtsy’s picture

Issue tags: -sprint

So I did not get to this due to higher priorities and it seems highly unlikely that I can return to it anytime soon. My employer does not favor me working on things that are not critical to D8's release and I don't have a lot of free time to speak of these days unfortunately :/ Removing from D8MI sprint due to lack of attention on this.

jhodgdon’s picture

This issue is fascinating. So I have this contrib module https://www.drupal.org/project/config_update -- it provides a report that you can run (UI or Drush) that shows differences between the config that is config/install and/or config/optional vs. the active config. It separates out modified, added, and removed config.

Immediately after installing with Standard install, you will see that the report is not empty, because modules/profiles modify config by doing things in code instead of in config files.

So ... I guess my point is: even if the config schema or contents of config/install do not change due to an update, the installed config is not equal to what is in the config files.

Gábor Hojtsy’s picture

@jhodgdon: True. However, for locale, it does not really matter what was the runtime installed config, as it would not be able to translate that anyway (that is not pre-parseable on localize.drupal.org). So for this issue we would only be interested in the original install time raw config, not whatever happened to it while/after being installed.

tkoleary’s picture

Version: 8.0.x-dev » 8.1.x-dev

Drupal 8.0.6 was released on April 6 and is the final bugfix release for the Drupal 8.0.x series. Drupal 8.0.x will not receive any further development aside from security fixes. Drupal 8.1.0-rc1 is now available and sites should prepare to update to 8.1.0.

Bug reports should be targeted against the 8.1.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.2.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.1.x-dev » 8.2.x-dev

Drupal 8.1.9 was released on September 7 and is the final bugfix release for the Drupal 8.1.x series. Drupal 8.1.x will not receive any further development aside from security fixes. Drupal 8.2.0-rc1 is now available and sites should prepare to upgrade to 8.2.0.

Bug reports should be targeted against the 8.2.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.3.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.2.x-dev » 8.3.x-dev

Drupal 8.2.6 was released on February 1, 2017 and is the final full bugfix release for the Drupal 8.2.x series. Drupal 8.2.x will not receive any further development aside from critical and security fixes. Sites should prepare to update to 8.3.0 on April 5, 2017. (Drupal 8.3.0-alpha1 is available for testing.)

Bug reports should be targeted against the 8.3.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.4.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.