Invalid argument exception when changing language of node with menu link to und or zxx

I'm not convinced that the API change proposed in #7 is in scope here. Adding such a method would constitute a b/c break and would delay this bug fix until Drupal 11, which I don't think is reasonable. If we want to add such a method, this could easily be done in a separate issue later.

Attached are updated patches for Drupal 10.1.x-dev and an interdiff with #6. I renamed the test class to MenuUiContentTranslationTest so that other integration tests with content_language could be added there in the future, if needed. I also replaced the two test methods with a data provider. The rest of the changes in the test file are for code that was deprecated in Drupal 8 or Drupal 9.

cspell again...

The last submitted patch, 16: 2904899-16-tests-only.patch, failed testing. View results →

Can the issue summary be updated. to include proposed solution.

Also the steps were written for D8 are they still the same for D10? Fine if they are just want to double check.

Thanks once more for your review @smustgrave!

The steps to reproduce still apply for the latest 10.1.x-dev, which is also confirmed by the new test coverage. I still did a new manual test following the STR in the IS and updated the core version, a few UI strings that changed slightly since 8.4.x and also updated the stack trace in the actual result. I also updated the IS with the missing info from the current issue summary template including a proposed resolution.

Thanks! That made it much more clear and was able to replicate.

Patch fixes the issue.

The last submitted patch, 16: 2904899-16.patch, failed testing. View results →

Meant to cleanup tags.

Also random failure.

The last submitted patch, 16: 2904899-16.patch, failed testing. View results →

Random test failure, see 🐛 [random test failure] InstallerExistingConfig[SyncDirectory]MultilingualTest::testConfigSync Fixed , retest was green. Back to RTBC per #20.

Doing RTBC triage. Thank you all for an up to date Issue Summary, it truly helps.

Reading the proposed resolution I wonder why this just being applied when saving a menu_link in the UI. Surely this can be done outside of the UI? And thus things will break? Am I not understanding something?

I think a followup is needed for #7, but as always, search first. Adding tag.

I do not see any comments here explaining what was done for a code review. And it is still listed as a remaining task.

Setting to NW because of my question above.

Reading the proposed resolution I wonder why this just being applied when saving a menu_link in the UI. Surely this can be done outside of the UI? And thus things will break? Am I not understanding something?

This is not an API issue. The issue is the way the Menu UI module uses the API to update a menu link. It tries to create a translation when it shouldn't. I tried to rephrase the proposed resolution so that this is hopefully more clear. Feel free to improve on that.

I think a followup is needed for #7, but as always, search first. Adding tag.

Filed ✨ Add an API method to LanguageInterface to simply checking, if a language is a pseudo language Closed: works as designed . If this needs to be postponed against a Drupal 11 meta or any tags or something, I'd appreciate your guidance.

Didn't do the review, so can't comment on that. Back to Needs review.

Follow up has been filed. Not sure if it should be postponed on this but a user has already started work on it.

#20 did the steps in the IS to verify the issue and the patch solved the issue.

+++ b/core/modules/menu_ui/menu_ui.module
@@ -95,7 +96,16 @@ function _menu_ui_node_save(NodeInterface $node, array $values) {
+        if (!in_array($node->language()->getId(), [
+          LanguageInterface::LANGCODE_NOT_APPLICABLE,
+          LanguageInterface::LANGCODE_NOT_SPECIFIED,
+        ], TRUE)) {

This can be simpler. Looking at \Drupal\Core\Entity\ContentEntityBase::isTranslatable() - we can change this to
if (!$node->language()->isLocked()) {

See

  /**
   * Locked indicates a language used by the system, not an actual language.
   *
   * Examples of locked languages are, LANGCODE_NOT_SPECIFIED, und, and
   * LANGCODE_NOT_APPLICABLE, zxx, which are usually shown in language selects
   * but hidden in places like the Language configuration and cannot be deleted.
   *
   * @var bool
   */
  protected $locked = FALSE;

But I think the following change is even better...

  if (!empty($values['entity_id'])) {
    $entity = MenuLinkContent::load($values['entity_id']);
    if ($entity->isTranslatable() && $node->isTranslatable()) {
      if (!$entity->hasTranslation($node->language()->getId())) {
        $entity = $entity->addTranslation($node->language()->getId(), $entity->toArray());
      }
      else {
        $entity = $entity->getTranslation($node->language()->getId());
      }
    }
    else {
      // Ensure the entity matches the node language.
      $entity = $entity->getUntranslated();
      $entity->set($entity->getEntityType()->getKey('langcode'), $node->language()->getId());
    }
  }

As it is using $node->isTranslatable() so we're a bit more logical. Ie. we say if we can translate this menu link and this node then make the languages match. If not then the menu_link must match the language of the node.

Thanks @alexpott for the review.

W/r/t pointing out LanguageInterface::isLocked(), I closed the follow-up ✨ Add an API method to LanguageInterface to simply checking, if a language is a pseudo language Closed: works as designed issue as "Works as designed".

I agree that the changes you suggested are shorter, more logical and more readable, so I updated the code.

I took the opportunity to switch to an MR based workflow. Added the MR under review to the IS as requested elsewhere by @xjm.

Doh, forgot to set to Needs review...

I should have updated the proposed resolution in the IS also, done that now.

Appears feedback has been addressed

Thanks for updating the issue summary :)

Committed and pushed ff1090382f0 to 11.x and 91acedade93 to 10.2.x. Thanks!

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