hook_menu API docs missing some elements
joachim - February 24, 2009 - 22:22
| Project: | Drupal |
| Version: | 7.x-dev |
| Component: | documentation |
| Category: | bug report |
| Priority: | normal |
| Assigned: | Unassigned |
| Status: | closed |
| Issue tags: | API clean-up |
Description
http://api.drupal.org/api/function/hook_menu
Browsing through the handbook pages on the new menu system, a fair number of keys seem to be missing from the API docs, which should be the authority.
http://drupal.org/node/300997 -- tab stuff, file stuff.
http://drupal.org/node/224170 -- load arguments
#1
The doc is also malformed (Return Value header is at the bottom of the page, after the text explaining what the form elements are, due to extra newline). I will patch both issues at once
#2
#3
#4
Here's a patch.
#5
The last submitted patch failed testing.
#6
#7
applied against head and the documentation changes were made. Looks good
#8
This will need ported once it is applied to D7.
#9
Committed to CVS HEAD.
#10
#11
#12
Isn't there the stuff about tabs too?
#13
+ * - "file": A file that will be included before the callbacks are accessed.+ * this allows callback functions to be in separate files. The file should
Either use a comma, or start the next sentence with an uppercase letter.
+ * the arguments corresponding to the second and fourth url argument;+ * as with other arguments, integers are automatically cast to url
...
+ * to the url index where the object's load function is specified; "%map"
"URL" is always uppercase.
#14
Oops, nice catch, sun. The attached patch fixes the grammatical problems pointed out in #13, and also removes an extraneous period I noticed from the hook_menu() docs.
#15
#16
This looks like an excellent grammar, punctuation, and capitalization update to me.
However, the original patch (which was applied) and this patch as well didn't fix one of the problems in http://api.drupal.org/api/function/hook_menu/7, as noted in comment #1 above -- which is that there is a big section of bullet points that belongs in the Return Value section, but because of an extra newline, that section is not being picked up as part of the Return Value section (it's up in the main documentation). So, it would be good to add that to your patch.
Also, note that a port to 6.x is also needed, which would incorporate the changes from #4 and this current patch.
#17
We're still missing:
'tab_parent' => NULL,
'tab_root' => NULL,
'block callback' => '',
and possibly others. I have no idea what the above do by the way. Is the author of the D6 menu system around?
#18
As far as I understand, the tab_ stuff does internal tab-related stuff that's best not to mess with. I can add documentation ("this is used internally") for those if you'd like. As far as I understand again, block callback refers to the callback that returns the block on the administration page ("Content management" / "Site building" / etc.)
I believe the attached patch should fix the problem with blank lines.
#19
Committed to CVS HEAD. I'm setting this to 'code needs work' so we can add the missing bits.
#20
Actually the 'file' and 'file path' elements have been removed from hook_menu() in Drupal 7 (http://drupal.org/node/224333#menu_file_path)
#21
I'll patch this...
#22
I looked through the code, including system_install(), _menu_router_build(), and other functions that deal with the menu system.
There were several components missing, and several that had to be removed.
I also did some grammatical/style/clarity edits, since I can never resist those :), fixed an over-80-character line, and added a section to the hook_menu() doc that explains the magic auto-loading functionality (which I was amazed at when I discovered it once by delving into the code, and think should be there in the doc, because it is certainly not obvious until you find out about it).
Anyway, here's a patch of revisions to the hook_menu() documentation.
#23
Very nice!
The autoloading stuff was very definitely in need of writing up.
Some very minor points:
+++ modules/menu/menu.api.php 13 Aug 2009 20:00:00 -0000@@ -34,24 +48,19 @@
+ * function. Integer values pass the corresponding URL component (see
+ * arg()).
Can this be made into a magic @see link?
+++ modules/menu/menu.api.php 13 Aug 2009 20:00:00 -0000@@ -14,19 +14,33 @@
+ * node_load(12345) will automatically be called, and this node object
+ * will be available as path argument 1 to the page and access
'path argument' make me think of what arg() gets -- the term 'load arguments' is used for this lower down, which is clearer.
This review is powered by Dreditor.
#24
Maybe we don't even need a link to arg() there at all? I just reformatted what was there into 80-character line, but thinking more about it, I don't really see what a link to the arg() function does for the explanation. In any case, any time you put a function name in like arg() with the parens, the API display module turns it into a magic link.
Regarding your second comment, this is not the same as a load argument actually. Load arguments refers to making the load_foo() function take extra arguments. This refers to making $node substitute for 12345 if your page or access function asks for path component #2. But I agree maybe the current wording could be confusing... Not sure what the best wording is... any ideas?
#25
Wait, I have an idea. New patch coming...
#26
Here it is...
#27
This time, I decided a paragraph at the top explaining the path component substitution also made sense.
#28
I corrected a couple of repetitions.
+ * can also register a link to be placed in the the navigation block+ * Your registered paths can contain also contain specialAlso, throughout the text, I see several uses of "pass into", "pass to" and such, related to the act of sending data to functions/callbacks. As a non-native English speaker, my bet would be on "pass on to" as the correct form, but I leave this matter to natives.
#29
The last submitted patch failed testing.
#30
Oh, testing bot...
#31
The last submitted patch failed testing.
#32
#33
The last submitted patch failed testing.
#34
That above patch indeed would not apply, due to other changes...
Here is a new patch.
I'm assuming that the file/file path stuff needs to be there again, since the registry went away? At least, I see it in node_menu(), so I am assuming it should be left in the hook_menu() doc now.
#35
Revamped. Code examples usually do better.
#36
I'll leave the bot to test how it applies, but I gave this a read-through.
Very good explanation of argument substitution! Learnt a few new things :)
Big improvement overall.
Just one tweak -- strange turn of phrase here:
+ * menu argument loader function, which would be mymodule_abc_load() then. For
Better this way:
+ * menu argument loader function, which here would be mymodule_abc_load(). For
#37
Also, what's this "again"? Awkward wording... I think maybe you are trying to say it is a nested array?
+ * hook_menu() implementations return an associative array that contains again+ * an associative array for each path. The definition for each path may refer to
Anyway, I think the prose in your version needs some work... it's probably no surprise that I liked my prose explanations better. :)
#38
Is the following better?
* hook_menu() implementations return an associative array whose keys* are the paths and whose element values are an associative array defining
* the path elements. ...
#39
+1 to earnie's change.
#40
[deleted]
#41
Just a note: #583398: Not clear that access callbacks have to be in the same file as hook_menu.
Both my patch in #34 and sun's patch in #35 address this (making sure the file element says it doesn't apply to access callbacks). Any future versions should too.
#42
Thanks for the reviews + suggestions!
Incorporated those.
Furthermore, I realized that we explain all that advanced argument handling stuff, but not a single word about the most trivial case: optional default arguments. Revised nightmare attached.
#43
This is great.
#44
Tagging.
#45
Committed to CVS HEAD. Thanks!
#46
Should we consider porting all/some of this patch to Drupal 6? I think it would be a good idea to have this explanation of auto-load and arg substitution there as well.
#47
Yes, very good idea.
Which parts aren't relevant to D6?
#48
Check to make sure, but I think "theme callback" and args were added in D7. Maybe other components? "block callback"? "tab root"?
Also might be good to wait a day or two and incorporate
#536788: Provide documentation on making menu tabs
into the same D6 port, assuming it gets accepted/committed.
#49
After a careful look through the D6 code, I believe that only these two items need to be omitted for D6:
theme callback
theme arguments
#50
That other issue #536788: Provide documentation on making menu tabs was committed to D7. So they can be ported to D6 together, if desired.
#51
Actually, I would wait on porting after all -- there is more discussion happening on that other issue... more changes to hook_menu doc happening... I am going to suggest that this patch NOT be ported, and we do the porting on the other issue instead.
#52
Automatically closed -- issue fixed for 2 weeks with no activity.