Dashboard has two admin pages called 'dashboard'

We could try to solve this for the Dashboard module only, but I don't think there's an obvious good way without making undesirable changes to the Dashboard UI elsewhere also. Plus, other modules could easily find themselves in this situation.

One way to solve this generically would be to have the code at admin/index look for duplicates within the module, and if it finds them, print the title of the parent menu item in parentheses or something?

So that way, the two items when listed at admin/index would become:

• Dashboard (Administration)
• Dashboard (Structure)

which I think makes a fair amount of sense.

From Drupal 7.0 Alpha 7 released, about RC1's string freeze:

Drupal interface translators

The interface strings of Drupal 7.0 are mostly frozen. The days of major changes to the UI strings are over, but we still are changing texts to fix bugs or to lend clarity in the interface. Around release candidate 1, a formal "string freeze" will be declared at which point it should be safe to start translating.

From Drupal core's release cycle:

What's a critical bug?

(...) that we can't possibly release without it being fixed.

Not sure but, if this blocks RC1, it's possible this is a critical.

Or maybe just something like a new "7.0-rc1 blocker" or simply "rc1 blocker" tag? But those are the criticals, anyway.

@juan_g: I'm presuming that David's suggestion wouldn't require new text strings, since the page titles that would be dynamically pulled would themselves have already be translated? Thus, it shouldn't block release.

I think you are right, if this is built by code rather than by hand, no new translations are needed. However, even in this case, probably the code should be ready before RC1, given that this affects the UI. Well, I don't know, maybe I'm exaggerating about this detail... :)

There is probably a more efficient way to seek out our duplicates than running two consecutive foreach's but at the very least this patch is a proof of concept for David's suggestion in #1

ooops ... wrong .patch file

Looks like a good start!

Should this code go inside system_get_module_admin_tasks() instead? Because Help module uses that too, and currently has the same problem with duplicate dashboard links on the help page for dashboard module.

Also, doing it inside that function might make it more efficient. You could probably avoid the double loop and maybe even some of the menu_link_load() calls (since system_get_module_admin_tasks() already has a database query that pulls lots of menu link info from the database, including 'plid', 'mlid', etc.) Not sure if the latter would work. If using menu_link_load(), you probably should check the result exists before trying to use it (since that function can return FALSE for items that don't have a parent).

By the way, for the specific case of the Dashboard module, we might wind up being able to solve this issue differently: #930670: Table-based dashboard admin page should be linked to from the dashboard only (not from the rest of the admin UI). But even if that happens, I still think it's a good idea to have a generic solution for this.

I wasn't able to loose the menu_link_load() calls ... but this is much cleaner

That looks much cleaner indeed! A couple of comments:

1. +        if ($duplicate_path = array_search($task['title'], $titles)) {
   

   I believe array_search() is one of those functions where the difference between 0 and FALSE return values is important, so you probably need to do an exact comparison (!== FALSE or the like) to make sure that if it returns 0 we don't bail out here.

2. +          if($parent = menu_link_load($task['plid'])) {
   ...
   +          if($parent = menu_link_load($admin_tasks[$duplicate_path]['plid'])) {
   

   Your habit of not putting a space between "if" and the following "(" is coming back :)

3. +            $task['title'] .= ' (' . $parent['title'] . ')';
   

   I *think* this is OK as is, but some people seem to be very strict about strings and think every string should be translated. Here that would result in something like t('@original_title (@parent_title)', array('@original_title' => $task['title'], '@parent_title' => $parent['title'])) which looks a bit ridiculous to me. I'm asking because I'm really not sure.

4. It's an edge case, but what happens if there are triplicates (or more)? Maybe you need a separate array to keep track of titles that were already modified so you don't ever append the ($parent['title']) to a title that already had it appended?

10.1: Good catch

10.2: Change is hard :)

10.3: I think this is fine because (I think) both $task['title'] & $parent['title'] were already translated when the menu items are loaded... right?

10.4: This patch uses $links[$path] instead of concatenating the task's title. This way, if there are triplicates, the title will still still be correct. The downside of this technique is that in the case of a triplicate, we repeat the process of altering the title for the triplicate. BUT I think this is such an edge case that this is ok. I can certainly be convinced that it would be better to simply have an array that keeps tack of processed duplicates...

Looking pretty good, except for the PHP syntax error :)

Reusing $links[$duplicate_path] seems like a nice idea, but am I missing something or shouldn't it be $links[$duplicate_path]['title'] instead?

Regarding 10.3, the issue wasn't with the translatability of the titles themselves, but rather with the strings that were being concatenated with them (the parentheses). And come to think of it, it probably does make sense we have to allow those to be translated. What would happen with the current code in a right-to-left language, for example? Not sure, but I think appending the parentheses in this way would mess that up. So t('@original_title (@parent_title)'...) may indeed be the way to go.

Ok .. I think we may have a winner here

We do!

I rerolled to remove semi-colons that were after some of the '}' brackets in the if-else block, but otherwise this looks fine to me.

I think this fix makes sense, and the way it's implemented it'll work for other menu items beside Dashboard module. (Yay for no special-casing.)

Committed to HEAD!

I don't understand what this change has changed really.

At admin/structure/dashboard I can place blocks into two dashboard regions, yet at admin/dashboard I still appear to be free to move them around to either region ... except that when I click 'Done', the ones set at the first page snap back. This is confusing and needs a follow-up fix.

joachim: This issue was not about the dashboard ... this was about the list of admin tasks that could be found at admin/index. On that page there was a possibility that two different admin tasks could have the same name. The example we used was that there were two admin tasks called "Dashboard" - a big WTF for a user - so we changed any tasks with duplicate names to TASKNAME (PARENT MENU ITEM NAME) so that the duplicates have some context and some differentiation.

The problem you are talking about is definitely unrelated. Feel free to open a new issue....

Done, thanks #936798: Dashboard uses unreliable method for identifying blocks..