This issue is related to #1124544: i18n conflict - Menu reaction doesn't have any effect . To reproduce the issue you need to have "context" module installed. Here are the steps:
1. create a context with a menu reaction and a sitewide context condition ( this should be working )
2. add i18n and variable ( dependency ) then enable "internationalization" and "Menu translation" modules
3. refresh, the menu is not set as active

I'm not sure about which module this issue really belongs to. Here is a quick fix but this may not be the cleanest solution.

In the file i18n_menu.module, change the end of i18n_menu_navigation_links function like this

  if (module_exists("context")) {
    $context = context_get_plugin('reaction', 'menu');
    $links = $context->menu_set_active($links);
  }
  return $links;
}
...
CommentFileSizeAuthor
#6 context_conflict-1139722-6.patch4.4 KBidflood
#4 context_conflict-1139722-4.patch1.9 KBidflood
#4 test_error1.jpg76.91 KBidflood

Comments

jose reyero’s picture

jose reyero commented
Category: bug » support
Status: Active » Postponed (maintainer needs more info)

Try tweaking module weights so context runs after i18n_menu.

idflood’s picture

idflood commented
Status: Postponed (maintainer needs more info) » Active

Thanks, I tried to change the weight of context.module to 1000 and -1000 without any changes ( I always "drush cc all" right after the modification ). But testing this a bit further showed me that the execute function from context_reaction_menu.inc is always called before i18n_menu_navigation_links in i18n_menu.module, no matter which weight they have.

But both i18n and context use the preprocess_page hook to alter the menu so setting the weight should have worked. Maybe I did something wrong, will try again later.

edit: tried again, but still no luck. I checked every permutations of weight/bootstrap for all "context" modules ( -700 / 700 weight, 0/1 bootstrap ). i18n is always run after context. But I certainly do something wrong here...

idflood’s picture

idflood commented

After some attempts at fixing this issue, it seems to me that the best approach is to change i18n_menu instead of context. The function i18n_menu_navigation_links() could reuse the $variable["xy_menu"] if it's already defined.

The reason is that with a "menu reaction" context the menus can be completely altered. Here is the code that show this in context_reaction_menu.inc

   if (variable_get('menu_main_links_source', 'main-menu') == variable_get('menu_secondary_links_source', 'user-menu')) {
      $vars['main_menu'] = theme_get_setting('toggle_main_menu') ? $this->menu_navigation_links(variable_get('menu_main_links_source', 'main-menu')) : $vars['main_menu'];
      $vars['secondary_menu'] = theme_get_setting('toggle_secondary_menu') ? $this->menu_navigation_links(variable_get('menu_secondary_links_source', 'secondary-links'), 1) : $vars['secondary_menu'];
    }

I may have some ideas of how to merge $tree and $vars['xy_menu'] but this would involve some "not so good" code.

idflood’s picture

idflood commented
Status: Needs review » Active
StatusFileSize
new test_error1.jpg76.91 KB
new context_conflict-1139722-4.patch1.9 KB

Here is a minimalist test that should highlight the issue. I'm not sure how the testbot will handle it since I added 'context' to the setup. There is also 2 notices that appear in the Drupali18nTestCase->setUpLanguages function by just adding context to the setup function. I attached a screenshot of the relevant local test result.

edit: the cleanest solution I can think of would be to add a 'hook_navigation_links_alter' to drupal core and use it instead of hook_preprocess_page (or maybe even a hook_menu_tree_page_data_alter). But this should work with d7 so I don't think it's an acceptable solution.

idflood’s picture

idflood commented
Status: Active » Needs review
idflood’s picture

idflood commented
Category: support » bug
Status: Active » Needs review
StatusFileSize
new context_conflict-1139722-6.patch4.4 KB

Here is a first draft of patch for this issue. It make the previous test pass and don't break other i18n tests. It 'simply' alter the current $variable["xy_menu"] instead of recreating a new one.

But I'm not sure previous tests cover all 'menu' situations and this patch could be certainly optimized in many ways.

What do you think?

wapnik’s picture

wapnik commented

@Jose Reyero time for maintainers to decide http://drupal.org/node/1124544#comment-4509004

jose reyero’s picture

jose reyero commented
Category: bug » support
Status: Needs review » Closed (won't fix)

Basically, I think this is a too big patch for what would be a 2 line patch for context module.

Other concerns are spending more time to fix potential new menu issues which already took some time to debug and performance: loading menus twice, then one more query for each menu link. Note this menu_link_load() for every item bypasses every menu caching mechanism around.

And btw, the test will just fail if you don't have ctools and context modules around. Wich leads us to the last point: we are translating only 'Drupal core things' with i18n. Everything else, please post a new module.

Otherwise, the patch looks good and would be a good start for some 'i18n_context' (or context_i18n) module.

tahiticlic’s picture

tahiticlic commented

The very first fixproposed here by idflood should work, but it doesn't (it worked with D6 version, where the same problem occured).

I think this is not the i18n_menu_preprocess_page which is fired last, since a dsm just before the "return $links;" shows that active link is correctly set.