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

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

apaderno’s picture

I 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).

te-brian’s picture

I 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:)

jhodgdon’s picture

The 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.

xtfer’s picture

Status: Active » Needs review
FileSize
372 bytes
1.64 KB

The 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.

xtfer’s picture

The last patch (#4) dropped a diff file on form submission... Submitting again.

Status: Needs review » Needs work

The last submitted patch, system-inc-675136-4.diff, failed testing.

jhodgdon’s picture

You need to put all of your changes into one single patch, because the testing bot will test them all as a group.

te-brian’s picture

I'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.

jhodgdon’s picture

te-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.

apaderno’s picture

If 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 (see hook_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, while hook_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.

jhodgdon’s picture

Thank you kiamlaluno, that is an excellent idea.

webchick’s picture

I'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...)

webchick’s picture

Priority: Critical » Normal

Also, I don't see Drupal bleeding from its eyeballs as a result of this bug, so it's normal.

jhodgdon’s picture

Priority: Normal » Critical
Status: Needs work » Needs review
FileSize
3.81 KB

Here 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]:

      // We can't use module_invoke_all() here or it would cause an infinite
      // loop.
      foreach (module_list() as $module) {
        $function = $module . '_hooks_info';
        if (function_exists($function)) {
          $result = $function();
          if (isset($result) && is_array($result)) {
            $hook_info = array_merge_recursive($hook_info, $result);
          }
        }
      }
      // We can't use drupal_alter() for the same reason as above.
      foreach (module_list() as $module) {
        $function = $module . '_hooks_info_alter';
        if (function_exists($function)) {
          $function($hook_info);
        }
      }

Status: Needs review » Needs work

The last submitted patch, 675136hooks_info.patch, failed testing.

apaderno’s picture

Priority: Critical » Normal

I 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).

jhodgdon’s picture

Title: hook_hook_info() is a really really really bad choice of names! » Document how/why hook_hook_info() is totally different in D6 and D7
Component: base system » documentation
Priority: Normal » Critical
Status: Needs work » Active

Sure, 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.

webchick’s picture

Could 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.

apaderno’s picture

I think that a comment on the Drupal 6 documentation page for hook_hook_info() saying that the hook has been renamed hook_trigger_info() in Drupal 7, and a a comment on the Drupal 7 documentation page for hook_hook_info() saying that is not related with the Drupal 6 hook would be enough.

I can change the documentation for Drupal 6.

jhodgdon’s picture

OK.

apaderno’s picture

I have updated the Drupal 6 documentation page for the hook, adding a line that says the hook has been renamed in Drupal 7.

jhodgdon’s picture

Thanks.

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.

te-brian’s picture

FileSize
734 bytes

@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.

jhodgdon’s picture

Status: Active » Needs review

Changing status for test bot.

jhodgdon’s picture

Status: Needs review » Reviewed & tested by the community
Issue tags: +Needs documentation

I'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.

webchick’s picture

Status: Reviewed & tested by the community » Needs review

This 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.

int’s picture

Priority: Critical » Normal
jhodgdon’s picture

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?

OK. I'm going to stop saying that now. I promise.

apaderno’s picture

Priority: Normal » Critical

The change from hook_hook_info() to hook_trigger_info() is documented in the update, under Trigger and Actions API overhaul; nothing in the updating to Drupal 7 page makes a reference to the hook hook_hook_info() used by Drupal 7, which could confuse who reads that hook_hook_info() is changed to hook_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.

apaderno’s picture

Priority: Critical » Normal

I am sorry; it took me more time to me to write my comment than the others. I am resetting the priority.

jhodgdon’s picture

OK then. We should remove the doc change in D6 that you committed and I revised earlier today.

webchick’s picture

"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.

apaderno’s picture

I 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 renamed hook_trigger_info() in Drupal 7, and that hook_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(), and hook_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.

apaderno’s picture

I also added a similar comment for the Drupal 7 hook_hook_info() documentation page.

jhodgdon’s picture

kiamlaluno: You are a star.

So all we need to do is add a section to the module page about the new hook_hook_info().

apaderno’s picture

@jhodgdon: To which module page are you referring?

jhodgdon’s picture

Sorry: 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.

apaderno’s picture

The documentation page about updating to Drupal 7 reports only that hook_hook_info() has been renamed, but not that hook_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.

jhodgdon’s picture

Well, 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?

webkenny’s picture

I 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 of hook_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.

jhodgdon’s picture

My 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...

webkenny’s picture

Ok, 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.

moshe weitzman’s picture

The 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.

jhodgdon’s picture

Status: Needs review » Needs work

IMO, 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.

jhodgdon’s picture

Status: Needs work » Fixed

I'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".

apaderno’s picture

What 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.

Status: Fixed » Closed (fixed)
Issue tags: -Needs documentation

Automatically closed -- issue fixed for 2 weeks with no activity.