Problem/Motivation

ConfigInstaller loops over all optional config of all enabled modules and removes everything that has missing dependencies or does not depend on the module being installed.

It checks the config dependencies against both the already installed config and other optional configuration. That means it might have all dependencies for a certain config entity but those dependencies might themself not end up being installed because they are missing some of their dependencies.

So far, I've only been able to reproduce this with my kernel tests that install a large number of modules and then install configuration for lots of them.

What happens is this:

1. I have an entity view display for a node type that depends on the user module explicitly and I have that node type, both as optional config.
2. I'm installing lots of modules, including node, user and the module that provides this optional config.
3. Then I'm installing config, starting with user (No specific reason other than that user is a required module so that seems to make sense)
4. First it installs default config, then optional config of that module and then optional config of other modules that depend on user.
5. the $list of config to possibly create now includes my entity view display and the node type. It loops through that list and the entity view display config depends on that node type config, which is in the list and user, so we keep it.
6. The we get to that node type config, which could be installed but does not depend on user, so we don't.

The resulting list of optional config to install now contains the entity view display but not the node type. And trying to save that throws an exception.

I don't know if this is reproducible outside of kernel tests too. In my case, I was able to work around this by changing the order of modules to install config for (install node config first, then user).

Proposed resolution

Check dependencies in reverse dependency order, so that config objects are checked before those that depend on them.

Remaining tasks

User interface changes

API changes

Data model changes

CommentFileSizeAuthor
#7 2623940-7.patch5.19 KBalexpott
#7 4-7-interdiff.txt4.52 KBalexpott
#4 2623940-3.patch4.37 KBalexpott
#4 2623940.3.test-only.patch2.47 KBalexpott
#2 config-installer-2623940-2.patch2.31 KBberdir

Comments

Berdir created an issue. See original summary.

berdir’s picture

berdir
Primary language German
Location Switzerland
commented
Status: Active » Needs review
StatusFileSize
new config-installer-2623940-2.patch2.31 KB

Haven't tested it yet, but I think this change would be sufficient to solve my specific scenario.

There are other possible cases where config dependencies don't work, @alexpott is working on a patch that might fix some or all of those as well.

Posting this in case the other idea doesn't work out.

Status: Needs review » Needs work

The last submitted patch, 2: config-installer-2623940-2.patch, failed testing.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented
Status: Needs work » Needs review
StatusFileSize
new 2623940.3.test-only.patch2.47 KB
new 2623940-3.patch4.37 KB

Here's a patch with a slightly different approach and a test

The last submitted patch, 4: 2623940.3.test-only.patch, failed testing.

berdir’s picture

berdir
Primary language German
Location Switzerland
commented
Status: Needs review » Needs work 
  1. +++ b/core/lib/Drupal/Core/Config/ConfigInstaller.php
@@ -189,16 +189,24 @@ public function installOptionalConfig(StorageInterface $storage = NULL, $depende
+    // Sort the config_to_create is the order of the least dependent first.
+    $dependency_manager = new ConfigDependencyManager();

    Did you mean to write *in* the order... ? 

  2. +++ b/core/lib/Drupal/Core/Config/ConfigInstaller.php
@@ -189,16 +189,24 @@ public function installOptionalConfig(StorageInterface $storage = NULL, $depende
       // Exclude configuration where its dependencies cannot be met.
       if (!$this->validateDependencies($config_name, $data, $enabled_extensions, $all_config)) {
         unset($config_to_create[$config_name]);
+        unset($all_config[$config_name]);

    Can we explain that unset with a comment somewhere? 

  3. +++ b/core/modules/config/src/Tests/ConfigOtherModuleTest.php
@@ -64,9 +64,11 @@ public function testInstallOtherModuleFirst() {
     $this->installModule('config_install_dependency_test');
-    $this->assertTrue(entity_load('config_test', 'other_module_test_unmet', TRUE), 'The optional configuration whose dependencies are met is now created.');
+    $this->assertTrue(entity_load('config_test', 'other_module_test_unmet', TRUE), 'The optional configuration config_test.dynamic.other_module_test_unmet whose dependencies are met is now created.');
+    $this->assertNull(entity_load('config_test', 'other_module_test_optional_entity_unmet', TRUE), 'The optional configuration config_test.dynamic.other_module_test_optional_entity_unmet whose dependencies are met is not created.');

    the descriptions should say "are not met" in the assertNull lines here.

    Additionally, it would be good to have a comment here that explains that the second case is an.. unexpected behavior, but is something that is not yet supported.

    This issue can't also fix that, that's OK, we just fix the exceptions that can happen right now in this scenario.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented
Status: Needs work » Needs review
StatusFileSize
new 4-7-interdiff.txt4.52 KB
new 2623940-7.patch5.19 KB

Thanks @Berdir - patch addresses the issues you've raised.

berdir’s picture

berdir
Primary language German
Location Switzerland
commented
Status: Needs review » Reviewed & tested by the community

Forgot about this, I think this is still good to go.

  • catch committed cd068ef on 8.2.x 
    Issue #2623940 by alexpott, Berdir: ConfigInstaller tries to install...

  • catch committed fd0012d on 8.1.x 
    Issue #2623940 by alexpott, Berdir: ConfigInstaller tries to install...

  • catch committed 08b5419 on 8.0.x 
    Issue #2623940 by alexpott, Berdir: ConfigInstaller tries to install...
catch’s picture

catch
he/him
Primary language English
commented
Status: Reviewed & tested by the community » Fixed

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

Status: Fixed » Closed (fixed)

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