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

Files: 
CommentFileSizeAuthor
#102 1966298-menu-bundle-101.patch5.79 KBdas-peter
PASSED: [[SimpleTest]]: [MySQL] 55,657 pass(es).
[ View ]
#102 interdiff-1966298-96-101-do-not-test.diff1.82 KBdas-peter
#96 1966298-menu-bundle-96.patch6.01 KBdas-peter
PASSED: [[SimpleTest]]: [MySQL] 55,692 pass(es).
[ View ]
#96 interdiff-1966298-93-96-do-not-test.diff2.68 KBdas-peter
#93 1966298-menu-bundle-93.patch6 KBdas-peter
FAILED: [[SimpleTest]]: [MySQL] 55,675 pass(es), 17 fail(s), and 0 exception(s).
[ View ]
#93 interdiff-1966298-90-93-do-not-test.diff4.07 KBdas-peter
#90 1966298-menu-bundle-90.patch3.36 KBdas-peter
PASSED: [[SimpleTest]]: [MySQL] 55,824 pass(es).
[ View ]
#90 interdiff-1966298-85-90-do-not-test.diff1.25 KBdas-peter
#85 1966298-menu-bundle-85.patch3.42 KBdas-peter
PASSED: [[SimpleTest]]: [MySQL] 55,739 pass(es).
[ View ]
#85 interdiff-1966298-82-85-do-not-test.diff1.03 KBdas-peter
#82 1966298-menu-bundle-82.patch3.43 KBdas-peter
PASSED: [[SimpleTest]]: [MySQL] 55,735 pass(es).
[ View ]
#82 interdiff-1966298-76-82-do-not-test.diff895 bytesdas-peter
#76 1966298-menu-bundle-76.patch3.62 KBdas-peter
PASSED: [[SimpleTest]]: [MySQL] 55,946 pass(es).
[ View ]
#76 interdiff-1966298-73-76-do-not-test.diff949 bytesdas-peter
#74 1966298-menu-bundle-73.patch3.22 KBdas-peter
PASSED: [[SimpleTest]]: [MySQL] 56,962 pass(es).
[ View ]
#74 interdiff-1966298-71-73-do-not-test.diff1.15 KBdas-peter
#71 1966298-menu-bundle-71.patch3.4 KBdas-peter
PASSED: [[SimpleTest]]: [MySQL] 56,103 pass(es).
[ View ]
#71 interdiff-1966298-52-71.diff1.9 KBdas-peter
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch interdiff-1966298-52-71.diff. Unable to apply patch. See the log in the details link for more information.
[ View ]
#52 1966298-menu-bundle-52.patch1.95 KBGábor Hojtsy
PASSED: [[SimpleTest]]: [MySQL] 55,621 pass(es).
[ View ]
#52 interdiff.txt769 bytesGábor Hojtsy
#48 1966298-menu-bundle-48.patch1.98 KBYesCT
PASSED: [[SimpleTest]]: [MySQL] 55,653 pass(es).
[ View ]
#48 interdiff-45-48.txt689 bytesYesCT
#45 1966298-menu-bundle-45.patch1.88 KBandypost
PASSED: [[SimpleTest]]: [MySQL] 55,766 pass(es).
[ View ]
#44 1966298-menu_link-bundle-44.patch1.02 KBGábor Hojtsy
PASSED: [[SimpleTest]]: [MySQL] 55,649 pass(es).
[ View ]
#41 1966298-menu_link-bundle-36.patch1.04 KBGábor Hojtsy
PASSED: [[SimpleTest]]: [MySQL] 55,492 pass(es).
[ View ]
#38 1966298-menu_link-bundle-noif-38.patch935 bytesandypost
FAILED: [[SimpleTest]]: [MySQL] 55,408 pass(es), 0 fail(s), and 44 exception(s).
[ View ]
#36 interdiff.txt1.22 KBandypost
#36 1966298-menu_link-bundle-36.patch1.04 KBandypost
PASSED: [[SimpleTest]]: [MySQL] 55,397 pass(es).
[ View ]
#33 1966298-33.patch935 bytesamateescu
FAILED: [[SimpleTest]]: [MySQL] 55,353 pass(es), 0 fail(s), and 44 exception(s).
[ View ]
#33 interdiff.txt1.23 KBamateescu
#31 interdiff.txt1.27 KBandypost
#31 1966298-menu-bundle-28.patch1.69 KBandypost
PASSED: [[SimpleTest]]: [MySQL] 55,653 pass(es).
[ View ]
#31 menu-bundles.png27.46 KBandypost
#26 1966298-26.patch930 bytesamateescu
FAILED: [[SimpleTest]]: [MySQL] 55,226 pass(es), 0 fail(s), and 44 exception(s).
[ View ]
#26 interdiff.txt1.23 KBamateescu
#24 Menus vs. bundles.png45.83 KBGábor Hojtsy
#24 Screenshot_4_23_13_8_12_AM.png73.64 KBGábor Hojtsy
#17 1966298-menu-bundle-17.patch797 bytesandypost
FAILED: [[SimpleTest]]: [MySQL] Drupal installation failed.
[ View ]

Comments

Priority:Normal» Major
Issue tags:+sprint

Marked #1945226: Add language selector on menus postponed on this one.

Title:Introduce menu bundles per menusIntroduce menu link bundles per menus
Component:menu system» menu_link.module

I'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?

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

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

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

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.

The 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 :)

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

I 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

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

So there could be configurable setting for that purpose, for example enabling translatability for menus should change "menu bundlability"

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

Any other opinions?

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

<?php
/**
* Implements hook_entity_bundle_info_alter().
*/
function translation_entity_entity_bundle_info_alter(&$bundles) {
  foreach (
$bundles as $entity_type => &$info) {
    foreach (
$info as $bundle => &$bundle_info) {
     
$enabled = translation_entity_get_config($entity_type, $bundle, 'enabled');
     
$bundle_info['translatable'] = !empty($enabled);
    }
  }
}
?>

Code shows that translatable flag stored in bundle info

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

Status:Active» Needs review
StatusFileSize
new797 bytes
FAILED: [[SimpleTest]]: [MySQL] Drupal installation failed.
[ View ]

Bundle is just another hook

Status:Needs review» Needs work

The last submitted patch, 1966298-menu-bundle-17.patch, failed testing.

Status:Needs work» Needs review

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

One concern I previously had about bundle-per-menu:

Some sites have one menu per language, to allow having different menu structure (beyond just title translation) per language. These would result in a ton of bundles, totally overhead.

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.

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

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

I 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" ?

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

StatusFileSize
new73.64 KB
new45.83 KB

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

Menus vs. bundles.png

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

Screenshot_4_23_13_8_12_AM.png

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.

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

StatusFileSize
new1.23 KB
new930 bytes
FAILED: [[SimpleTest]]: [MySQL] 55,226 pass(es), 0 fail(s), and 44 exception(s).
[ View ]

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

Issue tags:+Needs tests

Thanks! We need some tests for this to ensure it works, but it should be as complete to the extent of this issue as needed.

Status:Needs review» Needs work

The last submitted patch, 1966298-26.patch, failed testing.

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

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

and you already expressed one bundle per menu sounds already too much to you.

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.

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

Status:Needs work» Needs review
StatusFileSize
new27.46 KB
new1.69 KB
PASSED: [[SimpleTest]]: [MySQL] 55,653 pass(es).
[ View ]
new1.27 KB

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

<?php
function language_entity_supported() {
 
$supported = array();
  foreach (
entity_get_info() as $entity_type => $info) {
    if (!empty(
$info['fieldable']) && !empty($info['translatable'])) {
     
$supported[$entity_type] = $entity_type;
    }
  }
  return
$supported;
}
?>

So with hack we get
menu-bundles.png

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

StatusFileSize
new1.23 KB
new935 bytes
FAILED: [[SimpleTest]]: [MySQL] 55,353 pass(es), 0 fail(s), and 44 exception(s).
[ View ]

Interdiff is to #26.

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

Status:Needs review» Needs work

The last submitted patch, 1966298-33.patch, failed testing.

Status:Needs work» Needs review
StatusFileSize
new1.04 KB
PASSED: [[SimpleTest]]: [MySQL] 55,397 pass(es).
[ View ]
new1.22 KB

Removed out of scope changes
Added comment to condition

Status:Needs review» Needs work

Thanks for a passing patch :) :)

+++ b/core/modules/menu/menu.moduleundefined
@@ -151,6 +151,27 @@ function menu_entity_info_alter(&$entity_info) {
   );
+  if (isset($entity_info['menu_link'])) {
+    // Upgrade path could pass partial definition because config imported first.
+    $entity_info['menu_link']['entity_keys']['bundle'] = 'menu_name';

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.

Status:Needs work» Needs review
StatusFileSize
new935 bytes
FAILED: [[SimpleTest]]: [MySQL] 55,408 pass(es), 0 fail(s), and 44 exception(s).
[ View ]

Not 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 broken

Patch needs more work, this one just to point breakage

Status:Needs review» Needs work

The last submitted patch, 1966298-menu_link-bundle-noif-38.patch, failed testing.

Thanks, the above patches also demonstrated the fail :) So let's keep the condition and add test coverage :)

Status:Needs work» Needs review
StatusFileSize
new1.04 KB
PASSED: [[SimpleTest]]: [MySQL] 55,492 pass(es).
[ View ]

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

@andypost: still working on the tests? :) Thanks!

I have not enough time in upcoming week to work on tests

+++ b/core/modules/menu/menu.moduleundefined
@@ -151,6 +151,27 @@ function menu_entity_info_alter(&$entity_info) {
+  $info = Drupal::service('plugin.manager.entity')->getDefinition('menu');

Drupal::entityManager()

EDIT I'll try to write simple test for bundle tonight

StatusFileSize
new1.02 KB
PASSED: [[SimpleTest]]: [MySQL] 55,649 pass(es).
[ View ]

Changed that line as suggested and fixed minor wording in the code comment.

StatusFileSize
new1.88 KB
PASSED: [[SimpleTest]]: [MySQL] 55,766 pass(es).
[ View ]

Added test

Status:Needs review» Reviewed & tested by the community

Great, 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 :)

Status:Reviewed & tested by the community» Needs work

+++ b/core/modules/menu/menu.moduleundefined
@@ -151,6 +151,27 @@ function menu_entity_info_alter(&$entity_info) {
+  if (isset($entity_info['menu_link'])) {
+    // Upgrade could pass partial definition because config is imported first.
+    $entity_info['menu_link']['entity_keys']['bundle'] = 'menu_name';
+  }

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

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

I reworded it with what I hope is a clarification.

I checked other standards things and everything looks fine.
I did not try it manually.

Status:Needs review» Reviewed & tested by the community

Looks superb.

Status:Reviewed & tested by the community» Needs work

+++ b/core/modules/menu/menu.moduleundefined
@@ -151,6 +151,29 @@ function menu_entity_info_alter(&$entity_info) {
+  if (isset($entity_info['menu_link'])) {
+    // During upgrades from 7.x to 8.x the menu_link module is enabled. Set the
+    // menu_link key because calls to entity_info() before this occurs will not
+    // have it set.
+    $entity_info['menu_link']['entity_keys']['bundle'] = 'menu_name';
+  }

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

Yes, 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

Status:Needs work» Reviewed & tested by the community
StatusFileSize
new769 bytes
new1.95 KB
PASSED: [[SimpleTest]]: [MySQL] 55,621 pass(es).
[ View ]

Took @alexpott's text suggestion direct from #47. Should be good now?

Ah. I see. Gabor explained to me in irc that the if avoids a notice during upgrade.

Sorry to detour this.

fwiw, this looks good.

+1 to commit

Status:Reviewed & tested by the community» Needs work

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

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

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

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

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

Status:Needs work» Reviewed & tested by the community

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

Assigned:Unassigned» catch

Status:Reviewed & tested by the community» Needs review

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

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

@catch

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

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

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

Assigned:catch» Unassigned

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.

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

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

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

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

@catch:

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.

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.

All right, so what should we do here to continue? I still believe this is fine as-is, nothing broken here.

StatusFileSize
new1.9 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch interdiff-1966298-52-71.diff. Unable to apply patch. See the log in the details link for more information.
[ View ]
new3.4 KB
PASSED: [[SimpleTest]]: [MySQL] 56,103 pass(es).
[ View ]

Re-cap from my perspective:

Prerequisites:

  • Menu links have to be movable between menus.
  • Translatable menu links have to be provided by core.
  • Menu links are a non-fieldable entity type.

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.

Status:Needs review» Needs work

The last submitted patch, interdiff-1966298-52-71.diff, failed testing.

+++ b/core/modules/menu_link/lib/Drupal/menu_link/Plugin/Core/Entity/MenuLink.phpundefined
@@ -35,7 +35,11 @@
- *     "uuid" = "uuid"
+ *     "uuid" = "uuid",
+ *     "bundle" = "menu_name",

is not needed. also comma is useless

Status:Needs work» Needs review
StatusFileSize
new1.15 KB
new3.22 KB
PASSED: [[SimpleTest]]: [MySQL] 56,962 pass(es).
[ View ]

Reverted the changes in the annotation, but added the bundle_keys definition to menu_entity_info_alter() instead.

+++ b/core/modules/menu_link/lib/Drupal/menu_link/Plugin/Core/Entity/MenuLink.phpundefined
@@ -254,6 +254,13 @@ public function id() {
+  public function bundle() {
+    return $this->menu_name;

as we add bundle in _alter probably it makes sense to make this dynamic too

StatusFileSize
new949 bytes
new3.62 KB
PASSED: [[SimpleTest]]: [MySQL] 55,946 pass(es).
[ View ]

Sounds sane, and here's the updated patch.

I think it's ready, +1 rtbc

Status:Needs review» Reviewed & tested by the community

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

Assigned:Unassigned» alexpott

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

Status:Reviewed & tested by the community» Needs work

+++ b/core/modules/menu_link/lib/Drupal/menu_link/Plugin/Core/Entity/MenuLink.php
@@ -7,6 +7,7 @@
+use Drupal;

Our coding standards say that we shouldn't 'use' this class, but prefix it with a '\' whenever it's used in OO code.

+++ b/core/modules/menu_link/lib/Drupal/menu_link/Plugin/Core/Entity/MenuLink.php
@@ -257,7 +258,12 @@ public function id() {
+    $entity_info = Drupal::entityManager()->getDefinition($this->entityType());
+    if (!empty($entity_info['entity_keys']['bundle'])) {
+      return $this->{$entity_info['entity_keys']['bundle']};

But we don't need it anyway because we have $this->entityInfo() available.


Re #71:

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.

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.

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

Status:Needs work» Needs review
StatusFileSize
new895 bytes
new3.43 KB
PASSED: [[SimpleTest]]: [MySQL] 55,735 pass(es).
[ View ]

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

Damn, totally missed that - thanks for spotting this! Patch re-rolled.

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

Status:Needs review» Needs work

It 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 ;)

+++ b/core/modules/menu/lib/Drupal/menu/Tests/MenuTest.phpundefined
@@ -333,6 +333,30 @@ public function testBlockContextualLinks() {
+    $this->assertEqual($item->bundle(), 'tools', 'Menu link bundle matches the menue');

'menue' should be 'menu'

+++ b/core/modules/menu/lib/Drupal/menu/Tests/MenuTest.phpundefined
@@ -333,6 +333,30 @@ public function testBlockContextualLinks() {
+    $this->assertEqual($moved_item->bundle(), $menu->id(), 'Menu link bundle matches the menue');

Same here

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

Status:Needs work» Needs review
StatusFileSize
new1.03 KB
new3.42 KB
PASSED: [[SimpleTest]]: [MySQL] 55,739 pass(es).
[ View ]

Thanks for all the statements that will help to find the way to go.
Fixed typo.

I was just about to upload a fix for this. I have reviewed the interdiff and it looks good!

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

Menu links have to be movable between menus.

Why? I'd be in favor of removing that ability, just like we don't allow taxonomy terms to move between vocabularies.

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

+++ b/core/modules/menu/lib/Drupal/menu/Tests/MenuTest.phpundefined
@@ -333,6 +333,30 @@ public function testBlockContextualLinks() {
+   * Test menu link bundles.

Tests menu link bundles.

+++ b/core/modules/menu/lib/Drupal/menu/Tests/MenuTest.phpundefined
@@ -333,6 +333,30 @@ public function testBlockContextualLinks() {
+  function testMenuBundles() {

public function testMenuBundles()

+++ b/core/modules/menu/menu.moduleundefined
@@ -150,6 +150,29 @@ function menu_entity_info_alter(&$entity_info) {
+  $info = Drupal::entityManager()->getDefinition('menu');

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.

+++ b/core/modules/menu/menu.moduleundefined
@@ -150,6 +150,29 @@ function menu_entity_info_alter(&$entity_info) {
+  $config_names = config_get_storage_names_with_prefix($info['config_prefix'] . '.');

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.

StatusFileSize
new1.25 KB
new3.36 KB
PASSED: [[SimpleTest]]: [MySQL] 55,824 pass(es).
[ View ]

@tim.plunkett Thanks for the review.

Tests menu link bundles.

Changed.

public function testMenuBundles()

Changed.

I can't imagine this will pass. We cannot call entity_get_info() [which is what this effectively does] inside this hook.

menu_entity_bundle_info() now relies on a hardcoded config prefix, similar to taxonomy_entity_bundle_info().

Status:Needs review» Needs work

Making it impossible to move menu items between menus would also make it impossible to move a book page [between books]

I 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 be book-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.

I'm not comfortable of all the consequences of this proposal to remove the movability of menu items

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:

+++ b/core/modules/menu/menu.module
@@ -150,6 +150,28 @@ function menu_entity_info_alter(&$entity_info) {
+    $entity_info['menu_link']['entity_keys']['bundle'] = 'menu_name';
+    $entity_info['menu_link']['bundle_keys']['bundle'] = 'menu_name';

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.

+++ b/core/modules/menu/menu.module
@@ -150,6 +150,28 @@ function menu_entity_info_alter(&$entity_info) {
+function menu_entity_bundle_info() {
+  $bundles = array();
+  $config_names = config_get_storage_names_with_prefix('menu.menu.');

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.

Assigned:alexpott» das-peter

I'll give it a try to get that conversion of books and shortcuts done.

Status:Needs work» Needs review
StatusFileSize
new4.07 KB
new6 KB
FAILED: [[SimpleTest]]: [MySQL] 55,675 pass(es), 17 fail(s), and 0 exception(s).
[ View ]

Here'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 in hook_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 in book_menu_link_load() similar to type, but then it wouldn't react to the langcode parameter that can be passed to the MenuLink::label() method.
I thought about label_callback, but that's per entity type and not per bundle.

I don't believe we wanted to solve syncing book titles as in not make them editable anymore.

Status:Needs review» Needs work

The last submitted patch, 1966298-menu-bundle-93.patch, failed testing.

Status:Needs work» Needs review
StatusFileSize
new2.68 KB
new6.01 KB
PASSED: [[SimpleTest]]: [MySQL] 55,692 pass(es).
[ View ]

Looks like the property type is already used for other purposes. Changed that to bundle instead - locally tests look better again.

Status:Needs review» Needs work
Issue tags:-Needs tests, -D8MI, -sprint, -language-content

The last submitted patch, 1966298-menu-bundle-96.patch, failed testing.

Status:Needs work» Needs review
Issue tags:+Needs tests, +D8MI, +sprint, +language-content

#96: 1966298-menu-bundle-96.patch queued for re-testing.

Currently there's one question left: How to properly sync the book node title to the menu title?

HEAD already manages that: book_node_update() calls _book_update_outline(), so nothing special needed in this patch for that.

+++ b/core/modules/book/book.module
@@ -40,6 +40,29 @@ function book_help($path, $arg) {
+    if (strpos($entity->menu_name, 'book-toc-') !== FALSE) {

We want book-toc- to be at the beginning, not just anywhere, so === 0.

+++ b/core/modules/menu_link/lib/Drupal/menu_link/Plugin/Core/Entity/MenuLink.php
@@ -254,6 +265,18 @@ public function id() {
+    // If the bundle key is set - use it.
+    $entity_info = $this->entityInfo();
+    if (!empty($entity_info['entity_keys']['bundle'])) {
+      return $this->{$entity_info['entity_keys']['bundle']};
+    }
+    return parent::bundle();

Can this just be return $this->bundle;?

+++ b/core/modules/shortcut/shortcut.module
@@ -39,6 +39,29 @@ function shortcut_help($path, $arg) {
+    if (strpos($entity->menu_name, 'shortcut-') !== FALSE) {

=== 0

Yeah I also verified just now in code that the book title synching works.

StatusFileSize
new1.82 KB
new5.79 KB
PASSED: [[SimpleTest]]: [MySQL] 55,657 pass(es).
[ View ]

Thanks for the review! patch updated -> all points addressed.

Status:Needs review» Needs work
Issue tags:-Needs tests, -D8MI, -sprint, -language-content

The last submitted patch, 1966298-menu-bundle-101.patch, failed testing.

Status:Needs work» Needs review
Issue tags:+Needs tests, +D8MI, +sprint, +language-content

#102: 1966298-menu-bundle-101.patch queued for re-testing.

Status:Needs review» Needs work
Issue tags:-Needs tests, -D8MI, -sprint, -language-content

The last submitted patch, 1966298-menu-bundle-101.patch, failed testing.

Status:Needs work» Needs review
Issue tags:+Needs tests, +D8MI, +sprint, +language-content

#102: 1966298-menu-bundle-101.patch queued for re-testing.

Status:Needs review» Reviewed & tested by the community

The test fails were random, now it passes fine.

Title:Introduce menu link bundles per menusChange notice: Introduce menu link bundles per menus
Priority:Major» Critical
Status:Reviewed & tested by the community» Active

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

Title:Change notice: Introduce menu link bundles per menusIntroduce menu link bundles per menus
Priority:Critical» Major
Status:Active» Fixed

I've updated the menu links change notice with:

Menu bundles

Although menu items are not fieldable, they got bundles! The menu module handled items have bundles for each menu created. The book module and shortcut module also uses menu items to store values. Those are one bundle each. So all book menu items are in the same bundle and all shortcut menu items are in the same bundle as well.

Linked in to this issue. http://drupal.org/node/1914008

I started on #1945226-49: Add language selector on menus and could use some help there.

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

#20 (donquixote):

One concern I previously had about bundle-per-menu:

Some sites have one menu per language, to allow having different menu structure (beyond just title translation) per language. These would result in a ton of bundles, totally overhead.

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.

#88 (Gabor):

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

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

In line with #113, in reading the committed patch again, I am wondering why we didn't just do

<?php
public function bundle() {
  return
$this->menu_name;
}
?>

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?

It seems strange that book.module and shortcut.module have to alter in the bundle on every load

See #91.

Re #116: Ahh, that was super helpful, thanks. I had missed that part of the discussion. And also overlooked the attachLoad() hunk.

Issue summary:View changes

Add followup

Issue tags:-sprint

Remove sprint again :)

Don't suppose anyone knows the answer to this one entity type menu_item has bundles but no field definition?.

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

Issue summary:View changes

added bundle name = tools followup