hook_menu 'access callback' docs should mention boolean values work too and minor doxygen correction

Are you referring to 6.x for these issues? The code in includes/menu.inc seems to match what's documented for hook_menu for 6.x; as to the undocumented feature, that's a good catch, but the code in 6.x (v6.8, line 2341) is:

    if (is_bool($item['access callback'])) {
      $item['access callback'] = intval($item['access callback']);
    }

which is sort of the opposite of what you've pasted :)

api?
moved to Drupal project, Docs component

We'll need to fix this in the 7.x menu.api.php first, then it can be easy to backport to the 6.x docs.

Thanks for moving this into the correct component. It's not necessary to add a documentation tag if an issue is in the documentation component however. And it's a documentation bug, not a task.

OK, let's see here...

Looking at http://api.drupal.org/api/function/hook_menu/7 it says:

"access callback": A function returning a boolean value that determines whether the user has access rights to this menu item. Defaults to user_access() unless a value is inherited from a parent menu item.

Looking at http://api.drupal.org/api/function/_menu_check_access/7 we have:

 $callback = empty($item['access_callback']) ? 0 : trim($item['access_callback']);
  // Check for a TRUE or FALSE value.
  if (is_numeric($callback)) {
    $item['access'] = (bool) $callback;
  }

(and otherwise it calls the function.

So I guess the idea is to document that it can be not only "a function returning a boolean value", but also a number that can be cast to a boolean value. Sounds like a good idea, and it would be a good project for a novice Drupal contributor too.

Patch to add wording that jhodgdon described above.

Instead of mentioning that numbers that can be cast to boolean work, shouldn't we mention that booleans themselves work? I think that is much clearer.

The current patch says:

"A function returning either a boolean value or a number that can be cast to a boolean value" ...

What we need to get across here is that it can be a number, a boolean value, or a function returning either one.

Here's what I came up with... not sure if it's just more verbosity for lack of clarity though.

"access callback": A boolean value or a number that can be cast to a
boolean value, that determines whether the user has access rights to this
menu item. Note that this value can be either set explicitly or a return
value from a function. Defaults to user_access() unless a value is
inherited from a parent menu item.

I've also made a small change on the theme callback description in that function just to meet Doxygen standards - "(optional)" instead of "Optional."

Patch attached.

I think it should be documented the other way around, because using a callback is actually the "usual" use-case.

Good point, and I think it reads better in that order as well.

"access callback": A boolean value or a number that can be cast to a
boolean value, that determines whether the user has access rights to this
menu item. Note that this value can be either set by the return value of
a function or set explicitly. Defaults to user_access() unless a value is
inherited from a parent menu item.

Quick reroll - still had it up on my screen.

I think the idea is to leave it saying "a function..." and add something at the end saying it can also be a number or boolean?

I talked with jhodgdon in IRC through this issue; I think this version sounds much better. Credit to jhodgdon, I'm just rolling the patch :)

"access callback": A function returning TRUE if the user has access 
rights to this menu item, and FALSE if not. It can also be a boolean 
constant instead of a function, and you can also use numeric values 
(will be cast to boolean). Defaults to user_access() unless a value is 
inherited from a parent menu item.

I approve of this patch (obviously). Will let tstoeckler mark as RTBC...

Yes, that reads much better. One final question that just popped up in my mind:
Should we leave it at saying: "Defaults to user_access()" because user_access() is the function that ends up being called, or is "Defaults to 'user_access'" clearer, because that is what you have to put in the menu declaration? I wasn't sure, so leaving at needs review.

The nice thing about saying 'Defaults to user_access()' is that with the parens, it makes a link to the user_access() function on api.drupal.org. So I think we should leave that in...

That said, maybe we should change the text to say "the name of a function" and "Defaults to using user_access()" to make that abundently clear?

Actually I find "Defaults to using" less clear. And the api.drupal.org thing is a pretty good reason.
RTBC from my point of view.

Committed to CVS HEAD. Thanks again.