Problem

The 'bundle' property of menu links should be derived from the 'menu_name' property. On load this happens, but not when creating a new bundle. In that case, the class default of 'tools' is not overridden.

<?php
$menu_link
= entity_create(array('menu_name' => 'foo'));
print
$menu_link->bundle; // This should be 'foo', but is actually 'tools'.
?>

Related Issues

See also #1966298-91: Introduce menu link bundles per menus and #115-117

Files: 
CommentFileSizeAuthor
#17 drupal-2014617-menu-wrong-bundle-9.patch1.93 KBtstoeckler
PASSED: [[SimpleTest]]: [MySQL] 57,351 pass(es).
[ View ]
#17 interdiff-8-9.txt935 byteststoeckler
#10 drupal-2014617-menu-wrong-bundle-10-test-only.patch998 byteskfritsche
FAILED: [[SimpleTest]]: [MySQL] 55,108 pass(es), 2 fail(s), and 0 exception(s).
[ View ]
#8 interdif-6-8.txt1008 byteskfritsche
#8 drupal-2014617-menu-wrong-bundle-8.patch1.98 KBkfritsche
PASSED: [[SimpleTest]]: [MySQL] 55,536 pass(es).
[ View ]
#6 drupal-2014617-menu-wrong-bundle-6.patch1.81 KBYesCT
FAILED: [[SimpleTest]]: [MySQL] 54,557 pass(es), 35 fail(s), and 14 exception(s).
[ View ]
#6 interdiff-5-6.txt648 bytesYesCT
#5 drupal-2014617-menu-wrong-bundle-5.patch1.82 KBkfritsche
PASSED: [[SimpleTest]]: [MySQL] 56,705 pass(es).
[ View ]

Comments

This is not strictly related to D8MI, but sounds like makes #1945226: Add language selector on menus impossible to do.

YesCT says not impossible, but it would be important to fix to avoid workarounds nonetheless.

Status:Active» Needs review
StatusFileSize
new1.82 KB
PASSED: [[SimpleTest]]: [MySQL] 56,705 pass(es).
[ View ]

Initial patch for this, including one test.

This just sets bundle to menu_name on create, if set.

StatusFileSize
new648 bytes
new1.81 KB
FAILED: [[SimpleTest]]: [MySQL] 54,557 pass(es), 35 fail(s), and 14 exception(s).
[ View ]

Status:Needs review» Needs work

The last submitted patch, drupal-2014617-menu-wrong-bundle-6.patch, failed testing.

Status:Needs work» Needs review
StatusFileSize
new1.98 KB
PASSED: [[SimpleTest]]: [MySQL] 55,536 pass(es).
[ View ]
new1008 bytes

Sorry, didn't know about the inheritdoc. As the approch + comment I used here is used multiple times in D8 (FeedStorage, ItemStorage, FileStorage, NodeStorage, TermStorage, UserStorage). But I checked now the other parts, and sometimes they do it the other way around. Adding bundle to the values and creating entity afterwards. Which maybe would make more sense here, as the hooks getting otherwise the "tools" bundle. Attached new patch.

Also I didn't found another way to add this, without implementing hooks there (field_attach_create, menu_link_create, entity_create). But in my mind this is also no option here.

The patch looks awesome. Using $values in the way done here is the perfect approach IMO.

I think 3 things still need to be done here:

  1. +++ b/core/modules/menu_link/lib/Drupal/menu_link/MenuLinkStorageController.php
    @@ -69,6 +69,18 @@ public function __construct($entity_type, array $entity_info, Connection $databa
    +    // Bundle can be other menu_name, but most of the time is the same and can
    +    //   be omitted, but must be set to menu_name then.

    I think we can improve the comment here. Suggestion (feel free to improve):

    The bundle of menu links being the menu name is not enforced but the default behavior if no bundle is set.

  2. The code in MenuLinkStorageController::attachLoad() that currently sets the bundle when loading an entity should be removed. We should also add an additional test that verifies that this still works. I.e. something like:
    <?php
    $menu_link
    = entity_create('menu_link', array('menu_name' => $menu->id(), ...));
    $menu_link->save();
    $this->assertEqual($menu_link->bundle(), $menu->id());
    ?>
  3. A "tests-only" patch should be created to prove that adding the tests without the actual code changes currently fails. That proves the validity of the tests.

StatusFileSize
new998 bytes
FAILED: [[SimpleTest]]: [MySQL] 55,108 pass(es), 2 fail(s), and 0 exception(s).
[ View ]

1. Shouldn't this be

The bundle of menu links being the menu name is not enforced but is the default behavior if no bundle is set.

(English is not my first language and I didn't wanted to spam the test server with comment changes ;) )

2. This is not so easy. You can't remove this from attachLoad, because when the DB is queried it doesn't call entity_create and so it doesn't call the create method. Loading a existing menu link would result in the default bundle (tools) again. I have one alternativ solution, removing the default value for bundle and let bundle() return menu_name, if bundle is not set. This should work for loaded and created menu_links.

<?php
class MenuLink extends Entity implements \ArrayAccess, MenuLinkInterface {
  public
$bundle = NULL;
  public function
bundle() {
    if (
is_null($this->bundle)) {
      return
$this->menu_name;
    }
    else {
      return
$this->bundle;
    }
  }
}
?>

The proposed test is more or less the test which is already in the patch above.

edit: I also tried to overwrite the __construct method. But from a DB query the construct is called without values. attach_load seems currently to me the only way to add things to a object, but load is not called on create.

3. Attached test-only patch.

@tstoeckler @kfritsche Thanks for working on this. You are doing things at a level I can't. :)

1. Yes, definitely! (Not a native speaker either, btw)

2. Hmm, I had thought that because the correct bundle is set when creating the entity initially that the bundle gets saved correctly already and does not need to be set on load. I might be missing something, though. Did you test this?

3. Awesome! Hopefully It'll fail...

2. If you do a entity_create, save and check afterwards it works. The problem I mean is menu_load() and then check the bundle name. For this second approch, loading a already saved menu_link, is the attachLoad method needed, otherwise you will have the default bundle, which is "tools".

The problem I mean is menu_load() and then check the bundle name. For this second approch, loading a already saved menu_link, is the attachLoad method needed, otherwise you will have the default bundle, which is "tools".

Could you elaborate on that? I'm not sure I fully understand. My question is: How does a menu_link get saved without already having the proper bundle saved in the DB?

Status:Needs review» Needs work

The last submitted patch, drupal-2014617-menu-wrong-bundle-10-test-only.patch, failed testing.

Status:Needs work» Needs review

See #1966298: Introduce menu link bundles per menus - #91 and following. At this point this were introduced.
Bundle is not saved in the database, its set to menu_name in the attachLoad function. Other modules like book, are overwriting bundle with something else on load, but in the DB there is no explicit bundle field.

Status:Needs review» Reviewed & tested by the community
StatusFileSize
new935 bytes
new1.93 KB
PASSED: [[SimpleTest]]: [MySQL] 57,351 pass(es).
[ View ]

You're right! The bundle property is not saved. That is super weird IMO, but that's the way it currently is. That makes this patch good to go.
I just updated the comment myself in order to get this along (sorry for the wrong patch name, that was a brainfail). I did not change the code, so marking RTBC myself.

Priority:Normal» Major

Sounds like a menu saving with the wrong bundle is a major bug.

Status:Reviewed & tested by the community» Fixed

Committed 136112c and pushed to 8.x. Thanks!

Status:Fixed» Needs review

Status:Needs review» Fixed

Crosspost :)

Crosspost :)

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

Issue summary:View changes

with related issue, the source: menus into bundles