This patch makes the module_enable and module_disable function more robust.

This patch replaces _module_build_dependencies as that implementation was a little awkward in calculating only a two levels deep list of dependencies. And it makes building the graph impossible by adding all levels dependencies instead of one level needed to build a graph.

Unit tests are included for all new functions and for enabling and disabling modules.

module_enable and module_disable have now a guard to calculate all dependant modules based on the list of given modules.

In order to accomplish this a graph is build based on de modules installed. From this graph a Topological Sorted List (TSL for short) is generated.

Functional the frontend aka admin/build/modules should use the following to list all 'depends on' and 'required by' modules.

$depends_on = module_get_tsl( $module, TRUE);
$required_by_dependencies = module_get_tsl( $module, FALSE);

Note that the frontend will not break by this patch. The lists are now only smaller. But module_enable and module_disable don't care any more.

Futhermore this patch renders drush_mm module obsolete and provides probably a solution for #211182: Updates run in unpredictable order by giving a TSL for updating.

Applying this test makes #310617: Tests for module.inc obsolete.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

RobLoach’s picture

"This patch"?

webchick’s picture

Status: Needs review » Needs work

I don't think most normal humans know what tsl is. module_get_tsl() should probably be named like module_get_dependencies()

And yes. There is no patch.

clemens.tolboom’s picture

FileSize
25.08 KB

Patch attached.

@webchick: there is a difference between dependencies and tsl which is supposed to be sorted ... but I agree that the name is not 'nice'.

clemens.tolboom’s picture

Status: Needs work » Needs review
clemens.tolboom’s picture

Status: Needs review » Needs work

Abstract from a talk with chx@irc

  1. the current implementation of _module_build_dependencies does build the whole dependency. My trouble with it is that a graph cannot be calculated from that. So I typed it wrong :-(
  2. white spaces ... arghh ... now I learned to use coder-6.x on a d7 file :-)
  3. in order to get the result you have to reset DFS?
  4. Use dfs instead of df
  5. Use isset instead of key_exists!
  6. Can't you split the module into normal page request and module admin? Say a module.build.inc or something?
  7. And maybe a graph.inc? With graph and tls or better named drupal_depth_first_search and a drupal_topological_sort
clemens.tolboom’s picture

This is my progress so far. In case somebody thinks I'm not on it ;-)

What I did was adding graph.inc which contains classes. Don't be scared ... it looks great for this purpose to me. And they can now be used for other purposes.

Furthermore I put all 'extra' functions into module.admin.inc but I didn't want to move to much into it yet. If this patch is running fine I want to change the admin/build/modules form into using the TSL functionality then add hand.

There is one problem aka TODO in module.inc which includes the file hard coded. Somehow my unit tests fails to run without it :-(

I also got rid of the DFS algorithm and that nasty 'give me the list now' function call.

clemens.tolboom’s picture

Status: Needs work » Needs review
clemens.tolboom’s picture

Assigned: Unassigned » clemens.tolboom
Status: Needs review » Needs work

My idea for making a class library was a mistake due to enthusiasm. I missed the point chx had about these drupal_depth_first_search and a drupal_topological_sort functions. So I have to rewrite my classes into these functions.

My problem is chx is writing a patch too. So I hope to get some feedback :-) See also #314287: Refactor module dependency checking

Dave Reid’s picture

My problem with this patch is that it goes against the core principle of keeping lightweight and simple. You're introducing new files and APIs that are not likely to be reused, seeing how no one else has really chimed in here. If you can prove and justify that additions here would have a worthwhile benefit to Drupal as a core, I'd be more supportive, but it seems like #314287: Refactor module dependency checking is in a better direction and won't use a huge pot to cook a little meal. (sorry for the bad analogy)

chx’s picture

I am so sorry you went off without showing people what you are doing :( you have written a lot of code but alas you are adding code to an area which already suffers from a lot of code. I began working on this and the first results is http://drupal4hu.com/graph.patch -- see how easy it is to fill out the dependencies part with the help of a little graph code? I will submit this properly along with more issues.

clemens.tolboom’s picture

Status: Needs work » Postponed

@Dave Reid
Thanks for the feedback. You are probably right that this patch has no 'benefits' and 'reuse' explanation. My original patch was about module.inc and a lot of tests only :-)

I followed #5 where chx suggested to split the patch into module.admin.inc and graph.inc I did. In the proces I made 24 test on my code and introduced a class :-(

That same week I learned chx was doing a DFS too assuming this patch was dead. See #10.

clemens.tolboom’s picture

I put this patch on hold due to the fact that chx is working on it too on a higher level. Ie fixing modules/system/system.admin.inc and I like his 'graph with data' and documentation.

But I do have comments though on chx current patch from a library function perspective and it's implementation. I shall post it here for now.

Note that a Tree (=Menu?) extends DirectedGraph extends Graph

  1. system.admin.inc : Is the form now still in control? There is no safeguard in module_enable / module_disable which is why my patch came into existance.
  2. system.admin.inc : There are now new keys 'depends_on' and 'required_by' which are better names. But do we need these?
  3. There is a implicit cycle check in _drupal_depth_first_search(). I would prefer an explicit check for a cycle as I did in my patch Graph::isCircularMember(). I see the usefullness for _module_build_dependencies() though.
  4. I miss something like Graph::getParticipants()
  5. graph.inc : _drupal_depth_first_search does not have to be recursive. See my DirectedGraph:getTSL()
  6. I really would like to have generic Graph and Tree into drupal core. So if we could make graph.inc more general that would be nice. My classes do have addLinks, getParticipants, isCircular, reversedGraph etc ... but maybe Dave Reid is right and no others are in need of these?

I hope that chx submits his patch soon. Then maybe we could (re)use my tests for a start.

@chx : please notify me when your patch is ready. Then I can hopefully add some useful feedback on it :-)

Damien Tournoud’s picture

@clemens: implementing a TLS or a forward/backward route finder is something like 10 lines of code. We *really* don't need three classes and 20 helper methods for that.

Drupal is a CMS / Web development framework, do we really need a full-featured graph theory library for this? Probably not. Can you list some use cases stronger that "it would be nice to have"?

Plus, if we need it, there is probably a good library somewhere in the PHP world, we may not need to reimplement this from scratch.

clemens.tolboom’s picture

Status: Postponed » Closed (won't fix)

I think #314287: Refactor module dependency checking will do a better job. My patch is lacking checks for missing dependencies. In drush_mm there is a pre check for missing module *before* calling module_enable / module_disable with a TSL.