Our check did not count on the case that you might use a non-t callback so

if (!$link_translate || (!empty($item['title']) && ($item['title'] == $item['link_title']))) {

needs a change to

if (!$link_translate || ( ( !empty($item['title']) || $item['title_callback'] != 't' ) && ($item['title'] == $item['link_title']))) {

however this can be simplified because the only case ( !empty($item['title']) || $item['title_callback'] != 't' ) is FALSE when nor the title nor the title callback is specified in hook_menu (and because of that the default t is used) and that's broken code http://drupal.org/node/140311 does not list this as a valid case. So we can drop it and get the simple

if (!$link_translate || ($item['title'] == $item['link_title'])) {

code.

D7 patch and test is forthcoming but as this affects d.o. sprint I am inclined to ask for an exception and get it into D6 quick.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

pwolanin’s picture

looks like a reasonable approach, but I'll review in more detail.

drumm’s picture

+1 works on my test site that uncovered the bug.

chx’s picture

FileSize
1.33 KB

Resubmitting for bot review.

pwolanin’s picture

Version: 6.x-dev » 7.x-dev

patch applies with offset to D7 also

David Strauss’s picture

Could we add a unit test for this?

chx’s picture

Get http://drupal.org/node/204077 in and then we have it tested. That one produces three fails in blog test without this.

chx’s picture

Status: Needs review » Reviewed & tested by the community
webchick’s picture

Status: Reviewed & tested by the community » Needs work

While those comments very accurately describe what the code is doing, I'm sure, they read exactly like "wah wah wah" to someone without internal menu system knowledge. ;)

Let's describe the behaviour that we're testing for here, hm?

chx’s picture

Status: Needs work » Reviewed & tested by the community
FileSize
1.2 KB
webchick’s picture

Version: 7.x-dev » 6.x-dev

Committed to HEAD with a couple small grammar fixes.

Setting back to 6.x for consideration.

webchick’s picture

FileSize
1.16 KB

Sorry, here's the patch I used.

Gábor Hojtsy’s picture

Status: Reviewed & tested by the community » Fixed

Thanks, committed #11 to Drupal 6 too.

Status: Fixed » Closed (fixed)

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