I got to thinking after chx's multiple form_alter patch that there might be a faster/less expensive way to call hooks then looping through all enabled modules checking to see if they call a $module_$hook. chx and I were bouncing ideas back and forth and came up with this approach:

Modules that define new hooks create a hook called hook_define_hooks, which yields an array of hook names.
Variables called hook_$hook_name store an array of modules which implement that hook. These variables get updated whenever you enable or disable a module.
Now, instead of interating through all enabled modules, module_implements just iterates through the modules that actually define that hook.

You need to hit update.php once you apply this; Drupal will be quite broken. ;) And install.php is still broken too, so don't apply this to a clean install. ;)

Benchmarking would be appreciated eventually, but I imagine there will be some stuff I'm missing, so leaving "code needs work" for now.

CommentFileSizeAuthor
#2 define_hooks_0.patch15.58 KBwebchick
define_hooks.patch5.04 KBwebchick

Comments

nevets’s picture

nevets commented

I think this is over kill. The current method works well and makes for a clean API that is both straightforward and easy to use. What you propose adds an extra step of "registering" the hook as welll as making developement of modules a pain since one can no longer just add another hook, save and test. I can also see all the questions on the forums, "I added the xyz hook but nothing happens"

webchick’s picture

webchick
she/they
Primary language English
Location Vancouver 🇨🇦
commented
StatusFileSize
new define_hooks_0.patch15.58 KB

nevets: Yeah, that could be the case. I was interested to benchmark the difference and see if it's worth it.

Newer version, eliminates module_implements altogether, plus some other stuff from chx's feedback.

install is still borked.

chx’s picture

chx commented

I like this method and also like the hook registry. Makes easier finding, documenting (and in the future, self documenting) hooks.

drewish’s picture

drewish commented

i think this makes a lot of sense. the current method always seemed a bit wasteful to me.

personally, i don't think there's a good reason to break backwards compatibility. it seems like module_implements() could check the variable. the function name makes it obvious what was happening and provides a nicer interface for modules.

also, i'd like to see system_define_hooks() split up to define the respective modules that define the hooks.

webchick’s picture

webchick
she/they
Primary language English
Location Vancouver 🇨🇦
commented

drewish: Good point about breaking up system_define_hooks... I had intended to go back and break out the non-required stuff to their respective modules (comment_define_hooks, node_define_hooks...), but didn't get around it during the re-roll.

Additionally...
Chat from IRC to cover what I need to change next time I (or someone else) work on this patch:

chx: webchick: $hooks[] = user_call_func("$module_define_hooks"); eeeeeeeeeek
chx: webchick: $function = "$module_define_hooks"
chx: webchick: $hooks[] = $function();
chx: there is a call_user_func indeed
chx: but here it 's overkill
chx: webchick: variable_del("hook_$hook"); <= this is not what we agreed upon
chx: webchick: $current = variable_get; if ($current != $implementations) If ($current) variable_set else variable_det
rasmus: $function = "$module_define_hooks" makes very little sense
chx: rasmus: why?
rasmus: $function = $module_define_hooks;
chx: no, the variable is called $module
rasmus: well, then you need to {} it or concat it
chx: webchick: i somewhat favor concat
chx: webchick: $implementations = sort(variable_get('hook_help', array()));
chx: webchick: no dice
chx: webchick: $implementations = variable_get('hook_help', array());
chx: webchick: sort($implementations)

Yes, Rasmus Lerdorf helped review my patch! ;)

webchick’s picture

webchick
she/they
Primary language English
Location Vancouver 🇨🇦
commented

Ahem. :P

chx: webchick:  $hooks[] = user_call_func("$module_define_hooks"); eeeeeeeeeek
chx: webchick: $function = "$module_define_hooks"
chx: webchick: $hooks[] = $function();
chx: there is a call_user_func indeed
chx: but here it 's overkill
chx: webchick: variable_del("hook_$hook"); &lt;= this is not what we agreed upon
chx: webchick: $current = variable_get; if ($current != $implementations) If ($current) variable_set else variable_det
rasmus: $function = "$module_define_hooks"  makes very little sense
chx: rasmus: why?
rasmus: $function = $module_define_hooks;
chx: no, the variable is called $module
rasmus: well, then you need to {} it or concat it
chx: webchick: i somewhat favor concat
chx: webchick: $implementations = sort(variable_get('hook_help', array()));
chx: webchick: no dice
chx: webchick: $implementations = variable_get('hook_help', array());
chx: webchick: sort($implementations)

Hey, Rasmus Lerdorf helped review my patch! ;)

gerhard killesreiter’s picture

gerhard killesreiter commented

this appears a bit complicated to me. Why don't you cache the current data structure inside module_implements() in the cache table?

dries’s picture

dries commented

It's overkill unless it significantly increases performance.

webchick’s picture

webchick
she/they
Primary language English
Location Vancouver 🇨🇦
commented
Title: hook_define_hooks » Centrally register module hooks.
Version: x.y.z » 6.x-dev

Updating version to 6.x dev. merlinofchaos has an alternate version of this idea he's working on.

webchick’s picture

webchick
she/they
Primary language English
Location Vancouver 🇨🇦
commented

Marking dupe of http://drupal.org/node/116165 which is a better approach.

webchick’s picture

webchick
she/they
Primary language English
Location Vancouver 🇨🇦
commented
Status: Needs work » Closed (duplicate)