API page: http://api.drupal.org/api/drupal/includes%21module.inc/function/module_h...

Enter a descriptive title (above) relating to module_hook_info, then describe the problem you have found:

I suspect this collates hook_hook_info() but it would be nice to have a return that explains what you get back, even if it's to say it's a list of what that hook should return.

Files: 
CommentFileSizeAuthor
#21 added-return-for-module_hook_info-1803194-21.patch734 bytesalextataurov
PASSED: [[SimpleTest]]: [MySQL] 42,122 pass(es).
[ View ]
#17 added-return-for-module_hook_info-1803194-17.patch735 bytesalextataurov
PASSED: [[SimpleTest]]: [MySQL] 42,090 pass(es).
[ View ]
#14 added-return-for-module_hook_info-1803194-14.patch771 bytesalextataurov
PASSED: [[SimpleTest]]: [MySQL] 42,126 pass(es).
[ View ]
#4 added-return-for-module_hook_info-1803194-4.patch539 bytesalextataurov
PASSED: [[SimpleTest]]: [MySQL] 42,102 pass(es).
[ View ]

Comments

jhodgdon’s picture

Sure, it could use better documentation. Thanks for reporting -- just needs a patch... possibly a good Novice project?

joachim’s picture

I wasn't sure whether it was a novice project, given how complex the code looks. But let's see what happens :)

jhodgdon’s picture

Novices are often expert PHP programmers who are merely new to Drupal and our patching procedures, so I think it might be fine. :)

alextataurov’s picture

Status:Active» Needs review
StatusFileSize
new539 bytes
PASSED: [[SimpleTest]]: [MySQL] 42,102 pass(es).
[ View ]

Added @return description for module_hook_info() function.

jhodgdon’s picture

Status:Needs review» Needs work

Thanks for the patch!

I'm not sure this is quite accurate though. It looks like module_hook_info() returns a compilation of hook_hook_info() return values, and this doesn't match what hook_hook_info() has for its return value. They should match. Thus one of them must be incorrect... It looks to me as though hook_hook_info()'s documentation is correct, based on the one core implementation, which is system_hook_info()?

alextataurov’s picture

Yes, module_hook_info() function just collects over all modules return values of theirs hook_hook_info() implementations. The description of hook_hook_info() also looks for me more descriptive, but I thought that copying the description of hook_hook_info() and pasting it over module_hook_info() function is not a good idea.

I don't know, we may copy the description of hook groups from hook_hook_info() and paste it. Then description and @return statement for module_hook_info() may look this way:

/*
* Retrieves a list of what hooks are explicitly declared through hook_hook_info().
*
* Normally hooks do not need to be explicitly defined. However, by declaring a
* hook explicitly, a module may define a "group" for it. Modules that implement
* a hook may then place their implementation in either $module.module or in
* $module.$group.inc. If the hook is located in $module.$group.inc, then that
* file will be automatically loaded when needed.
* In general, hooks that are rarely invoked and/or are very large should be
* placed in a separate include file, while hooks that are very short or very
* frequently called should be left in the main module file so that they are
* always available.
*
* @return
*   An associative array whose keys are hook names and whose values are an
*   associative array containing:
*   - group: A string defining the group to which the hook belongs. The module
*     system will determine whether a file with the name $module.$group.inc
*     exists, and automatically load it when required.
*
* @see hook_hook_info()
*/
joachim’s picture

Duplicating text is probably not a good idea, as it means there are two copies to keep in sync.

Perhaps what suffices here is a @return that states you get an array with the same structure as hook_hook_info().

alextataurov’s picture

What about this description? Is it informative enough?

/*
* Retrieves a list of what hooks are explicitly declared.
*
* @return
*   An associative array whose keys are hook names and whose values are an
*   associative array containing group name. The structure of the array
*   is the same as return of hook_hook_info().
*
* @see hook_hook_info()
*/
joachim’s picture

* Retrieves a list of what hooks are explicitly declared.

^^ that seems totally wrong to me, but out of scope for this issue.

Hence yup, looks good!

jhodgdon’s picture

Good back and forth here! One thing on #9 - please fix up the whole function doc (including the first line) so that it makes sense rather than deciding we need a two-line patch here and a totally separate issue to fix the other line. When there are so few lines and all in the same function to fix, expanding the scope slightly to fix up the entire function doc just makes sense (unlike, say, taking a 1000 line patch issue and doubling the scope so there are now 2000 lines of changes to review). :) Thanks!

joachim’s picture

Fair enough.

In which case:

> * Retrieves a list of what hooks are explicitly declared.

- should be 'which', not 'what'.
- 'explicitly declared' -- as currently this just tells you what group they go in, I think this is a slightly ambitious description

alextataurov’s picture

Maybe this title for function:

> * Retrieves a list of hooks that's declared through hook_hook_info().

jhodgdon’s picture

#12 sounds good -- except remove "that's" (or change it to "that are").

alextataurov’s picture

StatusFileSize
new771 bytes
PASSED: [[SimpleTest]]: [MySQL] 42,126 pass(es).
[ View ]

I prepared a new version of patch. Also, I can prepare a patch for Drupal 7.x.

alextataurov’s picture

Status:Needs work» Needs review
jhodgdon’s picture

Status:Needs review» Needs work

Oops, the first line of the doc block still needs to start with /**:

-/**
- * Retrieves a list of what hooks are explicitly declared.
+/*

Also, I think we need some a/an/the added to this documentation:

+ *   An associative array whose keys are hook names and whose values are an
+ *   associative array containing group name. The structure of the array
+ *   is the same as return of hook_hook_info().

- containing *a* group name
- same as *the* return *value* of hook_hook_info()

Thanks!

alextataurov’s picture

StatusFileSize
new735 bytes
PASSED: [[SimpleTest]]: [MySQL] 42,090 pass(es).
[ View ]

Thanks for these corrections. English is not my native language:)

I prepared a new version of the patch, hope this is final.

alextataurov’s picture

Status:Needs work» Needs review
chanderbhushan’s picture

/core/includes/module.inc
function module_implements_reset() {

/** @see hook_hook_info()
*/
function module_hook_info() {

@return An associative array whose keys are hook names and whose values are hook groups.

jhodgdon’s picture

Status:Needs review» Needs work

I'm not sure what the comment in #19 is about... Anyway, the text in the patch looks good to me! There's an extra space at the end of one of the lines though that needs to be removed, though. If you're planning to keep contributing patches to Drupal (and I hope you will!), you might want to investigate whether your code text editor can be set to either remove all end-of-line spaces or highlight them. Thanks!

alextataurov’s picture

Status:Needs work» Needs review
StatusFileSize
new734 bytes
PASSED: [[SimpleTest]]: [MySQL] 42,122 pass(es).
[ View ]

I will try to be more careful in the future when I'm preparing a patch:) I removed this extra space and I prepared a new version of the patch.

jhodgdon’s picture

Assigned:Unassigned» jhodgdon
Status:Needs review» Reviewed & tested by the community

That looks fine, thanks! Assuming the test bot shows green, I'll get this one committed soon (probably will work for both 7.x and 8.x, we'll see).

jhodgdon’s picture

Assigned:jhodgdon» Unassigned
Status:Reviewed & tested by the community» Fixed

Thanks again - committed to 8.x and 7.x.

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