Come together with the global Drupal community in Rotterdam, 28 Sept – 1 Oct 2026. Sessions, contribution, connection, and Early Bird savings until 8 June.Problem/Motivation
If there is an exception during a module's installation then error message/exception is not persistent.
Proposed resolution
Store the excpetion in a log to allow for later retrieval.
Remaining tasks
Reroll
Review
User interface changes
None
API changes
None
Original report by matt2000
If hook_install fails, it can leave the system in an invalid state. We need to know when this happens.
This patch is also required to make #793274: Schema is left broken if hook_install fails testable.
| Comment | File | Size | Author |
|---|---|---|---|
| #45 | 3002.diff.txt | 5.65 KB | bhanu951 |
Issue fork drupal-793660
Show commands
Start within a Git clone of the project using the version control instructions.
Or, if you do not have SSH keys set up on git.drupalcode.org:
changes
Comments
Comment #1
matt2000 commentedComment #2
berdirThis pretty much eats the original error, doesn't it? This would make it really hard to actually find and solve the bug (think of porting a module to D7 and something as simple as still having the schema install function calls in there...)
I think we should log the exception message too. There are two ways to do it:
- Use _drupal_decode_exception() and extend the watchdog text
- #711108: Richer exception reporting for watchdog() makes it possible to pass exceptions directly to watchdog and it will handle the decoding itself. Note that it would not allow to add a custom addtional text to the watchdog call, so you might want to decode it yourself.
Comment #3
David_Rothstein commentedI think that ideally the error would bubble up so that a calling function can use it; e.g. the calling function might want to drupal_set_message() to indicate the module did not install correctly. Also, as I mentioned in the other issue, I think rather than just logging the error we should be calling drupal_uninstall_schema() to restore it back to its uninstalled state.
Since #793274: Schema is left broken if hook_install fails was committed but purposely left open for further work to happen, would it now be better to close this issue in favor of that one?
Comment #4
matt2000 commentedD7 really suggests that Schema Install and Module Install are two separate and independent things. I think automatically calling durpal_uninstall_schema() is not always the right thing to do, and should we left to the user. As long as we correctly identify whether the schema was installed, we already provide the mechanism for the user to do this. (via uninstall.)
As an example, imagine a module which creates an Entity, and sets up a number of tables for that entity via hook schema, and then imports a base data set (e.g., postal codes for a geo-location module) and sets up some FieldsAPI fields in hook_install. Maybe the data import takes significant time.
If the schema installs correctly, and the data imports successfully, but the fieldsAPI fields are not created because some other modules created a fields with the same name, do we really want to force the user to go back to square one? We've wasted a lot of cycles on perfectly good work if we do.
Or what if the order is different, and we successfully create the Fields before we fail? We would need to also remove the fields automatically, otherwise we'll cause an error when they try to re-install.
So I'm inclined to think the best route is to continue to decouple schema from install, not rejoin them.
(That's also why two separate issues seemed appropriate, although I have no problem with closing this one, if the code here is definitely not useful, and that is probably the case.)
Comment #5
berdirYou are probably right, not sure :)
That imho doesn't change the fact that we want to move the exception handling out of module_enable() into the calling functions or do you disagree ?(The obvious difference would be additional modules won't be installed when the exception bubbles out of module_enable().)
I think we want to handle this differently based on where we are calling this, according to http://api.drupal.org/api/function/module_enable/7, we are using the function during installation, update functions, installing new modules (and simpletest, which is not listed there).
Not sure how to handle it... Adding an argument to decide between continue and re-throwing of the exception is probably ugly...
Comment #6
matt2000 commentedI think I get the concern, but I think we need something inside of module_enable in order to make #793274: Schema is left broken if hook_install fails testable.
Comment #7
moshe weitzman commentedIMO, this is not critical. Why should hook_install() fail? In your example, developer should have loaded data in separate stage. That stage should be a drush command or if web is required, then use batch api.
Comment #8
superspring commentedI like the idea of recording the exception in watchdog. This gives easy access to the error.
Attached is a patch which catches the error, records it and allows the other modules to be installed.
Comment #9
superspring commentedAfter a review with @chx the exception is thrown again to prevent compounding problems and to ensure the user is notified of the fault.
Comment #10
chx commentedI am not quite sure what the point in this after all if hook_install dies, the only way forward is to restore your site from backup as your site in an unknown state. It's not impossible that even after fixing whatever caused the exception and deleting manually the schema version, a second run will choke on something else -- namely the first half of the installer that already ran and might not be re-entrant. It's just easier to restore. And if you restore, your logs are gone, too. So... why?
Comment #10.0
superspring commentedUsing Issue Summary Template.
Comment #11
chx commentedOK, so the issue summary now contains the motivation: provide more information in the log. Might be useful for developers so why not.
Comment #12
webchickNo tests?
Comment #13
superspring commentedSame patch as above but with tests.
Comment #14
swentel commentedWe don't really do urban language in our comments :)
Same here.
Comment #15
superspring commentedSame patch with less 'urban language'.
Comment #16
xen commentedChecking if this is still current....
Comment #17
xen commentedFixed up patch to latest head. module_enable doesn't exist anymore, the test modules info file needed rewriting to yaml, and other bits and pieces.
Comment #17.0
xen commentedForgot username.
Comment #18
jhedstromComment #19
kerby70 commentedReroll attached.
Some of the comments that were being added to are already in now.
Comment #20
mgiffordNo longer applies.
Comment #31
larowlanIs this still relevant?
Comment #33
quietone commentedI looked at ModuleInstaller and this change could still be applied so I am setting this to NW.
Comment #34
bhanu951 commentedComment #35
_utsavsharma commenteddid not find these files on 9.2.x
common_test_install_helper.module, common_test_install_helper.install, common_test_install_helper.info.yml , ExceptionTest.php.
Reroll for 9.2.x
Comment #36
bhanu951 commentedComment #40
bhanu951 commentedRe- Rolled the patch updated Test for 10.1.x branch.
Comment #41
smustgrave commentedHiding the files as the fix appears in the MR.
MR seems to have failures
Did not review.
Comment #43
andypostIt's often happening when some config entity is changed in hook_install but
$syncingvariable is not checkedIt fails randomly when install happening from existing config
Comment #44
quietone commentedChanging to the DX special tag defined on Issue tags -- special tags.
Comment #45
bhanu951 commentedUploading MR changes as backup patch before changing target branch.
Comment #46
nicxvan commentedComment #47
nicxvan commentedThis needs a recollection but would probably be nice to get in.