Steps to reproduce: Select Rules on admin/modules and accept enabling the dependencies (Entity and Entity Metadata) on the confirmation form. You get a white screen of death and your site is completely (unrecoverable) broken.
This happens because we assume that the module dependencies are ordered correctly in system_modules_submit(), but they are not. This patch delegates dependency resolution to module_enable(). Could have a performance impact, but in the current state the dependencies are just ordered randomly wrong.
Also tested with wsclient and profile2, same wrong dependency ordering.
Originally coming from #853782: Call to undefined function entity_metadata_text_formatted_properties().
Comments
Comment #1
klausiWorkaround for now: Enable Entity CRUD API and Entity Metadata before you enable Rules.
Comment #2
rszrama commentedThis has been affecting us w/ Commerce, too.
#906668: Installation fails when all modules are enabled at once - FieldException errors
Will give this a quick review...
Comment #3
rszrama commentedOn a quick test, this does seem to resolve my problems and certainly avoids any FieldException errors. My test was fairly complex, in that I simply selected the Commerce UI modules and let the form figure out it needed to enable all the Commerce API modules, their various dependencies, and their dependencies dependencies. : P
That said, I don't know enough about the changes to that submission form from alpha7 to HEAD (i.e. where'd the rsort come from?) to say this patch doesn't break other functionality. Just a +1 on the apparent fix.
Comment #4
klausiUh, this is even easier. Someone made a silly mistake by using rsort() instead of ksort() (or forgot to update the sorting function after switching module names and dependency weights).
Fixing a critical core issue by adding just one character, that would be nice :-D
Comment #5
catchIt should be possible to write a test for this.
Comment #6
marcvangendKlausi, just to make sure: your comment says "rsort() instead of ksort()" (reverse sort instead of key sort) but in the patch you're replacing
rsortwithkrsort(reverse key sort). Which one is correct, the patch or the comment?Comment #7
carlos8f commentedI introduced this bug as part of #651086: Cache clearing is an ineffective mess in module_enable() and system_modules_submit(). Certainly I meant to use krsort()! Let's write the tests in a follow-up.
Comment #8
carlos8f commentedThis is definitely a beta-blocker. Sorry about creating such a silly bug :-/
Comment #9
sunSorry, but we need to add tests to make sure we don't break this again.
Comment #10
carlos8f commentedWorking on tests.
Comment #11
carlos8f commentedTests inspired by #652394: Aggressive watchdog message assertion. We can see if the enable order is correct by examining the watchdog table.
Comment #12
carlos8f commentedShowing that the fix works, one patch contains the krsort() + tests and the other is just tests.
Comment #14
chx commentedthere is a resetAll method in simpletest to be used after module enabling.
Comment #15
carlos8f commentedNow we use $this->resetAll() (it was nice to learn about that!) plus I've fixed a slight inaccuracy by ordering {watchdog} by wid instead of timestamp (since timestamp will probably be the same for these entries), and a slight typo (
module_test has createsmodule_test creates). With this we should be good-to-go.Comment #16
chx commentedThat's one fine test.
Comment #17
Anonymous (not verified) commentednice work.
nitpick that i'm ok with leaving as is if i'm the only one who cares - can we implement hook_modules_enabled() in the test module, and check the order there? that feels a bit simpler and cleaner than querying and checking watchdog entries.
Comment #18
Anonymous (not verified) commenteddidn't mean to change the status, cross posted with chx. i'm happy for this to go in as is.
in case people don't like my suggestion, here's the idea behind it - as it is, the test relies on watchdog module and certain configuration, but doesn't explicitly enable the module or set up that config. like many other tests, it relies on the "hey, we have standard profile bloat, so this will work", which will make it harder to port to the testing profile later.
Comment #19
carlos8f commented@justinrandell fair enough :P Here we go again with parallel testing so we can see that the fix/test works.
Comment #20
Anonymous (not verified) commentedawesome, thanks carlos8f, RTBC.
Comment #21
webchickExxxxxxxcellent.
Great work, folks! Committed to HEAD. :)