Currently, hook_install is invoked before the schema version is recorded for a newly installed module. If hook_install fails for some reason, the modules schema version is still SCHEMA_UNINSTALLED, which then prevents the module from actually being uninstalled.

The fix is pretty simple.

CommentFileSizeAuthor
#4 broken_test.patch3.73 KBmatt2000
schema_before_install.patch1.07 KBmatt2000
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

matt2000’s picture

matt2000 CreditAttribution: matt2000 commented 
<chx> thats one interesting approach
<chx> while before the module schema was left uninstalled
<chx> now you have it installed
<chx> and completley broken :p
<matt2000> chx, no, before the schema was installed, but MARKED as uninstalled.
<matt2000> note that the position of drupal_install_schema has not changed.

...i.e, we're just putting all the schema stuff together, without giving hook_install a chance to interrupt what should be a connected process.

matt2000’s picture

matt2000 CreditAttribution: matt2000 commented

Tests can be provided for this only after #793660: Check for failure of hook_install

chx’s picture

chx CreditAttribution: chx commented
Status: Needs review » Reviewed & tested by the community

The linked issue really causes me to frown. We need a consistent strategy for exceptions and so on -- this issue should not be held up by that.

matt2000’s picture

matt2000 CreditAttribution: matt2000 commented
Status: Reviewed & tested by the community » Needs review
FileSize
broken_test.patch3.73 KB

Here's a test that should fail if we have #793660: Check for failure of hook_install but not this, and should pass if we have both.

matt2000’s picture

matt2000 CreditAttribution: matt2000 commented
Status: Needs review » Reviewed & tested by the community

Didn't mean to reset chx. To be clear, only the patch at the top is RTBC. Patch in #4 is purely academic at this point.

Status: Reviewed & tested by the community » Needs work

The last submitted patch, broken_test.patch, failed testing.

matt2000’s picture

matt2000 CreditAttribution: matt2000 commented
Status: Needs work » Reviewed & tested by the community

well yeah, I said it would.

Dries’s picture

Dries CreditAttribution: Dries commented
Priority: Critical » Normal
Status: Reviewed & tested by the community » Needs work

This is a hack, but probably OK given that the current situation is broken too. I studied the surrounding code for a bit, and it looks OK -- a better fix will require some work so I committed this to CVS HEAD, but I'm marking this 'needs work', especially given that we have the beginnings of a test in this issue.

David_Rothstein’s picture

David_Rothstein CreditAttribution: David_Rothstein commented

Yeah, I see what this is trying to solve, but it does not really seem quite correct... Why would we mark something as installed that hasn't actually been installed yet? And if hook_install() fails, do we really expect that hook_uninstall() will do any better? - or that the user will even figure out what's going on enough to run it?

Seems like the correct fix would be more along the lines of moving this code back where it was, but also putting it in a try-catch block; then if an exception is caught, running drupal_uninstall_schema() immediately so as to restore things as close to the original state as possible. I am not sure if that should happen in this issue or the other one.

matt2000’s picture

matt2000 CreditAttribution: matt2000 commented

Why would we mark something as installed that hasn't actually been installed yet?

The correct question is, what exactly has been installed, and what hasn't? We DID install the schema, and so we mark it as installed; we might fail at some other kind of set-up which may or may not be properly termed 'installation', but that's a separate questions. (And a separate issue, as of right now.)

Putting schema install and module install in the same try{} block is not a good idea, IMHO. Putting them both in their own try{} blocks is probably useful, but may not be the best approach. As chx pointed out, try{} protects us only from exceptions, not from other kinds of fatal errors.

  • Dries committed 4d795f3 on 8.3.x 
    - Patch #793274 by matt2000: schema is left broken if hook_install()...

  • Dries committed 4d795f3 on 8.3.x 
    - Patch #793274 by matt2000: schema is left broken if hook_install()...

  • Dries committed 4d795f3 on 8.4.x 
    - Patch #793274 by matt2000: schema is left broken if hook_install()...

  • Dries committed 4d795f3 on 8.4.x 
    - Patch #793274 by matt2000: schema is left broken if hook_install()...

  • Dries committed 4d795f3 on 9.1.x 
    - Patch #793274 by matt2000: schema is left broken if hook_install()...