Clean up some modules code
chx - October 13, 2008 - 03:47
| Project: | Drupal |
| Version: | 7.x-dev |
| Component: | base system |
| Category: | task |
| Priority: | normal |
| Assigned: | chx |
| Status: | closed |
| Issue tags: | Favorite-of-Dries |
Description
This patch here is just cleanup. It adds a generic enough DFS code. And, a system modules page test thanks to dmitri, i just needed to clean that up. $this->assertNoSloppy($dmitrig01)...
#1
Speaking of sloppiness I *know* about the missing error handler in graph.inc...
#2
So let me try to write up what happens here. Instead of using a rather naive and ugly approach to find depencies, we instead use a proper DFS algorithm to find forward and backward pathes from a given vertex. In our case, for every module we find what modules it depends on and which modules require it. I am not using the words "dependencies" and "dependents" any more because that's so ugly. Do you say "comment module is a dependent of forum"? How does "comment module is required by forum" sound? And then, as I went through system.admin.inc to change the wording as listed here, I found that the submit code gives us new opportunities if I simplify the form so that the form_values[modules] array and the data coming from the graph code nicely match together. And then I simplified the confirm form because anything you do there really can be done better in the submit handler, just compile the confirm text, stuff it into form_state['storage'] and be done. And then I added a whole bunch of comments... and grabbed dmitri's test and added to this whole pile of clean.
#3
No longer applies to head.
#4
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
#5
#6
#314287: Refactor module dependency checking was a duplicate.
#7
This one here, besides doing some cleanup adds one very important feature: enables and disables modules in dependency order. So if demofield modules depends on field module (it does) then field gets enabled first and then demofield.
#8
The last submitted patch failed testing.
#9
With graph.inc and graph.test , this time. Better comments... maybe. Somewhat duplicates #356747: Fix hidden module dependencies but that's necessary otherwise lil' testingbot would cry a river.
#10
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:
<?phpforeach ($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.
#11
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
<?phpforum_schema();
?>
without any module_* or whatever functions.
Additionally, custom_install() in custom.install contained:
<?phpzzz_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.
#12
* 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:
<?php+ $graph[1]['edges'][2] = 1;
+ $graph[2]['edges'][3] = 1;
+ $graph[2]['edges'][4] = 1;
+ $graph[3]['edges'] = array();
+ $graph[4]['edges'][3] = 1;
?>
#13
Keeping up with HEAD. Also, graph.inc is basically rewritten, I peeked at Clemens Tolboom's code at #220945: patch: drush mm enable/disable/dot/uninstall for inspiration. But also, it now finds subgraphs properly and stores them too. This results in optimal numbering for module installs and that's even mentioned in code comments.
#14
#15
Oh, crossposted. path is a graph terminus technicus while forward path is not, so I vote on keeping that. For reverse path, it's actually 'path in the reverse graph' so I think it's ok to call that 'reverse path'. I think this file is useful for other uses (at least taxonomy multiparents are a DAG) so I would like to keep it as a separate include. Now, for notation, that's an interesting question, however this is the easiest to code actually. I made it so that now 'edges' is not necessary which helps the DX, new patch has the test to reflect this. As you need to be able to quickly answer is there a path from A to B? this basically leaves us with two possible notations, $graph[a]['edges'][b] and $graph['edges'][a][b]. If you instead use $graph[a]['edges'] = array(b...) then you need to array_search which is slower. Lots.
#16
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.
#17
Fix some spelling and grammar.
#18
Remove the need to pass $traversal_data thanks to new PHP5 optional passed-by-reference arguments.
#19
And factored-out $graph[$vertex]['component'] into a variable during the weight calculation.
#20
Very nice, thanks! Also to sum up my discussion with Dries on notation: $graph['edges] = array('a' => 'b', 'b' => 'c') wont work if there is an edge from a to b and a to c. Also, $graph['edges']['a']['b'] wont let us do something like foreach ($graph as $module => $data) and use various properties of the given vertex. So we stay with the current notation.
#21
The last submitted patch failed testing.
#22
Improved the component detection code *and* quite extended the tests. I'm now reasonably confident about this.
#23
The last submitted patch failed testing.
#24
Surely does not make sense that some file test grows a fail because of this patch!
#25
Resubmitted for the test bot.
#26
Let's get this sucker IN!
#27
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 ?#28
#343502: Allow tests to require and test existance of contrib modules is somewhat related, but should not slow down this patch.
#29
- 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.
#30
The last submitted patch failed testing.
#31
That testbed result looks completely b0rked: http://testing.drupal.org/pifr/file/1/3299
#32
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.
#33
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.
#34
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.
#35
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.
#36
Fixed two PHPDoc @param descriptions thanks to Damien Tournoud.
#37
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 ;)
#38
Fixed all of drewish's issues in #37.
#39
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.
#40
Fixed some of the new doxygen.
#41
#42
+ * @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.
#43
Committed to CVS HEAD. Great work, folks.
#44
The "Required by" and "Requires" spans on the modules page are being escaped - attached patch unescapes them.
#45
The last submitted patch failed testing.
#46
I don't see how that patch can cause failures in "Forum functionality" tests...
#47
Committed follow-up fix to HEAD. Thanks!
#48
Automatically closed -- issue fixed for 2 weeks with no activity.
#49
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.