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.

$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

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

YesCT’s picture

Gábor Hojtsy’s picture

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

Gábor Hojtsy’s picture

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

YesCT’s picture

kfritsche’s picture

Status: Active » Needs review
FileSize
1.82 KB

Initial patch for this, including one test.

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

YesCT’s picture

Status: Needs review » Needs work

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

kfritsche’s picture

Status: Needs work » Needs review
FileSize
1.98 KB
1008 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.

tstoeckler’s picture

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:
    $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.
kfritsche’s picture

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.

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.

YesCT’s picture

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

tstoeckler’s picture

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

kfritsche’s picture

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

tstoeckler’s picture

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.

kfritsche’s picture

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.

tstoeckler’s picture

Status: Needs review » Reviewed & tested by the community
FileSize
935 bytes
1.93 KB

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.

Gábor Hojtsy’s picture

Priority: Normal » Major

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

alexpott’s picture

Status: Reviewed & tested by the community » Fixed

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

Gábor Hojtsy’s picture

Status: Fixed » Needs review
Gábor Hojtsy’s picture

Status: Needs review » Fixed

Crosspost :)

Gábor Hojtsy’s picture

Crosspost :)

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

Anonymous’s picture

Issue summary: View changes

with related issue, the source: menus into bundles