Not sure what component to tag this as -- could be simpletest, too.
When simpletest is invoked, it calls module_enable($modules, TRUE), the TRUE indicating to build dependencies. However, module_enable() simply reads the dependency strings from the info file, which may include version information. These strings have not been parsed as is done in _module_build_dependencies() (the latter calls drupal_parse_dependency()). The test to see if the module is in $module_data fails; the dependencies are never resolved; the modules are not loaded; and the tests fail miserably.
This forces you to list all (or more) of the dependencies in the parent::setup() call.
Comment | File | Size | Author |
---|---|---|---|
#4 | module-dependencies-1049116-4.patch | 4.18 KB | David_Rothstein |
#1 | 1049116-module_enable.patch | 779 bytes | solotandem |
Comments
Comment #1
solotandem CreditAttribution: solotandem commentedI am outdoing myself with the attached 2-line patch.
Comment #2
David_Rothstein CreditAttribution: David_Rothstein commentedHm, I think the problem is deeper than that. By using
$module_data[$module]->info['dependencies']
, doesn't that also mean the current code would miss, for example, anything that isn't an immediate dependency (e.g., dependencies of the dependencies)?Switching to
$module_data[$module]->requires
would likely fix both bugs at once.Either way, this could really really benefit from a test :)
[As a side note, I at first had no idea what this issue was referring to about version strings and such, because I didn't know we even had this feature in Drupal 7: #211747: Allow specifying version information as part of module dependencies]
Comment #3
carlos8f CreditAttribution: carlos8f commentedOops, I totally forgot that dependencies[] can have version info, too! The patch looks good then, but a test would be good to have, too.
As far as the "larger bug", I don't think we have one. Dependencies of dependencies are handled correctly, it's just the sneaky way that I wrote the while loop that is deceiving :) It uses each($module_list) (which advances the array pointer), and adds dependencies to $module_list as it goes, causing the loop to be psuedo recursive. I also wrote ModuleTest::testDependencyResolution() which creates a dependency chain and verifies that module_enable(..., TRUE) does enable dep's of dep's.
Comment #4
David_Rothstein CreditAttribution: David_Rothstein commentedAh, yes, I see what you're saying. Just that one bug, then. And I don't think you caused the bug :) The equivalent code in D6 was using $info['dependencies'] also, so the bug may have appeared quite a while ago, right when the feature was introduced.
I still think we should fix it by using the "requires" property, though. The info we need is already in that property, and that's a lot cleaner than string parsing. Plus, it matches module_disable()'s use of "required_by".
The attached patch does that, and also adds a test to ModuleUnitTest::testDependencyResolution(). It mostly just follows the pattern of the tests that are already there. I should have known about this test case - I added a couple things to it at one point :)
Comment #5
carlos8f CreditAttribution: carlos8f commentedNicely done.
Comment #6
Dries CreditAttribution: Dries commentedCommitted to CVS HEAD. Thanks!