The hooks that go in install files -- including hook_schema, hook_update_N, hook_modules_un/installed/en/disabled, should state whether the hook code has access to the module code and whether hooks are available to invoke.

As far as I can tell:

- hook_schema: depends on whether the module is being installed or uninstalled
- hook_modules_installed/enabled: has access to the module files and hooks from both the implementer module and the acting modules.
- hook_modules_uninstalled/disabled: has access to no module files or hooks. Module files can be loaded explicitly with drupal_load().

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

joachim’s picture

Turns out hook_modules_uninstalled() has to go in the module file, not the install: filed #1308610: move the location of hook_modules_uninstalled() to install files.

jhodgdon’s picture

Yeah, leave the modules_* hooks out of it -- they are of a different character than the other hooks (see http://drupal.org/node/1308610#comment-5114536)

So ... Do you have an answer to the question or are you just hoping someone else will research this and write the documentation you think needs to be written? If you'd like to do the research, that would be cool!

sven.lauer’s picture

Hm, this is really not a by-hook issue, but it generally depends: Starting with D7, the module file will be always loaded on install (and enable), but not on uninstall. But I do agree that the doc could be clearer on whether the functions defined in the .module files are available. In a nutshell: hooks that get called at uninstall (== hooks that MUST be in the .install file) cannot rely on the .module file being loaded (i.e. should not use functions declared therein or check carefully whether those are available).

joachim’s picture

@jhodgdon: the results of my research are in the OP and comment #1.

But surely the person who wrote the hooks in the first place would know? Documentation is not archaeology, as I occasionally say...

jhodgdon’s picture

Really, I thought writing documentation *was* pretty much archeology -- at least, when we are going back and fixing up docs that the original authors of the function didn't get right. :)

Sven: so can/should we patch something to add this information?

Jimsy’s picture

leave the modules_* hooks out of it man.

sven.lauer’s picture

Before rolling patches for this, let's at least wait until #1309298: Document that hook_(en|dis)able implementations should live in .install files is committed---saves us a reroll or two.

The idea would be to extend the current wording for, e.g. hook_enable() to

The hook is called every time the module is enabled. It should be implemented in the module's .install file, but the implementation can rely on the .module file being loaded.

And for things like hook_schema() that have to be available on uninstall:

A schema is defined by hook_schema() which must live in your module's .install file. As it is called at uninstall time, it cannot rely on the .module file to be loaded and hence should not use any functions defined there.

It seems a bit wordy/TMI, but this information is not really present right now.

joachim’s picture

> A schema is defined by hook_schema() which must live in your module's .install file. As it is called at uninstall time, it cannot rely on the .module file to be loaded and hence should not use any functions defined there.

I would be even more precise, and also suggest the workaround:

A schema is defined by hook_schema() which must live in your module's .install file. It is called at both install and uninstall time, and in the latter it cannot rely on the .module file to be loaded or hooks to be known. If the .module file is needed, it may be loaded with drupal_load().

sven.lauer’s picture

Hm, if we include

If the .module file is needed, it may be loaded with drupal_load().

I would feel compelled to warn people to not load the .module file on uninstall, as at uninstall time, an arbitrary number of modules that the current module depends on may be gone.

sven.lauer’s picture

Status: Active » Needs review
FileSize
2.23 KB

Rolling a patch, based on what I suggested in #7 and the refinement @joachim suggested in #8.

I did not touch the docblock of hook_uninstall(), as that one already mentions that the .module file won't be present (and warns about potentially missing dependencies).

jhodgdon’s picture

Status: Needs review » Needs work

Thanks -- I think this is good to document.

There are a few grammar/style things that need fixing:

a) "it cannot rely on the .module file to be loaded or hooks to be known." ==> "being loaded or hooks being known"

b) ".install file, but the implementation" ==> there are two spaces after the comma.

c)

+ * loaded. The hook will only be called the first time a module is
  * enabled or after it is re-enabled after being uninstalled. The module's

The wrapping needs to be modified here -- I am pretty sure "enabled" will fit on the previous line?

d) The wording here (which is repeated elsewhere):
"It should be implemented in the module's .install file, but the implementation can rely on the .module file being loaded."

I don't think that just because something is in the .install file, there is an assumption that the .module file will or won't be loaded, so using "but" kind of didn't make sense to me... Rather, I think the default assumption is that if code is part of a module, the module's other code would be loaded (which is why we have a special warning in hook_uninstall() about the code not being loaded). I think this should just be two separate sentences, without the "but" conjunction.

sven.lauer’s picture

Ugh, sorry for the style issues. Rerolling.

I had included the "but" because the first part of the sentence basically says "you should not put this hook in the .module file", so the "but you can rely ..." seemed to make sense. Also, at least with hook en/disable, We now have three unconnected sentences in a row, which seemed a bit stilted to me:

The hook is called every time the module is disabled. It should be implemented in the module's .install file. The implementation can rely on the .module file being loaded.

While looking at the hook_install doc, I noticed that it includes this:

 * See the Schema API documentation at
 * @link http://drupal.org/node/146843 http://drupal.org/node/146843 @endlink
 * for details on hook_schema and how database tables are defined.

... which does not seem to belong there? It belongs in the documentation of hook_schema (where it is actually duplicated).

Left this in for now, as it is a separate change. Follow-up issue?

sven.lauer’s picture

Status: Needs work » Needs review
jhodgdon’s picture

Status: Needs review » Reviewed & tested by the community

This patch looks OK to me. I agree that having three sentences with no connecting words together is not the optimal beautiful English prose, but then again I can't think of a connecting conjunction that would work to connect them, because they don't really have a definite relationship to each other.

I guess we could connect the last two:

It should be implemented in the module's .install file, and the implementation can rely on the .module file being loaded.

? I guess this is where you had "but" before... to me, "and" makes more sense. If you don't like that, then let's just go with RTBC for this patch.

I'm OK with the schema link being in hook_install(), since setting up database tables is something modules need to have done when they're installed, but if it bothers you, feel free to open up another issue. There should at least be an @see to hook_schema if this is removed.

sven.lauer’s picture

I say we leave the patch as is, without the "and".

jhodgdon’s picture

OK, I think we're all in agreement on RTBC then. :)

webchick’s picture

Status: Reviewed & tested by the community » Fixed

This looks like a nice improvement. I know I've been confused by this before as well.

Committed and pushed to 8.x and 7.x. Thanks!

Status: Fixed » Closed (fixed)

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