hook_install/hook_uninstall doc does not specify that the implementations must live in the .install file

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.

Uhm ... what? Trying again.

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!

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 ...).

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

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

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

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.

Makes sense.

Committed and pushed to 7.x. Thanks!

Still needs backport to D6.

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.

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.

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".

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

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.

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

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?

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?

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: 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.

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.

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().

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!

committed and pushed.