Closed (duplicate)
Project:
Drupal core
Version:
9.5.x-dev
Component:
base system
Priority:
Normal
Category:
Feature request
Assigned:
Unassigned
Reporter:
Created:
16 Jan 2011 at 21:17 UTC
Updated:
8 Mar 2023 at 15:38 UTC
Jump to comment: Most recent, Most recent file
Comments
Comment #1
joachim commentedSeems to work.
If this is the right approach, then presumably other hooks that live in .install files should get similar treatment?
Comment #2
damien tournoud commentedYou just have to load the install file of the corresponding module first.
Comment #3
joachim commented> You just have to load the install file of the corresponding module first.
Yes, you do.
But isn't the whole point of hook_hook_info() that you don't need to think about that?
Comment #4
sunhook_hook_info() transforms the
'group'into $module.GROUP.inc, which is not what you want. $module.install is incompatible with the pattern.Comment #5
joachim commentedOk then let's punt this to D8, because:
> But isn't the whole point of hook_hook_info() that you don't need to think about that?
Comment #6
xanoWhat about we rename MODULE.install to MODULE.install.inc for consistency? We can do this by setting the group for install file related hooks and slowly migrate .install files to .install.inc files. The only potential problem I see so far are hook_update_N() implementations.
Comment #7
joachim commentedNobody should be calling hook_update_N() implementations apart from the update system itself, so we can probably ignore those hooks.
As an alternative to a big renaming exercise, we could let the system that consumes hook_hook_info() information know that 'install' is a special case.
Comment #8
xanoProbably (never say never), but they're still hooks. In addition to that, hooks like hook_form_FORM_ID_alter() or hook_ENTITY_TYPE_ACTION() cause the same issue. I can live with hook_update_N() files living in MODULE.install.inc and letting the update system include the file just it does for MODULE.install. That would already be an improvement over the current approach. Ideally (perhaps after this issue gets in), we figure out a way to make hook_hook_info() work for dynamic hooks. Perhaps it should function like hook_forms()?
Then we would replace the current special case with a new one, which defeats this issue's purpose of creating unity and consistency. Also, renaming files isn't that big of a deal. History is kept, and with changes to a few functions that are hardcoded to work with .install files, we can make it work.
Comment #9
xanoNow the upgrade system will be completely overthrown in D8, and
hook_update_n()may cease to exist, we may be able to make *.install files work with the existing hook system.Comment #10
joachim commented> Then we would replace the current special case with a new one, which defeats this issue's purpose of creating unity and consistency.
Yes and no.
It replaces the special case with a new one, but crucially, it *moves* the special case somewhere else.
Consider who currently needs to know about this special case? Any developer who wants to invoke a hook needs to know the rule:
- use module_invoke_all() for any hook; the file is loaded automatically for you
- EXCEPT if it's a hook in MODULE.install, where you need to load the file yourself
If we make hook_hook_info() aware of hooks in .install files, then the rule for developers who want to invoke a hook becomes the much simpler:
- use module_invoke_all() for any hook; the file is loaded automatically for you
- there are no exceptions
Who now needs to know about this special case?
- anyone working with the internals of hook_hook_info() and module_implements() (which IIRC is where the file inclusion happens)
- anyone wanting to *invent* a new hook that belongs in MODULE.install
That's far fewer people. In fact, I'd say that's probably just core, as I've never seen a contrib module that invents hooks that go in .install.
So while we only move the inconsistency to a new place instead of cleaning it up, we drastically reduce the number of people who are affected by it.
Comment #11
xanoTagging.
Comment #12
xanoI'm not sure this API change will be accepted at this point. On the other hand, #1972304: Add a HookEvent will allow us to implement hooks as methods on any class registered as an event listener.
Comment #25
andypostClosing in favour of #2233261: Deprecate hook_hook_info groups, mark hook_hook_info() for deletion