hook_module_implements_alter() to put hook implementations into separate files - bad practice?

Question

In a number of modules I wrote, I use hook_module_implements_alter() to split up the *.module file, and to move hook implementations into separate files.

The main reason is that I passionately hate long files. (and yes, this does make me unhappy about a lot of the files in Drupal 7 and contrib)

Examples:

• nestedbox_core_module_implements_alter()
• xautoload_module_implements_alter()

I know there are a few things to consider:

1. It might not work for all hooks. E.g. boot or install, or hooks that are not called via module_implements().
2. It is important to include the respective file from within the hook_module_implements_alter() implementation, due to the way that module_implements() works.

Now I heard from a long-time and well-known Drupal developer that this practice is problematic. https://drupal.org/node/2230319 (topic starter, and comments #5 and #11).

Before I start moving functions around, I would like to hear from others whether and why this is bad, both technically and aesthetically.

EDIT: I guess this could be split in two questions:

1. Is it good or bad to split up your *.module file, so that hook implementations live elsewhere?
2. Is hook_module_implements_alter() reliable enough, or should I rather use include_once __DIR__ . '/MYMODULE.filename.inc' directly in the *.module file? What could possibly go wrong by using hook_module_implements_alter() for this?

Answer 1

I just found the piece of information that was missing.
EDIT: And another one.

Problem with module_invoke()

module_invoke_all() always calls module_implements(), which checks the module include file by checking both hook_hook_info() and hook_module_implements_alter().

However, module_invoke() only checks hook_hook_info() and not hook_module_implements_alter().

module_invoke() is called e.g. in _block_render_blocks() for hook_block_view(). This means that hook_module_implements_alter() is not a good solution to split a hook_block_view() implementation into a separate file.

This has been a problem in Crumbs, see https://www.drupal.org/node/2328535.

For other hooks it generally works ok, but you never know if a specific module wants to use module_invoke() instead of module_invoke_all().

module_implements() cache polluted in bootstrap.

Another problem is caused by a core issue reported here:
module_implements_cache() can be polluted by hook_boot() implementations calling module_invoke_all() directly or indirectly

This applies only to non-boot modules, where it can happen that the implementation of hook_module_implements_alter() is never discovered.

1. An unexpected chain of events causes module_implements($hook) being called from hook_boot().
   The $hook can be any random hook, it does not need to be related to the module we are working on.
2. This causes module_implements('module_implements_alter') to be called.
3. Drupal looks for all implementations of hook_module_implements_alter(). At this time, MODULENAME.module is not included yet.
   Therefore, for the rest of the request, Drupal assumes that the implementation MODULENAME_module_implements_alter() does not exist.
4. Later in the request, implementations of other hooks are being discovered, but the implementations by MODULENAME are not being found, because MODULENAME_module_implements_alter() is not being executed.

Alternatives

require_once

Alternatives have already been mentioned. Instead of these tricks, one can include the file directly. There may be a tiny performance impact but this is probably overshadowed by a lot of other things in Drupal.

But, instead of using module_load_include(), I would suggest a more direct solution:

require_once __DIR__ . '/crumbs.block.inc';

Or to be PHP 5.2 compatible:

require_once dirname(__FILE__) . '/crumbs.block.inc';

Why not module_load_include()?

There is generally no need to call module_load_include(), it could even be detrimental, if the *.module file is included from you-dont-know-where (e.g. from settings.php), and you don't know if module_load_include() is available.

This function is mostly there to help you determine the location of other modules. But if you are including within the same module, it is generally safer and easier and faster to work with relative file paths and explicit require_once.

Call to helper function

As mentioned by Jimajamma, you can also implement the hook in the main *.module file, and then include another file and call a helper function from there. This is done e.g. in Display suite.

This solution is ok, although I think it still clutters the main file, and gives you even more places to look.

And here likewise you can use require_once instead of module_load_include(), if it is within the same module.

Answer 2

Any major project I have worked on has broken things up into manageable and easily assignable pieces; basically ending up like @tenken mentioned here, or to the point where foo.module ends up being only includes:

<?php

module_load_include('inc', 'foo', 'foo.defines');         // bring in our define()s
module_load_include('inc', 'foo', 'foo.HOOK1');           // bring in our HOOK1 functions
module_load_include('inc', 'foo', 'foo.HOOK2');           // bring in our HOOK2 functions

// ...etc

Of course, I have also seen, and used, situations where admin only or otherwise rarely used hooks are included like:

function foo_HOOK(/* $args */) {
  module_load_include('inc', 'foo', 'foo.HOOK');
  return _foo_HOOK(/* $args */);  // _foo_hook() is defined in foo.HOOK.inc
}

Answer 3

If I try to breakup my .module file across multiple files I tend to do what Features for D7 does for its "included" files which it uses include_once() for:

include_once('MYMODULE.features.inc');

For D7 I still use my module's name "function namespacing" for these included files, and private module functions typically are prefixed as "_MYMODULE_foo()".

This answer kinda ignores your desired use of Autoloading classes and any OO related patterns you're trying to use within your code (code dependencies, etc).

In your linked issue it sounds like you want to use hook_module_implements_alter() to be a readable index of what files have what functions in your .module file. I think sane 'MYMODULE.PURPOSE.inc' filenaming is enough to clue a developer of the use of that include file within your module.