Clean up some modules code

No longer applies to head.

Quite the patch. Changes a few things and introduces graphing of dependencies (or should I say 'required by') with modules. This is a much smarter way of doing dependencies than we have now and will fix some issues.

I have not had a chance to fully work through the graph logic (which is the important part in this) I noticed when a cycle is detected 'cycle' is echoed and the script exits. Is there a way we can have a better error message and/or instead of killing the script fail to build the dependency and report back to the user there is an error in a certain module. This would be a real plus for troubleshooting. Though, I hope no module author ever builds in a dependency like that and this should be rare.

@chx thanks for the patch. This is a fun one to review.

For anyone looking for a little back reading on DFS see http://en.wikipedia.org/wiki/Depth-first_search

Re-rolled, hopefully fixed most of the indentation issues.

I've started to replace "depends_on" in PHPDoc comments where possible at first sight. We should use "required modules" or anything else that is human-readable instead.

Also, not fixed yet: Some browsers have major issues with underscores in CSS class names - those should only contain alphanumeric characters and hivens only:

     $description .= '<div class="admin-depends_on">' . t('Depends on: ') . implode(', ', $module['#depends_on']) . '</div>';

I have, apparently, very little experience with OOP, but this code in simpletest.module looks like ModuleTestCase was never executed:

    foreach ($classes as $key => $class) {
      if (method_exists($class, 'getInfo')) {
        $formatted_classes[$class] = new $class;
      }
    }

Note that it is perfectly possible that I'm 100%, ultimately, wrong here.

I forgot to mention that this patch works. I did not run the tests, but tested manually instead, using two custom modules, "custom" and "zzz_custom". Added dependencies on blog and zzz_custom for custom, and dependency on forum for zzz_custom.

zzz_custom_install() in zzz_custom.install contained

  forum_schema();

without any module_* or whatever functions.

Additionally, custom_install() in custom.install contained:

  zzz_custom_install();

equally, without any preloaders.

Upon installation (and usual confirmation page for required modules) of custom module, all modules were installed in the expected order of dependencies, without any (fatal) error.

* The name 'admin-depends_on' is pretty ugly.

* For consistency rename 'path' to 'forward_path'?

* I'm not convinced we need to factor this out in path.inc. This is small and easy enough that it could belong to an existing include file, IMO.

* The syntax to build and manipulate these graphs is a bit arcane. For example:

+    $graph[1]['edges'][2] = 1;
+    $graph[2]['edges'][3] = 1;
+    $graph[2]['edges'][4] = 1;
+    $graph[3]['edges'] = array();
+    $graph[4]['edges'][3] = 1;

Let's think some more about how we can improve the DX here. When manipulating graphs, OOP might make a lot more sense.

Here is a refactored version, with a fix for a bug chx identified in the previous one. Component identification is now performed directly in the DFS, and I removed the static. And added a lot of code comments.

Fix some spelling and grammar.

Remove the need to pass $traversal_data thanks to new PHP5 optional passed-by-reference arguments.

And factored-out $graph[$vertex]['component'] into a variable during the weight calculation.

Improved the component detection code *and* quite extended the tests. I'm now reasonably confident about this.

Resubmitted for the test bot.

I still think <div class="admin-depends_on"> is a really ugly CSS class name. Can we go with admin-dependencies or admin-depends-on ?

#343502: Allow tests to require and test existance of contrib modules is somewhat related, but should not slow down this patch.

- Renamed "depends_on" to "requires". So we have "requires" and "required_by" now, which is much easier to understand on first sight.

- Fixed PHPDoc in many places and also for drupal_parse_info_file() accordingly.

- Fixed the CSS class issue, also updated admin.css accordingly.

When the tests for this patch will pass, I think it's ready to go.

That testbed result looks completely b0rked: http://testing.drupal.org/pifr/file/1/3299

This *is* an issue with test slave #9, for some reason. The code is good, I ran the test suite manually on my dev box and that resulted in a 100% pass.

Forcing RTBC.

This sounds nitpicky (the patch is fantastic), could rename graph.inc to something like "dependencies.inc"? "Graph" is an extremely generic term, leading me to think it has to do with creating (visual) graphs or statistics. I know the internal code deals with graphs, but from the functional viewpoint, the include will be used exclusively (afaik) to generate dependencies.

i agree with quicksketch. the file name is lacking.

Aside from that the unit tests are missing documentation.

GraphUnitTest's assert*, displayArray functions need PHPDocs.

ModuleNoDependencyTestCase and ModuleNoRequiredTestCase need class level PHPDocs.

Fixed PHPDocs and comments.

Whether graph.inc should be renamed or not, I'll leave to chx. All I can point out is that even with my very limited understanding of this directed acyclic graph method, the file is probably properly named from a mathematical/technical standpoint. I assume that this is a similar discussion as with taxonomy vs. category, with the major difference that no end-user is facing this text. I would even suppose that those functions could be used when handling real graphs (e.g. SVG) - but again, I did not study CS and was poor in mathematics in school, so this are actually wild assumptions only.

Fixed two PHPDoc @param descriptions thanks to Damien Tournoud.

Sorry to be such a pain but I don't want to give webchick the satisfaction of getting to mark this as CNW.

Even though it's an internal helper function we need PHPDocs for the @params:

+function _drupal_depth_first_search(&$graph, &$state, $start, &$component = NULL) {

Needs PHPDocs for @params:

+  function displayArray($paths, $keys = FALSE) {

+    // need to show a confirm form.

We shouldn't abbreviate, let's say confirmation form.

+    // If this module is required by others then list those and make it
+    // impossible to disable this one.

how about "If this module is required by other modules list those and then make it impossible to disable this one."

ModuleDependencyTestCase and ModuleRequiredTestCase seem to share the same comment. I'd guess that they do different things though... I'd suggest copying the getInfo() description.

This could use some tweaking:

+  // disable it. Also, we find out if there are modules it requires and are
+  // not enabled.

Perhaps: "Also, we find out if there are modules it requires that are not enabled."

I wish the assert function's PHPDoc @params explained their data types, e.g.:

+   * @param $count
+   *   Assert tables that match specified base table.

might be better as:

+   * @param $count
+   *   Number of tables that match specified base table.

but that's pretty nit picky and we've got to leave something for webchick to point out ;)

Fixed all of drewish's issues in #37.

I'm happy with the name 'graph.inc' because the graph code could be used for non-dependency things. I'm also happy with calling it dependencies.inc until we're using it more generically.

+ * @param &$graph
+ *   A three dimensional associated graph array.
+ * @param &$state
+ *   An associative array. The key 'last_visit_order' stores a list of the
+ *   vertices visited. The key components stores list of vertices belonging
+ *   to the same the component.
+ * @param $start
+ *   An arbitrary vertex where we started traversing the graph.
+ * @param &$component
+ *   The component of the last vertex.

I don't recall core using the & on the @param tags.

+      if (!isset($files[$requires])) {
+        $extra['requires'][$requires] = t('@module (<span class="admin-missing">missing</span>)', array('@module' => drupal_ucfirst($requires)));
           $extra['disabled'] = TRUE;
         }
-        elseif (!$files[$dependency]->status) {
-          $extra['dependencies'][$dependency] = t('@module (<span class="admin-disabled">disabled</span>)', array('@module' => $files[$dependency]->info['name']));
+      elseif (!$files[$requires]->status) {
+        $extra['requires'][$requires] = t('@module (<span class="admin-disabled">disabled</span>)', array('@module' => $files[$requires]->info['name']));
         }
         else {
-          $extra['dependencies'][$dependency] = t('@module (<span class="admin-enabled">enabled</span>)', array('@module' => $files[$dependency]->info['name']));
-        }
+        $extra['requires'][$requires] = t('@module (<span class="admin-enabled">enabled</span>)', array('@module' => $files[$requires]->info['name']));
       }

The indenting seems odd here... maybe it's just and artifact of the patch. I didn't apply it and see.

Other than that I'm comfortable with the the docs and tests. I won't speak to anything else.

Committed to CVS HEAD. Great work, folks.

The "Required by" and "Requires" spans on the modules page are being escaped - attached patch unescapes them.

I don't see how that patch can cause failures in "Forum functionality" tests...

Committed follow-up fix to HEAD. Thanks!

Subscribing—I know it is committed, but I am about to change location and I want to come back and read what changed, and this is the easiest way to find it. Sorry to bounce it back to the top of everyone's issue queues.