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.
- Install the latest Drupal 8.x using the standard profile.
- Add additional language at
admin/config/regional/language/add
. - Export a single configuration item (eg. system.maintenance) at
admin/config/development/configuration/single/export
. - 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. - 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.
- Install the latest Drupal 8.x with minimal profile.
- Enable config and locale modules (
drush en config locale
) - Add additional language at
admin/config/regional/language/add
. - Run
drush config-export sync
to export current configs. - Go to exported folder and make sure translations exist and have localized strings.
- Change any item's text (Eg. in the system.maintenance.yml)
- Run
drush config-import sync --partial
to update active config. - 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)
- Install the latest Drupal 8.x with standard profile.
- Add additional language at
admin/config/regional/language/add
. - 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
Comment | File | Size | Author |
---|---|---|---|
#24 | interdiff-2740983-19-24.txt | 1.78 KB | VitalyM |
#24 | configuration_system-2740983-24.patch | 4.52 KB | VitalyM |
#19 | configuration_system-2740983-19-testonly.patch | 3.18 KB | VitalyM |
#19 | configuration_system-2740983-19.patch | 4.46 KB | VitalyM |
Comments
Comment #2
szeidler CreditAttribution: szeidler at Ramsalt Lab commentedI 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.
Comment #3
VitalyM CreditAttribution: VitalyM at FFW commentedYep. this is the first symptom I had suffered from too.
Comment #4
gaele CreditAttribution: gaele commentedComment #5
cilefen CreditAttribution: cilefen commented"Allow" cannot be followed immediately by an infinitive.
Comment #6
hideaway CreditAttribution: hideaway commentedThat 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 ..
Comment #7
ArlaI 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?
Comment #8
BerdirYes, looks like this doesn't actually need to be a non-default collection, apparently a site that *has* non-default collections is enough?
Comment #9
Kgaut CreditAttribution: Kgaut as a volunteer commentedI can confirm that this patch fix the problem for me
Comment #11
VitalyM CreditAttribution: VitalyM at FFW commentedFound 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:
Also ended with the following error:
With full patch applied:
Comment #12
cilefen CreditAttribution: cilefen commentedThis 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.
Comment #13
VitalyM CreditAttribution: VitalyM at FFW commented#12, The issue description says:
This sounds like "Cause loss/corruption of stored data." point from Priority levels of issues, so I considered it's Critical.
Comment #15
alexpottDiscussed with @xjm, @effulgentsia, @Cottser and @webchick. We agreed to keep this critical since the issue summary reports
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.
Comment #16
VitalyM CreditAttribution: VitalyM at FFW commentedUpdated 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.
Comment #17
alexpott@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.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
Comment #18
alexpottThe 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.Comment #19
VitalyM CreditAttribution: VitalyM at FFW commentedAdded test for
StorageReplaceDataWrapper
class based onConfigStorageTestBase
that helped to catch and fix one more issue inStorageReplaceDataWrapper::rename
method. The method causes infinite loops during rename operation and fails withPHPUnit_Framework_Exception: Segmentation fault
inDrupal\KernelTests\Core\Config\Storage\StorageReplaceDataWrapperTest::testCRUD
test. The fix added to this patch.Comment #20
VitalyM CreditAttribution: VitalyM at FFW commentedComment #22
alexpottNice idea to use ConfigStorageTestBase - full test coverage for free :)
Let's call this $initial_collection_name
Let's combine these lines to be:
$this->assertSame($collection, $new_storage->getCollectionName());
And combine these lines into
$this->assertSame($initial_collection_name, $this->storage->getCollectionName());
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.
Still need a comment to explain why this is here.
Comment #23
alexpott@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.
Comment #24
VitalyM CreditAttribution: VitalyM at FFW commented@alexpott thanks for review. I totally agree with the points, so here is improved patch.
Comment #25
alexpottThanks 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.
Comment #28
catch+++ 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!
Comment #29
BerdirI 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.
Comment #31
BerdirTrying a custom re-test against 8.1.x.
Comment #32
VitalyM CreditAttribution: VitalyM at FFW commentedJust 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.Comment #33
catchOK, cherry-picked to 8.1.x as well.
Comment #36
jigariusI'm not sure if I'm facing the same issue, but if you think it's related, it'd be good to have an opinion or some suggestions.
* Set up a working D9 site with English as the main language and Spanish as the second language.
* Configure a block or a view in 2 languages and export the config.
* Now, create a new directory with just the following files:
** views.view.blog.yml - modify the file and set display.default.display_options.title to "Hello"
** language/es/views.view.blog.yml - modify the file and set display.default.display_options.title to "Hola"
* Now, use drush config:import --partial --source=path/to/directory
* You'll notice that the config is updated in the default language, but not in the second language.
Is this the expected behavior? Is it normal for --partial import to ignore the language/es directory? Is there a workaround?
Comment #37
HLopes CreditAttribution: HLopes commentedAlso seeing #36, cim --partial ignores language config
Comment #38
noelia_11 CreditAttribution: noelia_11 commentedHere I am facing the same issue as #36, cim --partial ignores language config. Using 9.2.10 version.
Could anyone give us a workaround?
Comment #39
kburakozdemir CreditAttribution: kburakozdemir as a volunteer and at binbiriz commentedHere I am facing the same issue as #36, cim --partial ignores language config. Using 9.5.1 version.
Comment #40
leymannxVoting to reopen. Or do we create a new issue instead?
Comment #41
DavorHorvacki CreditAttribution: DavorHorvacki at Studio Present commentedFacing same Issue in 10.1.4
Yes, I guess someone should reopen issue