Original issue

(Fix committed as of #15)
If a i18n menu item is translated using i18n_menu_translated_menu_link_alter() the flag $item['i18n_menu'] = TRUE; is set to prevent double localization.
Later if the item is passed to i18n_menu_localize_tree() the function checks if the flag is set and skips the whole processing if so. (See #360 in i18n_menu.module
Unfortunately this also skips the localization of the subtree if one is present.

I suggest to move the subtree processing out of the condition and add a continue; for the case the whole item has to be removed.
Attached patch does that and seems to work just fine.

This issue is related to following 6.x issue: #966534: Customized subtree and i18nmenu_localize_tree()

Issues with the fix

  • Menu links can become disabled while editing a node (comments #17 and from #24 on)
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Jose Reyero’s picture

FileSize
3.4 KB

Um, interesting. Yes, the patch makes sense.

However, I was thinking whether we could forget about that 'localize tree' and do all the localization / hiding of menu items on hook_menu_translated_link_alter(), which only needs menu items to be set previously with 'options['alter'] = TRUE.

Please take a look at this other patch.

Status: Needs review » Needs work

The last submitted patch, i18n_menu_better.patch, failed testing.

das-peter’s picture

Status: Needs work » Needs review
FileSize
3.45 KB

Sounds reasonable - in fact I already used a similar hack over the last year to support our entity translation integration.
But I decided to upgrade all the modules now ;)

I gave the patch a shot and it seems to work. However I don't fully get the approach yet.

Is the goal of the patch is to make i18n_menu_localize_tree() obsolete?
If so we aren't quite there because currently only link_title is translated - title isn't.
Further it seems like the condition

if (!empty($item['customized']) && empty($item['language'])) {

should use !empty($item['language'])

I changed all those points in the attached patch.
Another change I made is related to the issue reported by the tests. Since hook_menu_link_alter is fired in menu_link_save before the defaults are set there are cases in which no module is defined. I guess it's sane to assume that if no module is defined the item should be handled as if "menu" is set as module.

Jose Reyero’s picture

I've started with your patch and ended up doing some partial rework, see new patch which I think will fix other related issues, improve performance (by properly handling menu item strings) and is more understandable than the old code.

Some more cleanup may be possible, though functions like i18n_menu_localize_tree(), not needed anymore by i18n_menu must be kept (though maybe it could be empty) as other modules are using it.

Now all the processing is done in hook_translated_menu_link_alter(), no need for replacing block menus anymore or primary and secondary links, etc... so I guess this should be some performance improvement + a serious logic cleanup.

As the patch is getting big, if you think this works, I'll commit it right away so we can focus on other minor improvements while it gets some testing in the dev version.

das-peter’s picture

As discussed in Munich I think it's a good idea to use hook_translated_menu_link_alter() instead the other approach.
In fact I've already used a similar hack for over a year and it worked like a charm - but of course I just covered a limited range of scenarios.
If you like I give the changes a try in our existing installations and provide feedback - but for that I need a patch :)

Jose Reyero’s picture

Ooops, sorry, forgot the patch again :-(

das-peter’s picture

Thanks for the patch :)
I've tested it on one of our installations, looks very good to me.
The only issue I had was related to the update hook, I've fixed that.
Further I fixed some coding standard issues in the already changed code parts.

Jose Reyero’s picture

@das-peter,
Your changes look good to me, thanks for the code cleanup.

I think this is RTBC but since that's a major rework, I'd like to give it some time in the -dev version to get some testing.

So @das-peter, @webflo, wondering whether we should commit this or put another stable release out first, in case there are other fixed issues we are waiting for.

das-peter’s picture

Sounds like a good idea to give it some time in the -dev version.

Jose Reyero’s picture

Title: i18n_menu_translated_menu_link_alter() skips item subtree processing. » Change menu translation approach. (i18n_menu_translated_menu_link_alter() skips item subtree processing)
Priority: Normal » Major
Status: Needs review » Reviewed & tested by the community

I wish we could have done another maintenance release in the meanwhile but it seems we were all too busy with Drupal 8 in the lat weeks.

Anyway, this has been delayed long enough, so it is to be committed ASAP and we'll do a new release by the end of the year.

Jose Reyero’s picture

Status: Reviewed & tested by the community » Fixed

Committed.

Wondering whether we should add some change notice, as this should get all menu items translated cleanly without the need of any other modules (i18n_contrib/i18n_menu_select).

Status: Fixed » Closed (fixed)

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

Jose Reyero’s picture

Assigned: Unassigned » Jose Reyero
Priority: Major » Critical
Status: Closed (fixed) » Needs work

I've just noticed items in other languages than current show up "disabled" in the menu edit pages, we need to fix it before release.

Jose Reyero’s picture

Status: Needs work » Fixed
FileSize
1.96 KB

Fixed with this patch.

Jose Reyero’s picture

It was not committed, done now.

plach’s picture

Jose Reyero’s picture

Status: Fixed » Needs work

This may need some more work.
From https://drupal.org/node/1800450#comment-6938800

when editing content, both my source (English) and translated language (Chinese) menus are listed, but the translated language menus show "(disabled)" next to each item -- even though they are enabled and working (tested)

Berdir’s picture

Uhm. This completely broke the menu localization. Trying to view the translate tab of a menu results in a fatal error which has been reported here #1889878: Call to a member function format_translation() on a non-object in i18n_string.pages.inc on line 80, git bisect points to this issue :)

The problem is that "$strings = $object->get_strings(array('empty' => TRUE));" does return no strings and then the UI explodes.

Jose Reyero’s picture

Priority: Critical » Normal

The solution to this latest error is on the other issue, linked from #18

soncco’s picture

After this update, all menus for all languages are displaying in the same place , the translation is not respected. The same problem here #1888448: Menu translation broken after upgrading to i18n 7.x-1.8 and variable 7.x-2.2.

bvanmeurs’s picture

Same here soncco.

The original issue https://drupal.org/node/1726906 has not been fixed. After updating to i18n-7.18 I still get all menu items.

Berdir’s picture

drzraf’s picture

after the update, speaking about the main-menu which appear in every page (including administration), all menu items show up in the main-menu when browsing at /admin/structure/menu/manage/*.
This is related to the hunk provided in comment #14 which was part of the commit.

agnese.stelce’s picture

Problem mentioned in #13 still persists (disabled menu items). One example here - http://drupal.org/node/1905268#comment-7013904. I encountered with this problem also on node edit form trying to edit node in different language then current. After node save it always leads to disabled menu item for that node.

raino’s picture

FileSize
26.25 KB

Disabled menu items problem persists when edit node in different language then current. (translate node).
After node save the menu item is disabled.

martins.bertins’s picture

Version: 7.x-1.x-dev » 7.x-1.8
Status: Needs work » Needs review
FileSize
780 bytes

Here's a patch I created.

cdnsteve’s picture

This seems to be the same patch on another issue.
It doesn't work for language neutral, see comments here:
http://drupal.org/node/1905268#comment-7257360

GiorgosK’s picture

I think people should at least be warned right on the project page about this strange bug since this issue is becoming stale but bug still persists

GiorgosK’s picture

Priority: Normal » Major
Status: Needs review » Needs work

I can't understand the logic behind patch in #26 and it actually does not work so reverted back to needs work

And because this can happen to any of at least 65K live sites and might go undetected or completelly HIDE a main menu or have the webmaster wondering what has gone wrong I am marking this major to get the attention of the maintainers
but feel free to change if I am wrong

I was trying to debug i18n_menu myself but I am not an expert on it and I got completelly lost

so instead of going back to 7.17 I created a TOTALLY HACKISH workaround

function MODULENAME_entity_presave($entity) { 
  if(isset($entity->menu)){
    $result = db_select('menu_links', 'm')
     ->fields('m',array('hidden'))
     ->condition('mlid', $entity->menu['mlid'],'=')
     ->execute()
     ->fetchAssoc();   
    $entity->menu['hidden'] = $result['hidden'];
  }
}

The premise of this solution is that when an entity is edited/saved (entity_presave) user can't have access to the HIDDEN option. Hidden is only available on the menu administration page. So I look up the previous value of HIDDEN and replace it.

Please use at your own risk

richsky’s picture

Issue tags: +Localize, +custom menu items

Hello, here are my two cents.

Let say I have a custom module providing a link to main menu. It can be localized if it stays in its defaults state. If I customize it, I can't get translation anymore. I know customized links go in menu_links, but I don't get why it doesn't show up in string translation anymore and doesn't show up translated.

This is quite anoying.

GiorgosK’s picture

is this issue related ?
#1905268: Disabled menu items

mErilainen’s picture

Assigned: Jose Reyero » mErilainen
Status: Needs work » Needs review
FileSize
634 bytes

I'm not sure if this is the right approach, but at least it doesn't use unsafe database query, and seems to work as it should. I don't think we should display the menu items depending on the interface language, because the node language will determine in which menu it can be placed.

trevortwining’s picture

Patch works as advertised. Thanks a bunch!

And yes, the issue in #31 is related AFAIK.

http_teapot’s picture

Patch works, thanks!

stefan.r’s picture

Title: Change menu translation approach. (i18n_menu_translated_menu_link_alter() skips item subtree processing) » Change menu translation approach (i18n_menu_translated_menu_link_alter() skips item subtree processing)
Issue summary: View changes
FileSize
455 bytes

#32 may not be the right approach as menu links are still considered part of the interface. A further issue with #26 and #32 is that they can change a menu item to be disabled if the language of the menu item doesn't match the node language, when it had been explicitly set to be enabled before.

I attached a patch which makes us not process the menu item at all if we're on the node edit page. This is the same approach we take for the menu item admin screen.

Possibly related: #2136815: hook_translated_menu_link_alter also runs upon editing nodes with menu links, not just upon menu link display

Scott Robertson’s picture

#35 did not work for me at all, even after changing arg(1) == 'edit' to arg(2) == 'edit' in the patch.

#32 Seems to be working for me thus far, however all of my non-English menu items still have (disabled) next to them in select lists.

joseph.olstad’s picture

removing my own comment

stefan.r’s picture

#36, it should indeed be arg(2) but what exactly is not working? How can I reproduce the failure? What this patch should fix is the menu links becoming disabled upon saving a node which is in a different language than the interface.

Revised patch + test attached.

Status: Needs review » Needs work

The last submitted patch, 38: i18n-fix-for-disabled-menu-items-testonly-1693074-38.patch, failed testing.

Scott Robertson’s picture

I tested #38 on a fresh install of Drupal and can confirm that menu items no longer become disabled, so I believe a contrib module was conflicting with the patch on my previous test.

stefan.r’s picture

Assigned: mErilainen » Unassigned
Status: Needs work » Needs review
JonesUI’s picture

I'm still experiencing the disabled menu bug in version 7.x-1.10 so I assume that a fix has never been applied?

weri’s picture

@mordaga no, the fix get applied when the patch is tested by the community. It would be great when you apply this patch and give some feedback when the patch works or not. Thanks!

JonesUI’s picture

I tried to apply the patch but my patching app (OS X command line) didn't like the patches (none of the ones I tried from above). I tried with version 7.x-1.8 (release) because that's what is associated with this issue but no beuno.

Should I be patching against a different version?

Scott Robertson’s picture

Hi mordaga,

You'll want to try apply the patch to the 7.x-dev branch, as patches are always written for the latest dev version, even if the issue is reported for the current release.

GuillaumeDuveau’s picture

Version: 7.x-1.8 » 7.x-1.x-dev

Just applied #38 with success, and it fixes the menu links becoming disabled upon saving a node which is in a different language than the interface.

stefan.r’s picture

mordaga, this patch should run against 1.10 as well

GuillaumeDuveau’s picture

Status: Needs review » Reviewed & tested by the community

"Reviewed and tested by the community" seems OK, I guess...

Anybody’s picture

The patch works great! Who takes it into the latest dev?

Jose Reyero’s picture

Status: Reviewed & tested by the community » Fixed

Committed, thanks

Status: Fixed » Closed (fixed)

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

Anonymous’s picture

Status: Closed (fixed) » Needs review
Related issues: +#2250501: Node edit page shows menu item (for node being edited) in all languages

Hello people, this code makes problems:

// Skip if administering this menu item through the node edit form.
  elseif (arg(0) == 'node' && arg(2) == 'edit') {
    return FALSE;
  }

See https://drupal.org/node/2250501

Can we change this lines?

stefan.r’s picture

Status: Needs review » Needs work
stefan.r’s picture

Status: Needs work » Closed (fixed)

Since this had already been marked as closed a while ago let's link this back to the relevant issue and continue discussion there :)

#2250501: Node edit page shows menu item (for node being edited) in all languages

The last submitted patch, 38: i18n-fix-for-disabled-menu-items-testandfix-1693074-38.patch, failed testing.

stefan.r’s picture

@Nemanja the reason that patch fails testing is because it has already been committed

mErilainen’s picture

What has actually changed? I can still see all the menu items in different languages, using 7.11.

stefan.r’s picture

@mErlianen: likely you only have this issue on the page itself anymore. Have a look at #2250501: Node edit page shows menu item (for node being edited) in all languages for reference.