Menu match by current content language not supported [#2865800]

Menu for 'currently used language' should be used, otherwise one cannot control what language will be set in breadcrumbs.

CASE:
- two enabled languages: default (EN), and non-default (non-EN)
- two menus for nodes: first set for default language (EN), second set for non-default language (non-EN)
- menu breadcrumb configured to use both above menus

PROBLEM:
- menu breadcrumb will use first found menu no matter what language is set

RESULT (Invalid):
- people displaying page in non-default language (non-EN) will see default language (EN) breadcrumbs
- changing menus' order in 'menu breadcrumb' will cause reversed situation: people displaying page in default language (EN) will see non-default (non-EN) breadcrumbs

My temporary solution:

  public function applies(RouteMatchInterface $route_match) {
        //...//
        foreach ($menus as $menu_name => $params) {
            // Look for current path on any enabled menu.
            if (!empty($params['enabled'])) {

                // --- tmp fix below --- //
                $menuObj = \Drupal::entityTypeManager()
                        ->getStorage('menu')
                        ->loadByProperties([ 'id' => $menu_name,]);

                if ($menuObj) {
                    $menuItem = reset($menuObj);
                }

                $menuLangId = $menuItem->language()->getId();
                $currentLanguage = \Drupal::languageManager()->getCurrentLanguage()->getId();

                if ($menuLangId != $currentLanguage) {
                    continue;
                }
                // --- end tmp fix --- //

                $trail_ids = $this->menuActiveTrail->getActiveTrailIds($menu_name);
                //...///
            }
        }
    }

Comment	File	Size	Author
#4	2865800-menu-match-language.patch	2.65 KB	rphair

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Comment #1

31 March 2017 at 14:48

pawel_r created an issue. See original summary.

Comment #2

rphair CreditAttribution: rphair at COSD commented 31 March 2017 at 15:37

Title:	Node translation not supported	» Menu match by current content language not supported
Assigned:	Unassigned	» rphair
Parent issue:		» #2840178: Need ability to only use menus with langcode matching current language

thanks @pawel_r, this was reported earlier in https://www.drupal.org/node/2840178 but I wasn't sure how to deal with it at the time. The logic above looks straightforward enough, except that your code uses the current interface language, though issues since then (2827786, 2855692) confirm that it should match the current content language.

I'm pretty sure that menu language comparison will also disqualify "taxonomy attachment" for the same menu in the code that follows. Just need to test this code is the best way of doing it, please stand by....

Comment #3

rphair CreditAttribution: rphair at COSD commented 31 March 2017 at 18:09

Parent issue:	#2840178: Need ability to only use menus with langcode matching current language	»
Related issues:		+#2840178: Need ability to only use menus with langcode matching current language

oops...

Comment #4

rphair CreditAttribution: rphair at COSD commented 1 May 2017 at 16:31

Status:

Active

» Needs review

File	Size
2865800-menu-match-language.patch	2.65 KB

Comment #5

rphair CreditAttribution: rphair at COSD commented 1 May 2017 at 16:35

patch relative to dev version, also checking it into dev now. Note that logic is changed a bit... please let me know how it's working in this case, if all OK will bring into new version 1.1 (comments about this welcome).

Comment #6

1 May 2017 at 16:37

rphair committed 931a062 on 8.x-1.x

Issue #2865800 by rphair, pawel_r: Menu match by current content...

Comment #7

pawel_r CreditAttribution: pawel_r as a volunteer commented 5 May 2017 at 09:27

I've tried DEV and it's working fine.

Logic is the same, yours code is nicer (when doing such 'fixes' I do prefer having just one place in code, it's easier to managed during updates etc. )

Comment #8

rphair CreditAttribution: rphair at COSD commented 5 May 2017 at 11:18

Status:

Needs review

» Fixed

thanks for testing @pawel_r: marked current dev as version 8.x-1.1.

Comment #9

pawel_r CreditAttribution: pawel_r as a volunteer commented 5 May 2017 at 16:44

thx!

Comment #10

weri CreditAttribution: weri at Previon Plus AG commented 19 May 2017 at 13:55

Status:

Fixed

» Needs work

I have to reopen this issue, because it does not work correctly with translated menus.

We have a menu defined as German (DE) for the default language and do translate the menu and menu items to French (FR). In this case we only have the correct breadcrumbs on the German page, but not on the French, because the main menu is filtered out in this case.

// Skip over any menu that's not in the current content language.
// TODO This also skips checking taxonomy attachment for that menu: OK?
// (see https://www.drupal.org/node/2840178#comment-11851137)
$menu_objects = $this->entityTypeManager->getStorage('menu')
  ->loadByProperties(['id' => $menu_name]);
if ($menu_objects) {
  $menu_language = reset($menu_objects)->language()->getId();
  if ($menu_language != $this->contentLanguage) {
    continue;
  }
}

I think we can not do

$menu_language != $this->contentLanguage

for all cases. Especially when only one menu is used for multiple languages.

I think there is not an easy solution to implement that generic for all situations, but It's possible to allow an additional configuration like "We have a menu per language" or so.

What did you mean?

Currently I switched back to version 8.1.0.

Comment #11

rphair CreditAttribution: rphair commented 19 May 2017 at 17:29

OK, what about implementing a third checkbox for each menu, about whether to implement this patched code? We'd have to answer these questions first:

How it could be described on the configuration page so people understand it? (e.g.: "Skip menu when not matching content language")
What's a "short name" for that check-box that would be familiar to people? (a column heading after ENABLED and TAXONOMY ATTACHMENT)
What should be the default? (depending upon which case is more common: the original posting, or comment #10)

Comment #12

weri CreditAttribution: weri at Previon Plus AG commented 19 May 2017 at 19:01

Implementing a third checkbox for each menu sounds good and straightforward.

Something like that: "Skip menu, when the defined menu language does not match the content language (recommended setting when you use a separate menu per language)."=
"Language handling"
The checkbox as named in "1." should not be checked by default. Because with version 8.1.0 version no language matching was implemented. When we have this additional configuration, nothing happens when I just upgrade from 8.1.0.

Comment #13

rphair CreditAttribution: rphair commented 22 May 2017 at 20:37

Agreed, have opened up another issue to track this as a feature request (https://www.drupal.org/node/2880451). Was hoping to push some code for this out to dev tonight but missed my window, please stay tuned & will have something there for you to test soon....

Comment #14

rphair CreditAttribution: rphair commented 27 May 2017 at 22:36

Status:

Needs work

» Needs review

dear followers of this issue, please try this patch (https://www.drupal.org/node/2880451#comment-12105533), now also in the dev version, to be sure it accommodates both preferences. As soon as this is confirmed by the community I would like release this as version 1.2 to reconcile the two use cases above & allow everyone to upgrade to the same version.

Comment #15

rphair CreditAttribution: rphair at COSD commented 5 June 2017 at 09:19

Status:

Needs review

» Fixed

Confirmed working by user community: https://www.drupal.org/node/2883391#comment-12115404 ...
Pushing out the dev version that reconciles this issue as 8.x-1.2.