Allow multiple access callbacks per menu item by changing hook_menu() API

From IRC:

chx: Amitaibu: you should not call drupal_alter for every access call
chx: Amitaibu: most of the work should be done at build time. maybe use an "access callbacks" key with a value of array('function1' => array('arg1', 'arg2')) and serialize it into the access arguments section and sore something special into the access callback like a - or -1
chx: Amitaibu: so menu rebuild could detect that 'access callback' is an array and act accordingl.y

Reworked code. Modules can hook_menu_alter() to add more access callbacks.

Follow the steps given in the issue description for the attached test module.

from IRC:

chx: Amitaibu: fairly good but i still have some gripes over it
chx: if (is_numeric($callback['callback'])) {
chx: + if (!$return = (bool)$callback['callback']) {
chx: + break;
chx: + }
chx: this part hits me a overtly complex and hard to understand
chx: Amitaibu: also, I think we would better off replacing foreach (unserialize($item['access_arguments']) as $callback) with foreach (unserialize($item['access_arguments']) as $callback => $arguments) , what do you think?

Reworked code to be less ugly...
Also added a simpleTest.

Code still needs work, I have got something wrong with the page callback/arguments.

1. With this patch maybe we can remove from {menu_router} 'access callback' and later 'page callback'?
2. Wouldn't it look nicer if $item will be loaded like this (which might make the scope of this patch larger, so maybe this can be in a follow up patch).

$item['foo/%bar] = array(
  'callbacks' => array(
    'access' => array(
      array(
        'callback' => 'some_foo_callback',
        'arguments' => array(),
      ),
      array(
        'callback' => 'some_bar_callback',
        'arguments' => array(),
      ),
    ),
    'page' => array(
      array(
        'callback' => 'some_baz_callback',
        'arguments' => array(),
      ),
    ),
);

This patch is already working.

After discussion with chx cleaned the code and placed also block and title callbacks in the new structure.
Code already seems to be working, but some optimization might be needed(?)

Patch still doesn't deal with removing the access_argument from the {menu_router}

Following chx remarks from IRC:
* Fixed ['0'] to be [0].
* Fixed indention.
* Fixed arguments being checked for parent without callback missing (access and page).
* $group_callbacks is now $item['group_callbacks'].
* page, title and block are now under $item['group_callbacks'][TYPE].

Indeed, this allowed reworking the code and removing a lot of redundant work.

@pwolanin,
Patch is trying to accomplish two things:
1. Accept a change in the hook_menu() structure which will be done once this patch is ready - as in comment #5.
2. Allow only the access callback to have multiple callbacks.
So to your question, the title still can have only a single callback.

1. Since we have $title => foo lets keep callbacks.
2. Use Crell's idea about having title under page.

How about this:

$items['my/path'] = array(
  'title' => 'foo',
  'callbacks' => array(
    'page' => array(
      'callback' => 'myfunc',
      'arguments' => array(),
      'title' => array('callback' => 'bar', 'arguments' => array('baz')),
    ),
    'access' => array(
      array('callback' => 'user_access', 'arguments' => array('do stuff')),
      array('arguments' => array('do more'),  // callback still defaults to 'user_access'
    ),
  ),
);

Patch according to this structure:

$items['my/path'] = array(
  'title' => 'foo',
  'page' => array(
    'callback' => 'myfunc',
    'arguments' => array(),
    'title' => array('callback' => 'bar', 'arguments' => array('baz')),
  ),
  'access' => array(
    array('callback' => 'user_access', 'arguments' => array('do stuff')),
    array('arguments' => array('do more'),  // callback still defaults to 'user_access'
  ),
);

Discussed with Crell on IRC. He has some more plans about this patch, that he will explain in a follow up issue. Until then, here's a re-roll of the patch according to this structure:

$items['my/path'] = array(
  'page' => array(
    'callback' => 'myfunc',
    'arguments' => array(),
    'title' => array(  'title' => 'foo', 'callback' => 'bar', 'arguments' => array('baz')),
  ),
...

We aren't dealing with multiple pages and titles, but we lay down the ground for it.

In this patch all title keys are under page, however:
+ // Page title can be in $item['title'] or in $item['page']['title']['title']
+ // We are giving the later a priority in case we have duplicate entries.

chx, are you ok with it, or do you think that title + title argument + title arguments should be able to exist also outside of page?

Discussed with Crell and chx in IRC and determined to remove title from page and follow #23.

chx: Amitaibu: I think this patch should be about "unified/simplified menu API syntax" which happens to include a) mutiply access callback handling b) simplified title callback handling.
...
chx: Crell: this patch is more about a unified callback array. array('acccess' => [callbackarray], 'title' => [callbackarray], 'page' => [callbackarray]) and so on.
...
Crell: chx: I originally asked it, not dictated it. :-) Once we support multiple page callbacks we really should move it down, but for now I'm OK with saving kittens.

will re-roll soon.

With this patch we can also get rid of the 'title' column in the {menu_router} as it's now stored in 'table arguments'.

I have noticed that in _menu_item_localize() we have this line
$item['title'] = t($item['title'], menu_unserialize($item['title_arguments'], $map));

which is never run and doesn't make sense (i.e. define title and title arguments), thus removed.

Re-rolled patch after DBTNG.

Since this issue will require a big find & replace in all module core's hook_menu(), maybe we can continue with separate kitten-friendly patches:

1. The current patch which lays the ground of the API change + multiple access callbacks.
2. find & replace hook_menu().
3. Add simpleTest for multiple access callback.
4. Remove from {menu_router} 'access callback' and 'title'.

Ok, changing status to CNR. re-rolled on a minor issue where == FALSE was used instead of !

hmm, now changing the status for real. Re-uploaded patch so it is tested.

Thank you test bot - indeed _menu_item_localize() wasn't handled properly.

pwolanin said:

In any case, if we want to switch to this nested system for just 'access' or for all, then the patch needs to change the schema, all hook_menu implementations in core, etc, to reflect it.

ok, so let's try it this way:
* Patch includes the API change and multiple access callback
* Changed title to reflect that.
* Patch changes schema by removing 'title', 'access callback'.

Two questions:
1. I've removed from menu_local_tasks() ->orderBy('title'). If we do want to sort by title we'll have to do it in several steps as the title is seralized in title_arguments.
2. I wanted to change the access_arguments in {menu_router} to access, but then it collides with $item['access'] in _menu_check_access(), So I kept the name as is, just altered the column description.

Last patch had some simpletest exceptions, which seem to be fixed in this one, setting to CNR, so the testbot will pick it up.

@chx, Maybe it's a good time to create a patch for core's hook_menu(), can you help me with that?

Now with a simpletest to test multiple access callbacks.

Indeed, I'll re-work it, so we'll end up eliminating 'title_callback' from {menu_router} as well.

We can come to using only the X_arguments, i.e.

  'access_arguments' => serialize($item['access']),
  'page_arguments' => serialize($item['page']),
  'title_arguments' => serialize($item['title']),
  ...

which will remove title, title_callback and page_callback. The reason would be consistency in the code, but if it's not needed i'll be happy to let it go and go back to the patch in #40 ;)

re-roll to keep in synch with HEAD