Standardize lazy-loading of optional include files

A reference to #557542: Cache module_implements() would be nice to understand the purpose of this patch. ;)

Very much in approval of this. Before setting it to RTBC, is there any place where we could use module_include_all? I believe the Token API uses modulename.token.inc.....

+1

If we introduce a cache for module_implements(), we need to make sure that module_include_all() is called for all possible groups, before any call to function_exists() or get_defined_functions().

One solution is to let module_implements() take the $group as a parameter, call module_include_all, and then do the function_exists() stuff to build its own cache.

If we want to scan for hook implementations all at once, this would not be possible, because we don't have a list of hook and group names available.

What about a bit of caching to remember which groups have already been included?

I'm afraid we can get some inconsistencies here, if module developers don't play as they are supposed to.

hook_hook_info()

The hook_hook_info() concept suggests that each hook is owned by exactly one module - similar to a permission, or a menu entry. I'm not sure if this is a reasonable way of thinking, at least it does not reflect how hooks used to work in D6:

• every module can implement hook_hookname.
• every module can call ('invoke') hook_hookname via module_invoke_all('hookname')
• Until now, a hook can be seen as a piece of a vocabulary. It is not owned by anything or anyone, it is rather a convention to let different players interact.

hook_hook_info will change the hook concept a lot, because it lets a hooks be 'owned' by one module.

Technical side effects of hook_hook_info():

• Even if the one module that claims to be the owner of hook_hookname (by implementing hook_hook_info) is disabled, other modules can still call module_invoke_all('hookname'), but now the groups will no longer be auto-included.
• Theoretically, different modules can define different group names for the same hook name. So, you get auto includes depending on which modules are defined.

This effect can be confusing.
In fact it can sometimes be useful, because it allows modules to conditionally include hook implementation files based on which other modules are defined.
But this mechanic is not very transparent, and can cause some confusing side effects if you use an older version of module_a with module_a_hook_hook_info() with a newer version of module_b with a "module_b.groupname.inc" file.

$group as an argument to module_implements()

The other solution is to pass the $group name as an argument in module_invoke_all() and module_implements(). Problems:

• If the hook has a group name, then it must be assured that every call to this hook has the $group argument, and it has to be the same group name with each call.
• If hook_hookname is 'born' with no group name, and at some later point it should get one, then all modules implementing that hook need to add the group name to their function calls. If only some modules do it, you will get terrible inconsistencies and side effects.

Example for the possible inconsistencies:

// called in the newer module A:
$implementations_A = module_implements('hookname', 'groupname');

// called in the older module B, which did not know about 'groupname':
$implementations_B = module_implements('hookname', NULL);

// result: both arrays are the same, because all the conditional files were included with A.

vs

// called in the older module B, which did not know about 'groupname':
$implementations_B = module_implements('hookname', NULL);

// called in the newer module A:
$implementations_A = module_implements('hookname', 'groupname');

// result: $implementations_A can contain more elements than $implementations_B.

Combination of hook_hook_info and $group argument in module_implements()

I don't know if you see the two things as alternatives or if you want to combine them.
Combining them does not really solve the problem, instead you have both problems at once.

The way out?

Possible solutions I see:

1. Be really strict about how contributed modules introduce new hooks, and never introduce a group name after a hook has been used the first time. I don't know if this is realistic.
2. Make group name == hook name. This can result in a lot of small files, but I don't see it as such a big problem.
3. Leave all hook implementations in *.module (and files that are always included from there). You can make them four-liners that require a *.inc file and then call a "private" function living in that *.inc file. This is not as nice, but it is a very solid solution.
4. Allow modules to "announce" (via the *.info file, for instance, or a hook_available_functions) hook implementations that are not in *.module and not included by default, and specify an include file for each of them. If the include file information is omitted, the module has to take care for itself to include these files - giving it the chance to make it a conditional include.
5. In addition to that, allow modules to define (via the *.info file, for instance) which additional files to include before doing any hook scans based on function_exists() or get_defined_functions().

What about a bit of caching to remember which groups have already been included?

This was actually a separate point..

<?php
function module_include_all($group, $module = NULL) {
  static $groups_processed = array();
  if ($groups_processed[$group] === true) {
    // nothing to do...
  } else if (!$module) {
    // scan all
    foreach (module_list() as $module_i) {
      $file = drupal_get_path('module', $module_i) . "/{$module_i}.{$group}.inc";
      if (file_exists($file)) {
        include_once($file);
      }
    }
    $groups_preprocessed[$group] = true;
  } else if (!isset($groups_preprocessed[$group][$module])) {
    // only deal with one module.
    $file = drupal_get_path('module', $module_i) . "/{$module_i}.{$group}.inc";
    if (file_exists($file)) {
      include_once($file);
    }
    $groups_preprocessed[$group][$module] = true;
  }
  // I don't see why we need
  // module_implements('', FALSE, TRUE);
}
?>

(this implementation does also allow to include the group for only one module.)

While I see your point, hook names are namespaced by module - so while any module can invoke any other module's hook (or core's), they should know who the hook is originally 'owned' by in the first place. The only situation where that namespacing breaks down is with hook_/foo/_bar_load() vs. hook_/foo_bar/_load() or if a module doesn't namespace it's own hooks but that's already a problem.

Marking CNW since the proposed approach is different to the current patch now.

.

@catch:
I don't know of any perm.module for hook_perm. I know of a menu.module, but it has little to do with hook_menu - the hook_menu stuff would work quite well with menu.module disabled. I don't know of any view.module for hook_view (not to confuse with views). I don't know of any nodeapi.module for hook_nodeapi. Well, this is all core stuff.

There are some module-namespaced hooks such as hook_uc_message or hook_views_plugins, but I don't know if this can be made a rule. And moreover, what do you want to do if the "hook owner" module is switched off? Do you want to prevent other modules from using this hook? Or do you only want to prevent the lazy inclusion?

For the group names, another possibility would be to prefix hooks by group name.

It would make a lot of sense to have
- hook_build_menu() instead of hook_menu()
- hook_build_perm() instead of hook_perm()
- hook_build_menu_alter() instead of hook_menu_alter()
- hook_build_block($op) instead of hook_block($op) (you would still have hook_block for actually rendering the blocks).

All of these are only needed occasionally when some registry or cache needs to be rebuilt, so it would be fine to put them in a modulename.build.inc file.

To make the transition easier, you would still invoke hook_menu and hook_perm, and then you would add the information from hook_build_menu and hook_build_perm.

Well, even if you disagree with the prefix thing, I think modulename.build.inc is not such a bad idea!

I don't know of any perm.module for hook_perm.

It's now hook_permissions() - and it's owned by /includes rather than a module so doesn't necessarily follow the same rules.

I know of a menu.module, but it has little to do with hook_menu - the hook_menu stuff would work quite well with menu.module disabled.

menu.inc, but same as above.

I don't know of any view.module for hook_view (not to confuse with views).

Old cruft, IMO.

I don't know of any nodeapi.module for hook_nodeapi.

What about node module for hook_node_$op - http://api.drupal.org/api/function/hook_node_load/7 hook_nodeapi doesn't exist in D7.

Things may not be perfect in HEAD, but it's a lot more standardised than D6. Either way, two modules trying to /define/ the same hook, rather than just invoking it, would be a bug, and a very rare edge-casey bug at that.

Ok, but again, what happens if you disable the definition of a hook, while there are still other modules calling and implementing it? Is this simply forbidden? Or do you want to introduce some rules to deal with it?

yesterday it was raised on IRC not sure for this patch but I presume this (or was it the m_i cache?) whether APC caches all functions or those that are executed.

[chx] so once a file is loaded its content is cached.
[Rasmus] opcodes, functions and methods, yes.

that settles it.

Discussed this a lot with Crell in irc. This approach should have a minimal impact on sites with opcode caches, and doesn't add any new requirements for module authors at all. The only issue is whether we can agree on a $module.registry.inc pattern in core to hold menu and theme hooks at all.

I really love this patch and entire approach. Especially, it will be tremendously helpful for contrib modules, which very very very often need to implement support for optional third-party contrib modules.

But it is still entirely optional - you can simply leverage it if you want to. And for a very large amount of code it makes sense, due to the clarification on opcode caches provided in #14.

We have some minor issues though:

- Wrong indentation in the API docs for hook_hook_info() as well as system_hook_info().

- Missing CVS Id and @file directive in the new files.

- Since $module.tokens.inc already uses this scheme, we should add this group already, which makes up another perfect example (because there are already tests).

I am quite happy with this too. Nice work.

This patch addresses sun's points.

It also exposes a flaw in module_hook_info(). It is caching its results even when called by simpletest during system_install(). The system_install() is called during drupal_install_system(). system_install() leads to a drupal_alter() which kicks in the module_hook_info(). Since no modules are installed, we cache am empty array and we get test failures. I'm hoping someone can sort this out.

To reproduce the problem, just try to get token replacement test (in System group) to pass with this patch.

CNR just to see what bot says.

#markup processing is back to #pre_render. It was a valiant attempt. It has to in place early since #theme_wrappers needs to wrap the markup.

woops. last patch was not meant to be posted here.

I understand it would make a rather large patch, but if we move node_menu() alongside node_theme() in our new node.registry.inc, I'd be so so so so so happy to set this to RTBC! :-) ...... Bonus points for any others that we manage to move in along with this patch. Either that or stick an ugly @todo in there ;-) .

Moved node_menu() to node.registry.inc.

Lets chalk up that bug I saw to local halloween gremlins. Bots happy.

Added capital R per tha_sun.

Haha, mixing up the patches :-) . In order to get things back on track, here's the patch at #22 with the node_menu() move to node.registry.inc, and without the whitespace changes. We can move others post-issue. Let's see how the bot likes it.

Same procedure as Rob tried in #30... This is the patch from #22, with node_menu() moved also...

Tests are failing, because the cached hook_info is never reset.

This patch should fix that.

This looks RTBC to me.

Summary:

1. We tried to put the registry into core to allow for lazy-loading of very infrequently used and optional code.
2. When the registry was in, we considered to move certain hook implementations into separate include files, because they are only ever needed when certain hooks are invoked.
3. We then removed the registry, which entirely removed the possibility for modules to move support code for certain APIs into a lazy-loaded include file.
4. Many contributed modules are using this pattern all over the place already, especially to allow for optional loading of support code for other contributed modules in case they are installed.
5. This patch basically puts back in this facility of the registry, but does so in a totally optional way, which has been optimized for performance already.
6. To underline the need for this, I'm repeating the clarification we gathered:
   

   [chx] so once a file is loaded its content is cached.
   [Rasmus] opcodes, functions and methods, yes.

Bam!

just a quick question, does this enable us to seperate our (small) (hook_)boot code from our large modules?

alexanderpas - no, it doesn't do that. when hook boot fires, it loads the .module so there is no point to moving hook_boot code elsewhere. this helps by making the .module smaller.

@Crell

I bet we could put that under the Performance exception.

+++ modules/node/node.module	14 Oct 2009 20:16:32 -0000
@@ -1688,189 +1644,6 @@ function _node_add_access() {
Index: modules/node/node.registry.inc

I'm not a big fan of the name 'node.registry.inc'. The word 'registry' has zero meaning to me when trying to look for a function. I can't think of a better name though.

As I said before, I'm not a fan of splitting the core files in smaller pieces as I think it makes for a DX regression. I guess people can use it in contrib if they want.

Sorry, but.. "DX" ?? (Ok, found it, "Developer Experience")

I am personally a fan of separate files for stuff that is not needed in a typical request, and that logically belongs together. Typically all "rebuild" stuff (menu_rebuild, theme registry etc) could very well live in separate files, for my taste.

Also makes it easier to apply concurrent patches that otherwise would mess up line numbers.

I'm not a big fan of the name 'node.registry.inc'. The word 'registry' has zero meaning to me when trying to look for a function. I can't think of a better name though.

What about "modulename.build.inc" or "modulename.rebuild.inc" or "modulename.definitions.inc" ?

I don't have a better suggestion than $module.registry.inc or $module.info.inc either. It's always a pain to have to scroll over these large hunks of arrays when you actually search something else in a module, so I'd really love if I could consult a single $module.registry.inc instead when needed.

Can we re-roll with the API change only and discuss the eventual code re-organization/movement in core later on?

I'm even willing to get of registry group to get this in. We shoudn't, but Contrib will use it even if core can't decide on a name for info hooks ... We are proposing only 1 new file for core.

>I'm not a big fan of the name 'node.registry.inc'. The word 'registry' has zero meaning to me

Agree. The word registry is overloaded with meanings and there are way too many different kinds registry in Drupal for this to make any sense just the word 'registry' by itself.

I did not like that 'registry' name before it was killed so I'm glad it's done way.

Please use a more specific word.

I think registry is appropriate, as we're talking about something similar to a "Metadata registry" here (not to mention "The Other" registry, see also: http://en.wikipedia.org/wiki/Registry)

Let's get this in first and continue this discussion afterwards.

The diffstat on #50 is 4 files changed, 96 insertions(+), 28 deletions(-) - including the new documentation. IMO that's a no brainer just to get token in.

The biggest use-case here is precisely things like token support, bot.module support etc. - code which is used very rarely or not at all on some sites. Even if we don't care about the memory savings on non-opcode sites (I care a little bit but not too much), then having a nice way to divide up inter-module includes like this makes a lot of sense, and already replaces some cruft in core with a nice API. So this one is very RTBC, and we can argue about core splits later.

Alrighty, committed.

I tinkered really long about how to proceed from here.

I think I found the answer that should be compelling for everyone:

#624324: Do not load support code for optional core modules

This lazy-loading was mainly targeted for contributed modules. Optional core modules have to be treated identically to contributed modules.

I am so glad in less a month we have properly discussed and got this in! Really glad. Especially that with commit messages messed up it takes a PI to find this issue.

This is one of these issues that will get a DrupalWTF sign two days after release. We spent years discussing whether we want to explicitly register hooks or not (hint: no!) and all of a sudden we decided yeah this one-off is a good idea? Unbelievable... I will try to come up with something useable and grokkable quick before alpha.

Oh I have the first real problem here. Modules implementing hooks on behalf of other modules won't work because module_load_include('inc', $module, "$module.$group"); loads the include strictly from the $module dir. That's a proper bug report... to begin with.

Say someone debugs her site and you need to explain what's a hook and where she can find 'em. Before patch? "A hook is just a function named the module name, the underscore and the hook name and it lives in the .module file." After patch? "A hook is just a function named the module name, the underscore and the hook name and it lives in the .module file unless ... unless of course the hook is defined in another hook to belong to a group where the group just means the part of the filename before .inc."

Second, there are tons and tons of hooks not implemented by core but fired by core. Let's say I do a huge, huge form_id specific form_alter so I find it a good idea to move it into an include file. This works! Yay. If I am unlucky enough that another module wants to implement the same hook , now there is a problem... Edit: if it wants to use another include file, that is.

Edit: we say we do not babysit broken code and the latter is such broken code. But tt works perfectly and saves memory until (much) later said another module comes. That's a like a hidden bomb not broken code per se.

Aaaaaaaand a solution!

Scrap hook_hook_info. Use $module.$hook.inc. That's easy to explain. It's impossible to ambush another modules either because the include file is fixed. We still need to implement this carefully to avoid the problem of implementing on behalf of other modules. Likely first iterate over the modules and see if there are any $hook.inc files and after including them all, now run over the modules and check for function_exists.

Also, see #675136: Document how/why hook_hook_info() is totally different in D6 and D7

And that sucks before sun said "A rather simple module like admin_menu exposes 3-4 hooks from which an integrating module will have to implement 2-3." so grouping is essential. We could group by module name, ie hook_admin_menu_foo_bar lives in foo_bar.

Do we have the problem with views_attach and views modules when trying to figure out where does hook_views_attach_foo belong? Yes, but if you have views module which has a hook_views_attach_foo then you already should not have a views_attach module so I do not think this is a terrible issue.

How's the explanation then? "The hook can live in .module or if foo module provided the foo_bar hook then in bar.inc". Not sure whether we win too much then :(

So...

Am I correct in reading the above comments that your idea is:

a) Get rid of hook_hook_info() entirely.
b) For any hook_xyz(), allow module mymodule to either put function mymodule_xyz() in file mymodule.module or mymodule.xyz.inc.
c) Have module_implements() and module_invoke() and node_invoke() and all the other such functions, including the ad-hoc invoke routines that are scattered around and through Drupal (the ones that made tracking down #675046: Make sure all hooks in D7 have documentation so difficult) follow this convention.

And what about hook_xyz_alter() implementations, called from drupal_alter()? I'm assuming they should be in mymodule_xyz() too?

And are you saying that there should be "groups" of hooks somehow that can go together into particular files? How would those be defined -- didn't udnerstand what you were saying on that? That was the purpose of the hook_hook_info() thing... either have it or don't, but don't do something crazy based on the first component of the hook name etc. Please. !!

Oh, forgot to say: I think this idea (if I have understood the idea correctly) is not yet fully defined and will be difficult to implement...

[off-topic]
This is yet one more of those issues where people don't have time to read all associated discussions, and even if they do, won't remember everything. And then we get sub-optimal decisions.

The same if I want to make sense of chx points above and find related posts, I need to read up a lot of stuff.

Wouldn't it be possible to have some kind of Wiki, where we can summarize the different solutions and their pros and cons, with links to the arguments, cleaned up of the usual noise?

I have the same problem in other threads, where I am sick of repeating myself. And my own unfinished thoughts in previous posts make it even worse.

EDIT: I posted an issue for that.
#682178: Drupal Strategy Wiki to sort+summarize information from the issue queue.
or alternatively
#682254: Allow multi-dimensional rating of issue comments
[/off-topic]

Not enough time I now see and with grouping we won't get anything better for now. /me weeps and leaves this issue alone.

The very first thing we should do in D8 is to change ALL hooks to use double-underscores as delimiter between module name and hook name, i.e.

views_attach_links()

becomes

views__attach_links()

or

views_attach__links()

...

Now I realize that this won't even cut it, so we likely need to get one step further and perform the very same change to hook names either:

views_comment_load()

becomes

views__comment__load()

That way, we can auto-determine the module name the hook belongs to. Therefore, all

hook__comment__*()

hooks can live in

$module.comment.inc

(auto-loaded)

Previous arguments have pointed out that module name is often not the best criterion for the grouping.
Can we replace "module name" with "group name" in your idea?

And for the double underscore idea, maybe you noticed the following discussion?
#548470: Use something other than a single underscore for hook namespacing

If we really want to change this, I would much prefer _hook_ to just a double underscore: It's more obvious, and invites to do a google search for _hook_ and also allows a cross-file sourcecode search.

@sun (#66):
We could still decide to load all candidates. That is, every prefix that is a module name + '_'. It might kill a bit of performance, but it's safe. If we don't want the second '__'.

So, for mymodule_hook_a_b_c() we would include
mymodule.a.inc
mymodule.a_b.inc
if both "a" and "a_b" are module names.

And if you want, we also load
mymodule.a_b_c.inc

Now we are left with a rare nameclash problem, and a very small performance penalty.

All of that IF we want hook filenames based on hook prefix / module name.

------

@chx (#57):

Oh I have the first real problem here. Modules implementing hooks on behalf of other modules won't work because module_load_include('inc', $module, "$module.$group"); loads the include strictly from the $module dir. That's a proper bug report... to begin with.

Modules implementing hooks on behalf of other modules should do that in their .module file, or any other file that is always loaded. Problem solved?