Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.This is a follow-up to #2655104: List unmet configuration dependencies instead of just failing. That altered the loop in ConfigInstaller::getMissingDependencies():
foreach ($all_dependencies as $type => $dependencies) {
$list_to_check = [];
switch ($type) {
case 'module':
case 'theme':
$list_to_check = $enabled_extensions;
break;
case 'config':
$list_to_check = $all_config;
break;
}
if (!empty($list_to_check)) {
return array_diff($dependencies, $list_to_check);
}
}
So it returns early immediately after the first iteration of the loop -- therefore, if the first type of dependency is satisfied, the others will not be checked. I expect this is why Berdir has seen an issue at #2817923: Demo tests are falling on HEAD that he posted about in the core issue (comment 61).
The method should not return early, it should build a complete list of all types of missing dependencies and only then return. This would also ensure that all types of dependency are checked.
| Comment | File | Size | Author |
|---|---|---|---|
| #9 | interdiff_6-9.txt | 1.21 KB | jofitz |
| #9 | check_all_config_dependency_types-2831816-9-complete.patch | 6.29 KB | jofitz |
| #9 | check_all_config_dependency_types-2831816-9-test_only.patch | 5.22 KB | jofitz |
Comments
Comment #2
james.williams CreditAttribution: james.williams at ComputerMinds commentedHere's a first attempt at a patch, I realise there are no tests, which would be needed, I presume?
Comment #3
james.williams CreditAttribution: james.williams at ComputerMinds commentedJust prompting a test run to make sure this hasn't broken anything else unexpected before we get too far down the line of writing a test for this actual case...
Comment #4
james.williams CreditAttribution: james.williams at ComputerMinds commented(and back to needs work as we need to write a test...)
Comment #5
jofitz CreditAttribution: jofitz at ComputerMinds commentedComment #6
jofitz CreditAttribution: jofitz at ComputerMinds commentedAdded tests for the issue.
Comment #9
jofitz CreditAttribution: jofitz at ComputerMinds commentedUpdate failing test after new dependency added.
Comment #13
jofitz CreditAttribution: jofitz at ComputerMinds commentedTest-bot error - setting back to "Needs review".
Comment #14
james.williams CreditAttribution: james.williams at ComputerMinds commentedThanks Jo! So, just to clarify what's going on:
1) A bug exists that means not all dependencies are checked.
2) check_all_config_dependency_types-2831816-9-test_only.patch contains a test showing that core currently has this bug (hence it failing). The test tries to install a module that contains config with a dependency that will not be met ... but installation succeeds - which it shouldn't. (And then the missing dependency is installed in the test and the subsequent assertion passes, showing that was indeed the problem.)
3) check_all_config_dependency_types-2831816-9-complete.patch contains that test, and the fix for the bug. The assertions in the test pass, showing that the installation initially fails due to the missing dependency (which is the correct behaviour), and then once the missing dependency has been installed, installation succeeds (correctly).
The other noise that went on here were some testbot struggles! (An unrelated test initially failed.)
Comment #15
alexpottNice find - this is a major bug in the config system.
Comment #16
alexpottNice test coverage. The code looks perfect. I thought about
$missing = array_merge($missing, array_diff($dependencies, $list_to_check));for sometime and pondered if this can cause duplicates - but it can't because config object names and modules and themes can't have overlaps.Comment #17
mkalkbrennerI just want to confirm that this patch solves a critical installation issue for search_api_solr_multilingual on drupal 8.3.x!
Comment #18
james.williams CreditAttribution: james.williams at ComputerMinds commentedI don't think the 8.2.x branch has this issue, I believe it was only introduced after #2655104: List unmet configuration dependencies instead of just failing? Before that change, the code only returned early on finding a missing dependency (and as it didn't report what was missing, that was sufficient), so it did check all types of dependency before returning when all were satisfied.
Comment #19
mkalkbrennerI confirm that.
Comment #20
alexpottNice to know we only broke this in 8.3.x :) - not so critical. But actually thinking about it if the installation can lead to configuration being created with unmet dependencies then this is a critical issue for 8.3.x.
Comment #21
catchCommitted/pushed to 8.3.x, thanks!