Drupal 7 defines a new hook_hook_info().
That would be fine, except Drupal 6 had a completely unrelated hook by the same name (having to do with triggers/actions) (it was a terrible name in Drupal 6, but still).
I don't think Drupal 7 should be allowed to ship with a completely different hook with the same name as a Drupal 6 hook. The hook name changes are bad enough without this type of thing...
See
http://api.drupal.org/api/function/hook_hook_info/7
http://api.drupal.org/api/function/hook_hook_info/6
Comment | File | Size | Author |
---|---|---|---|
#23 | hook_hook_info_doc.patch | 734 bytes | te-brian |
#14 | 675136hooks_info.patch | 3.81 KB | jhodgdon |
#5 | module-inc-675136-4.diff | 1.64 KB | xtfer |
#5 | system-api-php-675136-4.diff | 573 bytes | xtfer |
#5 | system-inc-675136-4.diff | 372 bytes | xtfer |
Comments
Comment #1
apadernoI agree it is a bad name choice, considering there is already such hook in Drupal 6.
It would have been better to not use that name in Drupal 6, but changing the hook name is not anymore possible; the only possibility is to choose a different name in Drupal 7 (something like
hook_hook_files_info()
, in example).Comment #2
te-brian CreditAttribution: te-brian commentedI kinda disagree. 'hook_hook_info', as far as the functionality of the new hook goes, is an excellent name. It describes what it does, and allows for further expansion of its responsibilities in future iterations of Drupal. Its certainly possible that new keys will be supported in the future that have nothing do to with files.
I do agree, however, with the unfortunate naming of the Drupal 6 hook. I'd rather have a cleanly named, expandable function name now, and deal with a slight migration wtf.
My 2 cents:)
Comment #3
jhodgdonThe problem is that it's not just a "slight migration WTF", in my opinion.
It's also a problem that the D6 and D7 pages on api.drupal.org link to each other, and yet are totally unrelated.
And yes, hook_hook_info() was obviously the wrong name for that hook in D6, which is why it was changed in D7.
Comment #4
xtfer CreditAttribution: xtfer commentedThe hook in question appear in 9 places over 3 files, so its relatively simple to change. For the purposes of demonstrating a patch I've chosen "hook_define_hooks", as I think this clearly says what the function does (better than hook_hook_info, in my opinion).
I think its worth making a change like this if it stops people from wondering why their 6.x hook_hook_info calls are doing odd things when they port to 7.x. Better that it just disappears completely.
Comment #5
xtfer CreditAttribution: xtfer commentedThe last patch (#4) dropped a diff file on form submission... Submitting again.
Comment #7
jhodgdonYou need to put all of your changes into one single patch, because the testing bot will test them all as a group.
Comment #8
te-brian CreditAttribution: te-brian commentedI'm still gonna argue that "hook_hook_info" is the better name. It aligns with all of the other info hooks in core. "hook_define_hooks" does not follow any convention that I am aware of.
The stats speak for themselves:
Hooks that contain the word 'define': 0 (zero)
Hooks that end in "_info": 17
The only name I can think of that makes sense is "hook_hook_group", but that is silly, because that is guaranteed to be changed in Drupal 8, as we will add at least one more hook configuration option.
So, I maintain my vote for "hook_hook_info", the way it is.
Comment #9
jhodgdonte-brian: Then please write up some documentation for hook_hook_info and for the module update page that explains why there is no connection between the D6 hook of the same name and this one. That's the only hang-up.
The D6 hook, we all agree, had a terrible name, and the D7 hook, we all agree (assuming we agree the hook has to exist in the first place) has a reasonable name.
The only problem is the conflict between D6 and D7 and the resulting confusion on api.drupal.org. Stating over again that the D7 name is a good name doesn't resolve the conflict.
Comment #10
apadernoIf we just want a different name to avoid confusion between two different hooks (and to avoid the documentation pages are shown together in APi.drupal.org), then the Drupal 7 hook could be called
hook_hooks_info()
. It is not the schema used to name similar hooks, which don't use a plural word (seehook_action_info()
,hook_node_info()
), but it would allow to use a hook name ending in_info()
, and be generic enough (hook_hook_files_info()
implicates the hook returns information about the files containing the hook implementations, whilehook_hook_group
implicates the hook just returns information about the hook group).I think this would be a good compromise, and it would allow to avoid the documentation problem.
Comment #11
jhodgdonThank you kiamlaluno, that is an excellent idea.
Comment #12
webchickI'd prefer a simple documentation clarification, personally. We're past API freeze at this point, and all of the other hooks I can think of use singular names (hook_user, hook_node, hook_permission...)
Comment #13
webchickAlso, I don't see Drupal bleeding from its eyeballs as a result of this bug, so it's normal.
Comment #14
jhodgdonHere is a patch that changes the name of hook_hook_info() to hook_hooks_info(). I also added a docblock for hook_hooks_info_alter(), which exists although I cannot for the life of me figure out why someone would want to alter the output of hook_hooks_info(), since it would likely mean that the system wouldn't be able to find some other modules' implementations of that hook, as they would be in the wrong file then? Anyway, if you look at how hook_hooks_info() is being called, you find this: [note: only change is the name of the hook, and this is in module_hook_info() in includes/module.inc]:
Comment #16
apadernoI am changing back the priority, as this is not really critic.
As the API freeze is already in act (as reported by webchick), then it's already too late to change this, as it is already late to change the hook name in Drupal 6.
The only solution is to update the documentation to make evident the difference between the two hooks.
It is more correct to say that the Drupal 6 hook should have had a different name, as the hook was not thought to return information about all the implemented hooks, but only to return specific information about the triggered events (maybe
hook_trigger_info()
should have been a better name).Comment #17
jhodgdonSure, hook_trigger_info() is what the D6 hook_hook_info() is called in Drupal 7, and that's because it's a better name.
This is a documentation nightmare if it is left as it is. I'll change the component to documentation and mark it as critical because I have zero idea how to make the doc work well.
Comment #18
webchickCould this not be satisfied with just a comment on the api.d.o page for this function (as well as a note in the module upgrade guide)? I guess I really am not understanding what is so scary here? People are going to need to do all sorts of things to port their modules from 6 to 7.
Comment #19
apadernoI think that a comment on the Drupal 6 documentation page for
hook_hook_info()
saying that the hook has been renamedhook_trigger_info()
in Drupal 7, and a a comment on the Drupal 7 documentation page forhook_hook_info()
saying that is not related with the Drupal 6 hook would be enough.I can change the documentation for Drupal 6.
Comment #20
jhodgdonOK.
Comment #21
apadernoI have updated the Drupal 6 documentation page for the hook, adding a line that says the hook has been renamed in Drupal 7.
Comment #22
jhodgdonThanks.
Still needs D7 patch and a section in the module upgrade page (if there isn't one already). I think the rename of hook_hook_info in D6 was probably already covered, but something should be added there to note that a hook with that name exists in D7.
Comment #23
te-brian CreditAttribution: te-brian commented@jhodgdon
Sorry If my post came off as abrasive at all. Just standing my ground because I believe a rename was the wrong way to go. I agree better documentation is the right path to take.
Not sure if this is the best (or most standardized) wording, but here is a patch that adds a note to the api documentation.
Comment #24
jhodgdonChanging status for test bot.
Comment #25
jhodgdonI'm fine with this patch, and it can't really cause any test bot problems.
kiamlaluno: I made a small tweak to your D6 fix.
When committed, please set back to Needs Work, because we also need to hit the module upgrade guide.
Comment #26
webchickThis patch looks won't fix from me. It is not Drupal 7's job to document the past, nor is it Drupal 6's job to document the future. That's api.drupal.org and the module upgrade page's jobs.
If we went this route, we'd have to lodge comments everywhere for how "This used to be hook_nodeapi" and "This used to take separate parameters rather than an array". No, thanks.
If this is documented in the module upgrade guide (can someone check) i think we can mark this fixed.
Comment #27
int CreditAttribution: int commentedComment #28
jhodgdonSo you're saying it's OK that if you're on http://api.drupal.org/api/function/hook_hook_info/6 or http://api.drupal.org/api/function/hook_hook_info/7, and you click on the Drupal 6 or Drupal 7 tab, it takes you to a totally unrelated hook?
OK. I'm going to stop saying that now. I promise.
Comment #29
apadernoThe change from
hook_hook_info()
tohook_trigger_info()
is documented in the update, under ; nothing in the updating to Drupal 7 page makes a reference to the hookhook_hook_info()
used by Drupal 7, which could confuse who reads thathook_hook_info()
is changed tohook_trigger_info()
, but then finds the tab for the hook also for Drupal 7.API.drupal.org doesn't collect the data it shows if not from the files committed under the project Documentation, or from the files *.api.php used in Drupal 7. The other way to add this kind of meta data is to add them into a Drupal comment in the documentation pages hosted in API.drupal.org; this could be preferable, even if somebody could object about the fact everybody can add a comment, writing something that is not exact, or not completely correct.
Using a comment in the documentation page is fine with me, and I agree that normally the online documentation refers to the actual implementation of a function, not the previous or the future implementation; if that would be the case, then the documentation for a Drupal 6 function should also report any change in the parameters from Drupal 4.7.
I agree this is a particular case, as Drupal doesn't usually use the same name for hooks with completely different purposes, but the note about this hook can be added in a comment. As comments on API.drupal.org cannot be used for support requests, then we should use them to report something not directly documented in the documentation page.
Comment #30
apadernoI am sorry; it took me more time to me to write my comment than the others. I am resetting the priority.
Comment #31
jhodgdonOK then. We should remove the doc change in D6 that you committed and I revised earlier today.
Comment #32
webchick"So you're saying it's OK that if you're on http://api.drupal.org/api/function/hook_hook_info/6 or http://api.drupal.org/api/function/hook_hook_info/7, and you click on the Drupal 6 or Drupal 7 tab, it takes you to a totally unrelated hook?"
I am.
The first thing I'm going to do when I get some sort of error with this hook is go to api.drupal.org and see what the hell is going on. I'm going to skip down to the comments, and as long as there's a comment explaining the change (which there currently isn't on the 7.x page -- could one of you take care of that?) then I will go, "Oh." and move on with my day.
Comment #33
apadernoI removed the text I previously committed in the documentation, and added a comment on the Drupal 6 page explaining that
hook_hook_info()
has been renamedhook_trigger_info()
in Drupal 7, and thathook_hook_info()
is a different hook not related with the hook present in Drupal 6.I added a similar comment for
hook_block()
,hook_nodeapi()
,hook_node_type()
, andhook_user()
.If there is anything else I should do, or if the comment I added should be someway changed, let me know.
I apologies for having changed the Drupal 6 documentation in a not correct way.
Comment #34
apadernoI also added a similar comment for the Drupal 7
hook_hook_info()
documentation page.Comment #35
jhodgdonkiamlaluno: You are a star.
So all we need to do is add a section to the module page about the new hook_hook_info().
Comment #36
apaderno@jhodgdon: To which module page are you referring?
Comment #37
jhodgdonSorry: the module update 6/7 page http://drupal.org/update/modules/6/7 doesn't have anything that I could see about the new hook_hook_info() and how modules might want to use it.
Comment #38
apadernoThe documentation page about updating to Drupal 7 reports only that
hook_hook_info()
has been renamed, but not thathook_hook_info()
is still used from Drupal. I am not sure if this should be documented there, as the page just reports what you need to change in Drupal 6 code to adapt it to Drupal 7; I guess the task of the page is only to help in the port.Comment #39
jhodgdonWell, you might need to know about hook_hook_info() because it has to do with some hooks wanting to live in separate files other than mymodule.module?
Comment #40
webkenny CreditAttribution: webkenny commentedI tend to agree with kiamlaluno on
hook_hook_info()
only being referenced as a name change in that documentation page because of it's title. It's about converting modules that are written in 6.x to 7.x - I think the "knowing about" the new use ofhook_hook_info()
is best left for http://drupal.org/developing/modules or some sub-section thereof.Seems like the right choice if we want to be semantically correct in the purpose of a given documentation page.
Comment #41
jhodgdonMy thought was that someone converting a module to 7.x might want to know that they should move their hook_token_info() implementation into a separate mymodule.tokens.inc file. Which is due to the new hook_hook_info().
They might also want to know that if their module defines hooks, there's now a standardized way to tell their module ecosystem that the hooks can go into separate files. By using hook_hook_info().
Both of these are reasons I think it belongs in the module update guide...
Comment #42
webkenny CreditAttribution: webkenny commentedOk, I'm on board with that reasoning actually. It's a different approach to it now. By phrasing it that way, you're saying "Hey! There are new best practices you should know about while you're updating your stuff!" and "There's some new things that may impact your current code design"
+1 - I'm a sucker for a best practices discussion.
Comment #43
moshe weitzman CreditAttribution: moshe weitzman commentedThe latest patch is adding a couple lines of docs. It doesn't mean that we are going to go adding cruft all over core. That slippery slope argument is non helpful IMO. This is just people helping people - briefly and sensibly. Lets not get in the way.
Comment #44
jhodgdonIMO, the hook names should have been changed, but I think the problem of api.drupal.org has been addressed with the comments that are now on both the D6 and D7 pages for these two completely unrelated hooks with the same name. Let's not go back there, because webchick is right, we haven't done the same thing for zillions of other functions that changed names between 6 and 7. Of course, it's not quite the same, because I don't think we are using the same function name for totally different functions, but...
Anyway, there is still one non-controversial (I think) thing that's remaining here: to add a few sentences to the upgrade page as per #41, and then mark this issue fixed.
Comment #45
jhodgdonI've updated the module update guide: http://drupal.org/update/modules/6/7#new_hook_hook_info
Unless someone wants to suggest edits to this section, I'm going to mark this as "fixed".
Comment #46
apadernoWhat jhodgdon added suits the purpose; it reports the difference between Drupal 6 and Drupal 7, and reports which is the new name for
hook_hook_info()
in Drupal 7.