Create an upgrade path to determine if default_config_hash should be added (2625258)

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?

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

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.

Patch no longer applies and needs a reroll.

Re-rolled against 9.1.x. Kindly review a patch.

Solving failed test cases , kindly review.

Forgot to change status , moving to needs review.

Thanks for the update. I have reviewed for formatting and general issues but not for overall logic and found a few things.

1. +++ b/core/lib/Drupal/Core/Config/ConfigManagerInterface.php
   @@ -7,6 +7,11 @@
   +   * @todo
   

   Needs comment.

2. +++ b/core/modules/system/src/Tests/Update/ConfigHashUpdatePathTest.php
   @@ -0,0 +1,41 @@
   +/**
   + * @file
   + * Contains \Drupal\system\Tests\Update\ConfigHashUpdatePathTest.
   

   Needs description.

3. +++ b/core/modules/system/src/Tests/Update/ConfigHashUpdatePathTest.php
   @@ -0,0 +1,41 @@
   +namespace Drupal\system\Tests\Update;
   +use Drupal\Core\Config\ConfigManagerInterface;
   

   Nitpick: Add empty line between namespace and use lines.

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

   Needs to be updated to drupal-8.8.0.bare.standard.php.gz.

5. +++ b/core/modules/system/src/Tests/Update/ConfigHashWithLanguageUpdatePathTest.php
   @@ -0,0 +1,43 @@
   +/**
   + * @file
   + * Contains \Drupal\system\Tests\Update\ConfigHashWithLanguageUpdatePathTest.
   + */
   

   Needs description.

6. +++ b/core/modules/system/src/Tests/Update/ConfigHashWithLanguageUpdatePathTest.php
   @@ -0,0 +1,43 @@
   +namespace Drupal\system\Tests\Update;
   +use Drupal\Core\Config\ConfigManagerInterface;
   

   Nitpick: Add empty line between namespace and use lines.

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

   Needs to be updated to drupal-8.8.0.filled.standard.php.gz.

8. +++ b/core/modules/system/src/Tests/Update/ConfigHashWithLanguageUpdatePathTest.php
   @@ -0,0 +1,43 @@
   +    // The system.mail has no translations therefore it should have the fake
   +    // hash.
   

   Nitpick: Change to get to one line, e.g.

   The system.mail has no translations so it should use the fake hash.

9. +++ b/core/modules/system/system.install
   @@ -9,6 +9,9 @@
    use Drupal\Component\Utility\Crypt;
    use Drupal\Component\Utility\Environment;
   +use Drupal\Core\Config\ExtensionInstallStorage;
   +use Drupal\Core\Config\ConfigManagerInterface;
   +use Drupal\language\ConfigurableLanguageManagerInterface;
    use Drupal\Component\Utility\OpCodeCache;
   

   Nitpick: Not alphabetical.

10. +++ b/core/modules/system/system.install
    @@ -1458,3 +1461,77 @@ function system_update_8901() {
    +
    +
    +
    +/**
    + * @addtogroup updates-9.1.0
    

    Nitpick: Extra empty lines.

11. +++ b/core/modules/system/system.install
    @@ -1458,3 +1461,77 @@ function system_update_8901() {
    +/**
    + * @addtogroup updates-9.1.0
    + * @{
    + */
    

    Needs description.

12. +++ b/core/modules/system/system.install
    @@ -1458,3 +1461,77 @@ function system_update_8901() {
    +  $optional_storage = new ExtensionInstallStorage($config_storage, ExtensionInstallStorage::CONFIG_OPTIONAL_DIRECTORY, ExtensionInstallStorage::DEFAULT_COLLECTION, TRUE, $install_profile);
    +
    +
    +  if (empty($sandbox['configs'])) {
    

    Nitpick: Extra empty line.

13. +++ b/core/modules/system/system.install
    @@ -1458,3 +1461,77 @@ function system_update_8901() {
    +      // @todo configuration entities.
    

    Update to be a sentence.
    

14. See #10 for more items.

Tests are passing now and tests were added to the patch.

The Needs Review Queue Bot tested this issue. It either no longer applies to Drupal core, or fails the Drupal core commit checks. Therefore, this issue status is now "Needs work".

Apart from a re-roll or rebase, this issue may need more work to address feedback in the issue or MR comments. To progress an issue, incorporate this feedback as part of the process of updating the issue. This helps other contributors to know what is outstanding.

Consult the Drupal Contributor Guide to find step-by-step guides for working with issues.