Specifically, if you call a function within hook_hook_info() that in turn calls a hook, you get an infinite loop as soon as you clear the cache. ka-boom!
This is a problem, because very often we'll have dynamic hooks that are per-something. Eg, a hook per-entity-type. (I'm working on a major reworking of the apachesolr module and just ran into this problem.) The obvious, well really only, way to do that is to iterate over entity_get_info()... which in turn calls module_invoke_all('entity_info'), which with an empty cache causes hook_hook_info() to be called... you get the idea.
I'm not entirely certain how to break this cycle, but I don't think that "well don't do that" is a viable solution as it pretty much kills dynamic hooks. Dynamic hooks are, of course, increasingly popular in Drupal and with good reason.
I'm considering adding a semaphore somewhere, but I'm not sure where that would actually, well, work.
| Comment | File | Size | Author |
|---|---|---|---|
| #9 | 968264-module-implements-wildcard-D7.patch | 3.4 KB | dave reid |
Comments
Comment #1
mattyoung commented~
Comment #2
dave reidYeah I'd probably say at this point "don't do that". I don't know how we can best support dynamic hooks with this. Why not generalize your hook to work for all entities (like core's hook_entity_load(), etc.).
Comment #3
Crell commentedThe hook in question is more like a form alter.
apachesolr_indexer_document_build
apachesolr_indexer_document_$entity_build
apachesolr_indexer_document_$entity_$bundle_build
(And then 3 layers of alter as well.)
Without dynamic hook support in hook_hook_info(), only the first can be moved out of the .module file yet all are useful only when indexing content, which will be happening only on cron anyway so it's pointless to have them loaded all the time.
Comment #4
dave reidSo why not make it apachesolr_indexer_document_entity_build($entity) and apachesolr_indexer_document_entity_bundle_build($entity, $bundle)? :)
Comment #5
Crell commentedFor the exact same reason we have hook_form_$form_id_alter(). To allow targeted operations more efficiently.
Comment #6
dave reidI just still get the feeling that hook_hook_info() will not work with dynamic hooks at all in D7. If someone clears all caches, both the module_implements and entity info caches will also be empty. We'd have to somehow make sure the entity cache gets generated before the hook cache, but I don't think that's possible...
Comment #7
dave reidI guess we could add some kind of wildcard condition like:
but that also seems too late for D7.
Comment #8
sunI don't think that something along the lines of #7 is too late for D7.
Last resort is to add a static to module_implements() to not recursively invoke hook_hook_info() + prevent caching of recursive calls - but that would (intentionally) break perfectly valid stuff. So in turn, something along the lines of #7 is required.
Comment #9
dave reidThis is what I had so far.
Comment #10
carlos8f commentedVariable name conflict -- here is an (array) $hook, while there is already a (string) $hook defined. I would prefer $hook to always be a string -- to be consistent with the rest of module.inc.
More docs! Should include @see hook_hook_info(). hook_hook_info() needs an api.php hunk for the 'wildcard' key. There should also be a note in the docs to avoid invoking hooks inside of hook_hook_info(), and to use a wildcard instead when dynamic hook names need to be grouped.
module_test.* should test the wildcard key.
Note that this also needs some work after #752226: module_invoke() doesn't work with hooks placed in include files via hook_hook_info() lands -- that patch does group/include resolution in module_hook(), which allows module_invoke() to support hook_hook_info(). So this patch would need to ensure that module_invoke() also supports wildcard hook grouping.
Comment #11
carlos8f commentedAlso, wouldn't be better DX if we could
not require a 'wildcard' key and just do
strpos($hook_key, '*') !== FALSE?Comment #12
catchIf no-one ran into this for 7 months it can't be that major.
Comment #13
joericapens commentedI ran into a real problem because of this:
http://drupal.org/node/1415278
Comment #22
catchClosing as 'works as designed', this is just unsupported.
We should be instead moving towards getting rid of hook_hook_info() via issues like #2237831: Allow module services to specify hooks.