Problem/Motivation

Follow-up to #2625258: LocaleConfigManager::updateConfigTranslations() deletes translations if a config object's name happens to match that of a shipped configuration object which adds a hash to configuration files. We need to provide a upgrade path for existing sites.

Proposed resolution

Discussed with @catch and @Gábor Hojtsy. We agreed that the upgrade path should:

  • check all active config and if name does not match - do not add hash
  • If name matches but has custom config translations - do NOT add hash
  • Otherwise add hash.

This way we don't get data loss and most sites will get localisations for default config.

Remaining tasks

User interface changes

None

API changes

None

Data model changes

Existing configuration entities created through the installer have a default_config_hash key set.

Files: 
CommentFileSizeAuthor
#9 2628004-9.patch7.12 KBalexpott

Comments

alexpott created an issue. See original summary.

MustangGB’s picture

Status: Postponed » Active
tstoeckler’s picture

check all active config and if name does not match - do not add hash

Can't we actually hash the config in ExtensionInstallStorage and the config in "ActiveStorage" and only add the hash if the hashes match?

alexpott’s picture

Well means any change to non-translated things will stop you getting the new translations

tstoeckler’s picture

Ahh right. Hmm... but that means that people who have deleted e.g. the frontpage view and have recreated one with the same name, still get the data loss problem, right? Hard tradeoff to make, not necessarily disagreeing with the issue summary, just wanted to pinpoint this.

alexpott’s picture

@tstoeckler yep it is tricky - I'm not sure what the best approach is tbh

alexpott’s picture

The biggest problem is knowing what to hash. We can't hash the active config because it might have changed. And hashing the default config provided by modules / profiles is tricky because whilst I don't think we've changed any default configuration files since 8.0.0 sites might be older than that. And they might change in the future. Also what about contrib? Default config has certainly changed there.

I'm not sure there is any reliable way of working this out. So the question is what would be the impact of doing something like this...

  1. If an active config name matches something provided by a module, profile or theme that is installed THEN
  2. If the config has no custom translations THEN
  3. Write a hash value to the config file BUT don't actually create a hash - use a value like 'pre-8.0.2 default config'

This way we'll know we can't use this information to work out which version of the extension's default config was installed BUT if the config has a translatable value that still matches the default value as determined by locale/config_translation then automatic translation will work because the system will consider it shipped config.

xjm’s picture

Priority: Critical » Major

@alexpott and I discussed this issue more. We have left this issue critical up to now for two reasons:

  1. We deliberately chose to skip our requirement for an update hook for the original critical issue #2625258: LocaleConfigManager::updateConfigTranslations() deletes translations if a config object's name happens to match that of a shipped configuration object, in order to stop the data loss problem as soon as possible.
  2. For those strings affected on sites installed before #2625258: LocaleConfigManager::updateConfigTranslations() deletes translations if a config object's name happens to match that of a shipped configuration object was fixed, the site owner currently has no recourse other than to manually translate those strings in perpetuity.

However, the workaround of manually translating this comparatively small subset of strings is available, and the issue does not meet any of the criteria for a critical issue on its own. Therefore, dowgrading to major.

Per @alexpott, we might also be able to make progress by separating this issue into two parts and handling the simple config case first.

alexpott’s picture

Status: Active » Needs review
FileSize
7.12 KB

Here's a starter - it's not yet working out if the translations are custom config translations. I think we need to look up locale tables to find out if the translation is a default translation or a custom one. Some guidance here would be helpful.

Kristen Pol’s picture

Thanks for the patch. I know you are still working on this and need to sort out the default vs custom logic but I scanned the code for other things which could be addressed when the patch is updated next.

  1. +++ b/core/modules/system/src/Tests/Update/ConfigHashUpdatePathTest.php
    @@ -0,0 +1,41 @@
    +      __DIR__ . '/../../../tests/fixtures/update/drupal-8.bare.standard.php.gz',
    

    Is this the best way to reference this file?

  2. +++ b/core/modules/system/src/Tests/Update/ConfigHashUpdatePathTest.php
    @@ -0,0 +1,41 @@
    +   * Tests that default configuration was updateed with default config hash.
    

    Typo: updateed => updated

  3. +++ b/core/modules/system/src/Tests/Update/ConfigHashUpdatePathTest.php
    @@ -0,0 +1,41 @@
    +    $this->assertIdentical(ConfigManagerInterface::UNKNOWN_DEFAULT_CONFIG_HASH, $this->config('system.site')->get('_core.default_config_hash'));
    +    $this->assertEqual('TestValue', $this->config('system.mail')->get('_core.default_config_hash'));
    +    $this->assertIdentical(NULL, $this->config('core.date_format.fallback')->get('_core.default_config_hash'), 'Default configuration entities have no hash.');
    

    It would be nice to have some comments for this logic similar to what's in the other test.

  4. +++ b/core/modules/system/src/Tests/Update/ConfigHashWithLanguageUpdatePathTest.php
    @@ -0,0 +1,43 @@
    +      __DIR__ . '/../../../tests/fixtures/update/drupal-8.filled.standard.php.gz',
    

    Is this the best way to reference this file?

  5. +++ b/core/modules/system/system.install
    @@ -1842,3 +1845,74 @@ function system_update_8014() {
    + * Update configuration is it is provided by a module or theme.
    

    Unclear wording.

  6. +++ b/core/modules/system/system.install
    @@ -1842,3 +1845,74 @@ function system_update_8014() {
    +
    

    Nitpick: Extra line.

  7. +++ b/core/modules/system/system.install
    @@ -1842,3 +1845,74 @@ function system_update_8014() {
    +    // Config with the hash already can safely be ignored. Configuration which
    

    Nitpick: Configuration => config (or vice versa) for consistency.

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.