Problem/Motivation

I was fighting a strange problem that config schema validation failed due to a string/integer mismatch when saving config in a form.

Turned out that it was a classic Heisenbug, as it only existed because I had a debug($config->get()) call before calling save.

And what happens then is that overriddenData is initialized. Then you save, it casts all the data and saves it correctly. Then comes \Drupal\Core\Config\Development\ConfigSchemaChecker::onConfigSave() and calls $saved_config->get()... and receives the old, overridden data and throws an exception.

Proposed resolution

Move $this->resetOverriddenData(); up, before actually writing the config and invoking the event.

Remaining tasks

User interface changes

API changes

Data model changes

CommentFileSizeAuthor
#3 config-save-overridden-reset-2841651-3.patch1.97 KBBerdir
#3 config-save-overridden-reset-2841651-3-test-only.patch1.04 KBBerdir
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Berdir created an issue. See original summary.

Berdir’s picture

Berdir
Primary language German
Location Switzerland
CreditAttribution: Berdir at MD Systems GmbH commented
Issue summary: View changes
Berdir’s picture

Berdir
Primary language German
Location Switzerland
CreditAttribution: Berdir at MD Systems GmbH commented
Status: Active » Needs review
FileSize
config-save-overridden-reset-2841651-3-test-only.patch1.04 KB
config-save-overridden-reset-2841651-3.patch1.97 KB

Failing test and a fix.

The last submitted patch, 3: config-save-overridden-reset-2841651-3-test-only.patch, failed testing.

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

Drupal 8.3.0-alpha1 will be released the week of January 30, 2017, which means new developments and disruptive changes should now 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.

Version: 8.4.x-dev » 8.5.x-dev

Drupal 8.4.0-alpha1 will be released the week of July 31, 2017, which means new developments and disruptive changes should now be targeted against the 8.5.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.5.x-dev » 8.6.x-dev

Drupal 8.5.0-alpha1 will be released the week of January 17, 2018, which means new developments and disruptive changes should now be targeted against the 8.6.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

borisson_’s picture

borisson_
Primary language Dutch
Location Mechelen, 🇧🇪
CreditAttribution: borisson_ as a volunteer commented

This makes a lot of sense and the patch still applies. It has sufficient testcoverage but it would be helpful to get a review from someone knowledgeable about CMI.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott at Acro Commerce, Thunder commented
Status: Needs review » Reviewed & tested by the community

Yep this makes sense. This won't break \Drupal\Core\Config\ConfigCrudEvent::isChanged() and that's what you should be using to determine things have changed in the \Drupal\Core\Config\ConfigEvents::SAVE event.

catch’s picture

catch
he/him
Primary language English
CreditAttribution: catch at Third and Grove commented

#2806009: Installing a module causes translations to be overwritten is a critical bug about config overrides, haven't tested yet, but this issue or something similar looks like it could be related.

  • catch committed ba12307 on 8.6.x 
    Issue #2841651 by Berdir: Config save resets overridden configuration...

  • catch committed 54acb54 on 8.5.x 
    Issue #2841651 by Berdir: Config save resets overridden configuration...
catch’s picture

catch
he/him
Primary language English
CreditAttribution: catch at Third and Grove commented
Version: 8.6.x-dev » 8.5.x-dev
Status: Reviewed & tested by the community » Fixed

Committed/pushed to 8.6.x and cherry-picked to 8.5.x. Thanks!

Status: Fixed » Closed (fixed)

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