How to use hook_menu_alter() to manipulate path access control

Question

/**
 * Implementation of hook_menu_alter().
 */
function joke_menu_alter(&$callbacks) {
  // If the user does not have 'administer nodes' permission,
  // disable the joke menu item by setting its access callback to FALSE.
  if (!user_access('administer nodes')) {
    $callbacks['node/add/joke']['access callback'] = FALSE;
    // Must unset access arguments or Drupal will use user_access()
    // as a default access callback.
    unset($callbacks['node/add/joke']['access arguments']);
  }
}

The above function is from the pro development drupal. I can't understand it well. Why must I unset the access arguments (unset($callbacks['node/add/joke']['access arguments']);)?

Thank you.

Solution 1

That entire example seems broken and bad. In short, a joke. First, let me answer your question, then I'll go on to explain why you shouldn't follow that example in practice.

From includes/menu.inc:

if (!isset($item['access callback']) && isset($item['access arguments'])) {
  // Default callback.
  $item['access callback'] = 'user_access';
}

Unsetting the access callbacks when you no longer need them (relying on a boolean now, after all) prevents the over-clever logic in Drupal's routing system from slapping in user_access() just so it has something to do.

Now, on to why that's bad code.

hook_menu() and hook_menu_alter() are both run on cache clear (more specifically when the menu routing system is rebuilt). This means that the permissions of whichever user hits the site to rebuild the menus will be hard-coded into menu routing behaviors. This is a very bad and inconsistent arrangement.

If you want to block access to a path based on a permission, you need to change the callback to something that will test for that permission. Then when the menu is rebuilt, it will check the new callback function per page load to see if the current user should be granted permission.

A simple example of this might look like:

/**
 * Implementation of hook_menu_alter().
 */
function joke_menu_alter(&$items) {
  $items['node/add/joke']['access callback'] = 'user_access';
  $items['node/add/joke']['access arguments'] = array('administer nodes');
}

Now we have a function which takes the node/add/joke path and declares that the only thing that matters is whether or not the user has administer nodes permission. Of course, that's a little more limited than the apparent intentions of the example, which were to preserve the existing access controls, but also require the user to have administer nodes permission.

That is also fixable, but is more complicated. To borrow some concepts from the Spaces project:

/**
 * Implementation of hook_menu_alter().
 */
function joke_menu_alter(&$items) {
  $path = 'node/add/joke';
  $items[$path]['access arguments'][] = $items[$path]['access callback'];
  $items[$path]['access callback'] = 'joke_menu_access';
}

function joke_menu_access() {
  $args = func_get_args();
  $access_callback = array_pop($args);
  $original_access = call_user_func_array($access_callback, $args);
  return $original_access && user_access('administer nodes');
}

We have successfully wrapped the original access callback in a new access callback, to which we can add whatever additional logic we need.

Note that in the last two function examples, I used the $path variable to keep the code simple. I also separated $original_access to it's own line and had it checked first, in practice I would check user_access() first as it would almost certainly be more performant than whatever happens in the original access callback.

Solution 2

The comment directly above that line explains it?

access callback is the function that is called (or TRUE/FALSE) and arguments is what is passed to that function. You are setting the callback to false and therefore always deny access to that router item.

And now, as the comment is saying, you also needto unset the arguments or Drupal will still use user_access() (The default access callback).

Comments

[Rick]

I just wanted to add a thank you even after up-voting for not only telling the OP how but why! Oh I wish there were a plus 10 when answers go this deep.