Duplicate modules cause epic fail
boombatower - November 6, 2009 - 22:36
| Project: | Drupal |
| Version: | 7.x-dev |
| Component: | base system |
| Category: | bug report |
| Priority: | critical |
| Assigned: | boombatower |
| Status: | closed |
Description
When two modules with the same name are placed in different locations (ie core vs sites) the system does some rather dumb things.
- Loads both install files, which results in nasty things like:
Fatal error: Cannot redeclare simpletest_install() (previously declared in /home/boombatower/software/simpletest/simpletest.install:109) in /home/boombatower/software/drupal-7/modules/simpletest/simpletest.install on line 54 - Uses the proper .info file.
- Actually loads the core PHP files and ignores the ones in sites folder.
#1
Everything in sites/ should always have precedence.
#2
Right, but they don't.
#3
Working on this.
#4
So far, drupal_get_install_files() returns the proper .install file locations, and only one of them for simpletest.
#5
system_rebuild_module_data() seems to exhibit the behavior I noticed.
It says simpletest is in modules directory, yet picks up the .info file information from the one in sites (double whammy).
Would seem it has to do with:
<?phpsystem_get_files_database($modules, 'module');
system_update_files_database($modules, 'module');
?>
Not updating to the proper file since the database has the wrong file, but:
<?php$modules = drupal_system_listing('/\.module$/', 'modules', 'name', 0);
?>
returns the right one (which is used for .info file parsing).
Not sure why this code is so extremely convoluted.
#6
It would seem that one function (for scanning file system) would be enough then...you update file listing ..and read .info files.
Secondly, I have no idea what the purpose of the get and then update the files database...as from scanning it would appear no change is made...the files table is simply re-written.
#7
Not sure what you mean by re-written in http://api.drupal.org/api/function/system_update_files_database/7. Old obsolete module records are deleted and new/discovered/updated (as in info in the module's .info file changed) modules are inserted/added. The point of the two functions (you should take a look at this nasty code in D6), was to re-use the functions with both themes and modules.
#8
The names are not clear, but either way since both modules and themes have info files they still share everything in common and should be able to use the same function, I am not seeing why the info file parsing is completely separate or if anything why they do not use the same method to seek the actual files.
Either way, the functions that seek the files do not function properly, when I get time I'll try and figure it out, otherwise it would be great if someone else had time to fix them.
#9
Win on the "key" instead of "$key".
#10
Great.
So what we do now is:
1) Add modules/simpletest/book.module.
2) Put some completely different stuff in there. Including hook_menu() to test a callback. Perhaps also an arbitrary alter hook.
3) Add a test that enables "book" module.
4) Verify that modules/book/book.module is NOT loaded and entirely unknown to the entire system.
#11
That doesn't really work since it should always override. We only care about levels when modules/ vs sites/all vs sites/xyz.
And we dont' have write access to sites/[non-files] or modules/ so unless we do somethign interesting it isn't really possible since we need to gen module on the fly. (or break rules)
#12
The last submitted patch failed testing.
#13
Double wammy, bad test as well, since $array1 == $array2 or $array1 === $array2 will only be equal if they are in the same order, which sadly the test it trying to check once in order and once not, but uses same assertion.
For example:
<?php
$array1 = array(
'foo',
'bar',
);
$array2 = array(
'bar',
'foo',
);
var_dump(array_diff($array1, $array2));
var_dump($array1 == $array2);
?>
Results in:
array(0) {}
bool(false)
Which is what we want.
#14
@boombatower: If you use array keys (which is what happens in module_list()), comparison does work:
$array1 = array(
'foo' => 'foo',
'bar' => 'bar',
);
$array2 = array(
'bar' => 'bar',
'foo' => 'foo',
);
var_export($array1 == $array2);
results in
true#16
@boombatower: Not sure what you mean by array_combine not working. "array_combine — Creates an array by using one array for keys and another for its values".
$expected_values = array_combine($expected_values, $expected_values);basically works like drupal_map_assoc().#17
Here are the arrays the test has when it runs.
<?php
$a1 = array ( 'block' => 'block', 'color' => 'color', 'comment' => 'comment', 'dashboard' => 'dashboard', 'dblog' => 'dblog', 'default' => 'default', 'field' => 'field', 'field_sql_storage' => 'field_sql_storage', 'field_ui' => 'field_ui', 'file' => 'file', 'filter' => 'filter', 'help' => 'help', 'image' => 'image', 'list' => 'list', 'menu' => 'menu', 'node' => 'node', 'number' => 'number', 'options' => 'options', 'path' => 'path', 'rdf' => 'rdf', 'search' => 'search', 'shortcut' => 'shortcut', 'system' => 'system', 'taxonomy' => 'taxonomy', 'text' => 'text', 'toolbar' => 'toolbar', 'user' => 'user', );
$a2 = array ( 'block' => 'block', 'color' => 'color', 'comment' => 'comment', 'dashboard' => 'dashboard', 'dblog' => 'dblog', 'field' => 'field', 'field_sql_storage' => 'field_sql_storage', 'field_ui' => 'field_ui', 'file' => 'file', 'filter' => 'filter', 'help' => 'help', 'image' => 'image', 'list' => 'list', 'menu' => 'menu', 'node' => 'node', 'number' => 'number', 'options' => 'options', 'path' => 'path', 'rdf' => 'rdf', 'search' => 'search', 'shortcut' => 'shortcut', 'system' => 'system', 'taxonomy' => 'taxonomy', 'text' => 'text', 'toolbar' => 'toolbar', 'user' => 'user', 'default' => 'default', );
var_dump($a1 === $a2); // Result: false
var_dump(array_diff($a1, $a2)); // Result: empty array
ksort($a1);
ksort($a2);
var_dump($a1 === $a2); // Result: true
var_dump(array_diff($a1, $a2)); // Result: empty array
?>
Only difference is default is in a new location.
Looking into this.
#18
Ah, yes @Dave Reid: your example uses "==" when assertIdentical() uses "===".
I'll update test to use == and then we don't need array_diff().
#19
#20
chx said it was worthy of a phpwtf: http://www.phpwtf.org/identical-arrays-vs-equal-arrays.
#21
Subscribing. I too have spent pain on this issue.
In my case:
#22
For clarification, #19 works great for me at solving the problem, just need someone to "review" the patch.
#23
Bugfix and a PHP ouch in the test. Please understand that this is a serious regression. The capability of site specific core hacks by copying a module and hacking there is rather important.
#24
+++ modules/system/system.module 9 Nov 2009 20:17:21 -0000@@ -2122,6 +2122,9 @@
foreach ($modules as $key => $module) {
+ // Filename key is used in system table so copy value.
+ $modules[$key]->filename = $module->uri;
Could we quickly clarify/improve this inline comment a bit? I don't get what it wants to tell me by reading it.
I'm on crack. Are you, too?
#25
#26
Here ya go.
#27
Marked #345031: Useless condition in system_get_files_database and #610214: Fatal error if you move or override an enabled module. sites/all/ vs sites/sitename/. Cannot disable, cannot recover. as duplicates.
#28
oo...lets get this in.
#29
Committed to HEAD.
#30
Automatically closed -- issue fixed for 2 weeks with no activity.