Jump to:
| Project: | API |
| Version: | 6.x-1.x-dev |
| Component: | User interface |
| Category: | feature request |
| Priority: | normal |
| Assigned: | Unassigned |
| Status: | closed (won't fix) |
Issue Summary
Could we extend doxygen documentation with a list of hooks that a function calls, including short description and link to the full documentation (if one exists)? So that we could compile this list into a new section for each function. Currently, if you want to know which hooks a function calls you have to skim through the function code. Given the importance of hooks to Drupal, this is not only a huge waste of time but also not very developer friendly with regard to novices (and seasoned programmers overwhelmed by the sheer mass of changes in D7 ;).
A real-life example: form_builder() is the function that executes the #after_build hook on form elements — but to find that out you have to read through two thirds of the code. And then, what exactly was the purpose of after_build hook again, how does it relate to form_alter, etc.?
See attached mockup about how I imagine this information could be presented.
Thoughts?
| Attachment | Size |
|---|---|
| api_hooks.png | 24.94 KB |
Comments
#1
I think this is an excellent idea, especially if it could be generated automatically.
The idea would be that a function that calls module_invoke() or module_invoke_all() -- API module would detect this and add it to a Hooks Invoked section of the function doc. A link back to the calling function would also appear on the hook doc page, in a "Functions that invoke this hook" section.
For other hooks that are not invoked through module_invoke() et al, maybe they could have a special comment in the code that would tell the API module that it's a hook invocation, so these links could be generated?
The other possibility would be to put this in manually, which is probably what would need to be done to generate something like the mockup. However, I think if it could be done automatically, it would be better -- a programmer could definitely read the code to figure out when the hook was invoked, and if it was manual, it would take extra work to get that into the doc blocks.
#2
Gosh, I wish I had something like this when I started:
<?php// @invoke hook_perm().
foreach (module_list() as $module) {
$foo += module_invoke($module, 'perm')) {
}
?>
#3
I think we should put it into the docblock, rather than (or in addition to) as a code comment. The reason is that docblocks -> api.module parsing, and I think we want this information indexed by the API module.
#4
From what I've learned so far, we have a custom parsing algorithm that parses function names in code already. So we should start with the best ideas for usability and wind it back only if it is not technically possible.
#5
Yes, this something we can do by walking through the grammar parser tree. API is used by a couple non-Drupal projects, but I'm fine with hard-coding Drupalisms in. Maybe there will be a separate Drupal rules file at some point. This will discourage the information-lacking "Implementation of hook_blah()."-only comments. We shouldn't ever ask for documentation to repeat the code.
#6
In my opinion, for usability the place for the comment is in the docblock. Anyone who has worked with Drupal hooks for a while understands that module_invoke() invokes a hook, and can figure out the hook name from the code easily. #2 adds nothing to that, for me.
If it's in the docblock, we could conceivably generate on the hook function page a "functions that invoke this hook" section. And it would be displayed with the other function doc (params, return, etc.)
#7
@jhodgdon
These are our own opinions about usability of course - the module_invoke pattern was a blocker for me learning Drupal for a long time.
So if I may rephrase. Since we have a custom parser, let's be open-minded about ideas.
Drumm can push back if it's undoable. For sake of argument, it would be totally simple to take all the @invoke's from a function body and list them in the documentation header.
#8
We don't need to add any documentation. This can all be determined by parsing code.
* Functions named hook_foo() define a hook.
* Functions named {module}_foo() are implementations of that hook.
* There isn't a universal way to invoke hooks, but we can detect any reasonably normal pattern, like using module_invoke_all().
This makes the existing "Implementation of hook_foo()." comments fully-redundant and they should be replaced with documentation of what it is actually doing.
#9
Just discussed this with sime and drumm. Our conclusions:
- We should let the parser do everything it can. So it can know that modulename_node_insert() is an implementation of hook_node_insert(), and that module_invoke('foo', ...) is invoking hook_foo(). And that can lead to lists of who invokes who, who's invoked by who, turning things into links, etc.
- We don't need to add @invokes before module_invoke() and module_invoke_all() because it doesn't add any new information.
- We also need to recognize node_invoke() and functions like that.
- We also need a way to add @invokes or something like that as a code comment, for the few cases that don't use one of the usual *_invoke() functions.
#10
Changing title (I keep getting confused looking at this issue title)
#11
@drumm (#8) I'm now working on a patch to create more meaningful comments for the hook implementations, however I don't think that "Implements hook_xy" comments are redundand, they have still some informative value for developers. There are other two related and slightly concurrent issues, how to make hook implementations comments and hook API pages better:
#983268: Use @implements Doxygen directive for hook implementations
and
#987450: On a hook page, list functions that implement this hook
Since I rather use some common standard, not inventing some special "drupal-only" (in this case) standard, I tried to search for some Doxygen descriptor, but it seems that doxygen does have nothing similar to @invokes ... the most similar descriptor is general @sa or @see... :-/
#12
Upping priority (marking "major" things that I'd like to see addressed sooner rather than later).
#13
I think I'm going to demote this to "won't fix". My reasoning:
We have some useful related functionality already recently added:
- With #294218: Turn hook names in theme() and module_invoke() into links, whenever a function calls module_invoke() and other similar functions, the hook name turns into a link to that hook documentation in the code.
- With #987450: On a hook page, list functions that implement this hook, hook pages list all implementations of that hook on the documentation page.
We have an even more useful bit of functionality requested on another issue:
#1394404: On a hook, theme hook, or theme template page, list functions that invoke this hook/theme
That would make it so if you were on the hook_foo() page, you would see in the "Functions that call this function" list, all the places that do stuff like module_invoke_all('foo'), etc. I think that is a higher priority than this issue, and would make the links from 294218 also go the other way.
Also, making the list requested on this issue is not consistent with how we deal with other function calls. On a function page, we have the code displayed. We don't make a list of every function this function calls -- you can see them in the code, and they're linked if you want to go there and see what the function does. Similarly, we shouldn't probably make a list of all the hooks this function invokes -- they are now also links and in the code.
Actually, the original mockup attached to this issue looks like it would be an additional section added to the documentation header, anyway -- it wouldn't be possible to generate it from the code, with the added documentation. I don't think that is all that necessary -- you can write this kind of documentation if you want to now (minus the header). And adding to the documentation standards an idea like "Whenever you invoke a hook, you need to add this xyz to the documentation header" is not ever very welcome in the Drupal development community.
So all in all... I think I'll close this as "won't fix" for now. It can be reopened later if someone feels really strongly about it, and can make a case as to why we need it.