Create a menu link field type/widget/formatter

+++ b/core/modules/menu_link/menu_link.info.yml
@@ -0,0 +1,8 @@
+dependencies:
+  - field

+++ b/core/modules/menu_link/src/Plugin/Field/FieldType/MenuLinkItem.php
@@ -0,0 +1,223 @@
+    $menu_options = menu_ui_get_menus();

this depends on menu_ui but I found no field dependency.
fields are provided by core

And you can probably avoid the dependency on menu_ui, because that function is just a very simple wrapper for returning all menus, if you don't use the optional argument.

also module contains only a field so maybe better to put it inside "menu_link_content" module and update its description

Biggest concern: I'm wondering if this really belongs in a separate module, or whether it belongs in core.

Review below. Ideally, pwolanin would also review this, because I didn't work on the form aspects of menus at all during the "homestretch" patch.

1. +++ b/core/lib/Drupal/Core/Field/TypedData/FieldItemDataDefinition.php
   @@ -43,6 +43,15 @@ public static function createFromDataType($data_type) {
   +  public function getFieldDefinition() {
   

   A new method on something as deep as this — needs sign-off from fago/yched/Berdir probably?

2. +++ b/core/lib/Drupal/Core/Menu/MenuParentFormSelectorInterface.php
   @@ -38,7 +38,7 @@ public function getParentSelectOptions($id = '', array $menus = NULL);
   -   * @param string $id
   +   * @param string $exclude_id
   

   Great!

3. +++ b/core/modules/menu_link/src/Plugin/Field/FieldType/MenuLinkItem.php
   @@ -0,0 +1,241 @@
   +  protected $menuParentFormSelector;
   

   This comes with a huge tree of services it depends on. Perhaps not c'tor inject this?

4. +++ b/core/modules/menu_link/src/Plugin/Field/FieldType/MenuLinkItem.php
   @@ -0,0 +1,241 @@
   +    $settings['available_menus'] = ['main'];
   +    $settings['parent'] = ['main:'];
   

   It's not guaranteed the main menu will exist.

5. +++ b/core/modules/menu_link/src/Plugin/Field/FieldType/MenuLinkItem.php
   @@ -0,0 +1,241 @@
   +      '#description' => $this->t('The menus available to place links in for this content type.'),
   

   "content type" implies this will only be used for nodes. Seems wrong.

6. +++ b/core/modules/menu_link/src/Plugin/Field/FieldType/MenuLinkItem.php
   @@ -0,0 +1,241 @@
   +    $definitions['menu_name'] = DataDefinition::create('string')
   +      ->setLabel(t('Menu'));
   +    $definitions['parent'] = DataDefinition::create('string')
   +      ->setLabel(t('Parent menu link'));
   +    $definitions['title'] = DataDefinition::create('string')
   +      ->setLabel(t('Menu link title'));
   +    $definitions['description'] = DataDefinition::create('string')
   +      ->setLabel(t('Menu link description'));
   +    $definitions['weight'] = DataDefinition::create('integer')
   +      ->setLabel(t('Menu link weight'));
   

   Where's the actual link itself?

7. +++ b/core/modules/menu_link/src/Plugin/Field/FieldType/MenuLinkItem.php
   @@ -0,0 +1,241 @@
   +      'description' => 'The link text.',
   ...
   +      'description' => 'The menu of the link',
   ...
   +      'description' => 'The parent of the menu link',
   ...
   +      'description' => 'The weight of the menu link',
   ...
   +      'description' => 'The link description.',
   

   Consistency with the labels in propertyDefinitions()?

   - The menu link title.
   - s/link/menu link/

8. +++ b/core/modules/menu_link/src/Plugin/Field/FieldType/MenuLinkItem.php
   @@ -0,0 +1,241 @@
   +  /**
   +   * {@inheritdoc}
   +   */
   +  public function insert() {
   +    parent::insert();
   +
   +    $this->doSave();
   +  }
   +
   +  /**
   +   * {@inheritdoc}
   +   */
   +  public function update() {
   +    parent::update();
   +
   +    $this->doSave();
   +  }
   ...
   +  /**
   +   * @TODO
   +   */
   +  protected function doSave() {
   +    $plugin_id = $this->getMenuPluginId();
   +    if (!$this->menuPluginManager->hasDefinition($plugin_id)) {
   +      $this->menuPluginManager->addDefinition($plugin_id, $this->getMenuPluginDefinition());
   +    }
   +    else {
   +     $this->menuPluginManager->updateDefinition($plugin_id, $this->getMenuPluginDefinition(), FALSE);
   +    }
   +  }
   

   Why does FieldItemInterface::preSave() exist, but not ::postSave()? That would allow us to remove most of the unclarity here.

   Regarding doSave(): I think you can copy most of the code (not docblock) comments from MenuLinkContent::postSave(), which is where it looks like you found this?

9. +++ b/core/modules/menu_link/src/Plugin/Field/FieldType/MenuLinkItem.php
   @@ -0,0 +1,241 @@
   +  }
   +}
   

   Should have a newline in between.

10. +++ b/core/modules/menu_link/src/Plugin/Field/FieldWidget/MenuLinkWidget.php
    @@ -0,0 +1,200 @@
    + * Provides a widget for the menu_link_item field type.
    

    s/menu_link_item/menu_link/

11. +++ b/core/modules/menu_link/src/Plugin/Field/FieldWidget/MenuLinkWidget.php
    @@ -0,0 +1,200 @@
    +   * The menu parent selector.
    

    s/menu parent/parent menu link/

12. +++ b/core/modules/menu_link/src/Plugin/Field/FieldWidget/MenuLinkWidget.php
    @@ -0,0 +1,200 @@
    +    if ($this->account->hasPermission('administer menu')) {
    

    Let's negate this, so that the else block can come first, which is much shorter, so you're not left wondering why this is wrapped in an if-statement.

13. +++ b/core/modules/menu_link/src/Plugin/Field/FieldWidget/MenuLinkWidget.php
    @@ -0,0 +1,200 @@
    +        '#title' => t('Provide a menu link'),
    ...
    +        '#title' => t('Menu settings'),
    ...
    +        '#title' => t('Menu link title'),
    ...
    +        '#title' => t('Description'),
    ...
    +        '#description' => t('Shown when hovering over the menu link.'),
    ...
    +      $element['menu_parent']['#title'] = t('Parent item');
    ...
    +        '#title' => t('Weight'),
    ...
    +        '#description' => t('Menu links with lower weights are displayed before links with higher weights.'),
    

    $this->t()

14. +++ b/core/modules/menu_link/src/Plugin/Field/FieldWidget/MenuLinkWidget.php
    @@ -0,0 +1,200 @@
    +        '#weight' => $entity->getEntityTypeId() == 'node' ? -2 : 0,
    

    Black magic!

15. +++ b/core/modules/menu_link/src/Plugin/Field/FieldWidget/MenuLinkWidget.php
    @@ -0,0 +1,200 @@
    +  public function preRenderMenuDetails($element) {
    

    Missing docblock.

    Should explain why this must live in a #pre_render callback.

16. +++ b/core/modules/menu_link/src/Plugin/Field/FieldWidget/MenuLinkWidget.php
    @@ -0,0 +1,200 @@
    +    // Extract menu/parent from single select element.
    

    s#menu/parent#menu and parent menu link#

17. +++ b/core/modules/menu_link/src/Plugin/Menu/Form/MenuLinkFieldForm.php
    @@ -0,0 +1,44 @@
    + * Provides the menu link edit form for the field based menu link.
    
    +++ b/core/modules/menu_link/src/Plugin/Menu/MenuLinkField.php
    @@ -0,0 +1,108 @@
    + * Provides a menu link plugin for field based menu links.
    

    s/field based/field-based/

I think this is critical, since otherwise we are not going to be able to solve the WTF of content menu links referencing "objects"

I have fixed what @Wim Leers had pointed, except for:

+++ b/core/modules/menu_link/src/Plugin/Field/FieldType/MenuLinkItem.php
@@ -0,0 +1,241 @@
+    $settings['available_menus'] = ['main'];
+    $settings['parent'] = ['main:'];

4. It's not guaranteed the main menu will exist.

It seems the Main menu is always available

It did not pass some tests for "Drupal\system\Tests\Menu\BreadcrumbTest" at my localmachine, but I am sending the patch anyway because these tests were failing even in a clean git repo. I hope it works for the testbot.

Some of these cleanups could go in a separate patch to make this easier to review:

-  public function parentSelectElement($menu_parent, $id = '', array $menus = NULL);
+  public function parentSelectElement($menu_parent, $exclude_id = '', array $menus = NULL);
 

So, I feel like were taking a step backwards here:

+  public function getTitle() {
+    return $this->pluginDefinition['title'];
+  }

I'd want to check if the site is multi-lingual and try to get a translation like we do for menu link content entities?

In other words, if you translate the node this field is on, you should be able to automagically get that as the title:

  /**
   * {@inheritdoc}
   */
  public function getTitle() {
    // We only need to get the title from the actual entity if it may be a
    // translation based on the current language context. This can only happen
    // if the site is configured to be multilingual.
    if ($this->languageManager->isMultilingual()) {
      return $this->getEntity()->getTitle();
    }
    return $this->pluginDefinition['title'];
  }

Per the above - I think this field shouldn't store the title at all, it should just use the entity label.

Some really outdated test code in the breadcrumb test is seems?

Here's a first pass at fixing, but getting a recursion error in the node form.

I think taking the title from the entity is really the 90% use case and what we should do for the field. In the 10% case of needing a different title, you can make a custom menu link.

Note also that this means content translators don't need 'administer menu' permissions, which they would if they need to access the menu link field in order to translate it.

The patch applies, but I am getting some funny FieldStorageConfigStorage errors at site install so retesting

I can see ONE thorn in the lions paw.

diff --git a/core/core.services.yml b/core/core.services.yml
index a4fcc53..132483a 100644
--- a/core/core.services.yml
+++ b/core/core.services.yml
@@ -343,7 +343,7 @@ services:
     arguments: ['@menu.tree_storage', '@plugin.manager.menu.link', '@router.route_provider', '@m
   menu.default_tree_manipulators:
     class: Drupal\Core\Menu\DefaultMenuLinkTreeManipulators
-    arguments: ['@access_manager', '@current_user', '@entity.query']
+    arguments: ['@access_manager', '@current_user', '@entity.query', '@entity.manager']
   menu.active_trail:
     class: Drupal\Core\Menu\MenuActiveTrail
     arguments: ['@plugin.manager.menu.link', '@current_route_match']

Trivial nudges in the correct direction ...

DefaultMenuLinkTreeManipulatorsTest now passes.

I was hoping to get a screenshot but patch is not simplytest compatible yet :)

DefaultMenuLinkTreeManipultors::loadEntities() now passes along the tree parameter.

SystemMenuBlockTest now passes and site install now works.

I think we should pull out the entity loading changes from the menu try and try to focus just on getting this all working smoothly before optiizing.

We discussed this on our core committer D8 triage call today, and couldn't come up with a firm decision on whether this is critical, so tagging accordingly.

schema stuff

This is looking great, aside from the things below:

1. +++ b/core/modules/menu_link/config/schema/menu_link.schema.yml
   @@ -0,0 +1,26 @@
   \ No newline at end of file
   

   :)

2. +++ b/core/modules/menu_link/src/Plugin/Field/FieldType/MenuLinkItem.php
   @@ -0,0 +1,249 @@
   +    $menu_definition['metadata']['translateable'] = is_subclass_of($entity, '\Drupal\Core\TypedData\TranslatableInterface');
   
   +++ b/core/modules/menu_link/src/Plugin/Menu/MenuLinkField.php
   @@ -0,0 +1,135 @@
   +    if (!empty($this->pluginDefinition['metadata']['translateable']) && $this->languageManager->isMultilingual()) {
   

   This should be 'translatable', I think.

However I tried to manually test it and I have a few remarks:

• A Menu field should be added to the Page content type in the Standard profile to match the current UX.
• After adding it manually, I tried to translate the menu field as part of the regular node translation workflow, but I was not able to mark it translatable in the content language settings. This may be fine, because otherwise I guess we would be creating two menu items, which would be both displayed, but I was not even able to translate it through the Menu UI. Ideally we should be able to enter translated values in the widget.
• The widget exhibits some wrong behavior (see the attached screenshot).
  
• We should restore the JS behavior which automatically populates the title field with the entity label (or at least the node title) when enabling the "Provide menu link" checkbox, together with the related state magic displaying/hiding the other form elements when clicking on it.

#50 apply successfully

After applying this patch I lost the linkage between existing nodes and their menu links, eg. menu links that were previously selected in the node edit form were no longer visible in the node edit form. I don't know if an upgrade path is in scope?

In the same category, I got a notice on an existing node and the tab with 'Menu settings' did not show up at all.

I don't think we should add this to every node type - it's currently configured via the node type edit form. Adding a field is really at the same level imho, though perhaps less obvious to new admins. So, we should give them an example via the standard profile.

Also, let's kill the title-copying JS? In HEAD it's not possible to leave the title field blank, if you do no link is created. With this patch, I think blank and using the label (and it's translations) should be the default behavior.

Why would we revert that usability improvement that goes back as far as D6?

@webchick - the idea being if you leave it blank it just uses the node title (label) and hence you could also get the translated title for free.

To be clear - I do *not* want you to have to type a title, I'm trying to make it more automatic. Perhaps even have the field hidden with a checkbox that says "customize link title"

In any case, this can wait to a follow-up if it's going to extend the debate since it's UI tweaking, rather than anything fundamental.

Ok, I'm glad to hear we're not thinking of removing that UX feature. But yeah, let's discuss it more in a follow-up since ideally this one would just match the existing UX with different backend stuff.

Cannot do a full review atm, but :

+++ b/core/modules/menu_link/src/Plugin/Field/FieldWidget/MenuLinkWidget.php
@@ -0,0 +1,207 @@
+    $element['#pre_render'][] = [$this, 'preRenderMenuDetails'];
...
+  public function preRenderMenuDetails($element) {

In other widgets we've tried hard to use static methods for this kind of FAPI helpers (added in the $form as [get_class($this), 'methodName'] rather than [$this, 'methodName']). Avoids needlessly serializing the Widget object as part of the $form serialization.

1. +++ b/core/modules/menu_link/src/Plugin/Field/FieldType/MenuLinkItem.php
   @@ -0,0 +1,262 @@
   +    $this->menuPluginManager = \Drupal::service('plugin.manager.menu.link');
   +    $this->menuParentFormSelector = \Drupal::service('menu.parent_form_selector');
   +  }
   

   Can't we use ContainerFactoryPluginInterface here too?

2. +++ b/core/modules/menu_link/src/Plugin/Field/FieldType/MenuLinkItem.php
   @@ -0,0 +1,262 @@
   +    // When the entity is saved via a plugin instance, we should not call
   +    // the menu tree manager to update the definition a second time.
   

   "the" can go in the previous line, perhaps also "menu".

3. +++ b/core/modules/menu_link/src/Plugin/Field/FieldType/MenuLinkItem.php
   @@ -0,0 +1,262 @@
   +    $menu_definition['metadata']['translatable'] = is_subclass_of($entity, '\Drupal\Core\TypedData\TranslatableInterface');
   

   I'm not sure whether this check needs to be dynamic or static, but in the former case I'd use is_subclass_of($entity, '\Drupal\Core\TypedData\TranslatableInterface') && $entity->isTranslatable(), otherwise I'd simply use $entity->getEntityType()->isTranslatable().

4. +++ b/core/modules/menu_link/src/Plugin/Menu/MenuLinkField.php
   @@ -0,0 +1,186 @@
   +      /** @var \Drupal\Core\TypedData\TranslatableInterface|\Drupal\Core\Entity\EntityInterface $entity */
   +      $entity = $this->getEntity();
   +      $entity = $entity->getTranslation($this->languageManager->getCurrentLanguage(LanguageInterface::TYPE_CONTENT)->getId());
   

   Please, use $entity = $this->entityManager->getTranslationFromContext($this->getEntity()); here, so we can apply also language fallback. The TranslatableInterface hint is not needed, as EMI::getTranslationFromContext() accepts any entity.

Oh, I forgot: this works beautifully now :)

Only one remark left: we are still missing the JS behavior hiding all widget elements when the checkbox is not checked:

This is an important UX fix, because values entered in the widget text fields are silently ignored if the checkbox is not checked.

@dawehner: That is built into TypedDataInterface:

public static function createInstance($definition, $name = NULL, TraversableTypedDataInterface $parent = NULL);

It seems #63 is a browser issue: Safari 8.0.2 and Firefox 34.0.5 (on Yosemite 10.10.1) are not working, Chrome 39.0.2171.95 is.

This is definitely something that should go in before we support an upgrade path.

I tried a slightly more advanced multilingual use case: having different menus with different menu items for different languages:

1. I set up an additional "Italian main menu" whose block is shown only Italian pages
2. I configured the regular "Main menu" block to be shown only on English pages
3. I created an English node and configured its menu field item to appear in the English Main menu
4. I translated the node to Italian and configured its menu field item to appear in the Italian Main menu

Expected result: I would get two menu items, one for each translation.
Actual result: only one menu item is created and things are breaking.

Here is my DB dump if you want to skip the setup phase, codebase is 84e2b6d2 + #65.

Indeed - core doesn't allow adding one link to 2 menus. However, you should be able to make a multi-value menu link field if that's your use case?

Ok - from IRC I understand the problem better: when you are translating the link field it allows you to change the menu-parent, but the menu-parent value is shared across all translations - it's not translatable itself and so we need to hide it or otherwise clarify the UI as a 1st fix.

There are two translatability use cases:

1. I mark the menu field translatable. In this case, there would be different menu values per language entirely according to our usual pattern. If I mark an integer field translatable, different languages have totally different integers on them.

2. I don't mark my menu field translatable but my menu link content entity is translatable. For this case as per MenuLinkContent::baseFieldDefinitions() only title and description is supposed to be translatable (=== marked with setTranslatable()). The widget would need to expose these as translable IF the menu link content entity referred is translatable. I think supporting separate menu items in this widget would be overkill. If I need separate menu items per language, I would mark the field as translatable, see point 1.

Here I made this handy figure for you, see attached :) MF: menu field, HU, IT, ES: languages. Looking at the patch, you marked the menu fields translatable, which suggests you also want to support (1) in some way?

Attached patch fixes the #states on the menu widget.

@pwolanin and @dawehner pointed out on IRC that the field is not referring to external entities anymore and all menu item related data is within the field. I think in this case the file field style column synching feature is in order here. See FieldTranslationSynchronizer.php and:

/**
 * Plugin implementation of the 'image' field type.
 *
 * @FieldType(
 *   id = "image",
 *   label = @Translation("Image"),
 *   description = @Translation("This field stores the ID of an image file as an integer value."),
 *   default_widget = "image_image",
 *   default_formatter = "image",
 *   column_groups = {
 *     "file" = {
 *       "label" = @Translation("File"),
 *       "columns" = {
 *         "target_id", "width", "height"
 *       }
 *     },
 *     "alt" = {
 *       "label" = @Translation("Alt"),
 *       "translatable" = TRUE
 *     },
 *     "title" = {
 *       "label" = @Translation("Title"),
 *       "translatable" = TRUE
 *     },
 *   },
 *   list_class = "\Drupal\file\Plugin\Field\FieldType\FileFieldItemList",
 *   constraints = {"ValidReference" = {}, "ReferenceAccess" = {}}
 * )
 */
class ImageItem extends FileItem { }

You define translatable column group in column_groups which are then exposed as translatable settings:

It should be checked if the columns you may want to exempt from translatability are properly synced as well by FieldTranslationSynchronizer. I don't know enough about the column_group handling to tell if you can leave out columns and then what happens to them, but that looks like what you are looking at.

I think we should add this in a follow-up issue. We need to just get this field in as a replacement for existing functionality first.

@pwolanin: in that case you need to make the field type explicitly not support translation; otherwise people can mark the field translatable and set entirely different menu items per language ATM. If you cannot / don't want to support that for some reason, then you need to deny that on the field type level for this iteration. The current patch sets default config for the menu fields as translatable and defines the field type as supporting translation as well, so when I enable my node type for translation, the menu item is currently translatable with all its values using this patch.

I don't advocate not implementing translation support of course, but if moving forward faster is more important, then so be it. I would assume the followup to make it multilingual would need to be critical as well then.

@Gabor:

I tested the current approach and it works fine if you limit yourself to menu item localization: you can translate title and description (and even weight) and it works just fine. We can lock the menu parent select for now, so one cannot select a different menu for each translation and then fix this properly in a (major?) follow-up.

@Peter:

To determine whether the menu parent select should be locked you can use:

$form_state->getFormObject()->isDefaultFormLangcode($form_state)

Ok, after long discussion with Gábor Hojtsy, plach, and dawehner: for now we will simply disable certain widget fields when in a translation and only update the menu link definition when saving the original (untranslated) entity.

It would be better to use the functionality from \Drupal\content_translation\FieldTranslationSynchronizer but that has a bug in that it doesn't sync columns that are omitted from any column groups in the annotation. We will tackle that as a follow-up once this bug is fixed #2403455: FieldTranslationSynchronizer does not sync columns that are absent from any column_group in the annotation

ok, here's a patch that fixes a bug with the default value, solves some of the bugs around translation forms, and removes the parent field from fieldSettingsForm and from the config and schema since it wasn't being used (was a carry-over). If we want to do something like that we should override the default value widget.

Started adding a test specific to the new field.

uh, forgot some annotation

Put back the config and gave it a hopefully less confusing name.

enforced a cardinality of 1 using a trick from larowlan from comment module (though hopefully a better API coming soon)

suppressed the default value form since we don't want to use it and it was causing config schema mismatches.

Added a default menu parent select form to the field config, though that could probably use some work (need to apply some validation, or some ajax)

Looks like @idebr's changes from #79 didn't get picked up.

oh, indeed - thanks. I didn't see that fix

+++ b/core/modules/menu_link/src/Plugin/Field/FieldType/MenuLinkItem.php
@@ -23,6 +23,15 @@
+ *   column_groups = {
+ *     "title-description-weight" = {
+ *       "label" = @Translation("Title, Description, and Weight"),
+ *       "translatable" = TRUE,
+ *       "columns" = {
+ *         "title", "description", "weight"
+ *       }
+ *     },
+ *   },

There is no test coverage apparent added for this one. Am I seeing that right?

@Gabor - well, it doesn't have any effect due to the other bug, but what kind of test coverage would you want?

Tested this and this going in the right direction :).
2 remarks probably other issues

1) When going to the menu overview you can't delete the menu links defined by content. Pretty annoying.
2) I still don't see the possibility to localize to any other menu item that is not content related. Are we going to fix that in core?

So, I think we might be better off with a generic delete form instead of making one specific to the field? How about this instead? MenuLinkDeleteForm is almost identical to MenuLinkResetForm - both under menu_ui. If menu_ui module is off, we don't need a delete form for the field, since it's just managed via the entity edit form.

I'm not yet sure what's failing in the new test, since it's running a bunch of generic content translation logic.

@pwolanin: if you don't expect the column_group annotation to do anything, then why include it? It makes the appearance that this is supposed to work. I don't think its a good idea to add code that would not even do anything? (Looking at the current patch in #98 the whole field type is missing, I assume that is a mistake).

@dawehner, @aspilicious: prior to this patch at least there are three ways to translate/localize a menu item depending on where it came from: (1) menu link content entity is translated as content via their fields (2) config exposed menu items such as views created items are translated within the views configuration (3) hardcoded menu items in modules are translated as part of the software (locale module UI). All of these have core UIs. All three UIs are different. It looks to me like all of these 3 sources for menu items are kept, so those should work. However this patch adds the menu item direct on the menu plugin manager with verbatim data instead of providing a derivative class like views does (see Drupal\views\Plugin\Menu\ViewsMenuLink). If this way of adding the menu item is to be kept then the menu plugin manager will need ways to support the translation of these items. That to me looks like work additionally to making the field data itself translatable. Until those two are done, this patch will be a regression for multilingual support (which is why I said we need the followup for that critical d8 upgrade path issue or resolved here).

@dawehner: you are right, I did not see MenuLinkField::getProperty(). So that combined with how the widget is currently coded indeed makes it possible to swap title, description and weight per language. The other translation methods don't allow for translating other things either (and for the matter, none other would let you "translate" the weight).

Reading the code then, I had this concern but cannot reproduce yet:

1. You create a site with 2000 articles.
2. You enable translation for articles.
3. Now you can translate menu items for all 2000 articles (the UI works), but it will not appear actually translated until you re-save the entities.

That looked to me due to how MenuLinkItem::doSave() "caches" the translatability of the container entity's type in the menu definition (via MenuLinkItem::getMenuPluginDefinition()), but only if the original language version was saved. The widget does not care for that cache but the MenuLinkField::getProperty() does, which is what is being used for providing the item at the end.

I may not be able to reproduce this because I used the site as an admin and then I have untranslatable fields as well which are saved to the original entity, so the original entity gets saved all the time. I should produce an environment where I am testing as a translator only so the original is not saved then (assumed for now :).

Set up a scenario with a translator role/user who had access to translate but not to administer content. Cannot reproduce the suspect menu bug from #100 because the menu field did not even appear. Not sure where is the logic for deciding which fields are "administer content" level fields, but menu items can now only be translated if you also have that, which sounds like the first problem to resolve before the one I suspected in #100.

@dawehner: yeah I did not expect to need the administer menu permission for translators (and therefore providing them an avenue to change anything in any menu) to be able to translate menus. That permission is NOT needed to translate any of the other menu items that I know of. With that permission granted though, it still works and I cannot reproduce the suspected bug in #100. I think we should have an API level test to ensure that the "original is saved when the translation is saved" is happening there too, because that is pre-requisite that this works for existing entities that were created before you made the entity type translatable.

oops - this is the patch that was supposed to be on #97 - I posted the wrong patch.

+++ b/core/modules/menu_link/src/Plugin/Field/FieldType/MenuLinkItem.php
@@ -0,0 +1,295 @@
+    $menu_definition['metadata']['translatable'] = $entity->getEntityType()->isTranslatable();

+++ b/core/modules/menu_link/src/Plugin/Menu/MenuLinkField.php
@@ -0,0 +1,185 @@
+    if (!empty($this->pluginDefinition['metadata']['translatable']) && $this->languageManager->isMultilingual()) {

So a key realization is the menu definition caches the translatability of the **entity type** such as node, user, etc. This is coming from the annotation and is not configurable. Configurability is for the bundle level. That is NOT saved here.

So my concern that change of translatability would be a concern when changing configuration is moot because this is not configurable, it is on the entity type definition level.

That means that this will look up translations for entities on multilingual sites all the time even if they cannot have translations ever. Fixing that would be an optimization and is fine to be done in a followup.

Back to needs review.

re-roll to fix conflicts.

So, I'm not sure wether it's worth including the translation test right now - changing the class so it extends NodeTranslationUITest seems to pass, but I'm not sure it's giving any useful new test coverage.

Yeah the MenuLinkFieldTranslateUITest in this latest patch does not actually test translating menu items AFAIS. It has code to add permissions to editors, admins and translators to do it but no code to actually do it. If this patch strives to implement menu translation, it should test translating the supported items IMHO. Otherwise we are adding untested code.

Also as discussed the actual problem with the UI based solution to limiting what you can translate as done in the patch is that it does not solve problems in the API. There is no protection for the API level to not mess up everything. If I do this:

// Create $node and add 3 translations within $node...

// Now update the menu name and title:
$node->menu->menu_name = 'boo';
$node->menu->title = 'Boo item';
$node->save();

Then depending on what language $node was at the time, will only update one translation with a different menu name, which may or may not end up actually changing the menu name on the item given you take the value only from the original language.

Solution is to either not let menu name be modified when you update a translation (throw exceptions, more custom code on your side) or implement column_groups / FieldTranslationSynchronizer which would actually sync the updated value to all languages in the above case transparently. Of course that would require #2403455: FieldTranslationSynchronizer does not sync columns that are absent from any column_group in the annotation to be resolved. So long as that is a followup, we need to accept that this patch will be on shaky ground for field translation as the API is concerned.

Ok, do you have another example we can start from? ContentTranslationUITest seems to be doing too much - maybe we need to start from ContentTranslationTestBase?

@Gábor Hojtsy - we could implement some manual synch, but in general I think we are trying to get the UI and the field storage in place here so we could make later changes without needing schema/data changes (i.e. so we are not blocking the upgrade path).

@dawehner/@pwolanin: re #113 the current patch makes the implementation possibly problematic for API consumers, that should not stop the patch from being committed IMHO but the followup needs to be critical to resolve it AFAIS or we have a fragile API.

@pwolanin: re tests, I think extending from ContentTranslationTestBase is cleanest, you don't want all the extra tests on NodeTranslationUITest to also run in your tests the same way. Alternatively adding new test methods directly to NodeTranslationUITest is an option as well. If matching menu item support for all entity types is a concern, then you can add new test methods in ContentTranslationTestBase itself as well, but not sure menu items on comments is something you strive for :) I think we can be fairly sure if it works on nodes translation-wise, it will work on other entities. Unless there is node specific code to support translation of it which I don't think there is.

postponing this since we are having a meeting about whether to change direction

Also there's related #2235457: Use link field for shortcut entity

I believe in the mega-menu links meeting we determined that this issue is no longer a release-blocker for Drupal 8. It would be great to replace the one-off form alter stuff in node module with a field that could go onto any entity, but since we've gotten by fine in Drupal 5+ with the form alter stuff, we can ship without it.

I would say 99% we'll kick this to contrib, since any generic solution in core would be essentially interacting with an entity reference instead.

moving some generic bits of the patch to #2408275: Create a generic menu link plugin delete form in menu_ui module

We're no longer changing direction.

I think we can now discuss the final fate of this issue?

Even better :)

This would potentially solve the fact that menu links that point to nodes aren't deployable at the moment.
We're hitting issues with default_content.

I was also thinking that this field type (or something very much like it) could be created as a replacement for the way book module handles hierarchy so it could be applied to any entity type.

re-rolled for conflicts in MenuNodeTest.php and menu_ui.module in 8.1.x.

Marking NR to check tests only

No working on the failing tests yet

1) MenuLinkField - EntityManager is deprecated. plumbed in the relevant replacements EntityTypeManager and EntityRepository
2) MenuLinkItem converted a few cases of t() to $this->t()
3) Fix lots of minor nits while running the new files through phpcs.

switched changes in #133 to use EntityRepositoryInterface.

drat ... my bad t() was used in a static function.

I think I may have introduced other errors .. I will fix them up

Looking at the first failing test ... in the reroll confusion - core has changed a javascript's filename and we did not keep the patch in sync

The library definition attached in MenuLinkWidget::formElement() -- see library menu_link.form has a bad reference to "js/menu_link.js"

Is anyone familiar with changes in that regard?

Reroll and fixed #2665992: @file is not required for classes, interfaces and traits.

Some easy fixes.

I think we have to keep deprecated schema for tests.

+++ b/core/composer.json
similarity index 96%
rename from core/modules/menu_ui/menu_ui.js

rename from core/modules/menu_ui/menu_ui.js
rename to core/modules/menu_link/js/menu_link.js

This JS might need an update.

I think we have to keep deprecated schema for tests. or add it to tests somehow to pass.

Working on this; re-roll first.

The failing test in MenuLinkFieldStandardTest. It fails because no menu link item gets created when creating an article with menu item. No record in the menu_tree table. The 'Provide a menu link' checkbox is off when re-editing a node to which a menu link was added. In code: $this->menuLinkManager->hasDefinition($plugin_id); fails to return a menu link definition.

Some observations while studying the code:

1. +++ b/core/modules/menu_link/src/Plugin/Field/FieldType/MenuLinkItem.php
   @@ -0,0 +1,283 @@
   +  /**
   +   * {@inheritdoc}
   +   */
   +  public function insert() {
   +    parent::insert();
   +
   +    $this->doSave();
   +  }
   +
   

   There is no parent::insert and no ::insert in any of the inherited interfaces. Can this be removed?

2. +++ b/core/modules/menu_link/src/Plugin/Field/FieldType/MenuLinkItem.php
   @@ -0,0 +1,283 @@
   +  /**
   +   * {@inheritdoc}
   +   */
   +  public function update() {
   +    parent::update();
   +
   +    $this->doSave();
   +  }
   +
   

   The same here for ::update.