Language prefix for custom menu link paths are saved but not used

Interesting, yes, I'd expected the same behaviour

I agree that you should be able to create menu items to pages in specific languages. At least you should definitely be able to link whatever content on your site with absolute links, those would not be resolved as internal and not converted like this. As a workaround to begin with.

As for why this resolves to the front page, if you use a /content/my-first-article path, that would also resolve to its proper internal link. Resolving language is one of the processing stages happening, so not sure how should we ignore it when processing the link from the menu item. In my view it makes more sense to try to get the language from the link and pass it over. I don't have enough knowledge about the menu system to be able to tell if your patch fixes the right place the right way, but it makes sense looking at it in isolation at least :)

+++ b/core/modules/link/src/Plugin/Field/FieldType/LinkItem.php
@@ -170,7 +170,11 @@ public static function mainPropertyName() {
+    $link->setOption('language',
+      \Drupal::languageManager()->getLanguage($this->getLangcode())
+    );

Not sure how is this expected to work. What would ensure that the parent's langcode is the langcode of this URL?

@eiriksm: I am not sure you are on the right track, the language codes are absolutely not necessarily the path prefixes and there also may be any other number of path prefixes, suffixes, eg. you might have a language negotiation provider that works off of a path suffix (which I've seen at a client for example). Assuming position and content of a language path prefix at such a general place as the Url class sounds like way too many assumptions.

Comparing the three proposed variants, your first patch still looks like the sanest to me if it works :)

Updated the issue summary with easier steps to reproduce, and a problem with the expectation is apparent to me. If you create a link to /node and the default path prefix is empty then should it always stick to your site default language? Then how is it possible to create menu items that are NOT tied to a specific language? Would that only be possible if all languages have a prefix? That sounds like a problem. So it looks to me that this needs to be somehow configurable other than in the path itself.

Hi !
Actually using /node I would expect to be moved to homepage in the langue of the last seen page, because going to admin/config/regional/language/detection you will see that this is as expected :
1- Move to /no/node this is your translated homepage
2- Move to /node by clicking on the link. Because of detection you will

• URL : check the language to point to from URL. It is not, so moving to next detection
• Session: because you were on the no page, I guess your session is now somehow linked to this language

Am I the only one that think this is expected ?

However, trying with specific language links: having three links to enabled languages : /ar/node, /fr/node, /en/node moving from one to the other does not work properly, both with and without patch #5.

@eiriksm: each menu item has their own language, you don't need content translation module to have language on menu items; I removed the content translation module step from the steps to reproduce because it complicates it unnecessarily;

The menu item language was/is supposed to specify the language in which you specified the title and the description. It is still possible that you may translate the menu item or the same title may lead to a page in another language, eg. a menu item "Drupal" even if untranslated would lead to pages in other languages if a different language is used. How should the language configured for the menu item enforce anything about what language is used to display the menu item? (Each menu item entity has a language itself).

@eiriksm: If it is not possible to save a menu link with language metadata and the menu link is resolved without language, then it should throw an error AFAIS.

When you save it. Not worth bothering site users with errors IMHO :)

Let's retitle this. I'll try to get a menu maintainer on this, so we know what they expected instead of wondering around different approaches that all have some legitimacy on their own.

So, I would agree that we should be storing the language option and using it to render the link if the user enters an explicit valid prefix.

However, the code in #10 looks a bit too crude.

The bug seems to rather be in PathValidator::getUrlIfValidWithoutAccessCheck() and/or in \Drupal\language\Plugin\LanguageNegotiation\LanguageNegotiationUrl which is detecting a language prefix but not setting anything onto the request.

It's not clear how Drupal actually sets the current page language from that.

@pwolanin: well, there may be many ways a link might have language, it may be domain based or a suffix in the URL (with a contrib language negotiation plugin), etc.

One idea above was to inherit the language of the item for generating the URL. I think that has limited use because the language of the item is supposed to tell us what language was used to input the label/description of the menu item. It should be possible to create an English menu item to node/1 and have it translate to other languages with its title and description and link to /fr/node/1, /de/node/1, etc. So whether we want to force the link language vs. whether we want to just define the language of the label/description look to be two different things.

And indeed, the menu item has a langcode on its own, while the link has an options array stored which lets us specify explicit language for the link if desired. So currently the problem is even if we attempt to specify that in the input (with path prefix), that is saved but is not actually used. One potential answer is menu items are not supposed to be possible to be forced to a language unless a full URL is specified, which would not be very deployment/staging environment friendly (to hardcode live links). Another potential answer is we should have a separate UI element to force it. And finally an answer could be to let it be specified at least with a prefix/suffix/whatever the language negotiators support and then adhered.

So, I think this is really a feature request.

The inbound path processor will strip off a valid language and find the route in a language independent way.

This doesn't seem like a regression from Drupal 7

Adding options to select the language to the menu link edit form sounds like a new feature that is out of scope for beta.

If anything is in scope for 8.0.x, it would be adding a validation error if the path includes a language prefix, or just a DSM saying it will not be used.

Let's at least fix the bug? The title I gave was a perfect explanation of the bug... The prefix is saved but then not used. We can resolve the bug without introducing a feature. (We could also resolve the bug with introducing a checkbox or something along the lines to force the language, but I can see how that is 8.1.x if the menu maintainers did not intend to support that in 8.0.0).

@pwolanin: what do you think of #25 for validation?

In 8, indeed, we allow you to enter a not-yet or maybe-never valid path. That is an important feature compared to 7.

#25 seems ok as a start, though the error message is not very helpful. Is language prefix the only case we care about?

Summarized last agreements in IS.

In 8, indeed, we allow you to enter a not-yet or maybe-never valid path. That is an important feature compared to 7.

All right, this makes it pretty hard then to figure this out, no? I mean the current validation attempts to resolve the path and if it was not-yet or maybe never valid, then those paths would not be allowed according to this validation, no? Or would Url::fromUserInput() allow and return nonexistent paths?

Actually, there is some weird behavior on the not-yet valid path. If I create a menu link linking to node/2 and it does not exist yet, and I create it later, the menu item is not marked as active. Editing the menu link and saving again fixes it.

Tested with patch and without the patch attached, and the behavior in both cases for adding a link to /es/node/1 is the same, the link is created, but it does not switch the language but point to the current language instead.

It passes for me locally too. jthorson had a look at the testbot machine at NYCCamp and we could not find anything obvious about the difference.

One difference between de new CI bots, my local env and the "old" QA bots is that the latter use directories in the installation. Tried to run the tests doing the same locally, and they passed. So this was not the culprit.

One substantial review item, one minor:

1. +++ b/core/modules/menu_link_content/src/Form/MenuLinkContentForm.php
   @@ -91,6 +100,49 @@ public function form(array $form, FormStateInterface $form_state) {
   +    // other errors, because to try to construct a URL from invalid input throws
   +    // an exception, and the user will already be notified from the parent
   +    // validation.
   +    if (!$form_state->hasAnyErrors()) {
   

   We should check if *this* element had an error already no? If some other element had an error (eg. you forgot to fill in a required field), this error is not supposed to be "kept secret" IMHO.

2. +++ b/core/modules/menu_link_content/src/Form/MenuLinkContentForm.php
   @@ -91,6 +100,49 @@ public function form(array $form, FormStateInterface $form_state) {
   +        // changing this URL to something else. For example if a user is entering
   

   More than 80 chars

One substantial review item, one minor:

1. +++ b/core/modules/menu_link_content/src/Form/MenuLinkContentForm.php
   @@ -91,6 +100,49 @@ public function form(array $form, FormStateInterface $form_state) {
   +    // other errors, because to try to construct a URL from invalid input throws
   +    // an exception, and the user will already be notified from the parent
   +    // validation.
   +    if (!$form_state->hasAnyErrors()) {
   

   We should check if *this* element had an error already no? If some other element had an error (eg. you forgot to fill in a required field), this error is not supposed to be "kept secret" IMHO.

2. +++ b/core/modules/menu_link_content/src/Form/MenuLinkContentForm.php
   @@ -91,6 +100,49 @@ public function form(array $form, FormStateInterface $form_state) {
   +        // changing this URL to something else. For example if a user is entering
   

   More than 80 chars

I think global $base_path is probably deprecated and let's just check for <front> and replace it instead of using str_replace()

It seems like there will be a lot of possible failures if there is a base path? I'm surprised the rest aren't failing.

Reviewed:

1. +++ b/core/modules/menu_link_content/src/Form/MenuLinkContentForm.php
   @@ -28,6 +29,13 @@ class MenuLinkContentForm extends ContentEntityForm {
   +   * The language manager.
   +   *
   +   * @var \Drupal\Core\Language\LanguageManagerInterface
   +   */
   +  protected $language_manager;
   +
   +  /**
       * The parent form selector service.
       *
       * @var \Drupal\Core\Menu\MenuParentFormSelectorInterface
   @@ -57,6 +65,7 @@ public function __construct(EntityManagerInterface $entity_manager, MenuParentFo
   
   @@ -57,6 +65,7 @@ public function __construct(EntityManagerInterface $entity_manager, MenuParentFo
        parent::__construct($entity_manager, $language_manager);
        $this->menuParentSelector = $menu_parent_selector;
        $this->pathValidator = $path_validator;
   +    $this->language_manager = $language_manager;
      }
   

   So these looked strange to me, because we don't change the constructor, but we save the language manager. So we already got it but did not save it. We did invoke the parent constructor with it. However, looking at ContentEntityForm its constructor does not actually take a language manager. So we should not pass it to the parent. Otherwise it looks odd, because one would assume the parent takes care of keeping it in a property, but we are adding it on here.

2. +++ b/core/modules/menu_link_content/src/Form/MenuLinkContentForm.php
   @@ -91,6 +100,58 @@ public function form(array $form, FormStateInterface $form_state) {
   +      $link_url = Url::fromUri($link_uri, [
   +        'language' => $language,
   +      ]);
   ...
   +        $user_url = $this->pathValidator
   +          ->getUrlIfValid($user_link);
   

   These should go on the same line IMHO, they fit. Easier to review.

Looks good to me now, thanks for addressing my complaints and working with @pwolanin :)

1. +++ b/core/modules/menu_link_content/src/Form/MenuLinkContentForm.php
   @@ -91,6 +100,57 @@ public function form(array $form, FormStateInterface $form_state) {
   +    // marked as having an error (for example an invalid link).
   +    if (!$form_state->getError($form['link']['widget'][0]['uri'])) {
   +      $link_uri = $form_state->getValue('link')[0]['uri'];
   +      $language = $this->language_manager->getLanguage($form_state->getValue('langcode')[0]['value']);
   +      $link_url = Url::fromUri($link_uri, [
   +        'language' => $language,
   +      ]);
   

   Do we not have a way to find these without hardcoding the array structure? Also is this really specific to menu links?

2. +++ b/core/modules/menu_link_content/src/Form/MenuLinkContentForm.php
   @@ -91,6 +100,57 @@ public function form(array $form, FormStateInterface $form_state) {
   +        // Find the user input and validate that no URL negotiator will end up
   +        // changing this URL to something else. For example if a user is
   +        // entering a URL with a language prefix.
   

   Why throw an error and not try to resolve it as an inbound path and use that? Or at least show that as a suggestion?

3. +++ b/core/modules/menu_link_content/src/Form/MenuLinkContentForm.php
   @@ -91,6 +100,57 @@ public function form(array $form, FormStateInterface $form_state) {
   +          // Compare the generated link from what Drupal thinks this is. We want
   

   Compare ... with what

4. +++ b/core/modules/menu_link_content/src/Tests/MenuLinkContentTranslationUITest.php
   @@ -100,6 +100,22 @@ function testTranslationLinkTheme() {
   +      // Link to the italian front page. Should not be allowed.
   +      'link[0][uri]' => '/it/admin/structure/menu',
   +    );
   

   Comment says Italian front page, code is menu admin.

Re @catch we could make the validation a form element specific validator in which case we don't need to dig into the array structure. We may still keep it encapsulated in the class in that case. @eiriksm wanna give that a try?

+++ b/core/modules/menu_link_content/src/Form/MenuLinkContentForm.php
@@ -91,6 +100,59 @@ public function form(array $form, FormStateInterface $form_state) {
+            $form_state->setErrorByName('link', $this->t('The link you entered is not valid. One reason can be that you entered a link with a language prefix. Did you for example mean to link to @url?', [

+++ b/core/modules/menu_link_content/src/Tests/MenuLinkContentTranslationUITest.php
@@ -100,6 +100,22 @@ function testTranslationLinkTheme() {
+    $this->assertRaw(t('The link you entered is not valid'));

The t() of the shorter string may very well not end up being the first part of the t() of the longer string. We should IMHO assert for the full t()-ed string. If you don't want to do that for some reason, we should not t() this short string.

@eiriksm: yes that looks like a different issue to me. Let's not change unrelated things, this one is complicated enough in itself :)

+++ b/core/modules/shortcut/src/Tests/ShortcutLinksTest.php
@@ -54,7 +54,6 @@ public function testShortcutLinkAdd() {
-      '/' . $path['alias'],

Reverted this line.

Given #2636424: Avoid testing for relative link starting with // in shortcut module landing and the prior concerns addressed since the last RTBC, looks good again :)

What about an upgrade path - if you have existing links now they can not be saved once the patch is applied? Also don't we need some help text in the widget description to tell users before they press submit?

Working on it on NewOrleans2016 sprint.

Re-rolled.

Patch no longer applies. Re-rolled.

.

Patch from #118 works not correct for me

Here's a re-rolled version, now for 8.7.x. It's true that it still appears to need some work generally though.

Time for another re-roll.

Added patch for Drupal 9.2.x

Fixed some CS issues.

Please ignore patch #131.
Fixed some CS issues.

Trying to fix failed tests.

fixed the test cases.

Needed a re-roll. I imagine the tests will still fail in the same way, but at least this one will apply again.

This came up as a bug smash daily triage target.

Some observations:

• The validation is now happening at the form level, there are other ways to create menu links (custom code, migrations) but we are not protecting against that
• The upgrade path is only fixing the menu items at the time of running the update, nothing prevents adding more invalid links. Wouldn't an entity level constraint be better at protecting us here?
• None of this needs to happen is there are not multiple langagues, but I don't see anything checking for that right?
• The test coverage seems very slim for the proposed changes.

Just re-rolling to apply to 9.5.x for now. But yes, @Lendude, those look like pretty valid points to address. Thanks for taking the time to review!

Here's a trivially re-rolled patch for 10.2.x

Re-rolled for 10.3. (I don't expect this to get into core any more; it's just most useful for sites needing to upgrade that have been using the previous patches here.)