module_rebuild_cache() returns non-existent dependent modules [#203794]

To see the problem, install a fresh install copy of 6.x rc1, then install civicrm_theme, but don't install civicrm. civicrm_theme depends on civicrm. On the admin/build/modules page, you'll see a warning and extra junk at the bottom of the table.

warning: Invalid argument supplied for foreach() in /path/to/drupal/drupal-6rc1/modules/system/system.admin.inc on line 578.

This happens because _module_build_dependencies() adds non-existent modules to the files array.

The attached one line patch fixes this. I am a little concerned with this fix, because I don't know if there are other places in Drupal that rely on this. If we need these non-existent dependent items for some reason, an alternate way of fixing this is in system.admin.inc, to check for $file->info before rending these items. However, I'd rather catch this at the beginning, which is why I'm submitting the patch that I have.

Comment	File	Size	Author
	rebuild.patch	816 bytes	douggreen

Comments

Comment #1

douggreen commented 25 December 2007 at 13:16

btw, I submitted a similar critical bug yesterday (without the patch), and it seems to have disappeared. So if this is a duplicate, forgive me. I'm not sure if I simply only previewed it and forgot to submit it, of if our multiple server setup somehow ate it. I did check the watchdog logs, and the original issue appears to have never been created, so I don't think this is a duplicate.

Comment #2

douggreen commented 25 December 2007 at 13:18

Title:

module_rebuild_cache() return non-existent dependent modules

» module_rebuild_cache() returns non-existent dependent modules

Comment #3

chx commented 25 December 2007 at 19:17

Thanks for the report and the patch. You have obviously debugged this problem, care to share results with us? What's the value of $dependency_name that does not exist as a module? If you could guess, what would be your guess, where does this name come from? I am not asking you to further debug the problem, but if it's "foo" and "foo" appears in some .info somewhere that'd be good to know.

Comment #4

douggreen commented 25 December 2007 at 22:02

I have a module named civicrm_theme which I've upgraded to 6.x. It depends on civicrm. The civicrm_theme.info file looks like:

name = CiviCRM Theme
description = define alternate themes for CiviCRM on the Drupal admin theme page
dependencies[] = civicrm
core = "6.x"

Since civicrm hasn't been upgraded yet (AFAIK), I haven't installed a corresponding civicrm.module. So, civicrm_theme depends on civicrm, and civicrm doesn't exist. In this case, the $dependency_name is civicrm, but it doesn't exist in the $files array, until the dependency adds it.

Comment #5

dries commented 26 December 2007 at 07:42

I'm not sure this is the best place to add such a check. I'd try to figure out why that file ended up in the $files array to begin with. I'd expect a valid $files array as input to this function.

Comment #6

douggreen commented 26 December 2007 at 12:25

@dries, If you're referring to my musings in the issue description, I was concerned that maybe these were put into the files array intentionally. After looking at this some more, it seems pretty clear that they shouldn't be there. And if that is the case, then then that is exactly what this patch does. This patch prevents it from getting added to the files array. They get added in the dependency checker, further down in this loop.

Comment #7

dww

we/he/they

commented 26 December 2007 at 19:14

Status:

Needs review

» Reviewed & tested by the community

I just noticed the same thing, and was about to submit my own issue about it, but searched a little and found this. In my case, my test site had the "interface_sortable" module installed (not enabled), but no "jquery_interface" module. Without this patch, I get a ton of PHP notices from system.admin.inc, and the extra junk at the bottom of the page, just as doug describes here. His patch solves everything in my case, and the diagnosis makes perfect sense. Yes, files that don't exist shouldn't be in the $files array, which is why we shouldn't add non-existent files to that array when processing dependencies (a bug introduced via http://drupal.org/node/194010). Without this patch, the whole "missing" error case for dependencies is broken. Once applied, missing dependencies are properly detected and flagged as such.

I can personally verify that this patch solves a couple of critical bugs, and it seems clean and sane, so I'm marking RTBC.