Big task cleanup: Remove type and tasks from hook_menu()

Getting rid of the default menu item for local tasks would be a step forward in developer UX.

Maybe - I have doubts about this approach, however.

This would help us with the nasty regression that we are working around in #362031: Ideas for dynamic tabs for OG Panels (can't dynamically add/remove/retitle tasks).

Possibly we shoudl do like D6 hook_help() and pass both the router path and the translated path into this hook to allow for easy conditionals.

Also, the returned data should be a full set of params to l()
e.g. so I can set query params, attributes, etc if needed. So, we need text, path, and options elements.

Tabs as menu links makes a load of sense.

@catch, no I don't think they make much sense as links - a lot of practical roadblocks.

subscribe

I'm keen to do some of this grunt work, but I feel like I'm pissing in the wind at the moment. If someone can whip up a before and after example, I'm sure I can nut the rest out. Otherwise, I don't understand what we're trying to accomplish here.

This is all I've managed to do for the users module, which is evidently wrong.

[removed lots of pasted code. it was wrong anyways.]

E.g. I've moved all of the MENU_DEFAULT_LOCAL_TASKs into the local_task hook, and changed the other MENU_LOCAL_TASKs to hidden. It seems no matter what I do, I'm getting all sorts of notices and errors (Notice: Undefined index: original_path in menu_local_tasks() (line 1384), etc).

I've had to do the "$user = new stdClass;" thing because menu_get_object('user', 1) or menu_get_object('user_category') or even just menu_get_object() return nothing.

Sorry to take up so much of your time chx!

I'm all over it, I understand what's going on now. I've attached unified diff's of the work so far (including fixing up some bugs in menu.inc).

This is what they are looking like, any other changes before I get too far into it?

Having to use strpos for every hook looks like a bad API to me.

Perhaps we can pass in an array of sub-paths?

essentially:

$path[0] == arg(0)
$path[1] == arg(0) .'/'. arg(1)
...

Similar (as suggested before) to D6+ hook_help - we need both the router path and the translated path, most likely.

Then we could do more simply something like:

if ($path[2] == 'admin/content/aggregator')
...

or, e.g. or a specific node:

if ($translated_path[1] == 'node/99')
...

Cleaned up based on comments and fixed another bug in menu.inc

Modules remaining:
modules/book/book.module
modules/comment/comment.module
modules/contact/contact.module
modules/forum/forum.module
modules/locale/locale.module
modules/menu/menu.module
modules/path/path.module
modules/system/system.module [50% done]
modules/taxonomy/taxonomy.module
modules/tracker/tracker.module

- Ensure tabs are not shown if there's only 1 tab for that level
- Convert system.module

Holy. Just had a quick peek at one of the patches (system.module) - do we really want to duplicate all menu router data? And did we talk back to merlinofchaos (who heavily struggled with local tasks in Panels/Delegator/Views) and Moshe (who mentioned difficulties in #6) whether this approach solves their needs?

I wasn't involved in any of those D6-local-task issues, but I'd imagine that those issues could also be solved by introducing this hook, but only passing what the menu system thinks the local tasks should be - leaving complete freedom of (dynamic or not) customization to modules, if required... ?

@chx:

count($item['link_map']) == 1 && $item['link_map'][0] === 'admin'

I don't really like this - see above suggesiton re: menu substrings vs alwys needing to test against each arg.

I've made the suggested alterations,

Modules remaining:
modules/locale/locale.module
modules/menu/menu.module
modules/path/path.module
modules/taxonomy/taxonomy.module
modules/tracker/tracker.module

I'm all done! I couldn't wait, sorry chx. I figure the API won't change too much so any changes we make will be minor.

I've also changed the key sorting, asort() won't do what we need. ksort() however seems to work as expected. See http://drupalbin.com/10112 for an example.

Ok, I'm largely in agreement with Peter. I really dislike the count() stuff. However, what about this:

This:

  // admin/content/aggregator
  if (count($item['link_map']) >= 3 && $item['link_map'][0] === 'admin' && $item['link_map'][1] === 'content' && $item['link_map'][2] === 'aggregator') {
    // ...
  }

Becomes:

  // admin/content/aggregator
  if (array_slice($item['link_map'], 0, 3) == array('admin', 'content', 'aggregator')) {
    // ...
  }

I'm not sure 1) how fast array_slice() is, or 2) if it will notice if the array doesn't have enough elements.

Second, would the use of a callback attached to the menu and an alter function be more efficient? With this setup, on every page you are still testing every possible path that can contain tabs. However, if we included a local task callback and a corresponding alter we could gain a lot of efficiency:

  // totally back of the napkin code:
  if (function_exists($item['local tasks callback'])) {
    $local_tasks = $item['local tasks callback']($item);
    drupal_alter($item['local tasks callback'], $local_tasks, $item);
  }

My immediate concern is that this has the disadvantage of making it difficult to add local tasks to an arbitrary path, because you have to somehow identify the callback. That could use some suggestions.

I think what this leads to is that maybe what I'd really like to see is a formalization of the creation of a tabset.

For example, all tabs that you might see at node/% would be a tabset. We would somehow need to name that. Which is part of how the callback gets named, but naming stuff after functions is ugly.

In IRC chx suggested a callbacks array, but:

node/% and node/%/edit and node/%/foo would all need to reference this tabset.

I still don't understand why squeezing a simple drupal_alter() to alter tabs/local tasks somewhere into the right location wouldn't solve the entire issue. You get the defaults, i.e. what the menu system _thinks_ should be output, but modules are able to alter that based on the current page or context...?

1. Allowing for a separate menu item and page title is a separate issue. All items below admin/settings could use that, since the menu should display "Region" only, but the page title should be "Regional settings".
2. Can be customized via drupal_alter(), no?
3. Simply use the first accessible (based on the user's permissions) after executing that _alter hook - based on weight?
4. Not sure what that means. But most probably also do-able via drupal_alter().

I am also fairly confident that these "Just a few ifs" are bad DX. They are worse than a giant switch. They are something that should be reserved only for the really dynamic cases.

I spent a little time thinking about this and I came up with this, because doing subtabs is a little bit tricky, but this allows it.

In hook_menu, the path 'node/%node' has 'tabset' => 'node/%node' as does 'node/%node/edit'.

  function node_menu_tabset() {
    $tabsets = array();
    $tabsets['node/%node'] = array(
      'node/%node' => array(
        'title' => 'View',
        'weight' => -10
      ),
      'node/%node/edit' => array(
        'title' => 'Edit',
        'weight' => -9
      ),
      'node/%node/revisions' => array(
        'title' => 'Edit',
        'weight' => -9
      ),
    );
    return $tabsets;
  }

Now, along comes project.module which wants to add some subtabs under edit. No problem:

  function project_menu_tabset_alter($tabsets) {
    $tabsets['node/%node']['node/%node/edit']['subtabs'] = 'node/%/edit';
    $tabsets['node/%node/edit']['node/%node/edit/issues'] = array(
      'title' => 'Issues',
    );
    $tabsets['node/%node/edit']['node/%node/edit/releases'] = array(
      'title' => 'Releases',
    );
  }

Dynamic tabs should work just fine with a third hook, which allows us to do the realtime stuff:

  function views_menu_tabset_realtime(&$tabset, $name, $item) {
    // Views looks at $item to determine that this page is a view:
    // ...
    // And if it *is* a view, it does this.
    $tabset["admin/build/views/edit/$viewname"] = array(
      'title' => 'Edit view',
      'weight' => 0,
    ),
    $tabset["admin/build/views/export/$viewname"] = array(
      'title' => 'Export view',
      'weight' => 0,
    ),
  }

Now, if we go with chx's function, Views is going to have to load every view on every page load to determine if it should add tabs. This is NOT 'just a few ifs' in that case. This could be loading 100 views, sifting through which ones have tabs and then adding them all. Every. Page. Load. Because there is simply now way to tell how many views have set up tabs on any given path without loading every view that has tabs and checking.

og_panels gets its problem solved easily:

  function og_panels_tabsets_realtime(&$tabset, $name, $item) {
    if ($name == 'node/%') {
      $node = menu_get_object('node');
      if (og_is_group()) {
       og_panels_add_tabs($node, $tabset);
      }
    }
  }

Now, og is at least not so much in a position to have to load every node to determine what its tabs are, but at the same time, it does need to be able to put them in absolutely dynamicly to make sure they work. And it won't use node/%node/foo, it will use node/15/foo for any given tab. That's an interesting problem all its own for other reasons.

If 90% of modules can populate the tabs at runtime and this give the best DX for the most people, then I think it might make sense for modules needing to define a ton of tabs figure out some caching or storage for them.

While decoupling from the router item totally would be ideal, I'm wondering if there will be problems in terms of deciding who "owns" the default tab.

A few problems. The tabset indexes as we discussed on IRC are not router paths, they are just convinience. This will lead to confusion.

If you name the tabset anything other than node/% then how will people figure out this magic name? There will be absolutely no intuitive way to understand that the tabset that goes on node/% will be named whatever you call it. What's wrong with naming it after the path? Using a standard like that, the naming is intuitive and obvious. *Not* naming it after the path will lead to confusion.

If 90% of modules can populate the tabs at runtime and this give the best DX for the most people, then I think it might make sense for modules needing to define a ton of tabs figure out some caching or storage for them.

Please define 'best DX'. What I've outlined looks like pretty good DX for me. The single hook that has a massive ugly series of ifs is not good DX.

I am suggesting that we cache basically normal tabs that aren't very dynamic just like we do menu items. Stick 'em in a the databse, possibly their own table, possibly share the menu items table. That much is fuzzy to me as I don't fully understand the table, and what table it's stored in is kind of an implementation detail that doesn't strike me as that important. As long as it exists. Now, if people are saying that storing this stuff is hard, I haven't actually seen people *say* that. Maybe there's this unspoken agreement that it is, but I don't see why it's hard to store this stuff the same way we do menu links.

That was what the 'project' example was for. Project has several subtabs under 'edit'. So there's a specific 'subtab' which points to another tabset. It assumes that particular tab is the default tab for that tabset, and possibly needs some way to alter that assumption if there are actually use cases to do so (I can't think of any offhand).

So when visiting node/%node/edit it sees that hey, there's a subtab set here, and renders that as the level 1 tabs. (In theory we could convince the system to support tabs beyond 2 levels which I've occasionally wished for but am not sure are a good idea in reality).

Hmm. But node/%/edit is a tab on node/% and has its own subtabs. Does this really work? I'm a little confused. It also means that you need a special syntax that differs a little based upon the default tab vs non default. What about:

What about:

  $tabset['node/%'] = array(
    'node/%' => array(
      'title' => 'View',
      'weight' => -10,
      'subtabs' => array(
        'node/%' => array(
          'title' => 'Foo',
        ),
        'node/%/view/bar' => array(
          'title' => 'Bar',
        ),
      ),
    ),
    'node/%/edit' => array(
      'title' => 'Edit',
      'weight' => -9,
      'subtabs' => array(
        'node/%/edit' => array(
          'title' => 'General',
        ),
        'node/%/edit/Issues' => array(
          'title' => 'Issues',
        ),
        'node/%/edit/releases' => array(
          'title' => 'Releases',
        ),
      ),
    ),
  );

Ok, I just read my example, and it's hideously mixed syntax. I apologize. Please, er, pretend that I was consistent. I think the basic premise still comes through.

Can we please remove referring to everything "tab"? Tabs are just one of the many user interface element variations menu items can be rendered to. In the same reason we have MENU_LOCAL_TASK, not MENU_(SUB)TAB.

Is there a current patch to review on here? (Also, subscribe - very much excited about advancements to the menu system... :)

I was pointed to here from #542658: Move action "tabs" out of local tasks, where I put out a hack to separate some of the tabs to "action buttons". The d7ux wind of thinking is that tabs should be used for "different views of the same thing", while things like "add" and similar actions would be shown in an "action area" on the page. It might be good to keep that in mind, so we can keep those in the menu system but expose them under a specific UI. @sun reminded us that we should keep this somehow under the menu system umbrella, so alternate navigation modules like admin_menu can display these just like before.

Tagging related issues.

Discussing w/ chx in NYC.

We are are settling on a taskset (tabset) concept - dynamic behavior comes from adding different tasksets for each path. We key them by router path and menu.inc filters out by the current path.

function hook_local_task($router_item) {
  // The value is the level at which the tasks are displayed.
  $tasks = array (
    'pony/%' => array(
       'pony' => 0,
       'pony_actions' => 'action',
       'pony_below' => 1,
       'pony_also' => 1,
    ),
    'pony/%/edit' => array(
       'pony' => 0,
       'pony_edit_below' => 1,
    ),
  );
  if ($router_item['map'][0] == 'pony' &&
      is_object($router_item['map'][1]) &&
      $router_item['map'][1]->pony_id == 99) {
    // On only pony 99 display an additional special tabset on the top level.
    $tasks['pony/%']['pony_express'] = 0;
    $tasks['pony/%/edit']['pony_express'] = 0;
  }
  return $tasks;
}

This gives use the name of a taskset, so we then invoke a corresponding hook, which returns a FIXED set of tabs. Example for node module:

/**
 * Implements hook_local_task().
 */
function node_local_task($router_item) {
  // The value is the level at which the tasks are displayed.
  $tasks = array (
    'node/%' => array(
       'node' => 0,
    ),
    'node/%/edit' => array(
       'node' => 0,
    ),
    'admin/structure/types' => array(
      'node_admin_types' => 0,
      'node_admin_types_action' => 'action',
    ),
    'admin/structure/types/manage/%' => array(
      'node_admin_type_manage' => 0,
    ),
  );
  return $tasks;
}

/**
 * Implements hook_taskset_TASKNAME().
 */
function node_taskset_node() {
  return array(
    'node/%' => array(
      'text' => t('View'),
      'weight' => -10,
     ),
     'node/%/edit' => array(
       'text' => t('Edit'),
       'weight' => 0,
     ),
     'node/%node/revisions' => array(
       'text' => t('Revisions'),
       'weight' => 2,
     ),
  );
}

/**
 * Implements hook_taskset_TASKNAME().
 */
function node_taskset_node_admin_types_action() {
  return array(
    'node_admin_types' => array(
      'text' => t('List'),
      'weight' => -10,
      'options' => array(),
    ),
  ),
}

/**
 * Implements hook_taskset_TASKNAME().
 */
function node_taskset_node_admin_types_action() {
  return array(
    'admin/structure/types/add' => array(
      'text' => t('Add content type'),
      'weight' => -10,
    ),
  );
}

/**
 * Implements hook_taskset_TASKNAME().
 */
function node_taskset_node_admin_type_manage() {
  return array(
    'admin/structure/types/manage/%' => array(
      'text' => t('Edit'),
      'weight' => -10,
    ),
  );
}

Sorry, but the NYC proposal in #62 is way too abstract (calling a hook to define multiple hooks to call, plus alter hooks).

IMHO, the most sound proposal so far was @merlinofchaos' in #44. I've tried to parse and understand the counter-arguments against it, but they are written in a very cryptic way. What I remotely understood are the questions:

1. Alright, make tabsets indexed by the normalized menu link path (%), not a router path (%node).
2. There's no need to have node/%/edit/foo specify that node/% is a remote parent. It's a tab on node/%/edit, and that's all we need to know.

Also:

1. To avoid to generate needless tab info for irrelevant paths, make the router item define a tab callback.
2. Invoke a hook to add tabs by other modules.
3. Naturally, invoke a single final alter hook to allow manipulations of the collected results.

function node_menu() {
  $items['node/%node'] = array(
    'page callback' => 'node_view',
    'tab callback' => 'node_menu_tabs_node',
    'tab arguments' => array(1),
  );
  // Ladies, router paths are still required.
  $items['node/%node/edit'] = array(
    'page callback' => 'drupal_get_form',
    // tab callback and arguments is derived to children.
  );
}

function node_menu_tabs_node($node) {
  $tabs['node/%'] = array(
    'node/%/view' => array(
      'title' => 'View',
      'weight' => -10,
    ),
    'node/%/edit' => array(
      'title' => 'Edit',
    ),
    'node/%/revisions' => array(
      'title' => 'Revisions',
    ),
  );
  $tabs['node/%/revisions'] = array(
    'node/%/revisions' => array(
      'title' => 'List',
      'weight' => -10,
    ),
    // This tab obviously cannot be generated on node/%/revisions,
    // it serves as a hierarchical pointer to tabs for a single revision.
    'node/%/revisions/%' => array(
      'title' => 'View revision',
    ),
  );
  $tabs['node/%/revisions/%'] = array(
    'node/%/revisions/%' => array(
      'title' => 'View',
      'weight' => -10,
    ),
    'node/%/revisions/%/revert' => array(
      'title' => 'Revert',
    ),
    'node/%/revisions/%/delete' => array(
      'title' => 'Delete',
    ),
  );
  return $tabs;
}

...but in general, I think this whole idea is doomed. Instead of detaching, we should be attaching. To path arguments. Java's Jersey is something worth to look at.

In a nutshell, we forget about the path, and only care for the argument:

  $tabs['%node'] = array(
    'view' => array(
      'title' => 'View',
    ),
    'edit' => array(
      'title' => 'Edit',
    ),
    'revisions' => array(
      'title' => 'Revisions',
    ),
  );
  $tabs['%node_revision'] = array(
    'view' => array(
      'title' => 'View',
    ),
    'revert' => array(
      'title' => 'Revert',
    ),
    'delete' => array(
      'title' => 'Delete',
    ),
  );

Do we still need this? While we are shifting to routes.

This case sounds very outdated. I guess this has been solved already in core.