Problem/Motivation

Configuration system doesn't allow importing a single item from a non-default collection.

Given minimal Drupal installation with config and locale modules installed. Let's add new language (for instance Russian) and download its updates. Then go to admin/config/development/configuration/full/export and get full config archive or just run drush cex sync. In the produced config you will find a folder named language.ru (ru stands for Russian language in this example) where translated configs are placed.

Now try to import any single item back to active storage from this page admin/config/development/configuration/single/import or using drush cim sync --partial, ensure some config item(s) are modified for the default collection.
I will use this one as example:

system.maintenance.yml

message: '@site is currently under maintenance. We should be back shortly. Thank you for your patience!'
langcode: en

The following errors occur before confirmation step:

Notice: Undefined index: language.ru in Drupal\config\StorageReplaceDataWrapper->listAll() (line 131 of core/modules/config/src/StorageReplaceDataWrapper.php).

Warning: array_keys() expects parameter 1 to be array, null given in Drupal\config\StorageReplaceDataWrapper->listAll() (line 131 of core/modules/config/src/StorageReplaceDataWrapper.php).

After submitting the form - nothing changes in default collection and all items in the language.ru collection are broken (they could be empty or contain data from the default collection). It can be verified by exporting full config once again and compare with the original one.

This issue makes absolutely impossible to rely on config import/export system.

Steps to reproduce

Scenario 1.

  1. Install the latest Drupal 8.x using the standard profile.
  2. Add additional language at admin/config/regional/language/add.
  3. Export a single configuration item (eg. system.maintenance) at admin/config/development/configuration/single/export.
  4. Copy and paste item's content at admin/config/development/configuration/single/import
    Configuration type - Simple configuration;
    Configuration name - system.maintenance;
    Change config's message property somehow and click Import button.
  5. Confirm form submission.

Expected results: Changes are saved w/o errors.
Actual results: Changes not saved, errors displayed, all language.ru configs filled with data from default language settings:

Error message

Notice: Undefined index: language.ru in Drupal\config\StorageReplaceDataWrapper->listAll() (line 131 of core/modules/config/src/StorageReplaceDataWrapper.php).
Warning: array_keys() expects parameter 1 to be array, null given in Drupal\config\StorageReplaceDataWrapper->listAll() (line 131 of core/modules/config/src/StorageReplaceDataWrapper.php).
Notice: Undefined index: language.ru in Drupal\config\StorageReplaceDataWrapper->listAll() (line 131 of core/modules/config/src/StorageReplaceDataWrapper.php).
Warning: array_keys() expects parameter 1 to be array, null given in Drupal\config\StorageReplaceDataWrapper->listAll() (line 131 of core/modules/config/src/StorageReplaceDataWrapper.php).
Notice: Undefined index: language.ru in Drupal\config\StorageReplaceDataWrapper->listAll() (line 131 of core/modules/config/src/StorageReplaceDataWrapper.php).
Warning: array_keys() expects parameter 1 to be array, null given in Drupal\config\StorageReplaceDataWrapper->listAll() (line 131 of core/modules/config/src/StorageReplaceDataWrapper.php).

Scenario 2.

  1. Install the latest Drupal 8.x with minimal profile.
  2. Enable config and locale modules (drush en config locale)
  3. Add additional language at admin/config/regional/language/add.
  4. Run drush config-export sync to export current configs.
  5. Go to exported folder and make sure translations exist and have localized strings.
  6. Change any item's text (Eg. in the system.maintenance.yml)
  7. Run drush config-import sync --partial to update active config.
  8. Run drush config-export sync to export configs again and see the differences.

Expected results: The active configuration is identical to the configuration in the export directory.
Actual results: Changes to default collection not saved, non-default collection items overriden by data from default collection corresponding items, errors displayed:

$ drush config-import sync --partial
Collection Config Operation
system.maintenance update
Import the listed configuration changes? (y/n): y
array_keys() expects parameter 1 to be array, null given StorageReplaceDataWrapper.php:131 [warning]
array_keys() expects parameter 1 to be array, null given StorageReplaceDataWrapper.php:131 [warning]
array_keys() expects parameter 1 to be array, null given StorageReplaceDataWrapper.php:131 [warning]
array_keys() expects parameter 1 to be array, null given StorageReplaceDataWrapper.php:131 [warning]
array_keys() expects parameter 1 to be array, null given StorageReplaceDataWrapper.php:131 [warning]
The configuration was imported successfully. [success]

Scenario 3. (Thanks to @Arla for idea given in #7)

  1. Install the latest Drupal 8.x with standard profile.
  2. Add additional language at admin/config/regional/language/add.
  3. Run drush config-edit system.maintenance, edit and save changes.

Expected results: Changes are saved w/o errors.
Actual results: Changes not saved and errors displayed:

$ drush config-edit system.maintenance

Collection Config Operation
system.maintenance update
Import the listed configuration changes? (y/n): y
array_keys() expects parameter 1 to be array, null given StorageReplaceDataWrapper.php:131 [warning]
array_keys() expects parameter 1 to be array, null given StorageReplaceDataWrapper.php:131 [warning]
array_keys() expects parameter 1 to be array, null given StorageReplaceDataWrapper.php:131 [warning]
array_keys() expects parameter 1 to be array, null given StorageReplaceDataWrapper.php:131 [warning]
array_keys() expects parameter 1 to be array, null given StorageReplaceDataWrapper.php:131 [warning]
The configuration was imported successfully. [success]

Note: If you get errors like this (Not reproducable prior to core 8.3.x):

Drupal\Core\Config\ConfigImporterException: There were errors validating the config synchronization. in [error]
Drupal\Core\Config\ConfigImporter->validate() (line 728 of core/lib/Drupal/Core/Config/ConfigImporter.php).
The import failed due for the following reasons: [error]
Site UUID in source storage does not match the target storage.

You should sync system.site uuid between collections using sql query or shell command:

	$ head -n1 PATH_TO_SYNC/system.site.yml >> PATH_TO_SYNC/language/YOUR_LANGUAGE/system.site.yml 
	$ drush config-import sync -y

Proposed resolution

Fix createCollection method of StorageReplaceDataWrapper class to valid object with correct collection.

Remaining tasks

Find out a way to allow importing of the config items from any collection using UI and drush cim.

User interface changes

API changes

Data model changes

Comments

VitalyM created an issue. See original summary.

szeidler’s picture

I ran into the exact same issue (with other languages). It pretended to import the configuration successfully, but nothing changed for me. Even missing configuration dependencies were not checked before starting the configuration import batch.

I can verify that the provided patch solved the problem.

VitalyM’s picture

It pretended to import the configuration successfully, but nothing changed for me.

Yep. this is the first symptom I had suffered from too.

gaele’s picture

Status: Active » Needs review
cilefen’s picture

Title: Configuration system doesn't allow to import single item from non-default collection » Configuration system doesn't allow importing a single item from a non-default collection
Issue summary: View changes
hideaway’s picture

That fixes the warnings, that's true, but I still have another problem. We are working on multilingual site and we are updating the configuration with "drush cim --source=/path/to/conf --partial". It works ok for the conf in default (english) language. For example, when I change view's header and the run "drush cim --source=/path/to/conf --partial", the view gets updated data. But if I change anything in slovak version under language/sk/view.view.my-view.yml, it never gets updated. Anybody has the same issues? We are trying to use similar workflow as we had with D7 apps with "drush fra", but somehow we are currently unable to update translated configuration, at least not with drush cim ..

Arla’s picture

I get the array_keys() warning when using `drush cedit` on a language-enabled site. The patch fixes it.

I don't know about @hideaway's issue though. Could it be a different issue?

Berdir’s picture

Issue tags: +Needs tests

Yes, looks like this doesn't actually need to be a non-default collection, apparently a site that *has* non-default collections is enough?

Kgaut’s picture

I can confirm that this patch fix the problem for me

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.

VitalyM’s picture

Found an easy way to test the issue by enabling language module in existing ConfigSingleImportExportTest class. Once enabled it makes some tests to fail until fixed with provided patch.

With testonly patch applied:

31 passes, 2 fails, 10 exceptions, 11 debug messages
Drupal\config\Tests\ConfigSingleImportExportTest

Also ended with the following error:

Fatal error: Uncaught Error: Call to a member function label() on null in core/modules/config/src/Tests/ConfigSingleImportExportTest.php:73

With full patch applied:

42 passes, 0 fails, 0 exceptions, 48 debug messages
Drupal\config\Tests\ConfigSingleImportExportTest

cilefen’s picture

This just went from Major to Critical with no explanation as to why. It may, in fact, be critical, but it would be good to know the reason.

VitalyM’s picture

#12, The issue description says:

After submitting the form - nothing changes in default collection and all items in the language.ru collection are broken (they could be empty or contain data from the default collection).

This sounds like "Cause loss/corruption of stored data." point from Priority levels of issues, so I considered it's Critical.

Status: Needs review » Needs work

The last submitted patch, 11: configuration_system-2740983-11-testonly.patch, failed testing.

alexpott’s picture

Discussed with @xjm, @effulgentsia, @Cottser and @webchick. We agreed to keep this critical since the issue summary reports

After submitting the form - nothing changes in default collection and all items in the language.ru collection are broken (they could be empty or contain data from the default collection).

It would be great if @VitalyM can detail the two different paths steps to reproduce. Using drush config-import and the single import (admin/config/development/configuration/single/import) are quite different. If these steps cna be detailed that'd be excellent.

The patch looks sensible.

VitalyM’s picture

Issue summary: View changes
Status: Needs work » Needs review
Issue tags: -Needs tests, -Needs issue summary update

Updated summary with new steps to reproduce removing tags for completed tasks. Switching back to needs-review cause failed patch fails intentionally to demonstrate the problem.

alexpott’s picture

Status: Needs review » Needs work

@VitalyM thanks for the nice issue summary update! I'm relieved that the drush command requires the --partial option. To be honest that option should not exist partial configuration imports are really complex beyond limiting it to a single piece of configuration. Dependencies are a nightmare.

@@ -20,7 +20,8 @@ class ConfigSingleImportExportTest extends WebTestBase {
+    'language',

We need something more in the test to show that enabling language matters. Otherwise someone could come along and remove this and we'd have no test coverage.

Perhaps a way around this would be to add a unit test to test \Drupal\config\StorageReplaceDataWrapper

alexpott’s picture

The new unit test for \Drupal\config\StorageReplaceDataWrapper could just test the changed method and add a @todo with a link to a followup issue to flesh out the testing.

VitalyM’s picture

Added test for StorageReplaceDataWrapper class based on ConfigStorageTestBase that helped to catch and fix one more issue in StorageReplaceDataWrapper::rename method. The method causes infinite loops during rename operation and fails with PHPUnit_Framework_Exception: Segmentation fault in Drupal\KernelTests\Core\Config\Storage\StorageReplaceDataWrapperTest::testCRUD test. The fix added to this patch.

VitalyM’s picture

Status: Needs work » Needs review

Status: Needs review » Needs work

The last submitted patch, 19: configuration_system-2740983-19-testonly.patch, failed testing.

alexpott’s picture

  1. +++ b/core/tests/Drupal/KernelTests/Core/Config/Storage/StorageReplaceDataWrapperTest.php
    @@ -0,0 +1,95 @@
    +class StorageReplaceDataWrapperTest extends ConfigStorageTestBase {
    

    Nice idea to use ConfigStorageTestBase - full test coverage for free :)

  2. +++ b/core/tests/Drupal/KernelTests/Core/Config/Storage/StorageReplaceDataWrapperTest.php
    @@ -0,0 +1,95 @@
    +    $expected_collection = $this->storage->getCollectionName();
    

    Let's call this $initial_collection_name

  3. +++ b/core/tests/Drupal/KernelTests/Core/Config/Storage/StorageReplaceDataWrapperTest.php
    @@ -0,0 +1,95 @@
    +    $new_collection = $new_storage->getCollectionName();
    +    $this->assertSame($collection, $new_collection);
    

    Let's combine these lines to be:
    $this->assertSame($collection, $new_storage->getCollectionName());

  4. +++ b/core/tests/Drupal/KernelTests/Core/Config/Storage/StorageReplaceDataWrapperTest.php
    @@ -0,0 +1,95 @@
    +    $actual_collection = $this->storage->getCollectionName();
    +    $this->assertSame($expected_collection, $actual_collection);
    

    And combine these lines into
    $this->assertSame($initial_collection_name, $this->storage->getCollectionName());

  5. +++ b/core/modules/config/src/StorageReplaceDataWrapper.php
    @@ -164,8 +165,10 @@ public function deleteAll($prefix = '') {
    -    $this->collection = $collection;
    -    return $this->storage->createCollection($collection);
    +    return new static(
    +      $this->storage->createCollection($collection),
    +      $collection
    +    );
    

    Who know what I was thinking when I wrote the original code... the fix looks great and makes perfect sense. Not we don't have to worry about setting replacement data on the collection because we don't (yet) support single config imports of translations. This fix opens up the possibility.

  6. +++ b/core/modules/config/src/Tests/ConfigSingleImportExportTest.php
    @@ -20,7 +20,8 @@ class ConfigSingleImportExportTest extends WebTestBase {
    +    'language',
    

    Still need a comment to explain why this is here.

alexpott’s picture

@VitalyM points 2,3,4 in #22 are just about making it easier to understand what is being tested. Less variables and better named variables will make it much simpler.

VitalyM’s picture

@alexpott thanks for review. I totally agree with the points, so here is improved patch.

alexpott’s picture

Version: 8.3.x-dev » 8.2.x-dev
Status: Needs review » Reviewed & tested by the community

Thanks for addressing my feedback. Nice work @VitalyM!

This patch fixes the bug described in the issue summary and provides good test coverage. I think this can go into the 8.2.x branch because the current behaviour is wrong and we still return a StorageInterface object - therefore there's no API change here.

  • catch committed 31dcaca on 8.3.x
    Issue #2740983 by VitalyM, alexpott: Configuration system doesn't allow...

  • catch committed 462c719 on 8.2.x
    Issue #2740983 by VitalyM, alexpott: Configuration system doesn't allow...
catch’s picture

Status: Reviewed & tested by the community » Fixed

+++ b/core/modules/config/src/Tests/ConfigSingleImportExportTest.php
@@ -20,7 +20,10 @@ class ConfigSingleImportExportTest extends WebTestBase {
+ 'config_test',
+ // Adding language module makes possible to involve non-default
+ // (language.xx) collections in import/export operations.
+ 'language',
];

protected function setUp() {

makes it possible - fixed on commit.

Committed/pushed to 8.3.x and 8.2.x, thanks!

Berdir’s picture

Version: 8.2.x-dev » 8.1.x-dev
Status: Fixed » Reviewed & tested by the community

I think we should consider adding this to 8.1.x too unless there is a reason against that. This can result in possible data loss and is very annoying while developing sites as it completely breaks the ability to update single configs or edit them with drush cedit.

There will still be at least one 8.1.* release AFAIK until 8.2.

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 24: configuration_system-2740983-24.patch, failed testing.

Berdir’s picture

Status: Needs work » Reviewed & tested by the community

Trying a custom re-test against 8.1.x.

VitalyM’s picture

8.3.x: PHP 5.5 & MySQL 5.5 Patch failed to apply

Just keep in mind that patch is not apply-able to 8.3.x because of changes makes it possible - fixed on commit. made by @catch in the commits. But it should work with 8.1.x.

catch’s picture

Status: Reviewed & tested by the community » Fixed

OK, cherry-picked to 8.1.x as well.

  • catch committed d04a8de on 8.1.x
    Issue #2740983 by VitalyM, alexpott: Configuration system doesn't allow...

Status: Fixed » Closed (fixed)

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