Install non-default modules one at a time to ensure proper module list is maintained during installation hooks
Jim Hollcraft - May 26, 2009 - 00:29
| Project: | Drupal |
| Version: | 6.x-dev |
| Component: | base system |
| Category: | bug report |
| Priority: | critical |
| Assigned: | boombatower |
| Status: | needs work |
Description
When date is installed using the setup function in simpletest, the .install code in the following module throws exceptions. Doing the following causes the install code in 'cck_date_test' to throw exceptions:
// Install date.
function setUp() {
parent::setUp('content', 'text', 'date_api', 'date_timezone', 'date', 'cck_date_test');
}
However, just removing date, eliminates the exceptions and test code run prefectly:
// Don't install date.
function setUp() {
parent::setUp('content', 'text', 'date_api', 'date_timezone', 'cck_date_test');
}
Strangely, if the code in the .install file is removed and placed in a test, no exceptions are thrown.
Attached is a small module I wrote just to demonstrate this problem.
| Attachment | Size | Status | Test result | Operations |
|---|---|---|---|---|
| cck_date_test.zip | 3.8 KB | Ignored | None | None |
#1
PHP Version 5.2.6
#2
#3
FWIW, here are the exceptions I'm getting when I try and run this:
I get the views one on each test in the group.
#4
The stack trace for this looks something like this:
1. SimpleTest enables the following modules in sequence:
- content
- text
- date api
- date timezone
- date
- cck_date_test (custom module which programmatically creates a node type with a text field)
If we remove the date stuff and sub in something like number, things work fine. So it's something that one of the date-related modules is doing on install (or that's our current theory).
2. When cck_date_test_install() calls http://api.lullabot.com/content_field_instance_create (which then calls http://api.lullabot.com/content_field_instance_expand), things start to break down because
$widget_types = _content_widget_types();(http://api.lullabot.com/_content_widget_types) is returning an empty array. Since the code here obviously expects this to never happen, exceptions start flying fast and furious.3. The only thing "special" about date module vs. other content types is that it's doing a call to date_install_clear(); at the end of its various hook_install() / hook_enable() / hook_uninstall(), etc. calls. Unfortunately, cvs annotate doesn't provide much of a clue as to why this was done. :(
4. And unfortunately, even when commenting out those calls, the issue still occurs. :( So we're left to wonder if it's not something deeper in the system somewhere (CCK, Date API, or SimpleTest?) that's causing the issue.
I know that's not a lot to go on, but any ideas? :\
#5
The patch at http://drupal.org/node/360925#comment-1627568 fixes one the table related "User warning" on all test.
#6
Placing
<?phptrigger_error(print_r(_content_type_info(TRUE), TRUE));
?>
in
cck_date_test_install()reveals that cck does not have any widget types as the errors seem to show.Array ( [field types] => Array ( ) [widget types] => Array ( ) [fields] => Array ( ) [content types] => Array ( ) )#7
Part of the problem seems to be that the module_list() is incomplete.
<?phparray ( 'system' => 'system', 'date_api' => 'date_api', 'date_timezone' => 'date_timezone', )
?>
And interestingly every time
module_list(TRUE);is called another set of exceptions is generated:Undefined index: text Notice content.crud.inc 235 content_field_instance_create()Undefined index: text_textarea Notice content.crud.inc 163 content_field_instance_collapse()
#8
After adding a ton of debug statements I think I discovered where the issue is. It is more rudimentary then originally thought.
The module_list() only contains the full list after
drupal_install_modules()has completed.cck_date_test_install()is called during its execution and does not have the full module list and therefore cck doesn't know about any widgets.This seems like it could be solved by installing core modules then additional modules. I will try that.
#9
Yep, that fixed !!!
Patch coming need to clean this up. Seems like we should fix this in core and then backport, patch should apply easily for those waiting on it.
#10
I believe this is the holy grail of SimpleTest bugs. Already a number of issues were marked duplicates of this one and I have seen plenty of others. This bug fits the description needed to causes a wide variety of bugs.
I have tested this on SimpleTest 6.x-2.x with the test described above and it works properly. Lets hope D7 is happy with it. :)
#11
This also fixes the bug at #440824: Reset module_list() and module_implements() on test setup.. (or at least in my case)
#12
Awesome work, Jimmy! :D
Is this something we can write a test case for? DAMN that was annoying to debug...
#13
Should be able to make a test module that calls a hook or something else that requires the module list to be intact during install time. I'll get to this a bit later.
#14
D6 patch, for testing.
#15
The last submitted patch failed testing.
#16
Hmm, I thought D6 was the keyword. I'll have to check that
#17
Subscribe. This patch is effectively fixing some nasty test failures we were having in #372743: Body and teaser as fields.
#18
boombatower: the patch in #14 fixes the issue for me
#19
Subscribe. Since the bug appears to be within
drupal_install_modules()itself, then for D7 maybe it would be better to fix it there?For a second I also thought this might be a symptom of #222109: $fixed_list breaks module_list() but I think it is separate...
#20
I would be fine with a fix in drupal_install_modules for D7, and the patch for D6. Although technically this is a bug in D6 as well and thus it could be backported.
#21
Indeed, the bug has nothing to do with Simpletest. On my installation profiles, I have to do ugly stuff like:
<?php// Load first the parents of all CCK modules.
drupal_install_modules(array(
'content',
'content_copy',
'imageapi',
'imageapi_imagemagick',
'imagecache',
));
// Load content modules that break if loaded at once.
drupal_install_modules(array(
'filefield',
'imagefield',
'text',
'number',
'fieldgroup',
'optionwidgets',
'nodereference',
'userreference',
));
?>
We need to rebuild the registry and the module cache between each installation, in
_drupal_install_module().#22
Appears to only be issue in D6 after some tinkering. I'll post further details shortly.
EDIT: I believe I know why.
#23
I have included patches for Drupal 6 & 7 that add tests that perform the exact same check. The one for D6 fails and the one for D7 passes. If you apply the D6 patch to SimpleTest the D6 test also passes. The tests check that the hook_menu() is available for a different module.
I believe this is due to the registry in D7, as in D6 the only file loaded during install is the .install file and in D7 the registry will auto load hooks which is the entire issue behind this. The module list and hook implementations is out of date and thus CCK and other modules do not work properly at install time.
I have placed
module_list(TRUE, FALSE)andmodule_implements('', FALSE, TRUE)in a number of places as well as inside_drupal_install_modulewithout success. It has to be related to either the order of module installation (if it gets out of order) or something else that occurs during thedrupal_install_modules()chain since my D6 patch forces SimpleTest to non-install core modules one at a time and it works.Still working on this.
#24
By changing:
<?phparray_filter($module_list, '_drupal_install_module');
module_enable($module_list);
?>
to:
<?phpforeach ($module_list as $module) {
_drupal_install_module($module);
module_enable(array($module));
}
?>
it works. Basically the same thing the SimpleTest patch was doing. I was trying more things in _drupal_install_module(), but it appears to want them together which makes sense, as hooks cannot be called until the module is enabled. This seems like a logic flaw which still exists in core, but seems like it is being hidden by the registry.
I am not sure this is a true solution, but it demonstrates the issue and a fix as the tests pass with patch and without SimpleTest patch.
#25
Sorry, my fault, i dropped the ball after graph.inc got in, modules should be batched really, and if i remember correctly then graph.inc gives you a pretty numbering of which modules goes in which batch,
#26
Batching modules will account for explicit dependencies, right ? But what if module foo does
if module_exists('bar')in its install process without an explicit dependency ('progressive enhancement') ?OTOH, rebuilding the registry after each module install raises the chance of timeout...
#27
This patch might be holding up #372743: Body and teaser as fields which is a big patch. Bumping this to critical for prioritization purposes.
#28
subscribe
#29
I am not sure exactly what chx is suggesting in #25 so I need further clarification or someone to write the patch.
#30
I *think* chx meant using graph.inc to sort out dependencies and determine sets of independent modules that can be installed together :
drupal_install_modules(1st set of modules);drupal_install_modules(2nd set of modules, that have dependencies on modules in the 1st set);
...
I'm still wondering about my comment in #26, though.
Also, not tightly related, but it might belong to the same design topic (or not - I'm not familiar enough with these areas), and is also a blocker for 'Body as field' : #479200: missing node_rebuild_types() in setUp()
#31
Perhaps we should commit the patch in #24 or the SimpleTest patch and leave the issue open for a cleaner fix since there seems to be a number of issues blocked by this.
#32
Appears to be another instance of this bug: http://drewish.com/node/134.
#33
Note that the patch in #6 got committed to D7 along with #372743: Body and teaser as fields, which was subject to massive reroll hell, so waiting for The Right Fix to be figured out here was getting harder and harder.
From comment #188 over there:
"If the fixes are deemed less-than-perfect, they can be polished later on by the competent and interested parties, which currently do not seem to care too much (also because the bug can only be reproduced by patching HEAD right now). If needed, the 'right' fix will be easier to fine-tune with the BAF patch in, since any 'wrong' fix will simply cause test failures."
Attached is the exact code that got committed - note the additional
node_type_clear();(explained in the same #188 comment in the Body as Field thread)back to CNW, if people want to advocate for a better fix.
#34
So it is clear this is a problem in core...what method do we want to take to solve this...seems like we should make a change to the drupal core install system...but which way.
As I've said before...given an excepted method of attack I am ready and willing to work on this.