Refactor ModuleListForm

+++ b/core/lib/Drupal/Core/Extension/DependencyEvaluationResult.php
@@ -0,0 +1,120 @@
+  public function isHidden() {
+    return empty($this->description);
+  }

This seems a little magical, wouldn't it be clearer if it just had a $hidden setting?

Is the existing test coverage enough to not add any here? I'm assuming that we have plenty of coverage for the module page, but might be nice to check and confirm that we do.

Since this is an important page that tends to be fairly heavy to load, do we need a quick profiling run for this to make sure we are not regressing here (nothing in the code makes me think we might, but who knows)?

Shuffle some deck-chairs so that its easier to insert a BC layer

I think we need a bit more info in the the issue summary. Why is this needed for #3005229: Provide optional support for using composer.json for dependency metadata? Can you expand on the changes a bit more? What are we doing to make a BC layer easier?

I like the introduction of the value object, and the static methods make it's usage intuitive. Although we could add a unit test, it seems too trivial to me.

@larowlan thanks for doing the profiling/test check steps. RTBC++

Random test fails.

So much complexity.

The whole hidden thing is really really not nice. What's a hidden dependency?!?! The answer is one that doesn't count. And that's where I think this solution might do with a little finesse because it got very confusing trying to work out what

+++ b/core/lib/Drupal/Core/Extension/DependencyEvaluationResult.php
@@ -0,0 +1,128 @@
+  /**
+   * Factory method to build a result where the dependency is hidden.
+   *
+   * @param string $dependency
+   *   Dependency machine name.
+   *
+   * @return \Drupal\Core\Extension\DependencyEvaluationResult
+   *   Compatibility result.
+   */
+  public static function hidden($dependency) {
+    $instance = new static();
+    $instance->satisfied = FALSE;
+    $instance->dependency = $dependency;
+    $instance->hidden = TRUE;
+    return $instance;
+  }

The $instance->satisfied meant in this case.

It really means ignore so rather than creating a DependencyEvaluationResult how about returning FALSE. Meaning we've ignored the dependency. That way this whole hidden thing has no meaning outside this form - which it doesn't atm but this patch makes it suddenly have.

Also I always find it remarkable that some much logic about where something can be installed or uninstalled is buried in this form. Plus all the uninstall logic side of things is moot because that's not handled here any more.

Feedback from #13 has been addressed to back to RTBC

1. --- /dev/null
   +++ b/core/lib/Drupal/Core/Extension/DependencyEvaluationResult.php
   

   So the only thing I'm pondering now is the location of this class. It's not a really a generic class at all. Like Drush or Console couldn't use it to improve or re-use their dependency messages whilst enabling a module. The logic they'd need is still all in the form. For me this class really boils down to a nice way to return a multiple values (the message and whether or not the dependency is satisfied) from the new buildDependencyEvaluationResult method. So for me this thing probably ought to be in the same location or very near that form.

2. +++ b/core/modules/system/src/Form/ModulesListForm.php
   @@ -350,16 +342,59 @@ protected function buildRow(array $modules, Extension $module, $distribution) {
   +  protected function buildDependencyEvaluationResult(array $modules, Dependency $dependency_object) {
   

   I think the arguments should be the other ways around. We're evaluating the dependency so it makes sense that it is the first argument.

I am adding #3060241 as a related issue because both the ModuleListForm and the ModuleInstaller should use the same component to resolve and validate dependencies, when a dependency validation fails they should both handle it properly.