Problem/Motivation

The creation of menu links was hard-coded at the Node API.
This feature/UI is mostly for content creators, not for site builders.

This is problematic for various reasons:

  • It adds a dependency of people using the menu_ui and menu_link_content module, even they don't have to necessarily
  • It makes it impossible for other entity types to get the same features
  • Solving that in a generic way for any entity helps to make the workflow more consistent

Proposed resolution

Separate the Menu Links as an independent module, which provides a menu link field.
Create a new field type/widget/formatter to allow the creation of menu links when a node (and any entity) is created or edited.

Remaining tasks

none

User interface changes

Added field and widget to be used at nodes and other entities.

API changes

* Addition of API to save link menu for entities.
* We no longer use menu_link_content entities for node links, but rather put them into the menu tree storage more similar to the menu links
defined in YMl files.

Beta phase evaluation

Reference: https://www.drupal.org/core/beta-changes
Issue category Task, because it is a subset of the task of #2351379: [meta] Define, then support exact use cases for link generation and storage
Issue priority Critical because it is a subset of a critical
Disruption TODO
Files: 

Comments

dawehner’s picture

FileSize
10.05 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 82,617 pass(es). View

Some initial work ... still needs for sure quite some work.

dawehner’s picture

FileSize
14.37 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 82,621 pass(es). View

Some work here and there. At least saving of them should actually work.

More work needs to be done on the menu link edit form ...

dawehner’s picture

Status: Active » Needs review
FileSize
32.7 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 75,418 pass(es), 29 fail(s), and 0 exception(s). View

The general functionality somehow exists now.

Status: Needs review » Needs work

The last submitted patch, 3: menu_link-2315773-3.patch, failed testing.

dawehner’s picture

Status: Needs work » Needs review
FileSize
32.13 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 79,487 pass(es), 29 fail(s), and 0 exception(s). View

Let's try some basic reroll.

@pwolanin
Given the few failures we might could really push this to be green.

Status: Needs review » Needs work

The last submitted patch, 5: 2315773-5.patch, failed testing.

dawehner’s picture

Status: Needs work » Needs review
Issue tags: +MenuSystemRevamp
FileSize
45.92 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 79,333 pass(es), 0 fail(s), and 1 exception(s). View
24.41 KB

There we go, everything beside one stupid failure is fixed.

Status: Needs review » Needs work

The last submitted patch, 7: 2315773-7.patch, failed testing.

andypost’s picture

+++ 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

Berdir’s picture

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.

andypost’s picture

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

dawehner’s picture

Status: Needs work » Needs review
FileSize
46.81 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 81,020 pass(es). View
2.22 KB

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.

Good idea, let's avoid the dependency.

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.

Well, its an orthogonal idea to menu_link_content, kinda the other way round actually. In menu_link_field you start with the entity and attaches a menu link to it.

There we go ...

Wim Leers’s picture

Status: Needs review » Needs work

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/

jibran queued 12: 2315773-12.patch for re-testing.

pwolanin’s picture

Priority: Major » Critical

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

Mac_Weber’s picture

Status: Needs work » Needs review
FileSize
10.88 KB
59.66 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 81,992 pass(es), 1 fail(s), and 1 exception(s). View

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.

Status: Needs review » Needs work

The last submitted patch, 16: menu_link-type_widget_formatter-2315773-16.patch, failed testing.

dawehner’s picture

It seems the Main menu is always available

It is fine to not fix, that logic was there before the patch as well.

Mac_Weber’s picture

Status: Needs work » Needs review
FileSize
49.12 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 82,152 pass(es). View
Mac_Weber’s picture

Issue summary: View changes
pwolanin’s picture

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);
 
pwolanin’s picture

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'];
  }

pwolanin’s picture

Status: Needs review » Needs work

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

pwolanin’s picture

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

pwolanin’s picture

Status: Needs work » Needs review
FileSize
46.07 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 81,934 pass(es), 2 fail(s), and 1 exception(s). View
15.21 KB

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.

Status: Needs review » Needs work

The last submitted patch, 25: 2315773-35.patch, failed testing.

dawehner’s picture

Status: Needs work » Needs review
FileSize
55.42 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 78,021 pass(es), 1,624 fail(s), and 621 exception(s). View
11.66 KB

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.

I'm sorry but I think not supporting title/description again WILL make that issue much harder than we want, given that it changes the UI
of the node form. In case you want to change the UI, just make that optional somehow or fix that later.

  1. +++ b/core/modules/menu_link/src/Plugin/Field/FieldType/MenuLinkItem.php
    @@ -90,14 +90,8 @@ public static function propertyDefinitions(FieldStorageDefinitionInterface $fiel
    -    $definitions['url'] = DataDefinition::create('string')
    -      ->setLabel(t('Menu link url'));
    

    That one was certainly wrong. Good catch!

  2. +++ b/core/modules/menu_link/src/Plugin/Menu/MenuLinkField.php
    @@ -95,14 +135,28 @@ public function updateLink(array $new_definition_values, $persist) {
    +    if (empty($this->entity)) {
    +      $entity_type_id = $this->pluginDefinition['metadata']['entity_type_id'];
    +      $entity_id = $this->pluginDefinition['metadata']['entity_id'];
    +      // Make sure the current ID is in the list, since each plugin empties
    +      // the list after calling loadMultple(). Note that the list may include
    +      // multiple IDs added earlier in each plugin's constructor.
    +      static::$entityIdsToLoad[$entity_type_id][$entity_id] = $entity_id;
    +      $entities = $this->entityManager->getStorage($entity_type_id)->loadMultiple(array_values(static::$entityIdsToLoad[$entity_type_id]));
    +      $entity = isset($entities[$entity_id]) ? $entities[$entity_id] : NULL;
    +      static::$entityIdsToLoad[$entity_type_id] = array();
    +      // Clone the entity object to avoid tampering with the static cache.
    +      $this->entity = clone $entity;
    +      if (!empty($this->pluginDefinition['metadata']['translateable'])) {
    +        $the_entity = $this->entityManager->getTranslationFromContext($this->entity);
    +        $this->entity = $the_entity;
    +      }
    +    }
    

    I am not convinced that we need to do this optimization now and here ... We could multi load also on the tree manipulation level, in case you really want it to do. The interdiff shows how it could work.

Status: Needs review » Needs work

The last submitted patch, 27: 2315773-27.patch, failed testing.

martin107’s picture

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

martin107 queued 27: 2315773-27.patch for re-testing.

The last submitted patch, 27: 2315773-27.patch, failed testing.

martin107’s picture

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']
martin107’s picture

FileSize
2.33 KB
57.75 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 78,005 pass(es), 1,683 fail(s), and 1,114 exception(s). View

Trivial nudges in the correct direction ...

DefaultMenuLinkTreeManipulatorsTest now passes.

martin107’s picture

Status: Needs work » Needs review

Status: Needs review » Needs work

The last submitted patch, 33: 2315773-33.patch, failed testing.

yoroy’s picture

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

martin107’s picture

Status: Needs work » Needs review
FileSize
57.92 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 82,366 pass(es), 1 fail(s), and 2 exception(s). View
1.52 KB

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

SystemMenuBlockTest now passes and site install now works.

Status: Needs review » Needs work

The last submitted patch, 37: 2315773-37.patch, failed testing.

pwolanin’s picture

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.

webchick’s picture

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.

dawehner’s picture

Status: Needs work » Needs review
FileSize
13.77 KB
50.35 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 82,657 pass(es), 466 fail(s), and 43 exception(s). View

Simplified stuff again.

Status: Needs review » Needs work

The last submitted patch, 41: 2315773-40.patch, failed testing.

larowlan’s picture

Status: Needs work » Needs review
Issue tags: +CriticalADay
FileSize
1.83 KB
51.59 KB

schema stuff

dawehner’s picture

FileSize
6.36 KB
45.59 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 83,277 pass(es), 1 fail(s), and 1 exception(s). View
7.64 KB
46.29 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 83,319 pass(es), 2 fail(s), and 0 exception(s). View

Removed some of the just somehow related hunks (added the node access optimization in more places)

Struggling a bit with config schema today.

The last submitted patch, 44: 2315773-43.patch, failed testing.

Status: Needs review » Needs work

The last submitted patch, 44: 2315773-43.patch, failed testing.

The last submitted patch, 43: menu-link-formatter.43.patch, failed testing.

dawehner’s picture

Status: Needs work » Needs review
FileSize
8.91 KB
48.69 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 83,329 pass(es). View

Brought back the title/description functionality to have a similar behaviour to HEAD.
This helped to fix the remaining test failures.

plach’s picture

Status: Needs review » Needs work
FileSize
135.4 KB

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.
dawehner’s picture

FileSize
10.26 KB
52.53 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 83,322 pass(es). View

HAHA, I should have tried my changes from this morning (removing - out of existing code seems not that bad, as expected).

A Menu field should be added to the Page content type in the Standard profile to match the current UX.

Good point, basically all are really good points.

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.

Changed that.

Couldn't address the other points in the meantime yet.

plach’s picture

Status: Needs work » Needs review
chanderbhushan’s picture

#50 apply successfully

idebr’s picture

FileSize
198.85 KB

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.

dawehner’s picture

FileSize
5.33 KB
56.71 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 83,378 pass(es). View

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.

There has been both a wonderful drush only based interface for making configurable fields as translatable (see tells ;)) and bugs in the existing code.
Now you can translate the actual content entites and it works as expected. Pretty neat!

Berdir and I talked a bit about that and we consised to maybe add the menu links to every node type using the same mechanism as body fields and OR using
hook_entity_base_field_info_alter().

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?

No it is not at that point in time.

pwolanin’s picture

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.

webchick’s picture

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

pwolanin’s picture

@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.

webchick’s picture

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.

dawehner’s picture

FileSize
59.03 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 83,528 pass(es). View
11.37 KB
  • Add the link to article as well
  • Make weight as well as description translatable as well.
  • A better weight to do the access checking
yched’s picture

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.

dawehner’s picture

FileSize
59.05 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 83,535 pass(es). View
1.18 KB

Fair, I don't have a problem with that.

plach’s picture

Status: Needs review » Needs work
  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.

plach’s picture

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’s picture

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

Mh, this works fine for me ... do you see some JS errors on the page, maybe?

dawehner’s picture

Status: Needs work » Needs review
FileSize
58.95 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 83,523 pass(es). View
2.57 KB

Thank you a lot for the review.

Can't we use ContainerFactoryPluginInterface here too?

Please enlighten me, but I have no idea where I could get

  public function __construct(DataDefinitionInterface $definition, $name = NULL, TypedDataInterface $parent = NULL) {

from.

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().

The second alternative sounds sane.

here, so we can apply also language fallback. The TranslatableInterface hint is not needed, as EMI::getTranslationFromContext() accepts any entity.

Ah, this is much better.

dawehner’s picture

Not sure what is going on, but it worked for me. (I can send the video if someone needs a prove)

dawehner’s picture

Not sure what is going on, but it worked for me. (I can send the video if someone needs a prove)

Berdir’s picture

@dawehner: That is built into TypedDataInterface:

public static function createInstance($definition, $name = NULL, TraversableTypedDataInterface $parent = NULL);
plach’s picture

Issue tags: +JavaScript

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.

plach’s picture

Issue tags: +D8 upgrade path

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

dawehner’s picture

That is a problem which is better solved at another day.

dawehner’s picture

Issue summary: View changes

So it turns out, unlike as #68 said, it is not built into the TypedDataInterface.

Worked a bit on the issue summary / beta evaluation.

plach’s picture

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.

plach’s picture

FileSize
747.27 KB
dawehner’s picture

Thank you for your testing @plach
Interesting idea for the usage. Note afaik HEAD also doesn't support that usecase right? It just creates one menu link entry, that is all:

-function menu_ui_node_save(EntityInterface $node) {
-  if (!empty($node->menu)) {
-    /** @var \Drupal\menu_link_content\MenuLinkContentInterface $entity */
-    $definition = $node->menu;
-    if (trim($definition['title'])) {
-      if (!empty($definition['entity_id'])) {
-        $entity = MenuLinkContent::load($definition['entity_id']);
-        $entity->enabled->value = 1;
-        $entity->title->value = trim($definition['title']);
-        $entity->description->value = trim($definition['description']);
-        $entity->menu_name->value = $definition['menu_name'];
-        $entity->parent->value = $definition['parent'];
-        $entity->weight->value = isset($definition['weight']) ? $definition['weight'] : 0;
-      }
-      else {
-        // Create a new menu_link_content entity.
-        $entity = entity_create('menu_link_content', array(
-          'title' => trim($definition['title']),
-          'description' => trim($definition['description']),
-          'route_name' => 'entity.node.canonical',
-          'route_parameters' => array('node' => $node->id()),
-          'menu_name' => $definition['menu_name'],
-          'parent' => $definition['parent'],
-          'weight' => isset($definition['weight']) ? $definition['weight'] : 0,
-          'enabled' => 1,
-          'langcode' => $node->getUntranslated()->language()->getId(),
-        ));
-      }
-      if (!$entity->save()) {
-        drupal_set_message(t('There was an error saving the menu link.'), 'error');
-      }
-    }
-  }
-}

For me it seems to be that there are two kind of usecases.

a) you want to have one menu link, with simply translates the label
b) you want to have two distinct menu links, in two different menus.

Note: A menu tree entry is always tackled to a specific menu, the translation works on runtime, when the link is rendered.

pwolanin’s picture

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?

pwolanin’s picture

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.

Gábor Hojtsy’s picture

FileSize
2.19 MB

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?

idebr’s picture

FileSize
58.95 KB
803 bytes

Attached patch fixes the #states on the menu widget.

Gábor Hojtsy’s picture

FileSize
41.98 KB

@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.

pwolanin’s picture

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.

Gábor Hojtsy’s picture

@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.

plach’s picture

@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)
pwolanin’s picture

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

pwolanin’s picture

FileSize
61.53 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Failed to run tests: failed during invocation of run-tests.sh. View
9.14 KB

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.

Status: Needs review » Needs work

The last submitted patch, 85: 2315773-85.patch, failed testing.

pwolanin’s picture

Status: Needs work » Needs review
FileSize
61.6 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 83,498 pass(es). View
501 bytes

uh, forgot some annotation

pwolanin’s picture

FileSize
65 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 83,515 pass(es). View
9.74 KB

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)

mikey_p’s picture

FileSize
65.08 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 83,612 pass(es). View
803 bytes

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

pwolanin’s picture

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

Gábor Hojtsy’s picture

+++ 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?

pwolanin’s picture

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

aspilicious’s picture

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?

dawehner’s picture

FileSize
74.47 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 83,709 pass(es), 57 fail(s), and 7 exception(s). View
11.26 KB

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?

We talked about that quite a lot and decided that we won't do that as part of this patch, given that it adds new levels of complexity. In general
though we can work on that later.

1) When going to the menu overview you can't delete the menu links defined by content. Pretty annoying.

Worked on that.

dawehner’s picture

Oh btw. I tried to write the test coverage mentioned in #91 but failed really early, so for now this patch just includes what is needed.

Status: Needs review » Needs work

The last submitted patch, 94: 2315773-94.patch, failed testing.

pwolanin’s picture

Status: Needs work » Needs review
FileSize
55.25 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 83,579 pass(es). View
12.31 KB

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.

Gábor Hojtsy’s picture

@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’s picture

+++ b/core/modules/menu_ui/src/Form/MenuLinkDeleteForm.php
@@ -0,0 +1,128 @@
+  public function linkIsDeletable(MenuLinkInterface $menu_link_plugin) {
+    return AccessResult::allowedIf($menu_link_plugin->isDeletable())->setCacheable(FALSE);
+  }

Given that each implementation is pretty much non-cacheable via that, I argue that it odesn't make sense here to return a access result and not just a boolean.

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).

Did you tried that out? We support translating the label, much like we do in HEAD. Do you think it is a critical requirement
for this patch to support moving translated menu links into a different menu? I am not sure, but I don't think HEAD supports that as well at the moment.

Gábor Hojtsy’s picture

@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 :).

Gábor Hojtsy’s picture

Status: Needs review » Needs work

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’s picture

@Gábor Hojtsy
The permission you need is 'administer menu' in case you want to able to configure that. This mimics primarily the behaviour of the custom node form in HEAD.

Gábor Hojtsy’s picture

@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.

pwolanin’s picture

FileSize
74.42 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 83,843 pass(es), 57 fail(s), and 7 exception(s). View

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

Gábor Hojtsy’s picture

Status: Needs work » Needs review
+++ 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.

dawehner’s picture

FileSize
3.78 KB

This is just an interdiff for a potential test in the future.

Status: Needs review » Needs work

The last submitted patch, 104: 2315773-97.patch, failed testing.

pwolanin’s picture

Status: Needs work » Needs review
FileSize
75.35 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 84,095 pass(es), 57 fail(s), and 7 exception(s). View

re-roll to fix conflicts.

Status: Needs review » Needs work

The last submitted patch, 108: 2315773-108.patch, failed testing.

pwolanin’s picture

Status: Needs work » Needs review
FileSize
73.89 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 84,361 pass(es). View
3.44 KB

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.

Gábor Hojtsy’s picture

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.

pwolanin’s picture

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’s picture

@Gábor Hojtsy
I ask myself wether we really need it, if we want to support different menu parents in future issues.

@pwolanin
Ah extending sounds partially helpful. If we do that, we could really easy just add some additional assertions and be done.
Otherwise we could implement an example for test entity types as well.

Gábor Hojtsy’s picture

@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.

pwolanin’s picture

Status: Needs review » Postponed

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

andypost’s picture

webchick’s picture

Priority: Critical » Major
Issue tags: -Needs D8 critical triage

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.

pwolanin’s picture

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.

pwolanin’s picture

Wim Leers’s picture

Status: Postponed » Active

We're no longer changing direction.

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

dawehner’s picture

Status: Active » Needs work

I'd even say a lot of the code still works

Wim Leers’s picture

Even better :)

larowlan’s picture

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.

dawehner’s picture

This would potentially solve the fact that menu links that point to nodes aren't deployable at the moment.

That makes it almost a bug, but well, there are other workarounds for that.

pwolanin’s picture

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.

dawehner’s picture

Pushed the patch to a contrib module http://drupal.org/project/menu_link_field without any testing.

dawehner’s picture

Version: 8.0.x-dev » 8.1.x-dev

At that point certainly a 8.1.x feature

dawehner’s picture

Rerolled the latest iteration.

pwolanin’s picture

Status: Needs work » Needs review
FileSize
69.34 KB

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

Marking NR to check tests only

Status: Needs review » Needs work

The last submitted patch, 129: 2315773-129.patch, failed testing.

dawehner’s picture

Status: Needs work » Needs review
FileSize
69.42 KB
891 bytes

Rerolled but nothing else.

Status: Needs review » Needs work

The last submitted patch, 131: 2315773-131.patch, failed testing.

martin107’s picture

Status: Needs work » Needs review
FileSize
69.77 KB
9.69 KB

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.

martin107’s picture

switched changes in #133 to use EntityRepositoryInterface.

The last submitted patch, 133: 2315773-133.patch, failed testing.

Status: Needs review » Needs work

The last submitted patch, 134: 2315773-134.patch, failed testing.

martin107’s picture

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

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

martin107’s picture

Status: Needs work » Needs review

The last submitted patch, 128: 2315773-128.patch, failed testing.

Status: Needs review » Needs work

The last submitted patch, 137: 2315773-137.patch, failed testing.

martin107’s picture

Assigned: martin107 » Unassigned

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?

dawehner’s picture

Status: Needs work » Needs review
FileSize
70.22 KB

Just a reroll for now.

Status: Needs review » Needs work

The last submitted patch, 142: 2315773-141.patch, failed testing.

Version: 8.1.x-dev » 8.2.x-dev

Drupal 8.1.0-beta1 was released on March 2, 2016, which means new developments and disruptive changes should now be targeted against the 8.2.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

The last submitted patch, 142: 2315773-141.patch, failed testing.

jibran’s picture

Status: Needs work » Needs review
FileSize
3.93 KB
69.6 KB

Status: Needs review » Needs work

The last submitted patch, 146: create_a_menu_link-2315773-146.patch, failed testing.

jibran’s picture

Status: Needs work » Needs review
FileSize
1.98 KB
71.04 KB

Some easy fixes.

dawehner’s picture

@jibran++

Status: Needs review » Needs work

The last submitted patch, 148: create_a_menu_link-2315773-148.patch, failed testing.

jibran’s picture

Status: Needs work » Needs review
FileSize
714 bytes
70.34 KB

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.

jibran’s picture

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

Status: Needs review » Needs work

The last submitted patch, 151: create_a_menu_link-2315773-151.patch, failed testing.

The last submitted patch, 151: create_a_menu_link-2315773-151.patch, failed testing.

Version: 8.2.x-dev » 8.3.x-dev

Drupal 8.2.0-beta1 was released on August 3, 2016, which means new developments and disruptive changes should now be targeted against the 8.3.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.3.x-dev » 8.4.x-dev

Drupal 8.3.0-alpha1 will be released the week of January 30, 2017, which means new developments and disruptive changes should now be targeted against the 8.4.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Sutharsan’s picture

Working on this; re-roll first.

Sutharsan’s picture

Gábor Hojtsy’s picture

Issue tags: +SprintWeekend2017
Sutharsan’s picture

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.