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.
Comment | File | Size | Author |
---|---|---|---|
#21 | added-return-for-module_hook_info-1803194-21.patch | 734 bytes | alextataurov |
#17 | added-return-for-module_hook_info-1803194-17.patch | 735 bytes | alextataurov |
#14 | added-return-for-module_hook_info-1803194-14.patch | 771 bytes | alextataurov |
#4 | added-return-for-module_hook_info-1803194-4.patch | 539 bytes | alextataurov |
Comments
Comment #1
jhodgdonSure, it could use better documentation. Thanks for reporting -- just needs a patch... possibly a good Novice project?
Comment #2
joachim CreditAttribution: joachim commentedI wasn't sure whether it was a novice project, given how complex the code looks. But let's see what happens :)
Comment #3
jhodgdonNovices are often expert PHP programmers who are merely new to Drupal and our patching procedures, so I think it might be fine. :)
Comment #4
alextataurov CreditAttribution: alextataurov commentedAdded @return description for module_hook_info() function.
Comment #5
jhodgdonThanks 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()?
Comment #6
alextataurov CreditAttribution: alextataurov commentedYes, 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:
Comment #7
joachim CreditAttribution: joachim commentedDuplicating 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().
Comment #8
alextataurov CreditAttribution: alextataurov commentedWhat about this description? Is it informative enough?
Comment #9
joachim CreditAttribution: joachim commented* 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!
Comment #10
jhodgdonGood 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!
Comment #11
joachim CreditAttribution: joachim commentedFair 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
Comment #12
alextataurov CreditAttribution: alextataurov commentedMaybe this title for function:
> * Retrieves a list of hooks that's declared through hook_hook_info().
Comment #13
jhodgdon#12 sounds good -- except remove "that's" (or change it to "that are").
Comment #14
alextataurov CreditAttribution: alextataurov commentedI prepared a new version of patch. Also, I can prepare a patch for Drupal 7.x.
Comment #15
alextataurov CreditAttribution: alextataurov commentedComment #16
jhodgdonOops, the first line of the doc block still needs to start with /**:
Also, I think we need some a/an/the added to this documentation:
- containing *a* group name
- same as *the* return *value* of hook_hook_info()
Thanks!
Comment #17
alextataurov CreditAttribution: alextataurov commentedThanks for these corrections. English is not my native language:)
I prepared a new version of the patch, hope this is final.
Comment #18
alextataurov CreditAttribution: alextataurov commentedComment #19
chanderbhushan CreditAttribution: chanderbhushan commented/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.
Comment #20
jhodgdonI'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!
Comment #21
alextataurov CreditAttribution: alextataurov commentedI 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.
Comment #22
jhodgdonThat 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).
Comment #23
jhodgdonThanks again - committed to 8.x and 7.x.
Comment #25
chanderbhushan CreditAttribution: chanderbhushan as a volunteer and at gai Technologies Pvt Ltd for gai Technologies Pvt Ltd commented