Untranslated menu items are displayed in menus

#114 does not work on Drupal 9.5.9

Here is a reroll of #114 for 10.1.0.

Removing the "Needs usability review tag" since #113 doesn't mention what needs to be reviewed.

This addresses a deprecation warning and #110.1.

I agree with #111 that it depends on the menu, but using \Drupal\Core\Language\LanguageInterface::TYPE_CONTENT is a sensible default. We probably need a follow-up to make this configurable.

I documented the currently proposed solution which is to display menu links for the current language, including links without a language. This makes it possible to support use cases like #1 by explicitly marking the links as language not applicable.

This does leave out some complex use cases where you want to rely on fallbacks, and want to also override the translation for a specific language because entities without a language cannot be translations. I think it would be fine to leave this for contrib since this seems more of an edge case.

This will result in a behavior change, but that's to be expected from a bug fix. We need to document in a release note and change record what the change is, and what is expected from the maintainers of the site.

Addressing phpcs violations

The last submitted patch, 123: 2466553-123.patch, failed testing. View results →

Added autowiring for the new service

Added autowiring for the new service

Tried testing without the patch

Enabled a 2nd language (Dutch)
Create a page in English
Translated for Dutch

Added a menu link the Main nav
Translated menu link for dutch

English link appears on english page
Dutch link appears on dutch page

Is there a step I'm missing?

Moving to NW for a change record though for the new service and interface.

Added change records.

Also tested by creating 3 menu links: 2 with a Spanish translation and 1 without.

Without the patch when I choose Spanish for the page, the 'Contact' menu link is still there

With the patch when I choose Spanish, the 'Contact' menu link is gone as expected.

Will be great to get this.

Needs work for merge request comments

We need more site builder focused Change record that details the behavior for non-developers. Basically we need to let people know that certain items in menus may no longer show based on language.

We could add explanation in this CR about to get the old behavior back through custom code. This would probably be to change the class for the new service and override filterLanguage to do nothing

Added site builder focused CR mentioned in #132.

+++ b/core/lib/Drupal/Core/Menu/LanguageMenuLinkManipulator.php
@@ -0,0 +1,57 @@
+      if ($link->link instanceof MenuLinkTranslationInterface) {
+        // If the link is translatable, but has no translation, hide it.
+        if ($link->link->isTranslatable() && !$link->link->hasTranslation($current_language)) {
+          unset($tree[$key]);
+        }

We discussed this briefly on Slack with @lauriii and @catch.

I read all my previous feedback again, and I think my feedback from previous comments are still valid.

Catch raised a good point that if you add a foreign language and start translating pages, suddenly your content menu items will be gone as they don't yet have translations. He also raised the same I did multiple times is that its hard to tell for people which items are coming from content items, views, other config, views, etc. so they likely can't predict how different items would behave with this solution.

I still think its not as simple as to punt all of those to a separate issue. The behaviour of menus would be very confusing. I also still think that punting this problem space to contrib where a complex solution or multiple complex solutions can exist is not a problem. We don't have complete solutions of other things in core either but rather focus on shared parts.

That said, @laurii raised that maybe the menu item exposure setting and behaviour should be on the content entity bundle settings then where the configurability of menu items is. I think that would be an improvement but would still have the drawbacks of very different behaviour depending on which translation system translates (or not translates) the menu item.

Pushed a commit that adds a generic hook_entity_access which checks for the new setting, and removes access to entities that are untranslated when \Drupal\language\Entity\ContentLanguageSettings::$show_untranslated_content is FALSE.

There are some challenges with this approach:

1. \Drupal\Core\Menu\DefaultMenuLinkTreeManipulators::menuLinkCheckAccess does NOT actually check if the user has access to the custom menu link, it checks if user has access to the target route. This can be confirmed by checking \Drupal\menu_link_content\MenuLinkContentAccessControlHandler::checkAccess which requires administer menu permission to view the custom menu links.
2. When checking the target route access, if the target is a node, \Drupal\node\NodeAccessControlHandler::access allows users to bypass the access control handler which makes the site building experience confusing because they would see the untranslated menu links, but the end user would not.
3. This also means that whether the menu link is visible or not, depends on if there is a translation for the target, regardless if the menu link has been translated or not, which is not the intended behavior. This would mean that if you link to a View for example, the menu link would be always visible regardless of its translation because the View would be always available in every language.
4. We could add an additional menu link tree manipulator to remove custom menu links that are untranslated when this configuration is enabled. However, there's no API to do this at the moment. See ✨ Allow MenuLinkTree manipulators to be altered Needs work and ✨ Allow MenuLinkTree manipulators to be altered Needs work where contrib maintainers are asking for this API to be added.

Based on #136, tried to push forward on a solution that implements the new API, moves the functionality to custom menu links module, and makes it configurable per-block basis. I still believe we should implement something like #136 in the long run (maybe first in contrib?), but since most of the changes in the original proposal are needed anyway, we might as well try to start with the smaller scope. 😊

The new API !4589 should address ✨ Allow MenuLinkTree manipulators to be altered Needs work and ✨ Allow MenuLinkTree manipulators to be altered Needs work too because it implements a new chained menu tree manipulator which can be called where it makes sense. I added it to all cases where the menu tree is being manipulated but we may want to revert it back to just \Drupal\system\Plugin\Block\SystemMenuBlock, and add rest of the uses in a follow-up. Main reason to do that would be that we'll have to manually construct context for each use depending on what makes sense for the use case. For example, in the case of \Drupal\system\Plugin\Block\SystemMenuBlock the MR is currently passing the block plugin instance so that the manipulator can decide how the links should be manipulated based on that.

He also raised the same I did multiple times is that its hard to tell for people which items are coming from content items, links.yml, views, other config, etc. so they likely can't predict how different items would behave with this solution.

I think this is a pre-existing problem which is made slightly worse by this issue. IMO we should have a separate issue to improve the UI to explain the difference between the different sources, and what they mean.

From a technical point of view, I think the solution is elegant and it's a desired new API (I've recently needed to override menu blocks in a project for altering manipulators).

As hide_untranslated_menu_links is FALSE by default, current behavior is kept, which is one of the concerns I had. I think this solution covers all the other concerns that Gábor raised though the years.

> He also raised the same I did multiple times is that its hard to tell for people which items are coming from content items, links.yml, views, other config, etc. so they likely can't predict how different items would behave with this solution.

This is a common issue in different translation scenarios. Is this string coming from content? Config? Interface translation?
Sadly is a drawback that comes with the power of Drupal, and I don't think it can be fixed through anything than education. The good thing is that it's a checkbox people can experiment with. Which also solves

> Catch raised a good point that if you add a foreign language and start translating pages, suddenly your content menu items will be gone as they don't yet have translations.

You can temporarely disable while you work on your content. So even if this is not perfect, I think is a great step forward to make menus + translations less of a problem for end users.

Unless we need an upgrade path to set the value, which I'm not sure of as it's the default, we can RTBC this one.

I think we could mark the 'hide_untranslated_menu_links' as nullable.

Opened a follow-up to improve the menu overview: 🐛 Distinguishing different types of menu links from each other is difficult Active .

He also raised the same I did multiple times is that its hard to tell for people which items are coming from content items, links.yml, views, other config, etc. so they likely can't predict how different items would behave with this solution.

I think this is a pre-existing problem which is made slightly worse by this issue.

OK if people strongly believe that making this worse is a good tradeoff for solving part of the problem then let's do it. It would be important that core solves this in a way that contrib can still easily swap out / override for "real" solutions that actually consider Drupal's complexity IMHO.

For example, this implementation considers this the "current language" (and it does not explain on the UI that it considers this the current language):

$current_language = $this->languageManager->getCurrentLanguage(LanguageInterface::TYPE_CONTENT)->getId();

On the other hand sites may consider their menu "interface" and thus remove items not in the interface language even if the page's content language is different from its interface language.

The differentiation between "interface blocks" and "content blocks" in terms of language support are explained in https://www.hojtsy.hu/blog/2013-jul-09/drupal-8-multilingual-tidbits-7-b... btw :)

This seems like a large enough problem that it would be acceptable to introduce the required complexity to come up with a solution that can address the problem at least in majority of the use cases. The majority use case, which is the public facing multilingual site navigation, specifically requires this to be based on \Drupal\Core\Language\LanguageInterface::TYPE_CONTENT. The great thing about the current implementation is that it's done completely in the custom menu links module, and it can be toggled off. Also, new manipulators can be introduced as required. 😊

Maybe we could change the description for extra clarity:

When selected, custom menu links without translation in the current content language are hidden.

If we're happy with the current solution in a high level, next step would be to update the change records to match the new implementation.

++ amazing stuff

I think the implementation and UI looks great, and is exactly the use case I’ve had in all multilingual websites I’ve built so far.

++ to get this in

But I would be happy as well to just get the interface and manipulator in. In that case core implementation can be done in a follow up :)

lauriii → closed merge request !4615

Updated the change records. Also added the applies method to \Drupal\Core\Menu\MenuLinkTreeManipulatorInterface so we can ensure that all implementations have a separate applies method.

Just tested on Drupal 10.1.2 and patch #125 works but the plain diff from the MR does not apply.

Not sure what's the differences are.

Which is the most updated code? path or MR?

@matthieuscarset the MR should be the most recent approach (slightly different from the approach in the patch #125). I have applied the diff from the MR to 11.x-dev without any problems.

-------------

I think that the current approach here with MenuLinkTreeManipulators is good and big advantage is this:

The great thing about the current implementation is that it's done completely in the custom menu links module, and it can be toggled off. Also, new manipulators can be introduced as required.

Similar behavior was required on most of our multilingual sites and reading the whole issue, it seems to be highly desirable feature among others too. So I think it will be great to have this possibility directly in the core and hopefully this can make its way into 10.2.

Discussed with @lauriii - we decided to tweak the new API to be clearer about the difference between the new menu tree manipulator and the existing ones. Specifically we need to always provide context for these global manipulators whereas the current manipulators eg checkAccess don't need this. This should also make it easier for contrib modules to provide new menu link tree contextual manipulators without having unintended side effects. And if they do have such side effects make them easier to fix because the API will guarantee that the context is passed in.

The MR should be ready for another review. The changes were mostly internal. From the perspective of the consumer of this API this should be a positive change because this makes the context available in more cases, and provides a deprecation that will make sure contrib also provides the context going forward. The API change to the signature of \Drupal\Core\Menu\MenuLinkTree::transform is documented in https://www.drupal.org/node/3380512 → .

Just posting an updated patch for D9, based on the idea from patch #75. We were running patch #114 for some time but started having issues when using language access and having different interface languages in combination wit users requesting non existing pages...
Patch from #75 did work for us, so I updated it with the more modern coding standards from #114.

I'm hoping to update to D10 soon and try the actual real proper fix from @laurili!

The CR https://www.drupal.org/node/3380509 → needs to be updated for the recent move to using a tag named menu_tree_contextual_manipulator plus it perhaps needs expanding to explain the motivations behind MenuLinkTreeContextualManipulatorInterface and how important it is to use applies to narrow down the possible side effects to the ones you want.

Good catch! Updated the CR 👍

I've gone through all the comments and code reviews and assigned issue credit. Hopefully got this right as there certainly are a lot of contributors interested in this one!

Reading all the comments on the issue - I think we need to address @niko-'s comment in #42 as we're still doing an unset here.

I believe I have addressed the feedback. I also made \Drupal\Core\Menu\MenuLinkTreeContextualManipulatorInterface extend \Drupal\Core\Cache\CacheableDependencyInterface because we need cacheable metadata from the manipulator itself. For example, \Drupal\menu_link_content\LanguageMenuLinkTreeManipulator needs to be cached based on the languages:language_content cache context.

I realized what I implemented in the MR is not entirely correct; the cacheable dependency should be added only when the transform has been applied on a certain menu link tree. However, I'm not sure how we can do that because the cacheable dependency is handled in \Drupal\Core\Menu\MenuLinkTree::build which doesn't have access to the context so that we could call \Drupal\Core\Menu\MenuLinkTreeContextualManipulatorInterface::applies again. Also, the return value of \Drupal\Core\Menu\MenuLinkTreeInterface::transform is \Drupal\Core\Menu\MenuLinkTreeElement[] which means there isn't a good way for us to just inject that in the return value of the transform.

The current implementation doesn't have a dependency to menu_link_content module anymore so I've moved it under system module since the whole functionality is tied to SystemMenuBlock now.

As a side note, the ->isTranslatable() check seems to fail in ContentEntityTranslation::isTranslatable() because the translatable key is populated by content_translation module in function content_translation_entity_bundle_info_alter(&$bundles).

It's perfectly valid to have multilingual menu links without content_translation module, although not very common 🤔

I tested this briefly without content_translation module, added a couple of menu links in different language and the "untranslated" links are hidden if you remove the isTranslatable() check.

Discussed #162 with @alexpott and we agreed that even though that is a pretty strange use case, we should drop the ->isTranslatable() check.

@alexpott also mentioned that it would be great if we could harden the unit tests with ->shouldNotBeCalled() e.g. $non_applicable_manipulator->getCacheContexts()->shouldNotBeCalled().

I tested this briefly without content_translation module, added a couple of menu links in different language and the "untranslated" links are hidden if you remove the isTranslatable() check.

It also seems to hide zxx and und links because hasTranslation() returns FALSE for them, as expected. One way to fix this would be to always return TRUE for locked languages in Drupal\menu_link_content\Plugin\Menu::hasTranslation():

  public function hasTranslation(string $langcode): bool {
    if ($this->getEntity()->language()->isLocked()) {
      return TRUE;
    }
    return $this->getEntity()->hasTranslation($langcode);
  }

Discussed #162 with @alexpott and we agreed that even though that is a pretty strange use case, we should drop the ->isTranslatable() check.

I incorrectly assumed earlier this would work just by removing the isTranslatable() check, but it doesn't work with other menu link plugins or Not applicable/Not specified links because they will always fail to hasTranslation() check and get removed, so we definitely need it.

After digging a bit, the root issue seems to be how ContentEntityBase::isTranslatable() works without content_translation module.

At the moment ::isTranslatable() works like this:

  public function isTranslatable() {
    // Check the bundle is translatable, the entity has a language defined, and
    // the site has more than one language.
    $bundles = $this->entityTypeBundleInfo()->getBundleInfo($this->entityTypeId);
    return !empty($bundles[$this->bundle()]['translatable']) && !$this->getUntranslated()->language()->isLocked() && $this->languageManager()->isMultilingual();
  }

So basically it checks if:

1. Entity bundle has translatable=true. The "translatable" key gets populated in content_translation_entity_bundle_info_alter for entities that has content_translation = enabled third party setting
2. The entity's original language is not "locked", so basically either "Not specified" (und) or "Not applicable" (zxx)
3. The site has more than one language enabled

Meaning it will always fail to step 1 without content_translation module.

I think there are a couple of ways to fix this, assuming we want to support this use case:

1. Fallback to entity type's translatable annotation if the bundle info has no translatable key. Something like:

  // \Drupal\Core\Entity\ContentEntityBase
  public function isTranslatable() {
    // Check the bundle is translatable, the entity has a language defined, and
    // the site has more than one language.
    $bundles = $this->entityTypeBundleInfo()->getBundleInfo($this->entityTypeId);

    if (!isset($bundles[$this->bundle()]['translatable'])) {
      $bundles[$this->bundle()]['translatable'] = $this->getEntityType()->isTranslatable();
    }
    return !empty($bundles[$this->bundle()]['translatable']) && !$this->getUntranslated()->language()->isLocked() && $this->languageManager()->isMultilingual();
  }

I committed this change earlier today and you can find the test result here: https://git.drupalcode.org/issue/drupal-2466553/-/pipelines/29526,

2. Override ::isTranslatable() in \Drupal\menu_link_content\Entity\MenuLinkContent with something similar.

Both changes might be out of the scope of this issue though.

Patch for D10 based in my D9 patch from #151. Just here to more easily migrate from D9 to D10 without having to worry about changed behaviour during the update.

Patch #166 worked in our project (D10.1.5). Thanks!

We were previously using the patch from #81 in our Drupal 9 site.
After upgrading to Drupal 10 we found that the newer patches (tried #125, #166) lead to an issue where unpublished menu translations were still being shown (and in some cases, the incorrect translation was showing).

To fix this, I've taken the `hasTranslation` function from #81 and added it to #166, see the attached interdiff for details.

Apologies if this distracts from the longer term solution that is being progressed in this post, but sharing in case it is useful to others.

Rebased to adapt to recent changes in the core.services.yml file

Rerolled the MR to a patch to work on D10.2.

fixed the phpcs checks

I'm not a huge fan of the context information provided to the tree manipulators. But I'm not sure there is an easier method. Tests are now green again.

Believe we will need an upgrade hook + tests for the hook for the schema change.

Hiding patches for clarity.

Updated #168 🐛 Untranslated menu items are displayed in menus Needs work patch to latest 10.2 - using "menu.language_tree_manipulator:filterLanguage" as I don't know how to migrate to current MR via $context which is locked to only allow SystemMenuBlock use.

Uploading a version of the 89182ecf commit, but without the tests, so that it can be applied to Drupal 10.2.x.. The MR is changing the core/tests/Drupal/KernelTests/Config/Schema/MappingTest.php file that does not exist in Drupal 10.2.

Confirm that the patch from #176 is installed correctly. Thanks!

Hi,

I'm testing in a clear copy of Drupal10.2. #175 works for me, #176 doesn't work for me.

Same thing, #175 works for me but #176 is still showing up untranslated menu items.

I can confirm that #175 works but #176 does not. We were previously using the patch from #171 and it doesn't work with core 10.2.

@programeta, @Ok4p1 and @HeikkiY the patch in #176 (and the MR) adds a "Hide untranslated custom menu links" configuration to the menu block that enables this functionality. #175 is a much simpler change that doesn't allow this to be configurable.

@sleitner merging the code for 11.x makes sense, but then you need to search for "11.0" and remove all the deprecations that were supposed to go in to Drupal 10.2, but will never be added.

#176 doesn't apply to 10.3.x anymore, so uploading a new patch for use with that.

Rebased again to see if the patch applied to 11.0.0-beta1, but it does not. Uploading a patch file to use on the 11.0.x branch.