Early Bird Registration for DrupalCon Portland 2024 is open! Register by 23:59 PST on 31 March 2024, to get $100 off your ticket.
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
Comment | File | Size | Author |
---|---|---|---|
#17 | drupal-2014617-menu-wrong-bundle-9.patch | 1.93 KB | tstoeckler |
#17 | interdiff-8-9.txt | 935 bytes | tstoeckler |
#10 | drupal-2014617-menu-wrong-bundle-10-test-only.patch | 998 bytes | kfritsche |
#8 | interdif-6-8.txt | 1008 bytes | kfritsche |
#8 | drupal-2014617-menu-wrong-bundle-8.patch | 1.98 KB | kfritsche |
Comments
Comment #1
YesCT CreditAttribution: YesCT commentednoticed in #1945226-55: Add language selector on menus
Comment #2
Gábor HojtsyThis is not strictly related to D8MI, but sounds like makes #1945226: Add language selector on menus impossible to do.
Comment #3
Gábor HojtsyYesCT says not impossible, but it would be important to fix to avoid workarounds nonetheless.
Comment #4
YesCT CreditAttribution: YesCT commentedSee also #1966298-91: Introduce menu link bundles per menus and #115-117
Comment #5
kfritscheInitial patch for this, including one test.
This just sets bundle to menu_name on create, if set.
Comment #6
YesCT CreditAttribution: YesCT commented#1906474: [policy adopted] Stop documenting methods with "Overrides …"
https://drupal.org/node/1354#inheritdoc
small doc fix.
I'm not sure about the general approach in the patch.
Comment #8
kfritscheSorry, 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.
Comment #9
tstoecklerThe 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:
I think we can improve the comment here. Suggestion (feel free to improve):
Comment #10
kfritsche1. Shouldn't this be
(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.
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.
Comment #11
YesCT CreditAttribution: YesCT commented@tstoeckler @kfritsche Thanks for working on this. You are doing things at a level I can't. :)
Comment #12
tstoeckler1. 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...
Comment #13
kfritsche2. 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".
Comment #14
tstoecklerCould 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?
Comment #16
kfritscheSee #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.
Comment #17
tstoecklerYou'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.
Comment #18
Gábor HojtsySounds like a menu saving with the wrong bundle is a major bug.
Comment #19
alexpottCommitted 136112c and pushed to 8.x. Thanks!
Comment #20
Gábor Hojtsy#17: drupal-2014617-menu-wrong-bundle-9.patch queued for re-testing.
Comment #21
Gábor HojtsyCrosspost :)
Comment #22
Gábor HojtsyCrosspost :)
Comment #23.0
(not verified) CreditAttribution: commentedwith related issue, the source: menus into bundles