List unmet configuration dependencies instead of just failing

This patch just saved a few hours of debugging time for me, works fine with 8.0.5, thanks!

Patch works fine. Now lets try to fix the tests

This would be such a nice feature!

+++ b/core/lib/Drupal/Core/Config/ConfigInstaller.php
@@ -537,6 +545,7 @@ protected function validateDependencies($config_name, array $data, array $enable
+            $this->missingDependencies[$config_name] = $missing;

Is there any way to do that without adding state to the class? Can't we do something with return values?

An alternate approach that does not add state to ConfigInstaller (although I did need to use an "out" param, which sucks, but ConfigInstaller is structured in a way that left me no other obvious choice). This patch does change a few protected methods, which according to @berdir might not constitute a BC break.

Ideally the UnmetDependenciesException would be formatted in such a way that we could say which dependents had which unmet dependencies, but this is still an improvement.

The patch in #13 applied cleanly but didn't seem to do anything. The unmet dependency message remained default. I did install the patch from #8 and was able to see the missing/unmet dependency.

Same here. The patch in #13 didn't work, but using the patch from #8 is still good in v8.1

I updated the patch in #8 agains 8.2.x. I included minor code style updates and updated the ConfigInstallTest to account for the new message format and the new ConfigObjects array.

Another update to the patch. This time it includes update to ConfigInstallProfileUnmetDependenciesTest and to ConfigInstallWebTest.

tested the latest (#20) on 8.1.3 and it worked - it helped to find the right unmet dependencies.

Thanks

Nice work gclicon getting this polished.

Initial pass at re-rolling the patch so that it actually applies. The test will need to be updated again, but this should atleast allow the patch to apply while I clean up the tests again.

Looks like I was mistaken. The patch applied properly and passed testing.

Works fine for me as well. Back to RTBC

I've just fixed coding style issues

This will be an excellent improvement!

It would be helpful to have screenshots of manually testing this and showing how the unmet dependencies are displayed in the UI.

+++ b/core/tests/Drupal/KernelTests/Core/Config/ConfigInstallTest.php
@@ -199,8 +199,11 @@ public function testDependencyChecking() {
-      $this->assertEqual($e->getConfigObjects(), ['config_other_module_config_test.weird_simple_config', 'config_test.dynamic.other_module_test_with_dependency']);
-      $this->assertEqual($e->getMessage(), 'Configuration objects (config_other_module_config_test.weird_simple_config, config_test.dynamic.other_module_test_with_dependency) provided by config_install_dependency_test have unmet dependencies');

I don't think we should be removing these assertions; if we need to update the message to match, that makes sense, but we should still test it.

I'll add those assertions back and take some screenshots.

I think this is a task (or potentially even a DX bug), so it might be eligible for backport to 8.1.x.

I manually removed a config dependency in the workbench moderation module for these.

Here's screenshots of the UI.

Before

After

And here's screenshots of a drush en -y workbench_moderation

Before

After

Regarding the assertions, they aren't so much removed as re-worked:

+++ b/core/modules/config/src/Tests/ConfigInstallWebTest.php
@@ -180,7 +180,7 @@ public function testUnmetDependenciesInstall() {
-    $this->assertRaw('Unable to install Config install dependency test, <em class="placeholder">config_other_module_config_test.weird_simple_config, config_test.dynamic.other_module_test_with_dependency</em> have unmet dependencies.');

+++ b/core/tests/Drupal/KernelTests/Core/Config/ConfigInstallTest.php
@@ -199,8 +199,11 @@ public function testDependencyChecking() {
-      $this->assertEqual($e->getConfigObjects(), ['config_other_module_config_test.weird_simple_config', 'config_test.dynamic.other_module_test_with_dependency']);

What I'm not sure of is why the config_other_module_config_config_test.weitd_simple_config would no longer be part of the assertion?

It is super sad that we didn't continued with the approach in #13 as its IMHO the much better way to solve that. I could imagine that the introduction of state in here will bite us one day.
Can we at least reset the values of the missing variable when we throw the exception? This at least let's one potential bug disappear, which exists when you reevaluate this function with some different state of the underneath.

It is super sad that we didn't continued with the approach in #13 as its IMHO the much better way to solve that. I could imagine that the introduction of state in here will bite us one day.

If somebody can get the approach from #13 working, it seems better to me to also not add state into the config installer if not needed.

This is a return to the approach in #13. It is a reroll, combined with test-changes, and exception language changes from the above patches.

The screenshots in #31 remain unchanged with this approach.

+++ b/core/lib/Drupal/Core/Config/ConfigInstaller.php
@@ -501,7 +498,7 @@ public function checkConfigurationToInstall($type, $name) {
+    return array_filter(array_keys($config_to_create), function($config_name) use ($enabled_extensions, $all_config, $config_to_create, &$missing_dependencies) {

This is the reason #13 wasn't working--the use statement for closures must explicitly declare variables that should be passed by reference.

I love functional style perhaps more than I should but isn't this a case where PHP's asinine syntax goes against using that style? Is there a good reason to do this?

$missing_dependencies = []; // I am not sure we want to reset here. Do we?
foreach ($config_to_create as $config_name => $config) {
  if ($missing = $this->getMissingDependencies($config_name, $config, $enabled_extensions, $all_config)) {
    $missing_dependencies[$config_name] = $missing;
  }
}
return array_intersect_key($config_to_create, $missing_dependencies);

Did I miss (ahem) something?

Thanks @chx, that approach is much more readable I think.

// I am not sure we want to reset here. Do we?

Agreed.

This fixes the remaining test, and updates some code docs in the exception.

+++ b/core/lib/Drupal/Core/Config/ConfigInstaller.php
@@ -455,9 +455,14 @@ public function checkConfigurationToInstall($type, $name) {
+      $missing = [];
+      foreach ($missing_dependencies as $dependent => $dependencies) {
+        $missing[$dependent] = $dependencies;
+      }
+      throw UnmetDependenciesException::create($name, array_unique($missing));

It really looks like this does a copy of the existing array. Do we actually need this? In case we do, what about writing a comment?

re #41: good find! No need to copy the array there.

This looks great for me.

I'm so looking forward to see this in. IMHO this helps a lot of people out there.

+++ b/core/modules/simpletest/src/TestBase.php
+++ b/core/modules/simpletest/src/TestBase.php
@@ -830,7 +830,7 @@ protected function assertErrorLogged($error_message) {

@@ -830,7 +830,7 @@ protected function assertErrorLogged($error_message) {
     }
 
     $content = file_get_contents($error_log_filename);
-    $rows = explode(PHP_EOL, $content);
+     $rows = explode(PHP_EOL, $content);
 
     // We iterate over the rows in order to be able to remove the logged error

Can be totally fixed on commit: unnecessary change.

1. +++ b/core/lib/Drupal/Core/Config/ConfigInstaller.php
   @@ -483,16 +484,22 @@ public function checkConfigurationToInstall($type, $name) {
   +  protected function findDefaultConfigWithUnmetDependencies(StorageInterface $storage, array $enabled_extensions, array $profile_storages = [], array &$missing_dependencies = []) {
   

   I ponder if we should just return $missing_dependencies instead. But this approach is more BC compatible so I guess this is fine. Perhaps we should file a 9.x issue to change this. Or maybe we consider this change acceptable for a minor release - which this has to be because of the string changes. I'll ping catch to ask.

2. +++ b/core/lib/Drupal/Core/Config/UnmetDependenciesException.php
   @@ -53,12 +68,10 @@ public function getExtension() {
   -      'Unable to install @extension, %config_names have unmet dependencies.',
   ...
   +      'Unable to install @extension due to unmet dependencies: @config_names',
   ...
   -        '%config_names' => implode(', ', $this->getConfigObjects()),
   +        '@config_names' => self::formatConfigObjectList($this->configObjects),
            '@extension' => $extension,
   

   I think we should change everything to % - these are all placeholdered. I probably used @extension originally because we shouldn't placeholder in the actual exception message.

I'm not seeing the test failures at all...back to RTBC.

I ponder if we should just return $missing_dependencies instead. But this approach is more BC compatible so I guess this is fine. Perhaps we should file a 9.x issue to change this. Or maybe we consider this change acceptable for a minor release - which this has to be because of the string changes. I'll ping catch to ask.

Well yeah, I think the hope is to get the task in at least for 8.2.x

@dawehner it can't go in 8.2.x because of the string changes and it is a task.

Back to needs work for #44.

I discussed with @catch are he's okay with changing the return value of findDefaultConfigWithUnmetDependencies

Sure, let's change that.

Let's fix the failures.

1. +++ b/core/lib/Drupal/Core/Config/ConfigInstaller.php
   @@ -455,9 +455,9 @@ public function checkConfigurationToInstall($type, $name) {
   +    list($invalid_default_config, $missing_dependencies) = $this->findDefaultConfigWithUnmetDependencies($storage, $enabled_extensions, $profile_storages);
   
   @@ -485,14 +485,24 @@ public function checkConfigurationToInstall($type, $name) {
       * @return array
   ...
   +   *   An array containing:
   +   *     - A list of configuration that has unmet dependencies.
   +   *     - An array that will be filled with the list of missing
   +   *       dependencies, keyed by by the dependents' names.
   ...
   +  protected function findDefaultConfigWithUnmetDependencies(StorageInterface $storage, array $enabled_extensions, array $profile_storages = [], array &$missing_dependencies = []) {
   ...
   +    return [
   +      array_intersect_key($config_to_create, $missing_dependencies),
   +      $missing_dependencies,
   +    ];
   

   Seems to me that $invalid_default_config items are also the keys of $missing_dependencies. I think we can rely only on $missing_dependencies. Also that indexed array with 2 items doesn't represent a list - semantically it's more an associative array.

2. +++ b/core/lib/Drupal/Core/Config/UnmetDependenciesException.php
   @@ -70,15 +83,16 @@ public function getTranslatedMessage(TranslationInterface $string_translation, $
   -    $message = SafeMarkup::format('Configuration objects (@config_names) provided by @extension have unmet dependencies',
   +    $message = SafeMarkup::format('Configuration objects provided by %extension have unmet dependencies: %config_names',
   

   If we already touched this line, let's drop the deprecated SafeMarkup and replace it with the proper class. Or just use double quote string embedding.

3. +++ b/core/modules/simpletest/src/TestBase.php
   @@ -830,7 +830,7 @@ protected function assertErrorLogged($error_message) {
   -    $rows = explode(PHP_EOL, $content);
   +     $rows = explode(PHP_EOL, $content);
   

   Unrelated.

Thank you for your review @claudiu.cristea

Seems to me that $invalid_default_config items are also the keys of $missing_dependencies

I agree with you, BUT, I think the level of BC breaking would be bigger by that. Now potentially subclasses break, but there is at least an easy way to fix it. When you drop $invalid_default_config it would be much harder for them to get their original functionality back. Its some consideration which could be made here.

I fixed the other bits.

Looking good - one minor thing.

+++ b/core/lib/Drupal/Core/Config/ConfigInstaller.php
@@ -485,14 +485,24 @@ public function checkConfigurationToInstall($type, $name) {
+  protected function findDefaultConfigWithUnmetDependencies(StorageInterface $storage, array $enabled_extensions, array $profile_storages = [], array &$missing_dependencies = []) {

No longer need the array &...

Oh yeah, that's true.

Good, then

Committed efcf521 and pushed to 8.3.x. Thanks!

It looks like this resulted in a behavior change that resulted in test fails in the sharemessage project: #2817923: Demo tests are falling on HEAD. Can someone comment there if this an expected or unexpected change? I have no problem with changing the test and it should also be backwards compatible, but this could be an unexpected side effect.

I've found an issue with this change. Follow-up issue here: #2831816: Only first set of configuration dependencies is checked on installation.

This patch applies cleanly to the 8.2.5 release.

Cheers

Tom