API page: http://api.drupal.org/api/drupal/modules--system--system.api.php/functio...

Describe the problem you have found:

hook_install and hook_uninstall do not actually mention that they need to go in a .install file! I guess it's something so basic as to be assumed knowledge ... but it should probably be mentioned.

hook_update_N and hook_schema both correctly specify this.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

sven.lauer’s picture

Title: Documentation problem with hook_install » hook_install/hook_uninstall doc does not specify that the implementations must live in the .install file
Version: 7.x-dev » 8.x-dev
Status: Active » Needs review
FileSize
1.83 KB

Nice catch, this clearly should be documented!

(Moving to 8.x, as patches go in there first---I assume this is also true for doc patches?)

Attaching a patch that clarifies where the hook implementations should go.

Status: Needs review » Needs work

The last submitted patch, 1247626-doc_hook_install_uninstall.patch, failed testing.

sven.lauer’s picture

Uhm ... what? Trying again.

sven.lauer’s picture

Status: Needs work » Needs review
jhodgdon’s picture

Issue tags: +Needs backport to D7

Not technically correct, I think? You can also put these hooks in the main .module file. It's not recommended practice, but it will work.

Also, when you move an issue to d8 (and yes, docs patches have the same convention), please add the backport tag. Thanks!

kingandy’s picture

I don't think hook_uninstall, at least, can just go in .module. From the D7/8 hook_uninstall docs:

When hook_uninstall() is called, your module will already be disabled, so its .module file will not be automatically included. If you need to call API functions from your .module file in this hook, use drupal_load() to make them available.

(Emphasis mine.)

I don't honestly know if hook_install suffers under the same constraint.

sven.lauer’s picture

Ah ... yes, I was sure about hook_uninstall(), but I extrapolated to hook_install() and from D6.

Indeed, in D7/8 in module_enable() in module.inc we have

   foreach ($module_list as $module) {
      ...
      // Load the module's code.
      drupal_load('module', $module);
      module_load_install($module);
      ...

So yes, the module file does get loaded, so if it would contain the hook_install() implementation, that would work.
See also http://drupal.org/node/224333#module_file_during_install (though this is clearly not intended to enable people put their hook_install() implementations in the .module file).

On the other hand, for hook_uninstall(), the existing comment quoted by kingandy is correct (from drupal_uninstall_modules() in install.inc):

  foreach ($module_list as $module) {
    // Uninstall the module.
    module_load_install($module);
    module_invoke($module, 'uninstall');
    drupal_uninstall_schema($module);
   ...

So, the module file does NOT get loaded on uninstall (and it should not, as modules that the current module depends on might be uninstalled and even removed from the codebase at the point hook_uninstall is called).

Basically, everything that has to be accessible during uninstall has to go into the .install file, while things like hook_install() implementations SHOULD go there, too. hook_schema must be in the .install file for this reason.

Attaching a modified patch.

Also tagging for backport to D6, where where things are as described in the first patch.

A question is whether the API really should allow to put the hook_install() implementation into the .module file, or whether the fact that this works is an unintended side effect (which does not make much of difference, but ...).

jhodgdon’s picture

Status: Needs review » Needs work

That text looks good to me, thanks!

The line wrapping needs to be fixed here:

+ * The uninstall hook must be implemented in the module's .install file.
+ * It will fire when the module gets uninstalled but before the
  * module's database tables are removed, allowing your module to query its own
sven.lauer’s picture

Status: Needs work » Needs review
FileSize
2.12 KB

Re-roll with line wrapping fix. No changes in wording.

jhodgdon’s picture

Status: Needs review » Reviewed & tested by the community

Looks fine for d8/d7. Then I guess mark for D6 status "to be ported". Thanks!

catch’s picture

Version: 8.x-dev » 7.x-dev

I thought we might need to add this for hook_schema() as well, but that's already got a similar note. Committed to 8.x, moving to 7.x for webchick.

webchick’s picture

Status: Reviewed & tested by the community » Fixed

Makes sense.

Committed and pushed to 7.x. Thanks!

jhodgdon’s picture

Version: 7.x-dev » 6.x-dev
Status: Fixed » Patch (to be ported)

Still needs backport to D6.

sven.lauer’s picture

Forgive my ignorance, but where does the hook documentation live in D6? It is not in my checkout of 6.x ...

Never did a 6.x doc patch before. I would be willing to do the backporting, if I knew how/where.

sven.lauer’s picture

Forgive my ignorance, but where does the hook documentation live in D6? It is not in my checkout of 6.x ...

Never did a 6.x doc patch before. I would be willing to do the backporting, if I knew how/where.

kingandy’s picture

FileSize
483 bytes

hook_uninstall (and its doc comment) seems to be in developer/hooks/install.php. I'm afraid I've no idea if this file would be present in a standard checkout, or if the 'developer' folder is something that's been added to the API version for documentation purposes.

I've manually built a version of this patch using the source presented on api.drupal.org (http://api.drupal.org/api/drupal/developer--hooks--install.php/6/source). The hook_schema doc already mentions that it must reside in .install and needs to be the current version of the schema, and it doesn't get called automatically in 6.x anyway, so that part of the patch is basically unnecessary ... similarly a lot of the hook_uninstall comment deals with when it runs in relation to removing the schema, which could go as well (again, the schema doesn't get automatically removed in 6.x). I've ended up with about two lines of difference.

kingandy’s picture

FileSize
1.06 KB

Update: I completely forgot the hook_install requirement (as noted in #7, hook_install does indeed need to be in the .install file in 6.x). Modified patch attached.

jhodgdon’s picture

Project: Drupal core » Documentation
Version: 6.x-dev »
Component: documentation » API documentation files
Status: Patch (to be ported) » Needs work

Oh sorry! The files are in the Documentation project git repository. Moving to that project.

So the patch will need to be rerolled.

One other thing:
"in the modules .install file." -- should be "module's".

sven.lauer’s picture

Status: Needs work » Needs review
Issue tags: -Needs backport to D7
FileSize
1.56 KB

Bringing this one home.

Re-rolled the patch against the documentation project's repository (the 6.x version). Followed kingandy's #17, as his reasoning about omitting the hook_schema related stuff is sound; fixing the apostrophe issue noted by jhodgdon

delraydavis’s picture

I can verify that the install hook will function properly in the module file, however the uninstall hook must be in the .install file.

A little janky I know but I am in the process of writing up my first module and stumbling through is the best way to learn.

I tested this thoroughly using landmark inserts and deletes into the database in conjunction with the install and uninstall hooks.

It should be noted both hooks function perfectly in the install file. So that's what I am staying with.

webchick’s picture

Wow, interesting. I had no idea that hook_install() would work in .module. :) Nice find!

jhodgdon’s picture

So it sounds like we should say for d6 that hook_install() should be in the .install file, rather than "must" as in the current patch?

sven.lauer’s picture

The report in #20 surprised me a little (okay, a lot), so I ran my own quick test, confirming that in D6, hook_install DOES NOT work if it is in the module file. (I did something very simple---just a hook implementation that only had a drupal_set_message() in it. The message did not show when the hook implementation was in the module file.

As I showed in #7, in D7, the .module file gets loaded on install. Here is the relevant portion of install.inc:

function _drupal_install_module($module) {
  if (drupal_get_installed_schema_version($module, TRUE) == SCHEMA_UNINSTALLED) {
    module_load_install($module);
    module_invoke($module, 'install');
    $versions = drupal_get_schema_versions($module);
    drupal_set_installed_schema_version($module, $versions ? max($versions) : SCHEMA_INSTALLED);
    return TRUE;
  }
}

Note that only the install file gets loaded (via module_load_install(), which does NOT load the module file). I also do not see were else it would be loaded.

Finally, in the module update guide, it says:

The .module file and its functions are now loaded during the install process, as long as hook_requirements() is satisfied. In Drupal 6, a .install file that wanted to use a function from the .module file would have to load it explicitly, but that is no longer the case. Now, as long as hook_requirements() is satisfied, the .module file is loaded and available during the install process.

http://drupal.org/node/224333#module_file_during_install

So either I am very much confused, or the hook_install() canNOT be implemented in the .module file in D6.

@delraydavis: What specific version did you use for your test?

jhodgdon’s picture

Sven: Just to clarify on your test - did the set message work when the install hook was in the .install file? I am never certain if messages are displayed during module installation. A database update or something else more durable might be a better test (though definitely more work to try).

I'm pretty sure than in D6 I have made modules with either install or enable hooks and put the entire thing in one .module file (not the best practice, but anyway)... but I would have to search around for examples...

sven.lauer’s picture

Sven: Just to clarify on your test - did the set message work when the install hook was in the .install file? I am never certain if messages are displayed during module installation

Yes. As I was not sure either, I did a separate test run to putting the hook in the .install file, and then the message showed.

sven.lauer’s picture

And, just to be completely sure, I ran another test using variable_set() in the install hook (this is with a current git checkout of the 6.x branch).

With this minimal test_hook_install_location.module file (no .install file):

function test_hook_install_location_install() {
  variable_set('test_hook_install_location', 'hook_install() implementation found!');
}

I ran the following:

$ drush -y site-install
[...]
$ drush -y en test_hook_install_location
[...]
test_hook_install_location was enabled successfully.                 [ok]
$ drush vget test_hook_install_location
No matching variable found.                                          [error]

Clearly, the variable is not set after the module was installed/enabled. And jsut to be sure, I followed this up with:

$ mv sites/all/modules/test_hook_install_location/test_hook_install_location.module sites/all/modules/test_hook_install_location/test_hook_install_location.install
$ touch sites/all/modules/test_hook_install_location/test_hook_install_location.module
$ drush -y site-install
[...]
$ drush en test_hook_install_location
[...]
test_hook_install_location was enabled successfully.                 [ok]
$ drush vget test_hook_install_location
test_hook_install_location: "hook_install() implementation found!"

This shows that the hook implementation does exactly what it should do, and it is recognized if it is in the .install file, while the same implementation in the .module file did nothing.

So the behavior is exactly as the patch in #19 says.

sven.lauer’s picture

And re: the second part of #24:

I'm pretty sure than in D6 I have made modules with either install or enable hooks and put the entire thing in one .module file (not the best practice, but anyway)... but I would have to search around for examples...

Hm ... with enable hooks, this would work (in either D6 and D7, I think), but it really does not seem to be working with hook_install().

jhodgdon’s picture

Status: Needs review » Reviewed & tested by the community

Sven - THANK YOU for this extensive and definitive test! My memory must be faulty, and perhaps the other person who tested was not quite as careful.

So it looks like the patch in #19 is RTBC for the docs project, Drupal 6 version. Thanks!

jhodgdon’s picture

Status: Reviewed & tested by the community » Fixed

committed and pushed.

Automatically closed -- issue fixed for 2 weeks with no activity.