Dependent modules are still installed when required modules return errors in hook_requirements('install')
| Project: | Drupal |
| Version: | 7.x-dev |
| Component: | system.module |
| Category: | bug report |
| Priority: | critical |
| Assigned: | Unassigned |
| Status: | needs review |
If a module fails requirements in hook_requirements('install'), it is not installed. However, modules that depend on this module will still be installed. As an example use case, module B depends on module A. Module A fails hook_requirements('install') when a user attempts to install it. If the user attempts to install module A and module B at the same time, module A will not be installed while module B will be. This could lead to fatal errors and other unwanted behavior as the functions in module A will not be available to module B.
This patch modifies the logic in system_modules_submit() to prevent modules from being installed if they depend on a module that returns errors in hook_requirements('install'). I have unit tests 90% completed that fail when the patch is not applied and succeed when it is. I will post soon. This bug exists in D6 as well, and was discovered in the Search Lucene API module. A lot of extra coding had to be done to get around it.
Thanks,
Chris
| Attachment | Size | Status | Test result | Operations |
|---|---|---|---|---|
| requirements-dependency-fix.patch | 812 bytes | Idle | Failed: 13668 passes, 0 fails, 8 exceptions | View details | Re-test |
#1
Nice sleuthing. Is there any chance you would refactor this submit handler a bit? It is really hard to follow all the flow. And other code like drush wants to reuse parts of it.
In any case, we look forward to your tests on this.
#2
Hi Moshe.
Anything to help out the Drush project :-). I agree that the submit handler is due for refactoring, as it took me a while to figure out what was going on. I will take a crack at it and will post for review. In terms of Drush, I am assuming that you are looking for the dependency checking logic within the function, or am I off on this one? Either way, that should probably be broken out into a separate API function anyways IMHO.
Thanks,
Chris
#3
The last submitted patch failed testing.
#4
Moshe,
I was able to refactor the submit handler so that it is bit more transparent in terms of what it is doing. It also fixes the issue raised by this post. The code is a bit more compartmentalized, so parts can be separated out into API functions if you feel that is the avenue that should be taken. Also, I didn't want to add this in without discussion, but I think it would be helpful to alert the user if a module couldn't be installed because the one it is dependent on fails hook_requirements('install'). I'm not sure if it would be better to simply display a message or not allow the changes to be made. Thanks in advance for your input.
~Chris
#5
Finally got the unit tests in working order. The attached patch is the same as the one above with unit testing added.
#6
The last submitted patch failed testing.
#7
Seems like recent additions broke the patch. Will rework with latest version of HEAD and re-commit.
#8
Refreshed with latest version of HEAD.