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.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

solotandem’s picture

Status: Active » Needs review
FileSize
779 bytes

I am outdoing myself with the attached 2-line patch.

David_Rothstein’s picture

Title: module_enable does not strip version strings from dependency » module_enable does not always enable all dependencies when it's asked to
Status: Needs review » Needs work
Issue tags: +Needs tests

Hm, 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]

carlos8f’s picture

Title: module_enable does not always enable all dependencies when it's asked to » module_enable() doesn't account for version strings in dependencies[]

Oops, 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.

David_Rothstein’s picture

Status: Needs work » Needs review
Issue tags: -Needs tests
FileSize
4.18 KB

Ah, 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 :)

carlos8f’s picture

Status: Needs review » Reviewed & tested by the community

Nicely done.

Dries’s picture

Status: Reviewed & tested by the community » Fixed

Committed to CVS HEAD. Thanks!

Status: Fixed » Closed (fixed)

Automatically closed -- issue fixed for 2 weeks with no activity.