No standard for documenting menu callbacks

Other patterns I've seen so far in the course of #1315886: Clean up API docs for includes directory, files starting with A-C:

• Foo callback for bar.
• Foo callback; verbs bar.
• Helper function for bar: verbs baz.
• Shutdown function; verbs the bar.

Do we want to include "Helper function" or "Shutdown function" in this standard?

I like the proposed standard that differentiates between page/access/title callbacks and which menu they belong to quite a lot. However, some of those menu paths get quite long, such as 'admin/config/regional/date-time/formats/%/delete' which is starting to just barely fit into 80 chars once you add "Access callback for " in front of it. Not sure if that's a big deal or not.

As a developer, when I look at the 1-line summary for a function on api.drupal.org, seeing "Menu callback:" in there is really helpful. Once I know it's a callback, I can then choose to read the detailed documentation about the function to learn more about the specific path, etc. Since that 80-char limit gets immediately reduced by starting the summary line with "Menu callback:" (or whatever we decide on), I'd argue to notrequire the path in there. (It could be optional/just suggested). I think the colon (:) is concise and clear.
My suggestion:
[Callback-type] callback: [Function goal]

Where:
1. Callback-type can be the general "Menu" (as in "Menu callback:"), or more specific (as in "Page callback:" or "Access callback:" etc.). If someone unfamiliar with the code is coming in after the fact to put in documentation, they may not be able to tell what kind of callback it is. Thus we should allow for the general "Menu callback:". But if the documentor (?) knows what kind of callback it is, then by all means the more specific callback type should be used.
   1. Will we need to specify the list of all acceptable "callback types" in the standards? Or is it best to provide a list of some of the accepted types (so the list is not exhaustive or exclusive)?
2. Function goal follows the existing standards for the brief description of a function (starts with a verb, is one full sentence, etc).
3. Put only one space after the colon, not two. (Is this right? I'm kind of assuming this.) This is consistent with how lists are formatted.

Ex:
Menu callback: Display a listing of tracked nodes on the site.
Page callback: Verb in the third person form starting a concise sentence.
Title callback: Generates a title based on phase of the moon.

If the callback path is known/applicable, then I like the suggestion that it should be put on its own line. It's a such an important piece of information. Being on its own line really goes a long way towards readability.

I've also come across functions described as Helper function: [....] as I'm working with #1310084: [meta] API documentation cleanup sprint. I agree with jhodgdon: let's agree on this standard first, then we can see if it applies to other cases ('Helper' functions, etc.) and address them as separate issues.

The title for this issue might need to be generalized to just "callbacks" instead of "menu callbacks" as in "No standard for documenting callback functions".

+1 to @aenw's proposed idea

An additional thought: should the calling function(s) be noted in the documentation by @see ? Ex:

/**
 * Page callback: Loads information for an API file.
 * 
 * Callback path: %api_filename
 *
 * Loads information for an API file. <blah blah blah.... >
 *
 * @see api_menu()
 */

It strikes me as a good idea, but perhaps it's overkill. Perhaps it should just be a suggestion?

I think there should, in general, be a link in some fashion to the calling function and/or the function where the callback is registered (api_menu isn't really the calling function, is it?).

This issue interacts with #1250500: [policy adopted; patch done] Find a (standard) way to document callback signatures, where we agreed on somewhat of a standard to document callback signatures in .api.php files (but I never got to propose a text for the standards page). Ultimately, callback implementations should link to these sample implementations (just as hook implementations now link to "hook_*" via the "Implements hook_*()." lines now. But I think this should not be done in the one-line summary for callbacks (at least menu callbacks and the like), as there is more important info to go there.

Oh, and re #5:

1. I don't think we should specify an exhaustive list, esp. because a contrib module might want to add its own type (and the Standards should apply equally to core and contrib).
2. We should decide whether, after the colon, we want to have a 3rd person verb ("Displays ...") or an imperative verb form ("Display ..."). The examples in #5 are mixed. Current standards say 3rd person for functions, imperative for hooks ...

Just a thought, but #7 gets me thinking in the direction of just using standard Doxygen things for this. Like:

/**
 * Returns a foody bar baz.
 *
 * @ingroup menu_page_callbacks
 * @see foo_menu()
 */

That might be silly, though. But the advantage would be that the 80 character description would be the same as all other functions and wouldn't need special explanation. It'd also force us to document menu_page_callbacks/menu_title_callbacks/menu_access_callbacks somewhere. ;) And we'd get a nice list of all of them like at http://api.drupal.org/api/drupal/includes--actions.inc/group/actions/7

I think having a standard @see to the caller (since callbacks aren't linked by the API module) is a very good idea. (I actually wish hooks did that as well.) I also like webchick's suggestion of having groups for the various types of callbacks. Edit: This would be consistent with the standard for form functions, although submission and validation handlers are apparently not included in the forms @ingroup (http://drupal.org/node/1354#forms).

Edit: Also, documenting the path(s) the callback gets used for on a separate line seems like a very good idea.

I guess the question is whether or not we want to generally allow the pattern Thing prefix: verbs a foo. On the one hand, it does make identifying the function's purpose easier in a quick scan; on the other, an @ingroup would accomplish the same goal. Plus, the prefix cuts into 80 chars, plus it introduces more specific standards for developers to learn (since the prefix would differ for each callback type).

Also, I'm trying to decide if there's a situation in which simply Foo callback. with no description is sufficient (like we already have with Form constructor for mymodule_foo_form()).

Oh, a minor thing: If we do use the prefixes, I think we should standardize on a colon instead of a semicolon, and the verb after the colon should be capitalized. As in:
Menu callback: Handles Ajax requests for the #ajax Form API property.

For an instructive example, see ajax_base_page_theme(). This is a theme callback that is used for multiple paths (which is always possible and half the point of using callbacks). Here's what I have trying out the proposed standard:

/**
 * Theme callback: Returns the correct theme for an Ajax request.
 *
 * Paths: system/ajax, file/ajax, file/progress
 *
 * Many different pages can invoke an Ajax request to system/ajax or another
 * generic Ajax path. It is almost always desired for an Ajax response to be
 * rendered using the same theme as the base page, because most themes are
 * built with the assumption that they control the entire page, so if the CSS
 * for two themes are both loaded for a given page, they may conflict with
 * each other. For example, Bartik is Drupal's default theme, and Seven is
 * Drupal's default administration theme. Depending on whether the "Use the
 * administration theme when editing or creating content" checkbox is checked,
 * the node edit form may be displayed in either theme, but the Ajax response
 * to the Field module's "Add another item" button should be rendered using
 * the same theme as the rest of the page. Therefore, system_menu() sets the
 * 'theme callback' for 'system/ajax' to this function, and it is recommended
 * that modules implementing other generic Ajax paths do the same.
 *
 * @see system_menu()
 * @see file_menu()
 */

In light of the multiple paths thing, do we instead want to do a list?

 * Paths:
 *   - foo/bar

and

 * Paths:
 *   - foo/bar
 *   - foo/baz
 *   - blah/this/that/cow/box/the_other_thing

If we add a group for each type of callback and use this pattern for paths, it would become:

/**
 * Returns the correct theme for an Ajax request.
 *
 * Paths:
 *   - system/ajax
 *   - file/ajax
 *   - file/progress
 *
 * Wall of text here...
 *
 * @see system_menu()
 * @see file_menu()
 * @ingroup menu_theme_callbacks
 */

Other callbacks for which standards might be helpful are callbacks used with PHP array functions (array_walk(), array_map(), uasort(), etc.) Lots of these in common.inc. E.g. drupal_sort_css_js().

+1 for #17 with 2 changes in the explanatory Notes section:

FYI: A callback can be used with something other than a menu routing function. It's a general PHP construct. So there might not always be a path involved. By 'calling function' that I meant a function that calls the one we're documenting. sven.lauer in #8 above uses the more accurate descriptor: where the callback function is registered. That likely won't make sense to folks that aren't on the geeky-coder end of the spectrum. So I think the standard should use both phrases so the readers will understand the intent.
Also, the callback function might be registered by more than one (calling) function. So we should be clear about what we want in that case.

I propose 2 changes to the Notes section: (Changes are in bold just to make them obvious (if ugly) here. I don't mean that to be in the final formatting.)
1) change "- After a blank line, tell what path or paths it pertains to. If there is more than one, format it as a list."
to
"- After a blank line, tell what path or paths it pertains to if applicable (e.g. in the case of a menu callback). If there is more than one, format it as a list."

2) change "- At the end, include a @see line that points to the hook_menu() implementation where this callback is mentioned."
to
"- At the end, include a @see line that points to the function where this callback is referenced (where it is called). If more than one function references this one, include one @see line for each."

Edited to clarify several points.

I like #17.

For #19:

• I agree with the remarks about callbacks. I'd add that since callbacks are "called" by a different mechanism than the more typical caller with mymodule_foo(), documenting them is important for helping explain how the code works.
• I like change #2. I think using the word "registered" would be fine if we explain it parenthetically, but maybe even just the word "used" would be simpler.
• On change #1, I think hook_menu() callbacks merit their own documentation section; it gets too abstract otherwise.

So, tweaking @jhodgdon's proposal:

Documenting hook_menu() callbacks

The page, access, title, and other callbacks that are denoted in hook_menu() are documented as follows:

/**
 * Page callback: Displays a list of content.
 *
 * Path: admin/node/list
 *
 * Longer description can go here.
 *
 * @see node_menu()
 */

Notes:

• In the first line, declare what type of callback it is (Page callback, Access callback, etc.), and give a short description of what it does (like you would for any function). Total line length: 80 characters, ending in "."
• After a blank line, tell what path or paths it pertains to. If there is more than one, format it as a list.
• After another blank line, include a longer description of the function, followed by parameters and return value if applicable.
• At the end, include an @see line that points to the hook_menu() implementation where this callback is used. If multiple hook implementations use the callback, include one @see line for each.

Then, we would repeat a similar pattern for other sorts of callbacks. I think we should extend the pattern to (e.g.) render API and the array sorting callbacks that gave me pause in common.inc. There are two possibilities for callback types (outside of, you know, hooks and stuff):

1. Generic PHP callbacks: E.g., those used with uasort(), array_map(), etc. For these, maybe:

   • uasort() callback: Sorts foo by bar given baz.
     or maybe:
     Sorting callback: Sorts foo by bar given baz.
   • If it's a function that actually does something on its own, the sort that gets used with array_map() and array_walk(), we could do either:
     array_map() callback: Verbs foo.
     or even just:
     Callback: Verbs foo.

   I think I prefer the latter in both cases. In both cases we'd want @see to the function where they are registered/referenced/used/called/mentioned. :)

   A special case of this is shutdown functions. In D6 and earlier, these were registered with general PHP register_shutdown_function(); in D7 we have drupal_register_shutdown_function(). I'd suggest documenting those under their own subheader, maybe like:

   /**
    * Shutdown function: Verbs foo bar.
    *
    * Longer description...
    *
    * @see drupal_register_shutdown_function().
   

   For shutdown functions, an @ingroup might be useful if we go that route. If so, as with the menu callback groups, I'd suggest followup issues for writing the standard, adding the group and documentation, and actually adding the @ingroup lines. (After the cleanup sprint is done).

2. Specific callback types defined in our API somewhere; these would be explained in (e.g.) the hook_menu() API docs or the Render API documentation. We'd want to give each of these their own section as we have above for hook_menu().

   For a render API example, see drupal_pre_render_markup(). I propose changing this docblock to:

   /**
    * Pre-render callback: Appends the contents of #markup to #children.
    *
    * This needs to be a #pre_render callback, because eventually assigned
    * #theme_wrappers will expect the element's rendered content in #children.
    * Note that if also a #theme is defined for the element, then the result of
    * the theme callback will override #children.
    *
    * @param $elements
    *   A structured array using the #markup key.
    *
    * @return
    *   The passed-in elements, but #markup appended to #children.
    *
    * @see drupal_render()
    */
   

#1250500: [policy adopted; patch done] Find a (standard) way to document callback signatures does not actually cover the issues discussed above; as far as I understand it is about documenting signatures for pre-defined callback patterns, not creating doxygen standards for callback implementations generally? However, I think the same pattern from this issue should be used for Render API, etc., so perhaps those topics can be followup issues.

#21 looks good to me.

Opened #1317826: Provide general documentation standard for callbacks.

I want to take exception to #19 with respect to:

That likely won't make sense to folks that aren't on the geeky-coder end of the spectrum.

Let us keep in mind that API docs are intended for developers. As such, the intended audience is skewed towards the "geeky-coder end of the spectrum" (at least in the sense of the term in which it is used here). Anyone who does module development will have to understand the difference between a callback being registered and a callback being called pretty soon (hook_menu implementations may be an exception, as those tend to be very homogenous anyways). Which does not mean that they necessarily understand these very terms, and I would be happy with a range of alternatives ("registered"/"referenced"/"declared", even "named") and/or parenthetical explanations, but obscuring the difference would not help newbies and thoroughly confuse non-newbies. Accordingly, I think that "used" is the worst possible choice, because it not only is ambiguous with respect to this distinction, but more generally vague.

Also note that the term "being called" is used throughout the doc for hooks, and there it means "when does drupal/this module invoke this function". So "being called" is not a good choice either.

I am also +1 on #21, and I also think that hook_menu implementations are a special enough case to have a separate standards-section for them.

Excellent points, sven. I agree that being more specific is much better. I didn't intend to obscure the meaning. I do tend to write documentation with an eye towards newbies (folks just getting into programming) and so sometimes go too far towards being general and not using the more technical -- but sometimes more appropriate -- terms.
I'm glad that you caught this. I think any of your suggestions would be a great improvement.

+1 on 21
also +1 on 23 :-)

Works for me. Since this is another one of those new standards that didn't exist in D7, this would be a D8-only change. Marking "Needs change notification".

Yay!

Oh, for the change notice, how about this? (Based on @jhodgdon's notice for #711918: Documentation standard for @param and @return data types). I can try to create the node if the text is okay... just scared because I have never done it before. :)

----------------------------------

The coding standards for the documentation blocks of callbacks for hook_menu() have been updated. (These are functions named in menu item keys like page callback, access callback, etc.) The updated standard recommends:

• Identifying the type of callback at the beginning of the documented summary.
• Identifying the path(s) for which the callback is used.
• Including an @see reference to the hook_menu() implementation that registers the callback.

Example:

/**
 * Page callback: Displays a list of content.
 *
 * Path: admin/node/list
 *
 * Longer description can go here.
 *
 * @param $foo
 *   Description of a parameter for this page.
 *
 * @see node_menu()
 */

See http://drupal.org/node/1354#functions for more details on the coding/documentation standard.

Hrm, this is RTBC... why? I can't even find a patch.

Not sure how you would propose the change in #21 in patch form. It just needs to go into the handbook, and I imagine jhodgdon will mark the issue fixed when she makes the handbook change. There will be separate patches for implementing the new standard throughout core.

Sorry, but it have to re-open this. Overall, the decision here looks a bit too fast-tracked to me.

I'm not sure how to express it properly, but I'm confident that some PHP developers being smarter than me in architectural design are able to. Anyway:

 * Path: admin/node/list

I am very uncomfortable with this. This adds a reference to some application logic (actually, configuration) to otherwise unrelated and plain PHP functions. In case of page callbacks, the function is only supposed to produce some output that merely happens to be located on a certain system path by default. But it's in no way guaranteed that this is actually the case. It is information the function should not be aware of in the first place.

Furthermore, the actual benefit of documenting the path is highly questionable. It's neither linked in the API docs, nor does it provide any clues about the path's relationship to other/surrounding system paths.

Additionally, I can already predict with very high confidence that these documented paths will get stale and overlooked as the code evolves. That is, because it's going to be hard to find all path references in phpDoc that contain dynamic arguments à la admin/structure/menu/item/%menu_link/delete.

As in that example, the path may contain one or more references to menu argument loaders (%menu_link => menu_link_load()). Those can change without notice. Argument loaders actually produce the value for a @param that the function expects. In turn, this is essentially duplicating documentation within the same phpDoc block.

Even if you'd remove the argument loader names, we'd still be left with dynamic/untranslated paths à la admin/structure/menu/item/%/delete. I seriously doubt that anyone is going to find these path reference instances while working on a patch that touches anything related to the path.

Lastly, it's not even "the path." A hypothetical patch that changes any detail in the path potentially has to touch countless of possibly related phpDoc blocks that would be completely irrelevant otherwise.

In short, this is unmaintainable.

 * @see node_menu()

Closely related to the above, the relation between a any kind of menu callback and a module's hook_menu() implementation is only marginal. The function only happens to be referenced and used in a menu router item. But what does that actually mean or imply for the function?

The question is: What information do you expect developers to gain from this @see reference?

In essence, these additions are kinda "reversed." It's not the callback/function that should know of or point to the menu. It's the menu that uses these functions, so if anything, then the menu should point to the callback/function.

The situation would look entirely different, if Drupal would actively use (hard-coded) PHP Annotations for its router system - which would look and work along the lines of:

/**
 * Page callback for a node listing.
 *
 * @path node
 * @access node_access()
 * @title node_page_title()
 * @cache
 */

...but that's obviously not the case (and likely won't ever be, since our router system is based on configuration).

The other changes introduced here look fine to me, but these two should be reverted.

I can see your worries about maintaining this documentation, as the doc is usually far from the code (in hook_menu[_alter]()) that implements it---though, if this is an accepted standard that is implemented throughout core (which the doc cleanup sprint would implement / is implementing), I don't see why it should not become common practice to update the callback documentation when updating the code that calls the callback (Still, it adds an extra step, so I agree it is a concern).

I don't agree, though, with the argument that this should not be documented, because the callback function should not know about the path it is assigned to---the function does not. For example, the API module also generates a list of callers for any function---even though, clearly, the function itself should not care about where it is called from. But this information is useful for developers, which is why it is generated and included in the doc.
One good argument for including the (default) path(s) in the doc is grepability---if I wonder where (in core) a given path is rendered, I have a good chance of finding it through this annotation.

> I would think when paths change someone would grep through the code to find anywhere using paths, and those documentation lines would come up?

Indeed -- for most path changes, you should grep the code to find any admin help text or operations in admin tables that create a link to that path. So documentation should come up in that search too.

I wasn't aware of this thread; would like to say +1 to this in general :)

While I follow sun's concerns, I also would prefer to keep the standard. I'd feel differently if it were somehow in the code for the callback, but this is documentation.

To me it's no different from form callbacks adding @see to the other members of their family. A validation callback doesn't "know" about a submission handler or vice versa, but having the information quickly available in the API documentation can save a lot of head-scratching.

I feel the same way about the menu paths. The callbacks can, of course, be reused at other paths, but I think that it's entirely legitimate to document the known ones, especially within a single module. It gets a bit dicier with, say, system.module, but overall I think the standard is helpful for anyone pulling on that strand of spaghetti.

It seems that there is considerable support for keeping the new standard ... can we close this again?

Hm, somehow I only just found out about this...

In general, I agree with sun's objections right down the line. Having a standard for the description line is fine. Binding the function to a particular path is not.

For documentation purposes on d.o, building such links automatically is doable. (I do not know how difficult, but it should "just" be a parsing question.)

For in code, we are not, as sun notes, using annotations, and AFAIK there are no plans to do so. It is not the same as @see form_submit_function. In that case, there is a natural tight coupling between the form and its submit function. So tight that in most other systems they would be methods on the same object.

In this case, one of the biggest improvements in Drupal 6 was the new menu system because, in part, it ripped apart the tight coupling that had been there before. The routing information (aka hook_menu() right now) and the responder logic (aka the page callbacks right now) should not be tightly coupled, in code or in documentation.

Honestly, I've never really had a big problem figuring out what path goes to what function. Ctrl+F has served me well, without even a simple regex.

Most menu loaders, page callbacks, and other assorted functions that are put into hook_menu() implementations are designed and built specifically for the purpose of menu routing, menu access checks, and page rendering.

Yes, most page callbacks are written for the specific and explicit purpose of being page callbacks, and documenting them as such is a good thing. However, in 95% of cases or better if your page callback is built for the specific and explicit purpose of being at a specific path, you have a bug and need to refactor your code. (The other 5% are either site-specific custom hackery around Drupal limitations or a redirect from another page, which is not a restriction of that page callback but the OTHER page, which is that page's problem.)

The same is true of most access callbacks. An access callback should care what it's passed, and only what it is passed. If an access callback cares that it's on foo/bar instead of foo/baz, and doesn't handle that via its function arguments, then you need to rewrite your access callback. Full stop.

This doc standards change needs to be rolled back, because it enshrines in documentation a bad practice that we have been trying to reduce and eliminate, namely having logic that is hard-coded to a specific path.

"But in every version of Drupal up through 7 at least, we have hard-coded the relationship between paths and these functions"

I think the point Crell and sun are trying to make is no, we don't. Or at least, not in well-written page callback functions. This doesn't have anything to do with WSCCI, it's simply good module design.

Take a function like user_autocomplete(). This is a page callback for the user/autocomplete path in user_menu(). It's also a generic function that takes a string as an argument and returns a JSON-encoded list of matching usernames. Although it looks like it isn't currently (at least in core), this function could potentially be reused in many different contexts, not merely as a menu page callback. For an example of a page callback that is re-used a bunch outside of the menu system, see drupal_get_form().

In both cases, documenting these functions as being tied directly to the menu system from which they're referenced actually harms understanding of these as generic API functions, and encourages developers to make bad assumptions. Such as doing silly things like checking if (arg(0) == 'node') in the page callback, which prevents people from re-using page callbacks under a different context. (The code required in Buzzr to relocate all of admin/* under config/*, for example, was truly a lot of "fun" to write for this reason. ;))

However, I think most page callbacks are more like D5-era style page callbacks like user_page(). These definitely are not generic, and are largely tied to the menu system, so I think jhodgdon and others' points are that it helps lay breadcrumbs for people to provide context from that page as to where it's referenced. Particularly since user_page() and user_menu() are in two separate files.

I'm not sure where I land on this, personally. I see both perspectives.

Actually, I think I do know where I land on this. #1404718: Make "functions that call this" show hook_menu() callback usage of functions :)

How about making the documentation say something along the lines of "Default path" or "Unmodified path" or somesuch? Make it clear that the path listed is the likely path in a default setup but not guaranteed to be the right one in all circumstances? For ones that are very likely to be called in multiple places, explicitly saying so rather than just not having a path listed might help as well.

Michelle

Crossposted with webchick... Oooh, like that one better. :)

Michelle

> How about making the documentation say something along the lines of "Default path" or "Unmodified path" or somesuch?

That's what I'm thinking too. Something like:

Page callback for core paths:
* admin/foo/bar

If we make it a bullet list even if it has just one item, it implies that it can be used by more than just that, therefore not hindering developer understanding that these are reusable components.

...therefore not hindering developer understanding that these are reusable components.

Is this what Crell and sun are saying? It's like a caller list to me. Caller list tells you who calls this function. Does that mean only those functions can call this function? Of course not; they're functions. Similarly so for callbacks. But if that's the problem, then #47 seems a reasonable solution.

+1 to #47. Saying "Path: foo" really implies that this is the path it is used on, period. #47 makes it clearer, I think.

Michelle

webchick is correct in #42. The problem is that documentation that reinforces bad development practices, even if they're common development practices, is bad. Good documentation should reinforce good development practices. It's true that if WSCCI accomplishes everything it wants to then this question becomes moot, as the architecture here would be changing radically, but we should still be in a good habit of positive-reinforcement documentation.

Providing additional documentation context is a legitmate goal, however. @see is generally a reasonable thing (as it doesn't imply anything beyond "it might be helpful to go look over there"). I am not fond of @see menu_hook_where_it_is_probaby_mentioned_but_we_cant_guarantee_it, but I could live with it. It's the path definition that I am most against. Standardizing the first-line documentation I am all in favor of.

So stepping back, what sort of "context" around a page callback would be useful to know, given that:
1) The path it's at should be irrelevant, and if it is there's a bug.
2) The access callback that affects that path is irrelevant, and if it isn't there's a bug.
3) Page callbacks are not defined by hook_menu; they are referenced from hook_menu.

Certainly most page callbacks are not useful except as page callbacks, and that's OK, but at the same time they should be useful as page callbacks anywhere. Play with Context Admin for a while and you'll see the sort of custom UIs you can build when you don't assume a 1:1 relationship between page callback and path. :-)

I really don't think the current standard nor the one in #47 in any way assumes or implies a 1:1 relationship between callback and path.

I think everyone (?) agrees that it is bad practice to have the callback implementation itself be aware on what path it is---given that they can be reused for other paths, moved to other paths, used as non-menu-callbacks (though: user_autocomplete() is a horrible name for a function that is supposed to be used generically).

So I suppose what the disagreement is about is this: Is including the path in the function doc encouraging the bad practice of making a page callback path-specific (or just making people THINK of callbacks as path-specific)?

As has been mentioned a couple of times now, giving the path is not much different from the list of calling functions---with one difference: This list is generated automatically, not hard-coded into the doc header. So I won't see it if I look at the header in the source. On the assumption that the doc header tells you things "about the function" it documents, one might argue that including the path(s) there gives a wrong impression.

But if the main worry is that including this information could give the impression that callbacks are / should be / can be path-specific, I think this could be mitigated by simply changing the format, perhaps along the lines of #47.

While we have everyone's attention, I want to draw attention to another recent standard change / introduction:
http://drupal.org/node/1354#render
(can't find the issue where we decided on this right now). Basically, the suggestion is to document render API callbacks like so:

/**
 * Render API callback: Validates the maximum upload size field.
 *
 * Ensures that a size has been entered and that it can be parsed by
 * parse_size().
 *
 * This function is assigned as an #element_validate callback in
 * file_field_instance_settings_form().
 */

I suppose some of the arguments against including the path(s) for menu callbacks could apply in case of this (very useful, IMO) last sentence (again, it would be sweet if file_field_instance_settings_form() could be automatically included in the caller list, but this is not going to happen soon, I suppose).

Though one difference between those and page callbacks is that render api callbacks specifically operate on a render array, i.e. are more tied to this particular usage than page callbacks are in principle (page callback = function that takes certain parameters and happens to return something renderable, while render api callback = function that takes a render array and operates on it).

#53 would be an improvement, but I still don't think it should be there at all. I can't remember the last time it took me more than 30 seconds with Ctrl+F (or possibly "find in project") to track down a page callback from a menu or a menu from a page callback.

Plus, modules don't provide "core paths". They provide page callbacks at a URL from a module; core doesn't enter into it.

I don't feel strongly enough to block this, but I agree. This is classic info that should be discoverable on api.d.o, not in our source code as a maintenance burden.

I'm fine with the standardization of the short-desc line. What about the @see hook_menu()? Shall we remove that as well?

Can we at least keep the @see?

Whoops, good catch! That's the only instance I see though; is there another?

Regarding the standard itself, I really don't feel too strongly either way. I think removing the paths listing but keeping the @see is a good compromise, I guess.

Reviewed #59 again and I still find only the issue mentioned in #61. So here's a reroll with that fixed.

I also checked all the pending RTBC docs patches in the queue:

• #1379126: Clean up API docs for trigger.module has some (but we'll likely want to just drop the callback changes anyway and create a D7-only version because of #764558: Remove Trigger module from core).
• #1332658: Clean up API docs for dashboard module has some.
• #1317620: Clean up API docs for includes directory, files starting with D-G has one.

I posted fixes for the latter two.

Did I just upload the same patch before, or what? Weird. I must be losing my mind...

I'd say bring back @return. Even if all page callbacks have the same @return array / "A renderable array or string", so what? It is saying what you get back, not what to do with it. (Also, not all page callbacks return a renderable array. That depends on the delivery callback they're expected to be used with, which is an entirely different design flaw in Drupal that we won't get into here.)

Gack. I think adding @return is out of scope perhaps for this patch? Removing a standard we don't like is one thing; but adding @return is a more "all of core cleanup" thing. We stripped a few of them, but far more never had them to begin with. Maybe a followup or several? I'm actually still not entirely sold on it, myself.

As I am re-rolling my clean-up patches to reflect the standard change, a clarification question: Given that @return is obligatory for the callbacks, will it be enough to do (in the "default" case of a renderable array for a page callback):

/**
 * @return
 *   A renderable array.
 */

Or do we want to require something more prolix like

/**
 * @return
 *   A renderable array representing foobar.
 */

(The latter will be quite redundant in may cases, as the function summary will already read:

/**
 * Page callback: Displays foobar.

)

I think

/**
* @return array
*   A renderable array.
*/

Will be sufficent in most cases. (Note the type specification.) If there's an exception, it should be self-evident that it needs more info case by case.

So, NR then? And we can add the return values in the cleanup issues going forward.

Not sure how or why the change notification block keeps disappearing, but the change notification is here:
http://drupal.org/node/1319812

It simply references 1354, so it's already correct.

I'm already using the standard as we've updated it in patch reviews. Anyone else want to chime in? If not, let's get a final review on the patch above and mark this RTBC. :)

I'm all for adding in the @return statements if just to have my IDE complain less. I'm finding a bunch of missing @return in node.pages.inc. Should I create a separate issue for cleaning that up?

Yes, please file separate issues for those cleanups so we can review them as they're ready. Thanks!

Cool, just leaving an issue about spinning off the "RETURN of the @return" issue: #1454322: Documentation: Proliferate the use of @return in PHPdoc of functions

As both a coding standards /and/ documentation patch, I think this is safely Jennifer's. :)