module_invoke() doesn't work with hooks placed in include files via hook_hook_info()

Subscribing. Will try to look at this later.

#4: module-invoke-fail-752226-4.patch queued for re-testing.

#4: module-invoke-fail-752226-4.patch queued for re-testing.

The errors seem unrelated, not sure what is going on here...

Cleaned up the code.

oopsie.

Also note that #977052: Implement hook_hook_info() for Field API hooks depends on this patch now.

I seriously have no idea how this patch is able to break those two tests.

hoho-holy cow! This also applies to module_invoke_all().

Plus, it goes even further... module_hook_info() is always empty.

That is, because module_hook_info() is statically cached. module_implements() or something else in the bootstrap process leads to an early invocation of module_hook_info(), which preemptively sets the static cache to an always empty array.

Alright, module_invoke_all() uses module_implements() already, so hook_hook_info() is already invoked. It didn't work previously, because of the already primed module_hook_info() static cache.

Alright, last patch passed.

The change to bootstrap_invoke_all() implies that none of the bootstrap hooks are allowed to use module_invoke() either.

If we do not want to go with this potential WTF, then I'd recommend to make module_hook_info() use a bootstrap-phase-specific $cid for both the static cache and the database cache; i.e.,

$cid = __FUNCTION__ . ':' . drupal_get_bootstrap_phase();
$hook_info = &drupal_static($cid);
...

I've implemented the suggestion in #23 over in #977052-13: Implement hook_hook_info() for Field API hooks

Crazy. subscribing

Extracting revised, relevant parts from #977052: Implement hook_hook_info() for Field API hooks -- which proves to be the actual test case for this functionality.

And so dies the last simple function in core. Hooks were supposed to be a simple concept. Can we roll back hook_hook_info?

As I think more and more , actually rolling back is a very enticing option.

Here is a question: does $module implement $hook? In Drupal 6 you checked for $module .'_'. $hook in $module.module. In Drupal 7 you check for $module . '_' . $hook in $module or $module.$group.inc where $group is defined in $module . '_hook_info' defined in $module.module.

Yes. This is very useful functionality for contributed modules that implement support code for other, non-required contributed modules and load the latter only on demand. However, the possibility of doing that comes at the cost outlined in the previous paragraph.

Decide.

I'd be fine rolling back hook_hook_info. I was initially for it but we are complexifying too fast.

As an intermediate Drupal developer, the concept of hook_hook_info() seems like overkill to me. Rather than fixing it at this stage in the game, can it just be removed?

I would like to keep hook_hook_info(). For large modules, the .module file easily becomes an unmanageable dumping ground of hooks, and to organize it one has to create hook wrappers which module_load_include() and run _hook_actual_hook_implementation(). This is a worse situation and hook_hook_info() can help simplify all that. A huge .module file is also terrible for memory usage. That was acceptable in the simple days of D5, but D7 is a beast and we should be saving memory where we can.

Asking for committer feedback.

I've merged #27 with some baseline test coverage for dynamic hook loading with both module_invoke() and module_invoke_all(). We still have failing tests for tokens and testModuleImplements().

I took some debugging adventures into this problem, but there are so many caching layers and edge situations that this could be a really tough problem to fix correctly. I would still love to fix this hook, though. It's just somewhat of a tightrope walk.

Setting to CNR for bot (should show that module_invoke() is broken while module_invoke_all() is OK).

Back to CNW to fix the tests.

hook_hook_info() has been in core since 2008, iirc. We're not pulling it out now.

Modified the tests to pull module_implements() into its own method (so caching doesn't interfere with the test), and added the expectation that module_implements() shouldn't actually load the include, only report that the hook is implemented (should reference hook_hook_info() instead of function_exists()). I think that is a reasonable expectation and a requirement of lazy-loading until invoke() calls.

@webchick: hook_hook_info() had a very different function < D7, and was renamed to hook_trigger_info() in D7. See http://drupal.org/update/modules/6/7#trigger_overhaul

Yes, I'm aware what hook_hook_info() is and does. :P

The fact remains, it's a very old change, contrib modules are making use of it, we can't remove it.

Hmm.. I have some work in progress here.

SimpleTest is having a problem running drupal_install_system(), dying on a cache_set() because MergeQuery_mysql class is not found. That doesn't make sense because MergeQuery_mysql isn't even implemented and should be falling back to MergeQuery via DatabaseConnection::getDriverClass(). I've attached the call stack for reference.

I removed the module_implements() expectation from the tests, because function_exists() has to be checked on a cache miss, but on a cache hit the include should not need to be loaded. Split the function_exists() part out of module_hook() into a new function called module_load_hook(), which takes care of lazy loading the include and conveniently returns the name of the function to run if found.

webchick, I do not know what mess is in CVS but that change was introduced a year ago http://drupal.org/node/588766#comment-2157220 http://drupalcode.org/viewvc/drupal/drupal/modules/system/system.module?...

Now that I look at the issue, it seems that it never got a proper discussion and I was really unhappy even back then. Code freeze, what code freeze?

The old hook_hook_info() is now called hook_trigger_info().

Here is a patch.

I got this to work.

Note that the bug described in http://drupal.org/node/588766#comment-2463764 as far as I can see, is still a problem (ie if views module wants to provide node specific hook implementations in node.views.inc then module_load_include will look inside node module directory). Note that this not easily fixable!

Also the "ambush" is also a real possibility -- namely, if two modules happen to define the same dynamic hook (I used hook_form_$form_alter as an example) into different groups.

This one has a more correct static cache for module_hook_info().

@chx: Implementing hooks 'on behalf of another module' should be done just like implementing hooks for the module itself, i.e. the hook implmentation should begin with the own modules name, not the module the implementation references. In your example about node.views.inc, views module should just place it's hooks in views.views.inc in that case, and place views_...-hooks in there, not start putting node_...-hook implementations in views.module. Otherwise we really get a mess, and we could set an example for contrib to end up with potentially duplicate function names.

Thanks for adding tests. Partially reverted to module_hook(), because we do not want to introduce a new API at this point.

Kept the idea of returning the $function name instead of TRUE, since that really makes sense and avoids needless subsequent string concatenations.

In the end, I don't think it makes sense to support hook groups/include files prior to full bootstrap, because not all subsystems and modules are loaded yet, so the environment in which code is executed is very limited anyway. Therefore, attached patch shortcuts the pre-full-boostrap case.

@Stevel care to check the code? When you module_invoke('node', 'views_whatever') then it will module_load_include('inc', 'node', 'node.views'). That, in turn becomes drupal_get_path('module', 'node') . "/node.views.inc"; That's the bug, plainly.

Even if we are to keep this (it's crystal clear from the DBTNG and this issue that we do not want Drupal code easily discoverable any more) that does not mean that we can change the function signature of module_hook.

@justinrandell: Performance-wise, what module_implements() does would equal a module_hook_multiple() - i.e., instead of checking each module separately, it checks once whether the hook has a registered group, and if it does, it loads the corresponding include files.

I'm against changing the function return of module_hook(). That was intended for the new module_load_hook() function only.

And I think allowing boot modules to use includes is ok: when boot modules call invoke() their includes and the includes of other boot modules should be honored. The patch in #46 allows this, but we might want to add a separate static cache for the hook_boot() phase to support it.

@justinrandell: module_implements() iterates over all enabled modules in a fast fashion. module_[load_]hook() separately invokes function_exists(), subsequently needs to re-read the cached module_hook_info() to check whether there's a group, if there is one, try to reload the module include file, and finally invoke function_exists() again. module_implements() already knows all of that upfront, so it conditionally checks that only once and performs the operation for all modules at once.

@carlos8f/all: A (string != '') === TRUE, so this tweak is subtle. It has a positive impact on modules that actually need to use module_hook(); you can see the actual difference in the last and second to last patch over in #977052: Implement hook_hook_info() for Field API hooks. I highly doubt that there is any code that actually checks module_hook('foo', 'bar') === TRUE, since module_hook() can only return TRUE or FALSE thus far, so a type-agnostic comparison would be ill in the first place -- however, no strong feeling about that, so if everyone insists, we can revert to TRUE/FALSE.

However, negative on introducing a new, separate module_load_hook(). It's plain confusing to have an API function module_hook() that doesn't respect its own API and developers suddenly need to use a separate module_load_hook() to actually do what module_hook() is intended to do: check whether a hook exists.

The more I think and look at this code the more I'm agreeing with @sun that the boot phase should not support lazy includes. It's too much meta data to be loading and caching when we don't even have the benefit of common.inc. So I'm OK with hook_hook_info() returning array() during the boot phase, and #48 looks OK in that light.

@justinrandell, the answer to #51: that because module_implements() calls hook_hook_info() before the foreach(), it follows that hook_hook_info() called again by module_hook() inside the aforementioned foreach would be overkill, and we can just replace with function_exists().

I think this just needs a reroll to revert the module_hook() return value (it's completely unessential to the patch and therefore doesn't warrant even a slight API change) then I would say it's RTBC.

On a slightly related note, there is also a bit of oddness that I am gradually becoming more aware of -- module_load_include() (and friends) is dependent on common.inc, because it uses drupal_get_path() as apposed to directly using drupal_get_filename(). That means that during the bootstrap_invoke_all() phase module_load_include() is broken. As is drupal_get_path(), a very oft-used function. That bug has existed for quite some time, since in a D6 site using module_load_include() in the global space normally works, but not once you implement hook_boot(). I don't see a good reason why we don't just move drupal_get_path() to bootstrap.inc, which would make module.inc less dependent on a full bootstrap.

Reverted to TRUE/FALSE, slightly improved comments.

Latest patch passed. I've updated the patch in #977052: Implement hook_hook_info() for Field API hooks with this code and it works, too.

A minimal bugfix, tests, and a pending use case. That sounds like a plan.

I personally was never a big fan of the 'group' feature -- it is hard to discover. In core, it seems to be used by the system.token.inc only. We could add a @todo to remove it in D8 -- that would hopefully prevent people from using it in contrib.

Well, contrib is already making pretty extensive use of this pattern. For example, almost all of View's hooks go into a module.views.inc, afaik rules has a module.rules.inc, tokens has module.token.inc, etc. (which is how that ended up in core)

It actually helps for discoverability, in my experience, because I can download a module and do "ls" to find out "Oh, ok. This module provides views and token support." When we teach students in Lullabot workshops how to add views integration to their modules, I've never heard anyone confused about "Why does this need to go into a separate file?" It actually makes a lot of sense; per-module is a perfectly logical way to break up the code. So I'm not sure I'd put @todo: remove around this, personally...

And core would be making more use of this, but #345118: Performance: Split .module files by moving hooks was essentially won't fixed by you in http://drupal.org/node/345118#comment-1645264 due to it adding increased complexity on "where do I find that hook?" for minimal gain in only the most sub-optimal of server setups. I think we left with the consensus of "Let's just add the mechanism for now, see if we can make better use of it in D8."

Dries: hook_hook_info() is used only by token in core because the other obvious hooks people didn't want to bother moving at the time. It is, primarily, a performance improvement.

For Drupal 6, we managed a 20% performance improvement by moving page callbacks out into lazy-load files. That was the single biggest performance boost we got in Drupal 6, which was nonetheless still slower than Drupal 5.

For Drupal 7, we first tried to use the registry for lazy-loading hooks to reduce our code weight. The overhead from that proved too high so it was removed. hook_hook_info() was the next-best-thing: A clear, straightforward, uniform way to group code in a lazy-load-friendly way to greatly reduce code weight, and a nice DX improvement as a side-effect because you don't have 4000 line files that are impossible to read and cause some IDEs to choke because they're just too damned big.

There was discussion of hook_hook_info() for doing per-hook weights, too, but that never materialized in D7. But the hook was structured in a forward-looking way specifically for that purpose.

We've known for over 3 years, with benchmarks and profiling data, that the two slowest parts of Drupal are bootstrapping all of our code off of disk and the theme/rendering layer. We knew that before Drupal 6 was released. Yet both of those got *slower* in Drupal 7 because we layered more code into the core system and more complex code into the theme system. And yet the typical page load still does not use the vast majority of the code we load off of disk. We have only made a bad problem worse.

Splitting less-used hooks off to lazy-load files is not something core invented, but it is the only way forward I can see to reduce our code bloat short of a massive rearchitecture. In your SF keynote you noted that 95% of Drupal sites still run on cheap-ass-shared-hosting, or run by a sysadmin who barely knows what he's doing. Those sites don't have APC available to them, and so will get *killed* by our wasteful code weight. I already consider Drupal 7 incompatible with shared hosting for this reason, and we are going to get eviscerated in the market for that.

Really, have only 3 options:

1) Increase our use of manual-lazy-load operations for indirect procedural code (hooks, theme functions, etc.) to reduce our baseline code weight.

2) Retool most of Drupal to be heavily OO and use PHP autoload for lazy-loading (and hope our autoload implementation can handle that).

3) Add a hard-requirement on APC to the installer and just refuse to install without it, thus cutting out the vast majority of web hosts.

I hate to sound all doomsday-ish, but that's how I see it right now. The current "load all 100,000 lines of code every page request and use 2000 of them" approach is simply not sustainable.

So yeah, in short "discourage people from using hook_hook_info() in contrib" translates to "encourage people to write bloated code that doesn't run on shared hosts." That's a situation of our own making. If anything, we need to be using it a lot *more* in core than we are now, not less.

If I'm reading #58 directly, it reads essentially "if you're not in full bootstrap mode, all of this lazy-load magic doesn't work, sorry, kthxbye, otherwise it's as before". Is that right? If so, I am fine with that restriction. That's where most of our hooks are called anyway.

I'd like to add a 4th option to @Crell's list of options (for D8):

Addressing the case of hooks (theme functions might follow a similar pattern), module_implements(), instead of just returning a flat list of modules that implement a hook, returns an associative array (module => path_to_implementation). This could be discovered by the following pattern:

1. check the global space (includes .module file)
2. check in $module.$group.inc
3. check $info['files']

Once function_exists(), the path can be discovered automatically with ReflectionFunction::getFileName(), and cached. That way, whenever the hook needs to be invoked, it can be loaded knowing the exact location. The code doesn't need to load in any other case, except on a cache miss of module_implements(). Taking that further, any time there is a function name defined as a string in our code, the target file location should be discovered and cached.

I know this is very similar to the ill-fated registry, but perhaps more focused and practical. Knowing where functions are located in a vast codebase is a basic requirement of calling those functions. And let's please not assume APC; if we want to be friendly to folks with shared hosting. For that matter, APC has no shared cache when using fastcgi, so if you want to run PHP under nginx, Cherokee, etc., APC works clumsily at best.

Here's a demo that I wrote of the method above.

Thanks for summarizing the history -- I must admit that my memory was a bit rusty. Reading Crell's comment, it makes more sense. While not pretty, it sounds like a reasonable compromise.

If anyone is up for it, I'd love to see both approaches benchmarked.

hmm, what is meant by "both" approaches? #58 is RTBC, that is the only approach we have right now.

yeah. I also think we got slightly sidetracked here. This patch fixes API functionality that ought to work, but doesn't. We are ~3 days before RC1, so any talk about ideas or better concepts is out of scope right now. Happy to discuss for D8.

With the other approach I meant #67. Probably not D7 material.

So can we move forward here and fix this issue? #977052: Implement hook_hook_info() for Field API hooks depends on this patch.

Sounds like this patch would be a good one to commit. Is there anything I can do to help that along?

#67 is just a peek at what we can try in D8. It was part of a patch I had been optimistically writing for this issue (sharing concerns with others that hook_hook_info() is not totally useful) but it soon became clear that the whole module system would need a rewrite for it to really work :)

#58 is still RTBC.

Committed the patch in #58. Let's come back to this issue once we start work on Drupal 8.

I opened a ticket for D8 discussion on this matter: #983470: Improve lazy-load mechanism for procedural code

So have died the last simple concept in Drupal. RIP.