Problem/Motivation

Follow-up from #2750167: Config translation synchronization fails

Proposed resolution

Re save all configuration. We can't really know how all config overrides have implemented \Drupal\Core\Config\ConfigFactoryOverrideBase() so the safest thing to do is just to re-save all configuration and let the save event clean up for us.

Remaining tasks

User interface changes

None

API changes

None

Data model changes

None

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

catch created an issue. See original summary.

alexpott’s picture

alexpott’s picture

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

Drupal 8.2.0-beta1 was released on August 3, 2016, which means new developments and disruptive changes should now 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.

alexpott’s picture

Discussed with @xjm, @webchick, @effulgentsia and @Cottser. We agreed to keep this critical because:

  1. We haven't confirmed that we're still not facing the issue with config sync discovered in #2750167: Config translation synchronization fails has been fixed on existing sites
  2. Existing sites which have config in this situation will have expected config change when they do anything that re-saves this config
xjm’s picture

Version: 8.3.x-dev » 8.2.x-dev
alexpott’s picture

Issue summary: View changes
Status: Active » Needs review
FileSize
6.23 KB

Here's the whack it with a hammer upgrade path approach.

xjm’s picture

Issue tags: -beta target

The beta allowed changes include critical issues, so the beta target tag is not applicable.

dawehner’s picture

I agree with alex, its not really worth to try to optimize the update here. In case we miss any case, it will hurt much more, than a little bit more time to run the update.

  1. +++ b/core/modules/system/src/Tests/Update/ConfigOverridesUpdateTest.php
    @@ -0,0 +1,55 @@
    +    $this->assertFalse($config_override->get($key_to_be_removed), 'The spanish override for the missing field has been removed.');
    

    Can we use something like assertNull?

  2. +++ b/core/modules/system/src/Tests/Update/ConfigOverridesUpdateTest.php
    @@ -0,0 +1,55 @@
    +    $this->assertTrue($config_override->isNew() && empty($config_override->get()), 'After updating the system.cron spanish override does not exist.');
    

    Its weird to not have splitted up this into two assertions. It makes debugging harder than it would have to be

alexpott’s picture

Re #9.2 I did that because isNew() should be enough and doing it on separate lines looks weird to me. But so be it... here's the change.

dawehner’s picture

Status: Needs review » Reviewed & tested by the community

Thank you alex!

  • catch committed 1ba97fc on 8.3.x
    Issue #2776357 by alexpott: Upgrade path for Config translation...

  • catch committed de65a87 on 8.2.x
    Issue #2776357 by alexpott: Upgrade path for Config translation...
catch’s picture

Status: Reviewed & tested by the community » Fixed

Committed/pushed to 8.3.x and cherry-picked to 8.2.x. Thanks!

alexpott’s picture

Let's hope no one is doing anything made on config save... mind you if they are config import would probably be broken too.

Gábor Hojtsy’s picture

Issue tags: +D8MI, +language-config

Wow, thanks!

Status: Fixed » Closed (fixed)

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