Problem/motivation
Menu items currently are all in one bundle called "Menu link". The language subsystem has a standard language setup process for language settings and inheritance (whether language selectors are shown on content entities, etc.) which is bundle based. So long as menus only have one single bundle, you cannot make certain menus multilingual while others are single language and you cannot hide/show language selectors per menu and you cannot make menu items translatable on a per-menu basis. You can only set these settings globally for all menus. This is not very realistic for multilingual sites.
Proposal
Introduce bundles on a per-menu basis.
Followups
#2004674: Remove the feature of being able to move menu items between menus
#2014617: Unsaved menu links have the wrong bundle (always 'tools')
Comment | File | Size | Author |
---|---|---|---|
#102 | 1966298-menu-bundle-101.patch | 5.79 KB | das-peter |
#102 | interdiff-1966298-96-101-do-not-test.diff | 1.82 KB | das-peter |
#96 | 1966298-menu-bundle-96.patch | 6.01 KB | das-peter |
#96 | interdiff-1966298-93-96-do-not-test.diff | 2.68 KB | das-peter |
#93 | 1966298-menu-bundle-93.patch | 6 KB | das-peter |
Comments
Comment #1
Gábor HojtsyComment #2
Gábor HojtsyMarked #1945226: Add language selector on menus postponed on this one.
Comment #3
amateescu CreditAttribution: amateescu commentedI'll be away until the middle of next week, so I can only add a few things here to start the discussion.
Not using menus as bundles for the menu_link entity type was a design decision in the original thread (#916388-39: Convert menu links into entities) and I still fully support this decision.
Can't we find another way to still use the language setting from Menu entities but without them acting as bundles for Menu links?
Comment #4
Gábor HojtsyWell, so every other entity is bundle based along the same lines as multilingual settings would make sense. We can rebuild the multilingual settings API and UI to *not* be bundle base but make up a new categorisation system that falls back on bundles for everything but menus, but that would complicate the core multilingual system (and introduce yet another grouping scheme for entities) for a theoretical benefit of a contrib module. I'd prefer the contrib module deal with making funky things, not the core API to be over-complicated for theoretical contrib benefits.
Comment #5
plachI didn't follow closely the menu links to entity conversion, bur from an outsider perspective having menus as bundles would make lots of sense to me. It would be fully symmetric to content types (config entity) / nodes (content entity) and vocabulary (config entity) / terms, for instance. That said, if there is a very good reason for not doing it we can easily have language settings configured at entity-type level as we do for users.
Comment #6
Gábor HojtsyI don't think a global menu language setting for all menus is a good fit for sites. People want language-tied menus, translated menus, etc. intermixed, while a global setting would make it wide open instead of allowing for finer customisation. The point of not having bundles on the menu level that I've read is to be able to assign different fields to menu items on the first level of a menu vs. second-third, etc. levels, so your first level items can have icons, and other levels will not have that input. So its the same problem that we have for language but proposed with different segmentation. (So to avoid the need to add icon fields for all levels of the menu, you would only add to some levels). I think this would make menus stand out a huge deal from other entity types, where other entity types have a common pattern of bundle and instance relations. Also, fieldable menu items are not even proposed for core I believe, so the very different application of bundles for menus would be done for a theoretical contrib module and at the same time totally conflict with existing core functionality for language, which I don't think is a great idea. Just as much as language can make up a wholy new system to group entities by not-bundles to support this twisted menu approach, the menu field module in contrib can come up with a grouping system to hide/show certain field input on different levels of the menu as needed and that would not complicate the core API for language (as well as complicating the menu bundle system) just so a future contrib module can do something simpler for fields.
Comment #7
amateescu CreditAttribution: amateescu commentedThe reason(s) are clearly explained in the comment that I linked in #3, and if you say it's easily doable in any other way, then I would happily support that :)
Comment #8
Gábor Hojtsy@amateescu: what plach said is if we ignore that anybody would want per-menu langauge settings, then its easy to do, because (like users), we can have one setting for all menus. Obviously once that theoretical contrib module starts to introduce bundles cross-menus (eg. a bundle for 1st level menu items), then you'll have separate language settings for it, so you can have menus, where the first level can have varying language settings, but not other levels. See the problem?
Comment #9
andypostI think it's ok to enable bundles for menu link conditionally via hook_entity_info_alter() once multilingual enabled but it needs live some room for contrib
Comment #10
Gábor Hojtsy@andypost: if, when you enable language module, you get bundles per menus, that does not really leave much flexibility for contrib, unless said contrib wants to state it will not work with any foreign langage or multilingual site, only pure English sites. I don't think we are striving for that goal.
Comment #11
andypostSo there could be configurable setting for that purpose, for example enabling translatability for menus should change "menu bundlability"
Comment #12
Gábor HojtsyI don't think its in our interest to have "one way streets" like that. Eg. you install a contrib module that wants to use menus differently, then you need to make the site multilingual, and you cannot do that per menu... Or vice versa... drupal_set_message(t('Tough luck, now you cannot make the menu multilingual')); is not the type of solution we want.
Comment #14
Gábor HojtsyAny other opinions?
Comment #15
andypostI think there's no other way except to make Menu module to implement alter for menu_link entity definition and allow bundles - the only sane way to do it but we get none-fieldable bundles #1937384-11: Remove 'bundles' key from the MenuLink class annotation... that's not bad at all. Without menu module no reason to manage menu entities and their translatability.
Probably there's another way to do it - change a way like translation attached to content enitities - why there's a requirement in bundle and not a some string filter as "bundle key"
Code shows that translatable flag stored in bundle info
Comment #16
Gábor Hojtsy@andypost: as said above, complicating the translation system/API in core for a possible contrib gain is not something I think is a good idea. I'm not aware of core plans to have non-per-menu bundles, so whatever is done towards that is for the theoretical contrib solution. In the meantime core does already have a fully working translation module that ties to bundles.
Comment #17
andypostBundle is just another hook
Comment #19
Gábor HojtsyWe can even alter this in from language module only, if people are so concerned for this, however then other contrib modules doing bundles in a different way would screw language support up, so they would be incompatible with multilingual sites. That is one kind of trade-off that we can choose, if we want.
Comment #20
donquixote CreditAttribution: donquixote commentedOne concern I previously had about bundle-per-menu:
If we could find a solution that allows separate menu trees per language, but which all use the same bundle and menu entity, this concern would become obsolete.
Besides, this would be a huge win in other aspects as well.
Comment #21
Gábor HojtsyOne thing that bundles allow for is you can tie each entity instance of a bundle to a specific language. So if you have separate menus per language, one good UX feature in core is you can tie all entities inside that menu to a specific language and never expose a language selector on those items, because you already know the language of the item. If we use bundles across menus, we cannot do that per bundle/menu as in right now, so you'd need to face and decide on a language selection at all times. Pretty unfortunate that you cannot even use core features to make your site building process simpler then.
Also, the deeper problem for language vs. menus is if you bundles are not per-menu, the translatability will be a mess. Field translatability settings in the field API are per bundle. So one would need to make those disconnectable/disconnected from bundles as well altogether. Then you have the configuration page for translatability, where if the language setup is per menu but bundles are not, you'd have possibly items from different bundles in one menu, so you'd have different bundles showing up for each menu. I don't even see how would core be able to map menus to bundles given core does not really know how bundles are used if not defined there (eg. if you only use certain bundles in certain menus), so that sounds like a major UX nightmare there in the configurability.
Comment #22
donquixote CreditAttribution: donquixote commentedI do not consider that a desirable solution.
Imagine a site with 40 languages - you would get 40 bundles.
If you ever do display settings, or field settings, etc, you need to do that 40 times.
I imagine we can do better than that.
There must be a way to restrict the language (or other settings) on items within a menu tree, other than having a separate bundle for each. E.g. children must have the same language as the root item.
Or what about making them all "language neutral" ?
Comment #23
donquixote CreditAttribution: donquixote commentedThe direction to take, as I see it (after IRC talks):
- Language settings are always per bundle.
- Core will do bundle-per-menu by default. The menu settings form can then control the settings (language and maybe fields) for this bundle.
- Contrib can introduce additional bundles per menu. However, it then has to provide additional configuration forms per bundle, instead of per-menu. Also, the code to actually build and render those multi-bundle menus might need additional logic, to be provided by contrib.
- Multiple menus sharing the same bundle? Nobody seems to be interested in that (esp if we can solve #20). But we can leave that open to discussion.
Anyway, no matter how crazy contrib will go in cross-menu bundles and cross-bundle menus:
Language configuration always happens per bundle, not per menu (unless they coincide, as in core).
@Gabor, does that sound ok?
Comment #24
Gábor Hojtsy@donquixote: my understanding from yesterday's IRC discussion was that @amateescu wants to explore making the whole language system for fields/entities not tie to bundles, so cross-menu bundles would be possible. He cites your comment at http://drupal.org/node/916388#comment-6390458 often as having good points to have cross-menu bundles. So my understanding is the two models would compare something like this:
In the bundle per menu model that is proposed here, each menu would be its own bundle. Yes, each menu item could only have the same set of fields, and yes I believe that is already a huge step forwards from Drupal 7 where menus are not entities and not fieldable. In the cross-menu bundle model that @amateescu is advocating (and sounds like @donquixote you are much less so now), bundles are used who knows how across menus. Since core would not define bundles, the definition and use of those bundles across menu levels would be governed by a contrib module. I made up a relatively "simple" example here, where items on the same level share the same bundle, but nobody said that would be how it would work.
With this cross-menu bundle model, if we assume field API as-is now, you can make certain bundles translatable, so you can have certain levels of certain menus translatable or everything translatable but not X menu translatable and Y menu not. That is just not possible if the menu wants to use any bundle that is cross-menu applicable. If it makes up bundles for the specific menus, then it will end up with *even more* bundles then having per menu bundles, since now you need per-menu bundles for different levels even. Current field API assumes field settings are either global to the field across all entities or per bundle. Current field API does not have a concept of field settings that are neither global neither bundle tied, but instead hang on some arbitrary key that is set from the outside.
@amateescu said he wants to explore setting up this third configuration method in field API and expand the language configuration system as well to not tie to bundles, so it can work for a scenario like this. That would mean both the field API complicated and the language config system for entities expanded to support this scenario that we only encountered for menus, so there would be less hackery in contrib to support cross-menu fields.
Core also has a user interface to configure field translatability that is bundle-bound (this screenshot for content type, but same applies to taxonomy terms for example per vocabulary, etc.):
So as per @amateescu's plan menus would be configurable for language/translation support per menu while bundles would not be per-menu. Translatability however is not per-bundle really, it has per-field details under bundles. So if the bundle supports translation, its fields have a say if they support translation (and in some cases like with image fields, even on the sub-field level). If we set translatability/language settings on the per menu level but different bundles can appear on the same menu with different fields, this UI would need to express settings for them, so if bundle B appears in menu X it has fields G, H and J translatable but not if the same bundle appears in menu Y. This is a core user interface, so if we want to have menus use cross-menu bundles, this UI would need to support the scenario.
@amateescu also said he will not have time to *start* exploring this before May 18th, at which point we will be 6 weeks away from API freeze. I think the extent of the changes that would be required to change field API as well as the language configuration API and make up and implement a configuration user interface that would make sense in core, just so a possible future contrib module to add fields on menu items would use slightly less hacks is a definite example of overkill. Said contrib module would need to deal with things like when you want to move a menu item one level up, where a different bundle instance should reside so you need to loose some fields from your menu item and possibly set some more given they are required and swap bundles. I don't think said contrib module could not also deal with hiding/showing fields as configured for different levels without them being different bundles.
Once again I think over-complicating the field API, the language configuration API and the content translatability configuration UX in core just so a possible future contrib module for a specific use case would be slightly less hackish is not a very good tradeoff.
Comment #25
Gábor Hojtsy@donquixote: a short and concrete answer to Language configuration always happens per bundle, not per menu (unless they coincide, as in core).: no, it does not make sense. See in the cross-menu bundle scenario if you make bundle B translatable, you can only have one level of the menu translatable. If you make Bundle A translatable, you cannot make the second menu not be translatable. If you then need to make up more bundles so you can wall translatability per menu, you end up with a lot more bundles than one per menu, it will be multiple bundles per menu, and you already expressed one bundle per menu sounds already too much to you.
Comment #26
amateescu CreditAttribution: amateescu commentedWhile I still think this is not the right way to go, it seems that I'm the only one left defending that position and I'd prefer not to be called out for stopping 'progress'. We'll see if we can change anything before July 1st or not.
Comment #27
Gábor HojtsyThanks! We need some tests for this to ensure it works, but it should be as complete to the extent of this issue as needed.
Comment #29
donquixote CreditAttribution: donquixote commentedIt will be up to contrib to organize this mess, and also to justify the use case.
Contrib can either find a way to let those different settings happily live together in one menu, or it can attempt to sync the settings for those bundles. My point here is to find a solution where we don't have to change the core bundle architecture.
I think more bundles = more maintenance. Even if we would come up with a UI where they are synced, the multiplicity of bundles might bite you in the end.
This is why I initially asked to have only one bundle by default.
The reason I no longer ask for that are (a) your language argument, and (b) to avoid casting existing items to a new bundle, when you change menu settings.
Comment #30
Gábor Hojtsy@donquixote: thanks, looks like this patch is a good enough current direction then.
As for the patch, the only (repeated) error is: Undefined index: "module" at config.inc on line 394.
Comment #31
andypost"bundle" is a key in table so "menu_name", tests could be copied from TermTranslationUITest
We need bigger changes in translation to allow translation of none-fieldable entities.
Currently only 'fieldable' entities could be translated, also translation controller needs to be added to menu_link
So patch includes hack to skip "fieldability check"
So with hack we get
Comment #32
amateescu CreditAttribution: amateescu commentedWhy are those hacks needed in this patch?? This is only about using menus as menu_link bundles. Also, menu.module depends on menu_link.module, so no need to check if the menu_link entity info exists.
Comment #33
amateescu CreditAttribution: amateescu commentedInterdiff is to #26.
Comment #34
Gábor Hojtsy@andypost, the scope you are looking for would come in with #1945226: Add language selector on menus which is postponed on this patch. Let's not expand scope from introducing bundles on menus. We should introduce menus language in #1945226: Add language selector on menus which should make menus appear on the language settings page too. Not in scope for this patch though.
Comment #36
andypostRemoved out of scope changes
Added comment to condition
Comment #37
Gábor HojtsyThanks for a passing patch :) :)
I don't fully understand this comment. I assume it is supposed to explain why do we need the condition around it?
We'd also need some automated tests to verify the bundles are in fact created, no? Such as for one of the built-in menus let's say.
Comment #38
andypostNot sure how to figure out the comment but sometimes (in upgrade tests and probably install) there's no
$entity_info['menu_link']
so tests are brokenPatch needs more work, this one just to point breakage
Comment #40
Gábor HojtsyThanks, the above patches also demonstrated the fail :) So let's keep the condition and add test coverage :)
Comment #41
Gábor HojtsyRe-uploading #36. As said above #38 was merely posted to demonstrate the fail. We still need tests to verify the default menus get bundles AFAIS.
Comment #42
Gábor Hojtsy@andypost: still working on the tests? :) Thanks!
Comment #43
andypostI have not enough time in upcoming week to work on tests
Drupal::entityManager()
EDIT I'll try to write simple test for bundle tonight
Comment #44
Gábor HojtsyChanged that line as suggested and fixed minor wording in the code comment.
Comment #45
andypostAdded test
Comment #46
Gábor HojtsyGreat, thanks! Although I posted a patch twice above, one was a re-post of a previous one and the other was a very minor code change including a code comment fix so it is not anywhere my patch :) Looks ready to go as per above discussion. @amateescu validated the direction above as well. This is the only issue blocking #1945226: Add language selector on menus now, so looking forward to being able to jump back there :)
Comment #47
alexpottI think we can improve this comment as upgrade does not pass anything to entity_info()... how about...
// During upgrades from 7.x to 8.x the menu_link module is enabled. Calls to entity_info() before this occurs will not have the menu_link key set.
Comment #48
YesCT CreditAttribution: YesCT commentedI reworded it with what I hope is a clarification.
I checked other standards things and everything looks fine.
I did not try it manually.
Comment #49
Gábor HojtsyLooks superb.
Comment #50
alexpottThe point here is to NOT set the bundle on a non-existent entity type. At the start of a 7.x to 8.x upgrade the menu module might be enabled and the menu_link will not be... so we have to check here to see if the menu_link entity exists. Once the menu_link module is enabled then we'll set the bundle entity key appropriately.
Comment #51
andypostYes, the main cause that there's some execution of hook_entity_info_alter() during upgrade before the menu_link module is enabled so we have to check this state
Comment #52
Gábor HojtsyTook @alexpott's text suggestion direct from #47. Should be good now?
Comment #53
YesCT CreditAttribution: YesCT commentedAh. I see. Gabor explained to me in irc that the if avoids a notice during upgrade.
Sorry to detour this.
fwiw, this looks good.
Comment #54
andypost+1 to commit
Comment #55
catchI don't think this will work if you move a menu link from one menu to another - in that case the bundle would change, but any information that depends on the bundle is going to be orphaned.
Comment #56
plach@catch:
Can't modules detect bundle changes through
hook_entity_update
? If I'm not mistaken nothing prevents a module from changing the type of a node right now.Comment #57
Gábor Hojtsy@catch: I don't think there is any data per bundle for existing menu items in core. There are no fields, and even with #1945226: Add language selector on menus the data per bundle is for menu items created in the future, not menu items created already. The data there is per bundle not per menu item, so if you move a menu item from a menu to another one, your language selector might appear or disappear, based on config for the new place. That is not a property of the menu item though.
Given that I believe there is no per-entity data variance based on bundles in core, what kind of data do you mean would be orphaned? If I'm correct about nothing in core, do we need to provide a core system to resolve arbitrary per-entity data to move? How is that even possible to do?
Comment #58
catch@plach, you can load and save nodes changing the node type, but there's no UI for it. Core provides a UI for moving menu items to different menus so the problem is pre-existing. If a module makes menu items fieldable is it then the responsibility of that module to handle field mappings across bundles? That seems like something that either the field/entity system supports or it doesn't, rather than something that would need to be implemented one-off for each entity type.
Comment #59
Gábor Hojtsy@catch: I don't know how the field system could support transporting a text field to an image field or an entity reference to a textarea field of different names. I also don't think we should solve this in core. Is there anything in core that is stored per bundle on menu items? I bet no because we are just introducing them and not adding on any such data.
Comment #60
Gábor HojtsyI tried this in practice as well. Merely making the menu name the bundle does not make Drupal store any additional data on the menu. The menu name for the menu item changes as well as it changed before the patch (in the menu_links table), so the "bundle transfer" works perfect in core given no bundle specific data or types.
I don't know of plans in core to make it possible to attach any other bundle specific data to menu items.
Then indeed a contrib module adding fields or other bundle specific data would need to resolve how to transform them and/or handle their orphanage when menu items are moved. This is indeed the only entity type in core supporting "bundle jumping" through the UI, but there is no per-bundle data. Those entity types, where per-bundle data is present (such as taxonomy terms or nodes), bundle jumping is not supported in core. Either way it comes down to bundle jumping for custom data not being supported, just in one case there is no custom data, the other case there is no way to jump from one bundle to another.
I see this issue as making menus consistent with other entities in how it uses bundles and don't see any way that this breaks data or makes people loose data after this is committed as-is in core.
Comment #61
Gábor HojtsyComment #62
catchI still need to think more about the bundle jumping. I can partly sympathise with 'there's no bundle-specific data stored so it's not a problem yet' but can see it coming back to bite us later. There's been discussion on making menu links fieldable (not sure where that is currently though) and that suddenly gets near-impossible once there's bundles-per-menu.
What happens once #1945226: Add language selector on menus is in, if a menu switches back and forth between menus that are language enabled vs. not?
Comment #63
chx CreditAttribution: chx commented> #58 you can load and save nodes changing the node type, but there's no UI for it.
You can't. That doesn't work, or that doesn't work as you think it does. To elaborate, you can change the $node->type->value but even if you do that the $node->bundle() won't change and so any code relying on that will break nicely. That probably should be a separate issue to throw an exception trying to change $node->type->value. Check EntityNG.php, the bundle property is set in the constructor, the bundle method returns it but there's nothing that changes it.
What you can do is to copy the values over to a whole new entity, save it, delete the old one.
Comment #64
Gábor Hojtsy@catch
I attempted to visualize and summarise the fieldability discussions in #1966298-24: Introduce menu link bundles per menus above. The proposal to have menu items fieldable was even focused on making menu items on different levels of the menu fieldable in different ways, so not only if you moved items across menus would you have this problem but even if you move a menu item across different levels of the same menu. Even worse of a problem there. So you would have menu items from different bundles across and over different menus.
What that issue is about is settings for * initialising* new menu items with language and showing of the language widget *on the form* for existing menu items. So the only thing that pertains to existing menu items is whether the widget is visible or not. It is true it can happen that you had a menu where the widget was set to visible *and* you have a French item that you move to a menu where the widget was set to not be visible and items defaulted to English, you'll have a French item among English items and you cannot change it. *However* the same could happen if you had the language widget visible earlier and then later set it to not be visible and tie to English. All the existing items in the menu will retain their language, only new items would be tied to English. This applies to any entity type and bundle, so same if you set a node type to tie to a language and not display the language widget, it would only apply to new entities created. So I don't think this is any new concern, the same situation is easy to achieve with other natural steps and applies across all entity types/bundles.
Note that this issue is a pre-requisite to #1945226: Add language selector on menus which would still only allow having the standard/common way to configure language for menus. Then if we actually want to have menu items translatable on top of that, we'd still need to do #1842858: [PP-1] Convert menu links to the new Entity Field API and #1966398: [PP-1] Refactor menu link properties to multilingual. So this patch in itself is not really a be all and end all, its a step on a long way. (Not saying we can get multilingual menu items into Drupal 8 in time, but at least per menu item language configuration in a usable way would be a bare minimum).
Comment #65
catchThose are two different situations though.
In one, there's a conscious decision by the site administrator to disable a feature (then Drupal just ignores that existing data relies on that feature and lets them do it, which isn't great but that's generally what happens elsewhere too). If someone gets confused by this, then the answer is 'well, you disabled it but we didn't change any data in case you changed your mind five minutes later, and you can enable it again to fix stuff manually if you want to').
What you describe with menu items will happen just from enabling one feature in the UI, then using a completely separate feature to move the menu item. Then there's stuff in a locked/stuck state and nothing was actually explicitly disabled for that menu item anywhere at all - it's just using the options the admin interface has as they're separately designed for but don't work together. There's a whole bunch of issues where Drupal provides UI options that let you corrupt your own data like #232327: Deleting node type leaves orphan nodes and this has that feeling about it even if it's not actually introducing that behaviour yet.
That might be something to worry about in #1966398: [PP-1] Refactor menu link properties to multilingual more than here maybe. Unassigning me. If another committer thinks this is completely fine then go ahead.
Comment #66
Gábor HojtsyI think leaving the menu item data as-is in fact fulfills we didn't change any data in case you changed your mind five minutes later, and you can enable it again to fix stuff (quoting you directly), so although you might not have a UI to change the menu item in the new menu, if you change your mind, we kept your data intact, you can move it back to the old menu or the other intended menu, and it will be intact. We cannot just assume you wanted to apply the language of that new menu to the old item. You might as well have old items in that menu and now only want to add new items in the new language or something else. Or you might be in the process of moving there some other language items and then making it multilingual in that order. We cannot really assume your workflow here I think. What we do is we keep your data. The settings for the language default are only for new items, the settings for the widget is the only thing that applies to all items new or old. So I see this as *protecting* your data vs. corrupting your data.
I agree once/if fields are introduced on menus, this would be an issue. It would be even bigger of an issue if bundles are introduced differently (cross menus). People looking to have fields on menu items planned to deal with this at the same time when they introduce fieldability, which does not sound like feasible in core anyway in the following 7.5 weeks available for it. That solution may need to work with a generic entity bundle-field-remapper. I think thats a significantly big feature to not fit into the D8 timeline, however it is in no way required to introduce these bundles.
Comment #67
catch@Gabor yes I think 'protecting' vs. 'corrupting' depends on the use case, which we don't know.
chx's #63 means that once menus items are converted to EntityNG the bundle renaming isn't' going to actually work though - because it goes from not explicitly supported but possible, to not allowed.
Comment #68
plachI might be wrong but I think that if you do
$entity->get($bundle_key)->value = 'bar'
, save and load again things should work. Anyway I think that if you can't change an entity bundle programmatically it's an API flaw, not somehting that should block us here.Comment #69
Gábor Hojtsy@catch:
It currently works well. We cannot really put in provisional code that uses an API that menus are not yet converted to. I think moving the bundle for the menus would need to be solved either way. It is true, if core will not introduce any bundles, then the moving of the bundle will need to be resolved in contrib too like I believe it will be for moving/mapping fields. Above you advocated for either solving the field mapping even in core or declare that not supported. If we declare that not supported and lump up the bundle problem there, then contrib will not be able to use this API in any way to have any kind of bundles on menus. So I don't think that is a way forward.
In short, this patch does not break anything, bundle changes work well. If/once a conversion would make it not work, we would need to resolve that there I think.
Comment #70
Gábor HojtsyAll right, so what should we do here to continue? I still believe this is fine as-is, nothing broken here.
Comment #71
das-peter CreditAttribution: das-peter commentedRe-cap from my perspective:
Prerequisites:
Adding bundles to menu links is like following a de facto standard for translatable entities.
If we don't do that we've to introduce a new approach for entity translatability just on behalf of menu link entity type.
A possible reason for not using bundles is that, unlike anywhere else in core, menu links have to be moveable between bundles - since the menu of a menu link will be its bundle.
Moving non-fieldable entities between bundles is supported by core, since the base properties of an entity type are shared in all bundles.
As soon as an entity type is fieldable, moving entities between bundles isn't supported by core because there's no handling for fields that can differ by bundle.
However, since menu links won't be fieldable the reason for not using bundles is an artificial one, based on the assumption that a contrib module could make menu links fieldable.
Now, if there's a contrib module that does this, dealing with fields when moving menu links between bundles is definitely in the problem space of this contrib module and not core.
That's why I'm all for using bundles.
However, I think we should consider to create a followup issue like "What happens with fields and how to handle them on a bundle switch".
But all this doesn't justify to block this issue, a required core feature, from getting done.
If someone can bring up another, substantive, reason why bundles on menu links are a no-go I'll happily change my opinion.
Attached patch adds a test to check if moving a menu link from one menu to another in the UI changes its bundle.
Further it fixes the method to return the bundle of a menu link and adds some metadata about the bundle.
Comment #73
andypostis not needed. also comma is useless
Comment #74
das-peter CreditAttribution: das-peter commentedReverted the changes in the annotation, but added the
bundle_keys
definition tomenu_entity_info_alter()
instead.Comment #75
andypostas we add bundle in _alter probably it makes sense to make this dynamic too
Comment #76
das-peter CreditAttribution: das-peter commentedSounds sane, and here's the updated patch.
Comment #77
andypostI think it's ready, +1 rtbc
Comment #78
Gábor HojtsyLooks good to me as said above. Others did not raise concerns about the bundle problem and now we even have test for it to see if it ever breaks.
Comment #79
catchMoving this over to Alex for a second opinion. I still have a bad feeling about this but I can accept it doesn't cause any actual problems now.
Comment #80
amateescu CreditAttribution: amateescu commentedOur coding standards say that we shouldn't 'use' this class, but prefix it with a '\' whenever it's used in OO code.
But we don't need it anyway because we have $this->entityInfo() available.
Re #71:
Actually, I hope they will be fieldable pretty soon in core, when we'll convert the parent property to a Field API field based on Entity reference..
I'd also like to be able to use core's Link field instead of the custom implementation that we have now, but for that we can probably get away by only using its widgets and formatters.
That being said, like catch, I also still think this will bite us hard in the long run.
Comment #81
Gábor HojtsyI agree "this will come back and bite us hard" but I don't agree this is the patch making it so. I think the combination of menu items being content entities and Drupal allowing you to move items across menu levels and menus makes it complicated once there is bundle specific data on menu items. (We are not introducing such data here). So unless we want to prohibit the bundlbility and field ability, or more correctly possibility to attach per bundle data to items in any way, we are gonna have problems. Is it because of this patch? I don't think so. It is intrinsic to menu items as content entities and letting us freely move them around at the same time. I think that already set us up with this problem, it is not new here. Since core does not seem to be about to solve this, what I am looking at is getting the most out of this without directly running into the problem itself in core. Either we go ahead a bit more in core or not, this problem is **already ** there and will likely not be solved in core.
Comment #82
das-peter CreditAttribution: das-peter commentedDamn, totally missed that - thanks for spotting this! Patch re-rolled.
I can't say anything about this - the prerequisites in my re-cap are my current state of knowledge.
If this ^^ is the case it's, of course, a substantive concern.
What means we need to evaluate further which solution brings the most pain / gain.
How about doing this evaluation tomorrow at the sprint (@DrupalCon)?
Comment #83
swentel CreditAttribution: swentel commentedIt took me a while to parse the issue, and also read the other issue that is postponed on this one, and started thinking about all the use cases I've been struggling with the last years re: menu / menu links and languages. #1945226: Add language selector on menus will even solve so many use cases that simply are a pain in the ass right now.
I think we need to stop being paranoid about bundles and the potential issues when they become fieldable. Leave fieldability in contrib though.
So yes, let's just do this.
Needs work for a typo though ;)
'menue' should be 'menu'
Same here
Comment #84
fagoEntity bundles are not about fieldability/extensibility only, they are in fact entity subtypes - even in D7. While practice has been that bundles are used mostly for fieldability/extensibility, we've started using them for more cases were using sub-types fit. So we've adopted the concept and start using it more - that's great. Thus, as bundles are about more than fieldability/extensibility I do not think our usage of them should be bound to possible fieldability/extensibility usage issues.
[off-topic?] On the actual problems with changing bundles: Do we have to implement it that way? Instead of changing the bundle, couldn't we create a duplicate entity, change the bundle there and save it then. So we've got a new copy and we can delete the old one.
Comment #85
das-peter CreditAttribution: das-peter commentedThanks for all the statements that will help to find the way to go.
Fixed typo.
Comment #86
bshaffer CreditAttribution: bshaffer commentedI was just about to upload a fix for this. I have reviewed the interdiff and it looks good!
Comment #87
effulgentsia CreditAttribution: effulgentsia commentedI like the idea of supporting different default language and content translatability settings per menu. However, like @catch, I do not like the idea of core shipping with a UI that allows the bundle/subtype of an entity to be changed after initial save.
Why? I'd be in favor of removing that ability, just like we don't allow taxonomy terms to move between vocabularies.
Comment #88
Gábor HojtsyI talked about this with @effulgentsia and came up with book module as a counter-example for this. What book module does is it maintains menu links for books and menu provides the hierarchy backend. And it works exactly how I explained to @effulgentsia. I promised to double-check.
Book module uses the book hierarchy and also defines 'a menu' for each book. The menu has the id book-toc-$bid where $bid is the node id of the root page of the book. So if you want to create a new book out of an existing page (I think more likely when you refactor books - ask the docs team :) or you want to move pages between books, none of that would be possible anymore. Since they are plain menus.
See http://api.drupal.org/api/drupal/core%21modules%21book%21book.module/fun... and http://api.drupal.org/api/drupal/core%21modules%21book%21book.module/fun... to understand the use of bid / menu name relations.
So essentially making it impossible to move menu items between menus would also make it impossible to move a book page outside of a book or change the root node id of any existing book ever. Do you propose this is the way to go?
As far as the patch goes, it makes sure to actually test the bundle changing works when moving menu items, so my thinking is as long as the test is there, whatever we do to the entity API, it should prove the feature keeps working. I'm not comfortable of all the consequences of this proposal to remove the movability of menu items and therefore much of the flexibility of books too. I don't see how is this required collateral damage to this patch.
Comment #89
tim.plunkettTests menu link bundles.
public function testMenuBundles()
I can't imagine this will pass. We cannot call entity_get_info() [which is what this effectively does] inside this hook.
It is why it was split out in the first place.
Checking taxonomy_entity_bundle_info(), which calls taxonomy_vocabulary_get_names(), which just calls config_get_storage_names_with_prefix() and hardcodes the config prefix directly.
Comment #90
das-peter CreditAttribution: das-peter commented@tim.plunkett Thanks for the review.
Changed.
Changed.
menu_entity_bundle_info()
now relies on a hardcoded config prefix, similar totaxonomy_entity_bundle_info()
.Comment #91
effulgentsia CreditAttribution: effulgentsia commentedI discussed this with Gabor, das-peter, swentel, and alexpott. The idea we came up with is for the bundle of a menu link in any menu named
book-toc-$bid
to bebook-toc
. That would allow reorganizing book outlines without the menu links changing bundles.However, it would mean that you could only configure translatabiliy globally for all books, not per-book. But, that's okay, because we should also have the 'book-toc' bundle defined as
'translatable' => FALSE
, thereby removing the ability for the administrator to enable a translation UI for them, because we don't want to present a UI for translating book menu links: their translations should be managed entirely via the book nodes, and book module already syncs node title changes to the corresponding book menu link.Alex Pott recommended that the scope of this issue include the above condensation of book link bundles, but to punt the removing of non-book menu link movability between menus to a follow up issue.
With that in mind, here's a code review:
Since we need to condense bundles for book menus, we can't use 'menu_name' as the bundle key directly. Instead we need to define a new menu link property (I suggest 'type', unless someone has a better name in mind) to use as the bundle key. However, this new property does not need a new database column: MenuLinkStorageController::attachLoad() can just do
$menu_link->type = $menu_link->menu_name
, and then book_menu_link_load() can alter it for book-toc-* links.book-toc-* menus don't have menu.menu.* config entities backing them. Therefore, we also need a book_entity_bundle_info() implementation to add the 'book-toc' bundle (with 'translatable' => FALSE).
There's also shortcut-* menus without menu.menu.* entities, so we'll need a shortcut_entity_bundle_info() implementation to add the desired bundle(s). I think it makes sense to also condense them to a single bundle ('shortcut') in the same way as for book. If you agree, then we also need a corresponding shortcut_menu_link_load() to do that.
Comment #92
das-peter CreditAttribution: das-peter commentedI'll give it a try to get that conversion of books and shortcuts done.
Comment #93
das-peter CreditAttribution: das-peter commentedHere's a first attempt to get this done.
I've now added the bundle key to the MenuLink entity definition as it has to be always available.
The new bundle key is
type
, which is set programmatically when the entity is build.The book and shortcut module use
hook_TYPE_load()
to overwrite the type and set a hardcoded bundle for their entities. And both modules declare their fixed bundles inhook_entity_bundle_info()
.Currently there's one question left: How to properly sync the book node title to the menu title?
I could set the
link_title
inbook_menu_link_load()
similar totype
, but then it wouldn't react to thelangcode
parameter that can be passed to theMenuLink::label()
method.I thought about
label_callback
, but that's per entity type and not per bundle.Comment #94
Gábor HojtsyI don't believe we wanted to solve syncing book titles as in not make them editable anymore.
Comment #96
das-peter CreditAttribution: das-peter commentedLooks like the property
type
is already used for other purposes. Changed that tobundle
instead - locally tests look better again.Comment #98
das-peter CreditAttribution: das-peter commented#96: 1966298-menu-bundle-96.patch queued for re-testing.
Comment #99
Gábor HojtsyOpened #2004674: Remove the feature of being able to move menu items between menus as a followup.
Comment #100
effulgentsia CreditAttribution: effulgentsia commentedHEAD already manages that: book_node_update() calls _book_update_outline(), so nothing special needed in this patch for that.
We want book-toc- to be at the beginning, not just anywhere, so
=== 0
.Can this just be
return $this->bundle;
?=== 0
Comment #101
Gábor HojtsyYeah I also verified just now in code that the book title synching works.
Comment #102
das-peter CreditAttribution: das-peter commentedThanks for the review! patch updated -> all points addressed.
Comment #104
Gábor Hojtsy#102: 1966298-menu-bundle-101.patch queued for re-testing.
Comment #105
andypostWe could extend this for #1882552: Deprecate menu_list_system_menus() and menu_ui_get_menus() and finally implement a embeddable element or ER in #1882218: Remove static menu link creation for menus in menu_enable() and elsewhere
Comment #107
das-peter CreditAttribution: das-peter commented#102: 1966298-menu-bundle-101.patch queued for re-testing.
Comment #108
Gábor HojtsyThe test fails were random, now it passes fine.
Comment #109
alexpottCommitted 5165b47 and pushed to 8.x. Thanks!
We need to open a followup to remove the ability to re-parent menu items into another menu. The convincing argument for me is this is very similar to the decision we made when taxonomies became an entity.
I think we need to update the menu_links change notice.
Comment #110
Gábor HojtsyFollowup at #2004674: Remove the feature of being able to move menu items between menus.
Comment #111
Gábor HojtsyI've updated the menu links change notice with:
Linked in to this issue. http://drupal.org/node/1914008
Comment #112
YesCT CreditAttribution: YesCT commentedI started on #1945226-49: Add language selector on menus and could use some help there.
Comment #113
YesCT CreditAttribution: YesCT commented#1945226-55: Add language selector on menus
I'm getting 'tools' as the bundle for every menu. should be each menu's bundle is the same name as the menu's id.
Comment #114
donquixote CreditAttribution: donquixote commented#20 (donquixote):
#88 (Gabor):
I think these two problems are related, and could be treated in a similar way.
We want separate menu trees that share their bundle settings (preferably by having the same bundle).
Comment #115
tstoecklerIn line with #113, in reading the committed patch again, I am wondering why we didn't just do
It seems strange that book.module and shortcut.module have to alter in the bundle on every load. Should we open a follow-up or am I completely missing something?
Comment #116
effulgentsia CreditAttribution: effulgentsia commentedSee #91.
Comment #117
tstoecklerRe #116: Ahh, that was super helpful, thanks. I had missed that part of the discussion. And also overlooked the attachLoad() hunk.
Comment #117.0
tstoecklerAdd followup
Comment #118
Gábor HojtsyComment #119
Gábor HojtsyRemove sprint again :)
Comment #120
ekes CreditAttribution: ekes commentedDon't suppose anyone knows the answer to this one entity type menu_item has bundles but no field definition?.
Comment #121.0
(not verified) CreditAttribution: commentedadded bundle name = tools followup
Comment #122
effulgentsia CreditAttribution: effulgentsia commentedPer #2301317-53: MenuLinkNG part4: Conversion.3 and the following response, this feature is being removed in that issue and punted to contrib. Please comment there with any concerns about that.
Comment #123
YesCT CreditAttribution: YesCT commentedIs there already a contrib project that will (eventually) do this? Can we find someone willing to maintain it to open a sandbox?
Comment #124
Gábor HojtsyI think this is pretty sad given all the work that went into this :/ I believe the issue summary still stands:
At least that shipped menu items are not content entities helps a bit with this, although the menu language settings will not govern their language status, which will be one hell of a feat to explain. I guess will take some time to figure out how to do this for people either way :) At least we have the building blocks in place to store and manage those translations.
Comment #125
effulgentsia CreditAttribution: effulgentsia commentedLet's continue the discussion of #124 in #2311295: Introduce MenuLinkContent bundles.
Comment #126
ozinCouple months ago we create module Menu Item Extras which adds bundles to menus and templates, also it has integration with Views and provides couple extra features which are specific for the MenuLinkContent entity, but it has also some issues which we can't fix in our module and it require core fixes.