7

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:

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?
  • I want others to weigh in on the split, but I think it may be good. I am really in the why of #2. – mpdonadio Apr 3 '14 at 1:18
  • (btw, some of what is in the answer is stuff i already know, but that is nevertheless relevant and essential to make this useful for other people. And they confirm that splitting up is a common practice. So both get +1 from me.) – donquixote Apr 3 '14 at 13:39
4

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.

  • Thanks for the thoughtful breakdown. I needed a helpful shove to get me to consolidate a half dozen related but separate custom modules for a client into one. I think I'll stop trying to over think it, and simply require the extra files. A good naming convention for these files will help, too, and then the whole thing becomes much more parsible. – Charlie Schliesser Sep 18 '15 at 15:53
  • Why would you combine different modules into one? Separation is good! (depends, of course) – donquixote Sep 18 '15 at 21:18
  • Oh separation of concerns is a must! And modules that can remain abstract (I.e be modules), should. On this particular project, I seem to have at one point thought that the same paradigm applied to the business logic that was unfolding and evolving within the app. It's not really the same case, and several of the modules aren't really modules but collections of PAC logic that make no sense without each other. – Charlie Schliesser Sep 18 '15 at 21:27
  • Why don't you use hook_hook_info_alter() instead? Do the same considerations/problems apply if I use hook_hook_info_alter() to split the hook implementations in a separate file? – Елин Й. Mar 14 '16 at 12:10
  • hook_hook_info_alter() applies to all implementations of this hook. Thus, it will have unintended side effects on other modules. – donquixote Mar 14 '16 at 13:17
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
}
  • yes, seen that and done it myself. both of it. although I recently prefer just include_once __DIR__ . '/filename.inc'. So.. i guess we are clear then about question 1, it is far from unorthodox to split stuff up. Remains question two, whether anything bad could happen if you use hook_module_implements_alter() for that. Or how big the performance benefit could be for not including the file on an average request, where e.g. hook_menu() is not called. – donquixote Apr 3 '14 at 4:59
  • Btw I don't personally like the _foo_HOOK() trick all too much, it makes me look in two places instead of one.. – donquixote Apr 3 '14 at 5:00
  • Btw, you can simplify to module_load_include('defines.inc', 'foo'). But what's lrfm? – donquixote Apr 3 '14 at 13:41
  • lol, lrfm was the module I cut and pasted that out of and forgot to change to foo :) – Jimajamma Apr 3 '14 at 14:10
  • as for your simplification, yes, that would certainly work, but the first argument is supposed to be the type of file, and per the examples in api.drupal.org/api/drupal/includes%21module.inc/function/…, I imho doubt what the "drupal powers at be" intended, but certainly less typing. – Jimajamma Apr 3 '14 at 14:17
1

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.

  • The idea is to include the respective file only when it is needed. I am thinking about performance there, but I don't know the actual performance impact. I guess with APC or similar it does not make a big difference, but I never measured it. – donquixote Apr 3 '14 at 1:07
  • 1
    @donquixote if you are thinking about performance, then install APC or other opcode cache and don't bother with splitting so much. – Mołot Apr 3 '14 at 11:44

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.