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.
Comment | File | Size | Author |
---|---|---|---|
#6 | module_enable_disable_310898.patch | 32.62 KB | clemens.tolboom |
#3 | module_en_dis_test.patch | 25.08 KB | clemens.tolboom |
Comments
Comment #1
RobLoach"This patch"?
Comment #2
webchickI 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.
Comment #3
clemens.tolboomPatch 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'.
Comment #4
clemens.tolboomComment #5
clemens.tolboomAbstract from a talk with chx@irc
Comment #6
clemens.tolboomThis 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.
Comment #7
clemens.tolboomComment #8
clemens.tolboomMy 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
Comment #9
Dave ReidMy 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)
Comment #10
chx CreditAttribution: chx commentedI 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.
Comment #11
clemens.tolboom@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.
Comment #12
clemens.tolboomI 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
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 :-)
Comment #13
Damien Tournoud CreditAttribution: Damien Tournoud commented@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.
Comment #14
clemens.tolboomI 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.