The menu system can do a lot of amazing stuff, but it has some intractable limitations with regard to how local tasks (tabs) are handled. Specifically, it has difficulty managing one moderately common D5 use case in a fashion that is remotely performant; for some cases, replicating those reasonable use cases even becomes infeasible.

There are two cases to consider. The first is an instance where a tab ought to exist for a subset of a particular router item. For example, consider the hook_menu() declaration for node/%:

  $items['node/%node'] = array(
    'title callback' => 'node_page_title',
    'title arguments' => array(1),
    'page callback' => 'node_page_view',
    'page arguments' => array(1),
    'access callback' => 'node_access',
    'access arguments' => array('view', 1),
    'type' => MENU_CALLBACK,
  );

In this case, the 'subset' would be node types; it is perfectly reasonable, for example, for modules to want to add tabs only to certain node types. Panels Node, for example, defines the following menu item:

  $items['node/%node/panel_layout'] = array(
    'path' => $base . 'layout',
    'title' => 'Panel layout',
    'page callback' => 'panels_node_edit_layout',
    'access callback' => 'panels_node_edit_node',
    'access arguments' => array(1),
    'page arguments' => array(1),
    'type' => MENU_LOCAL_TASK,
    'weight' => 2,
  );

The problem is, unless the access callback is specifically written to run a check on node types and return FALSE if it's not the right node type, then this local task will appear on ALL node types. At minimum, that's a DX issue - we're requiring devs to think about routing issues inside the access callback. And it's messy - but it works, and alone doesn't merit a core patch.

The second case is uglier. Rather than needing to provide local tasks based on a subset, local tasks may need to be provided based on an individual id. The exemplar for this is og_panels, which allows group admins to define additional local tasks per individual groups. For this case, there are essentially two options:

  1. Skip menu wildcarding and loaders, and just define the path statically: menu items for node/20/foo, node/20/bar, node/20/shoggoth/beeblebrox...
  2. Use the native wildcard loaders (i.e., define node/%node/foo, node/%node/shoggoth/beeblebrox), but then write some really hairy access callbacks that basically amount to a switch statement on a nid. (Or at least, a SELECT * FROM {example_tab_storage_table} WHERE nid = %d...)

Both of these approaches have major drawbacks.

Static path drawbacks

Potentially massive, MASSIVE bloat of your {menu_links} and {menu_router} tables. Like, multiplicatively larger than your node table. Not only would you have to record a router item for each tab you want to add for EACH fully static path, but you'd have to duplicate all the existing ones (node/%/view, node/%/edit, etc.) for each and every node. That means a {menu_router} table 2x the size of your node table on an otherwise vanilla drupal install. menu_rebuild() will be fun, dropping then recreating a few million rows on a reasonably large site.

Wildcard/dynamic path drawbacks

Kinda hellatious access callbacks. If you go this route, it means you have to devote a section of your access callback to eliminating all the tabs you don't want. It can be written somewhat efficiently - imagine a table with nids and subpaths; you can just select the subpaths permitted for that nid, compare them to the provided subpath, and duck out of the access callback early with FALSE if the requested subpath isn't in that list. Even if that's what you do, though, a reasonably large site running og_panels or its ilk will grind through that callback hundreds, possibly thousands of times for each and every one of the tabs that have been defined. Even if the access callback is efficient, it means drastically increasing the number of times that the meat of menu_local_tasks gets iterated over.

Second, since you get only one router item per path, you're open to path conflicts that make little conceptual sense. Think of it from a g.d.o group owner's perspective: Group A should be able to define their own set of tab paths and never have to care whether or not Group B has defined a tab at, say 'home' (aka, node/%node/home). Yes, that can be handled by an additional layer of indirection in the module (in fact, this problem isn't so critical for og_panels because Panels is designed to handle layers of indirection like that, and I don't think it'd be too hard to fix), but again, I think that's a DX concern.

An important note: this problem can NOT be circumvented by intelligent use of tab_root or tab_parent. The reason why is the query that retrieves local tasks to be rendered in menu_local_tasks:

$result = db_select('menu_router', NULL, array('fetch' => PDO::FETCH_ASSOC))
      ->fields('menu_router')
      ->condition('tab_root', $router_item['tab_root'])
      ->orderBy('weight')
      ->orderBy('title')
      ->execute();

The router item's tab root is either going to be a wildcard path or a static path - those are the only two (sane) options (Note that this is the query responsible for the situation described in the static menu approach, where you have to duplicate all the wildcard subtab router items for the static ones as well). This query is RIGHT. It's a fast, uncomplicated query that drills straight to what we need; it should not be changed. The simple fact is that there's no clear, efficient way to store that level of dynamic metadata in the db such that we can delegate all the decisionmaking about conditional tabs to the db itself. Some of the work needs to be done at runtime...which is what this patch is about.

This patch adds a new menu property, 'local_task_hook'<code>, which (if defined) is used during <code>_menu_translate() to compose and invoke a dynamic hook of the form hook_conditional_local_task_$router_item['local_task_hook']. This hook is a sort of pseudo-menu hook - implementations of the hook return a hook_menu()-style array of items that are dynamically reincorporated into the router item, and are used both during the menu_execute_active_handler() (for actual page, title, access callbacks, etc.) and during the theming layer, when the local tasks are rendered in menu_local_tasks(). It is a pseudo-menu hook in the sense that:

  • It can only handle a subset of the normal menu item properties ( 'type' and 'tab_root' are constrained, for obvious reasons
  • Rather than expecting an array keyed on full paths, it expects an array keyed on the path relative to the tab root (so, a key of 'beeblebrox/shoggoth' returned on an invocation from node/%node's local_task_hook invocation will be reintegrated into a router item with $router_item['path'] == 'node/%/beeblebrox/shoggoth').

By doing this dynamic runtime substition into the router item, we obviate the need to perform either complex db-based gymnastics or iterations over massive arrays of possible tabs just to drill down to the ones we want. Those are replaced with a single, targeted hook invocation that basically operates like a specialized, only-when-needed version of D5's hook_menu() with $may_cache == FALSE.

CommentFileSizeAuthor
#36 drupal.menu-local-tasks-alter.patch700 bytessun
#27 runtime_tabs-353208-23.patch6.83 KBpwolanin
#26 runtime_tabs-353208-22.patch6.27 KBpwolanin
#14 runtime_tabs.patch4.47 KBchx
#11 runtime_tabs-353208-11.patch5.16 KBpwolanin
#7 runtime_tabs.patch4.41 KBchx
#6 runtime_tabs.patch781 byteschx
conditional_local_tasks.patch8.49 KBsdboyer
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

sdboyer’s picture

sdboyer CreditAttribution: sdboyer commented

Please note that there's a fair bit of kluge in the initial version of this patch, and that it's not 100% working, either. With the way I'm merging things in and out, the default local task on a given callback is still picking up the href from the merged-in clt rather than the initially retrieved router item; I'm not quite sure why that's happening. There are also several other issues with what's going on in menu_local_tasks(); really, I ran out of steam and wanted feedback before really polishing some of those smaller details :) Bottom line is, the dynamically substituted menu items are working and we've got the menu data we need inside of menu_local_tasks() to make it go, it just needs a more artful hand than I seem to be able to manage right now.

Also, as currently written, it's not very smart about handling tab parenting; if you want to define secondary local tasks (say, foo/beeblebrox, foo/shoggoth), then you need to explicitly set those items to have tab_parent = 'foo'. Definitely one place where this needs help.

Also also, I can picture a slightly different way of approaching this, where we attach the hook to callbacks in interleave that into the system. That would be a more optimal approach, I think, somewhat more analogous to drupal pipes/ctools' delegators. But, this approach does (or is close, at least) to working, and there's something to be said for that.

sdboyer’s picture

sdboyer CreditAttribution: sdboyer commented

Oh, and. Here's the dummy module code I was using to test this patch:

function dummy_conditional_local_tasks_node($map) {
  $items = array();
  $items['foo'] = array(
    'title' => 'foofie',
    'title callback' => 'dummy_title_callback',
    'title arguments' => array(1),
    'page callback' => 'dummy_page_callback',
    'page arguments' => array(1),
    'access callback' => 'dummy_access_callback',
    'access arguments' => array('view', 1),
    'weight' => 14,
  );
  $items['bar'] = array(
    'title' => 'barbar',
    'title callback' => 'dummy_title_callback',
    'title arguments' => array(1),
    'page callback' => 'dummy_page_callback',
    'page arguments' => array(1),
    'access callback' => 'dummy_access_callback',
    'access arguments' => array('view', 1),
    'weight' => -2,
  );
  return $items;
}

function dummy_title_callback($node) {
  $i = 'break on me';
  return $node->title;
}

function dummy_page_callback($node) {
  $i = 'break on me';
  return node_page_view($node);
}

function dummy_access_callback($op, $node) {
  $i = 'break on me';
  return TRUE;
}

The hook declaration at the top is all that really matters, but the callbacks do get hit like they should.

sdboyer’s picture

sdboyer CreditAttribution: sdboyer commented

Aaaand one last initial addendum: I wrote this up with the idea that it could be backported to D6. Actually, I wrote it against D6 initially, and migrated the patch quickly and easily through the wonders of git! =) This is just as much a blocker for some modules and implementation needs in D6 as it is in D7, but I suppose the most pressing need would be that g.d.o will have some serious headaches in trying to update to D6 if this isn't available.

moshe weitzman’s picture

moshe weitzman CreditAttribution: moshe weitzman commented

subscribe ... as mentioned, this is a big deal for the D6 upgrade of groups.drupal.org. as for the implementation quality, i defer to the menu experts.

chx’s picture

chx CreditAttribution: chx commented

I think per-nodetype tabs can be handled by the router system currently and the per-node tabs (og and friends) could be handled by menu links. If you want to present custom tabs on say node/12 then you save links to say tabs_node_12 with menu_link, define the targets in hook_menu as MENU_CALLBACK and done. The menu.inc change to this would be a few lines where menu_local_tasks tries to menu_tree_page_data(tabs_node_12) and present that as tabs, too.

chx’s picture

chx CreditAttribution: chx commented
Status: Needs work » Needs review
FileSize
runtime_tabs.patch781 bytes

Here is a solution which lets you add router items by specifying a list of array('tab_parent' => , 'path' =>); arrays in hook_menu_add_tabs.

chx’s picture

chx CreditAttribution: chx commented
Assigned: sdboyer » chx
FileSize
runtime_tabs.patch4.41 KB

Now comes with test and API documentation. The code itself in running Drupal is as small as it can be.

sdboyer’s picture

sdboyer CreditAttribution: sdboyer commented

That looks a bit like the patch I originally wrote. Problem I had is, just painting the extra tabs on in menu_local_tasks() means the tabs show up, but there's nothing there to grab them during menu_execute_active_handler(). So, to be clear for anyone ELSE reading the thread :), the suggestion here is that the menu items being added via the new hook will point to router items that are declared during hook_menu() as MENU_CALLBACKs, rather than as MENU_LOCAL_TASKs (like you'd expect). That way there's still a proper router item that will be snagged during menu_execute_active_handler(), then the extra input from this hook will integrate them into the rest of the local tasks.

I've only looked at the patch, not stepped through this with a microscope, but it's MUCH cleaner than my original approach, and well-worth the somewhat unintuitive two-step process where you declare something you want as a tab to be a MENU_CALLBACK. Three cheers for chx, thanks for stepping up to this...

pwolanin’s picture

pwolanin CreditAttribution: pwolanin commented

This approach seems very sensible - but I'm a little worried that more info is not passed into the hook (e.g. the map, path, etc).

Also, for DX maybe we can define a MENU_DYNAMIC_LOCAL_TASK type that is just an alias for MENU_CALLBACK? Essentially so when you read the code you get the idea that this path may become a tab?

chx’s picture

chx CreditAttribution: chx commented

I do not really know what else you want to pass in when everything is available from menu_get_item() and menu_get_object() already. about MENU_DYNAMIC_LOCAL_TASK, I dislike the idea. We usually do not have two ways of doing the same and when I want to reuse an existing callback its unclear whether I need to hook_menu_alter to M_D_L_T so let's just skip that.

pwolanin’s picture

pwolanin CreditAttribution: pwolanin commented
FileSize
runtime_tabs-353208-11.patch5.16 KB

ok, reviewed in more depth. A minor change in this path - we are really getting back from the hook what we generally call 'href' not 'path' - i.e. it's the path with any wildcards replaced by specific values. For consistency we should use $item['path'] as the code above does.

New patched with just these minor changes. The new test still passes. Overall this looks good and is a very simple change that fixes the regression we saw from 5.x to 6.x.

Status: Needs review » Needs work

The last submitted patch failed testing.

David Strauss’s picture

David Strauss
he/him
CreditAttribution: David Strauss commented

The idea of registering the callbacks in hook_menu() but controlling the display from hook_menu_add_local_tasks() seems like a bad design. We've always structured the menu system on the idea that "access granted" = "accessible" and (type depending) "visible."

What happens when you decide to not display the tab via hook_menu_add_local_tasks() but a user of the site accesses the URL it would go to? Presumably, your MENU_CALLBACK type menu item would still show the page. Then, you're back at square one perfecting you access callback and arguments to match your visibility rules.

chx’s picture

chx CreditAttribution: chx commented
Status: Needs work » Needs review
FileSize
runtime_tabs.patch4.47 KB

David, I do not get you. The point of defining the router item ahead of time is to make sure that the same access callback controls the visibility of the tab and the page itself. W/ the current menu system, many times you can type in a URL and land on a page you have access to but it's not linked. Hey, this was even so in D5... The PHP syntax error the bot found was in the API docs...

pwolanin’s picture

pwolanin CreditAttribution: pwolanin commented

Ok, so it's not clear this yet gets us to where we want to be. I cannot yet find a way so far to make the other tabs appear when I am on the dynamic tab path. @chx - have you gotten this to work? Is that how it worked in D5?

sdboyer’s picture

sdboyer CreditAttribution: sdboyer commented

I really need to get out of the habit of responding to issues that I haven't thoroughly tested myself.

@David: Not really square one. The problem with complex access callbacks that I pointed to in the original post doesn't have to do with the access callbacks being fired below menu_execute_active_handler() in the call stack (i.e., when a user has actually requested that page), but below theme(). The problem is when there's a huge flood of irrelevant tasks which ARE accessible to the user - but still shouldn't be visibile, because they're not relevant to the current node type.

So really, the the principle described in your original statement gets it wrong. Even for menu item types that get painted in theme(), "access granted == accessible," but should not necessarily mean "visible." IMO, these are two different questions, and though they're the same in most cases, this issue demonstrates why it's not always so.

David Strauss’s picture

David Strauss
he/him
CreditAttribution: David Strauss commented

"W/ the current menu system, many times you can type in a URL and land on a page you have access to but it's not linked."

There's only one condition under which that should happen; it's when you need a callback that uses a non-menu link to get to a path.

What I don't understand is the need to have menu items that have persistent paths but conditional local task tabs. If visibility of a tab is contingent on a node being, say, a Panel node, why would you want to the path to work with Story nodes? Why would you hide a tab except for the reason that it's not relevant or the user doesn't have permissions?

sdboyer’s picture

sdboyer CreditAttribution: sdboyer commented

What I don't understand is the need to have menu items that have persistent paths but conditional local task tabs. If visibility of a tab is contingent on a node being, say, a Panel node, why would you want to the path to work with Story nodes? Why would you hide a tab except for the reason that it's not relevant or the user doesn't have permissions?

Those are the two reasons for not having tabs show up. As it's currently designed, the menu system handles the latter case just fine. It's the former that's a problem: router items have no way of answering that 'relevance' question without some delegated at-runtime logic. They have no concept of any subdivisions within a given wildcard path item (e.g., node type on the node/% path). Hence the need for conditional local tasks with our persistent path menu items.

David Strauss’s picture

David Strauss
he/him
CreditAttribution: David Strauss commented

@sdboyer But why should going to the URL directly still work if a menu item is deemed irrelevant?

sdboyer’s picture

sdboyer CreditAttribution: sdboyer commented

Mmmm, I see what you mean. Heh, s'funny. This case you're describing isn't a problem with my original approach, where those pseudo-items would never exist in the first place if the node isn't of the correct type. That's the benefit of munging about during menu_execute_active_handler(). Although there are clearly other problems with that approach.

I need to think about this some more. Unforunately, that means getting back to this later this week, at the earliest.

sdboyer’s picture

sdboyer CreditAttribution: sdboyer commented

So, actually, I think the answer is that for these conditional local tasks, anyone creating them should expect that there could be cases where their callback could be fired (by someone directly accessing a URL, as you've described), which means that they should wrap their page callbacks in a check to make sure they've got the right node type, else 404. I don't think that's an unreasonable expectation.

David Strauss’s picture

David Strauss
he/him
CreditAttribution: David Strauss commented

@sdboyer I think it's inconsistent to expect most menu items to have access controlled through the menu system but have these conditional local tasks have access managed by manually throwing a 403 or 404 from the page callback. Your suggested approach also invites security issues stemming from inconsistent implementation of visibility and access rules because they'll occur in different places in the code.

sdboyer’s picture

sdboyer CreditAttribution: sdboyer commented

Visibility and access aren't the same thing. Assuming that they are is part of the root of the whole problem here. If you disagree, let's talk about that. Otherwise, get over it - they're different things, which means they may need to be handled at different places in the code.

What you're saying doesn't make sense, and is not what I'm suggesting. The conditional local tasks are NOT access-managed through the some other callback. Their access mechanism is unchanged; really, it's BETTER, because there's no need for contorted logic in that access callback in order to get the tab to only appear at the correct times. That's the original purpose of the patch, as I stated in the OP.

The problem I was responding to was the 'relevancy' question - what happens when someone manually enters a URL, say node/33/foo, where 'foo' is a conditional local task that shouldn't appear on node 33. ASSUMING the requesting user makes it past whatever access callback is defined, then we proceed to the page callback, where a 404 is thrown. NOT a "403 or 404." A 403 doesn't make sense, because, say, Story nodes should never have Panel Layout configuration attached to them in the first place. Not because the user doesn't have sufficient perms to do it.

And again, the only time this would actually happen is if someone directly entered a URL. When they do that, they're not bypassing any of the normal access checks - those still happen. There's nothing inconsistent there.

---

I could see a way of doing this where we allow a new menu property that hands the $map to a callback defined in the menu item for the conditional local task, and the function returns a boolean indicating whether or not the menu item it provides is relevant for the provided map. If we delegate that responsibility to runtime, then it might be sufficient to let us stop divining from these other aspects of the router whether or not the local task should be used. And I think could easily be done either the execute or theme stack.

Mmmm...actually, that's kinda interesting. I need to think about that some more.

chx’s picture

chx CreditAttribution: chx commented

No need for such wrapping you can check in your access callback easily.

Edit: sorry, crossposted with sdboyer.

David Strauss’s picture

David Strauss
he/him
CreditAttribution: David Strauss commented

The problem I was responding to was the 'relevancy' question - what happens when someone manually enters a URL, say node/33/foo, where 'foo' is a conditional local task that shouldn't appear on node 33.

I still have a problem with a design where, using your example, the check that excludes node 33 has to be in two places: the local task hook and the page callback.

I agree now that security and relevancy aren't the same problem, but I still don't see why the solution should be completely different.

pwolanin’s picture

pwolanin CreditAttribution: pwolanin commented
Assigned: chx » Unassigned
FileSize
runtime_tabs-353208-22.patch6.27 KB

Here's a new patch that works better.

Let me re-summarize from the top the main (and perhaps only) case where this is relevant: Assume I have a specific node (node/99) and due to the special properties of that node, I create a callback with the path node/99/peter. Since this callback is specific to this node, I don't want or need to use a wildcard loader, but instead the path goes into the router table as 'node/99/peter'. There is currently no way to make this callback appear as a tab on just node/99 currently without defining it as a tab on all node pages (which means it gets loaded and processed on each node view), or without defining node/99 itself as a separate path in the menu router. This latter could work for small numbers of nodes (and might have to be sufficient for D6), but if each node is, for example, a user profile that can be customized by adding arbitrary tabs, this is very bad.

In any case where there are wildcards involved in the callback path, probably one should be using a access callback as is the case now to control visibility.

pwolanin’s picture

pwolanin CreditAttribution: pwolanin commented
FileSize
runtime_tabs-353208-23.patch6.83 KB

Re-rolled with more comments and more relevant example code.

David Strauss’s picture

David Strauss
he/him
CreditAttribution: David Strauss commented

Using Peter's example in #26, you would register node/%node/peter as a MENU_CALLBACK, but you would have to put visibility-checking code into the new local tasks hook and the *same* check in the callback for node/%node/peter to return a 404 if the tab isn't visible. While you could put the check in its own function, you'd still have to call the function from both places.

Status: Needs review » Needs work

The last submitted patch failed testing.

pwolanin’s picture

pwolanin CreditAttribution: pwolanin commented

Apparently the test bot is broken again...

For what og_panels wants to do for D6, it seems like you could just define the path 'node/%node/%' and handle the rest of the logic in the access callback and page callback. Obviously this is a little unfriendly for other modules that might want the same path, but at least it avoids bloating the # of items in cache router. og_panels could also get creative with a path with more pats and with path aliases.

Crell’s picture

Crell CreditAttribution: Crell commented
Status: Needs work » Needs review

subscribing. This bugs me too, but I haven't read through the code above so can't speak to this solution yet.

Status: Needs review » Needs work

The last submitted patch failed testing.

pwolanin’s picture

pwolanin CreditAttribution: pwolanin commented

suggestions for og_panels and similar modules for D6: http://drupal.org/node/362031

Chris Johnson’s picture

Chris Johnson CreditAttribution: Chris Johnson commented

A related use case which does not (to me) seem possible without horrible hacks is the following:

I have a couple of menu items which define page paths, i.e. they include my module.pages.inc file and display full pages of information. However, they can only work AFTER a site admin has configured the module.

I can throw checks for correct configuration into my page code all over the place, doing the opposite of the decoupling and encapsulation that every good programmer dreams about. ;-) Or I could, if it were possible, simply disable the menu items dynamically, so that they didn't appear and the URL paths, if used, didn't go anywhere (404).

I can code checks into my access callback, but without even more hackery, the user will get 403 forbidden, instead of 404 not found (or best yet "page not available until admin does his job").

So between the original issue above and my use case, and any related use cases we might dream up, it seems like there is a need to have part of the menu be dynamic, like in the old $may_cache days.

pwolanin’s picture

pwolanin CreditAttribution: pwolanin commented

@Chris - we cannot and should not go back to dynamic path definition. If you want Drupal to report a 404, have your page callback return DRUPAL_NOT_FOUND

sun’s picture

sun
Primary language German
Location Karlsruhe
CreditAttribution: sun commented
Category: feature » task
Priority: Normal » Critical
Status: Needs work » Needs review
Issue tags: +API clean-up
FileSize
drupal.menu-local-tasks-alter.patch700 bytes

This is critical for CTools' port to D7.

I don't see why we can't simply invoke drupal_alter(). Information about dynamic tabs doesn't live (properly) in the menu system anyway (and would be a hazzle to get in there), they are generated by view/panel/whatever anyway, and local tasks are built only once per request, and a single drupal_alter() doesn't hurt.

merlinofchaos’s picture

merlinofchaos CreditAttribution: merlinofchaos commented
Status: Needs review » Reviewed & tested by the community

I am completely in favor of this patch and this code.

The code is trivial. tha_sun is right; this allows us to remove about 250 useless lines of code from CTools that I have written that basically goes a very long and tumultuous route to accomplish this.

Setting RTBC to get webchick and/or Dries' attention.

FYI, the use-case for me is this: When viewing a managed page or a view, it is really convenient to have an 'edit' tab available. This tab is completely unrelated to the menu path, and should never be stored in the menu system, at least as a tab, in any case. It's actually rather dynamic.

In reality, it'd be even cooler to have it in the toolbar, and I should look into if toolbar can flexibly add items not from the menu system. That said, this is still really handy.

catch’s picture

catch
he/him
Primary language English
CreditAttribution: catch commented

I've had to do some horrible things in theme_menu_local_tasks() before, this hook would've prevented those atrocities too.

Crell’s picture

Crell CreditAttribution: Crell commented

Why did it take us this long to come up with this solution? :-) Simple, uses existing workflows and patterns, and looks like it should add a ton of flexibility. I like.

sun’s picture

sun
Primary language German
Location Karlsruhe
CreditAttribution: sun commented

btw, this mini patch will also allow us to display an "Add new content" local action on admin/structure/types that points to node/add, and stuff like that. I know there is at least one UX issue about that somewhere.

sdboyer’s picture

sdboyer CreditAttribution: sdboyer commented

It didn't take us "this long" to come up with this solution. Something like this - that only paints tabs on, but doesn't do anything with underlying router items - was discussed and dismissed early on because we were looking for a more elegant solution. We did actually come up with one that was fairly good, if still a little klugey, but when I came back to update it to the most recent changes, it was made more complicated by the fact that the local tasks at any given path are now cached - yet another layer to be worked through.

Frankly, I'd be surprised if this actually adequately simulates tabs to the point where I'd advocate it for a core solution. I mean, if you guys have posted this and tested it, I'm probably missing something (and there have been more changes to menu_local_tasks() since I last looked), but the big question that jumps to mind is whether or not this addition makes the proper tab_root and accompanying sibling/parent tabs render correctly when the painted-on local task is called.

I'll look at this sometime in the next four days and see if my concerns are unjustified, but I suspect this is still a step backwards in the

(PS - I did start to work on this shortly before code freeze, but did a boneheaded thing and confused drupal_static() with variable_get/set(), and thought I had another layer of caching to fight through. Kinda gave up :P Now that I've cleared that up in my own head, a patch that follows up on the direction need not be far away)

sun’s picture

sun
Primary language German
Location Karlsruhe
CreditAttribution: sun commented
Category: task » feature
Priority: Critical » Normal
Status: Reviewed & tested by the community » Needs work
Issue tags: -API clean-up

sdboyer wants to tackle a more "built-in" approach in here.

Spin-off: #599706: Allow to alter local tasks/actions

Hence, reverting everything.

merlinofchaos’s picture

merlinofchaos CreditAttribution: merlinofchaos commented

I'd be leery of a 'more elegant solution' which leaves us with 'no solution' before code slush is over. This one handles the immediate needs of Views and CTools. It does not necessarily solve the problem for og_panels, which is that the active trail gets lost. These are different problems, though, and I'd hate to think that this problem goes unsolved while we wait for a more elegant solution. I'll note that hte more elegant solution didn't appear in Drupal 6 at all...

sdboyer’s picture

sdboyer CreditAttribution: sdboyer commented

I'd considered arguing (because the approach that has now been committed in #599706: Allow to alter local tasks/actions I do, clearly, consider broken), but figured I'd just write the patch. Clearly didn't get time for that this week.

Oh well. I guess we just get to let core facilitate multiple different behaviors for local tasks in D7.

sun’s picture

sun
Primary language German
Location Karlsruhe
CreditAttribution: sun commented
Version: 7.x-dev » 8.x-dev
Issue tags: +MenuSystemRevamp

Note that the simple solution proposed here has been committed over in #599706: Allow to alter local tasks/actions already; that issue just holds a follow-up patch to clean up some mess, which still needs review.

In light of considerations for a larger menu system revamp, tagging and moving.

sun’s picture

sun
Primary language German
Location Karlsruhe
CreditAttribution: sun commented
Issue summary: View changes

fixed closing h3 tags

jhedstrom’s picture

jhedstrom
Primary language English
Location Portland, OR
CreditAttribution: jhedstrom at Phase2 commented
Version: 8.0.x-dev » 8.1.x-dev
Issue tags: +Needs issue summary update
dawehner’s picture

dawehner
Primary language German
CreditAttribution: dawehner as a volunteer and at Tag1 Consulting commented

So we have hook_menu_local_tasks_alter() now, which kinda solves the usecase for this issue already ...

pwolanin’s picture

pwolanin CreditAttribution: pwolanin as a volunteer commented
Status: Needs work » Fixed

ok, I think we can call it fixed then

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.

xjm’s picture

xjm
she/her
Primary language English
CreditAttribution: xjm at Acquia commented
Title: Allow for runtime-conditional local tasks » Allow for runtime-conditional local tasks (hook_menu_local_tasks_alter())
xjm’s picture

xjm
she/her
Primary language English
CreditAttribution: xjm at Acquia commented
Version: 8.1.x-dev » 7.x-dev