This is simply wrong. With the introduction of the "core" info file option, this release attempts to make sure that the administrator can not enable incompatible modules. However, on the admin/build/modules page, it is possible to enable incompatible modules, by enabling the modules that depend on them.

The attached patch disabled modules who depend on other modules that are incompatible.

I marked the patch as "code needs work" it does not handle modules that depend on modules that depend on modules that are incompatible.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

douggreen’s picture

Status: Needs work » Needs review
FileSize
3.53 KB

The attached patch includes the recursive dependency check mentioned above and is rerolled against the newly split out system.admin.inc.

nedjo’s picture

Thanks for the patch Doug.

The basic issue, if I understand correctly, is: dependency checking needs to ensure that the modules a given modules depends on are not only present but also work under the available PHP version. Is this right? And can you explain again, just what the bug is in the current implementation?

The main cost of the patch is that we iterate through the files array twice instead of only once. This double iteration seems to be needed, though, as we need full arrays of incompatible modules before testing each module (and its dependencies).

Will the reason for a given module being disabled be clear to users? Will users be informed that the disabled module depends on another module that is incompatible? If not, is some additional user feedback needed?

douggreen’s picture

The reason for the patch is, that the current implementation does not catch this:

Module1 depends on Module2
Module2 depends on Module3
Module3 is not core 6.x compatible

In this case, you can enable Module1. The patch fixes this.

I believe that real cost to the patch is that it recursively loops through the array, multiple times. I think that this can probably be solved by passing the $compatible array by reference and setting value on the return. I'll look a little closer and see if I can get this working, then submit another patch.

moshe weitzman’s picture

thanks doug ... feel free to refactor this dependency checking a bit. i tried to reuse it in drush.module but couldn't because it was tightly coupled to the modules form.

moshe weitzman’s picture

Priority: Normal » Critical

this defeats the whole purpose of core compatibility. promote to critical. we must fix before final release.

douggreen’s picture

FileSize
3.57 KB

The attached patch adds one line of code (as I suggested above) that I think limits the amount of recursion necessary, by making sure that modules that are depended on by multiple modules, are only recursed once.

My test case is three .info files (and corresponding empty .module files)

test1.info:

name = Test 1
dependencies[] = test2
core = 6.x

test2.info

name = Test 2
dependencies[] = test3
core = 6.x

test3.info

name = Test 3

While this is a test for core compatibility, the attached patch should also work for php -- although I haven't tested that.

Dries’s picture

Status: Needs review » Fixed

Works for me. Thanks for the test files. Committed to CVS HEAD.

Anonymous’s picture

Status: Fixed » Closed (fixed)