Wrong local task titles for taxonomy term and user account pages

Re-classifying and re-titling this issue a bit.

IIRC, we discussed these introduced issues in IRC already, and IIRC it was moshe who made the very good suggestion that we should _not_ invoke 'title callback' for local tasks and just use the 'title'.

First, I had doubts with that idea, because there may be modules that want to output dynamic tab titles (e.g. a counter as suffix) - but then we figured that we now have http://api.drupal.org/api/function/hook_menu_local_tasks_alter/7 and if there is such a need, then modules can easily alter/override/extend a tab's title.

So, yes, ideally we just output the 'title' and do not invoke 'title callback' for local tasks.

Attached is a patch which provides appropriate "Edit" title for the local tasks.

That patch obviously removes the proper page titles for those pages. See #1 for the proposed solution.

subscribe

This would be one possible fix.

+++ includes/menu.inc	28 Nov 2009 19:21:02 -0000
@@ -1667,6 +1667,11 @@ function menu_local_tasks($level = 0) {
+      // Do not invoke title callbacks for local tasks and actions. To output
+      // dynamic local task titles, modules can implement
+      // hook_menu_local_tasks_alter().
+      $item['title_callback'] = 't';
+      $item['title_arguments'] = array();
       _menu_translate($item, $map, TRUE);

We could and actually have to even make it support BOTH:

If $item contains a 'title', we apply this logic.

If a local task defines NO 'title', then we of course cannot invoke t() on it ;)

I'm on crack. Are you, too?

See

http://api.drupal.org/api/function/_menu_translate/7
http://api.drupal.org/api/function/_menu_item_localize/7

New patch with suggested logic.

looks pretty solid, I can't seem to test if it is indeed translatable because for some reason it's only picking up strings in js files but I can confirm that t() was called with the title so it looks like this did the trick

will try to look into writing a test for this

This seems to make sense. Should we enforce this behavior for all scenario's or only for local tasks?

It looks like this is also an issue with the "My account" link in User Navigation which is renamed to the currently logged in user's username.

Perhaps we need a method for flagging menu items that should not use the title_callback when rendering the item as part of a menu, but still use it for generating the page title. Or as c960657 suggests in #1 only use title_callback for generating the page title and not the menu link/tab title. As long as their is still some way to dynamically set a menu item's title since it is a useful feature.

Only for local tasks, because otherwise, the title callback would only be used for the page title and not for menu lin........mmmmh.

True is, we also have http://api.drupal.org/api/function/hook_translated_menu_link_alter/7, which allows to dynamically alter a menu link.

erm. Not sure.

Ideally, someone like webchick should chime in.

+      $item['title_arguments'] = array();

This prevents you from using translation tokens in local task titles. The title is still run through t(), so why do this?

Because there are no translation tokens for static menu item titles. If there is 'title callback' + 'title arguments', then the arguments apply to the title callback only. t() needs a keyed array anyway.

'title' + 'title arguments' is what I meant, which would send the arguments to t(), but (I verified) this patch would disable that for local tasks. Just trying to make sure it wasn't a mistake.

"title arguments": Arguments to send to t() or your custom callback

Good catch! :) So we need to remove the removal of title arguments in case there is no title callback, but a title. Errrr? Yeah. ;P

I'm kind of iffy on this. It doesn't seem consistent, since the 'title callback' should filter the 'title', unless given some arguments:

      ...
      if (empty($item['title_arguments'])) {
        $item['title'] = $callback($item['title']);
      }
      else {
        $item['title'] = call_user_func_array($callback, menu_unserialize($item['title_arguments'], $map));
      }
      ...

Under sun's proposal 'title' would be intentionally blank (when using a custom callback), so 'title callback' would no longer act as a filtering device when no arguments are given.

I don't see how the proposed solution really fixes anything by discarding the custom title callback in menu.inc. The original problem was that 'title callback' was being used where it shouldn't have been (on the local task title when it's already on the parent page title). I believe sivaji's patch in #2 is correct.

That said, it doesn't address the original question of convention, i.e. whether we should make sure all edit tabs are labeled "Edit" or switch to something like "Edit term" or "Edit account" to allow for multiple edit tabs.

I'm leaving this on 'needs review' to re-examine #2. Also, I am postponing #654032: Regression: 'My account' link can no longer be displayed since it has some overlap with that patch.

Not entirely sure how and why 'title' would be intentionally blank. I didn't propose anything like that.

But anyway. In the meantime, I'm warmed up for Dries' idea and think that entirely skipping the 'title callback' for everything and only using it for the page title makes absolutely sense and should be doable. That is, because menu links are rendered based on their values in {menu_links}. Furthermore, local tasks can be considered as a very small fragment of a menu tree, only rendered differently. Especially, when considering the patch in #631550: Stale + improper logic for MENU_VISIBLE_IN_TREE and MENU_VISIBLE_IN_BREADCRUMB, which - technically - would allow us to retrieve local tasks as menu links just like we retrieve any other menu tree.

Hence, title callbacks are used for page titles only. And even for page titles they may not even be used at all, in case the page callback invoked drupal_set_title() manually.

However, figuring out where exactly this needs to be altered is not easy. The starting point surely is the !isset() condition in http://api.drupal.org/api/function/drupal_get_title/7

@sun: sorry for the mix-up. To explain, your patch resets the title callback to t() only if 'title' is not empty. That would require you to have a blank 'title' to use a custom callback.

As for #19, skipping the 'title callback' completely for link titles, is that for the best? It would lead to link titles not matching the (dynamic) page title they link to, but I could see how that could be desired. The 'title callback' currently affects menu links as well, as far as I can tell. A module that provides menu links with dynamic titles would then have to use some sort of alter.

I think the 'title callback' stuff could use its own issue. Right now we have three competing issues in this thread:

• Some "Edit" tabs are inconsistently labeled
• Decide whether to use "Edit XXX" to facilitate multiple edit tabs
• These 'title callback' shenanigans :)

Local tasks as menu links sounds exciting.

1 + 3 in your list are the same, and if you install HEAD, and go to the user account page on user/1, then you will see a tab that has the username as title - instead of the "Edit" you expected.

@sun:

1. is making sure edit tabs are consistently named "Edit".
3. is figuring out if disabling title callbacks for X would improve things in some way.

1 can be done without 3, since the "Edit" local task doesn't need a title callback attached to it. It's just a mistake that taxonomy/term/1/edit and user/1/edit have title callbacks, unless the tabs are *supposed to be* titled after their corresponding user/term names. If so, I'm totally mistaken and I apologize. If not, see the patch in #2.

Without that title callback, you wouldn't see "carlos8f" on http://drupal.org/user/454578/edit

@sun: My current knowledge is this, which I'll try to double check: my username there is coming from the parent item (user/454578) title callback. the user/454578/edit item is just a tab and should not have a title callback! It would only need one in the case that the tab title needs to be dynamic, such as 'Edit carlos8f's account' or something.

Confirmed. Install HEAD, apply patch #2, go to user/1/edit, and you should see 'admin' as the page title and 'Edit' as the tab title.

This is just a really simple bug. The patch in #2 is correct. Dusting it off and sending it to the test bot...

I'm basically ok with this patch, especially due to the reasoning. However, I'm not sure whether this solves the issue for all possible scenarios. As mentioned before, I think it would make sense to entirely skip the title callback for local tasks and menu trees, and only use it for the page title, if present. Ideally, I'd like to have pwolanin chime in here as the menu system maintainer.

We need to get this issue closed since #654032: Regression: 'My account' link can no longer be displayed and #480484: User page title always set to username are both waiting for this. Let's not over-think this bug fix :) Please make a separate issue for the proposed menu system changes.

Committed to CVS HEAD. Thanks.