Custom menu link entity type should not declare "bundle" entity key

@Chi Do you mean MenuLinkContent class should not have "bundle" key declared in entity_keys?

 *   entity_keys = {
 *     "id" = "id",
 *     "label" = "title",
 *     "langcode" = "langcode",
 *     "uuid" = "uuid",
 *     "bundle" = "bundle",
 *     "published" = "enabled",
 *   },

In https://cgit.drupalcode.org/drupal/tree/core/modules/menu_link_content/s...

Or can you add more information?

Let us see what the test-bot say.

Like Drupal 7 even if the content entity does not has any bundle it has one default bundle same as entity name? In this case, shouldn't `bundle` key is supposed to be there?

Actually the idea with menu links was to explicitly have a bundle so that contrib could utilize that to e.g. add different fields for different types of menu links. So I'm not sure why this patch wants to remove it.

@tstoeckler As you explained the purpose of `bundle` key for custom menu link entity. Should we add this to documentation?

@Chi, so menu link bundles are not controlled by a config entity type. Instead a contrib/custom module could implement hook_entity_bundle_info() to hardcode a new bundle and attach custom logic to that bundle.

I think we should go back to the original purpose of removing the bundle key that core does not use at all and fills the database.

The user entity has no bundle key but contrib modules can alter that and it. An example:

https://git.drupalcode.org/project/user_bundle/blob/8.x-1.x/user_bundle....

I wouldn't care about the menu_link_content implementing its own way of being mono bundle but is making more difficult to add fieldable menu items.
This part of field_ui expect mono bundle entities like user to have no bundle key:

https://git.drupalcode.org/project/drupal/blob/8.8.x/core/modules/field_...

So if you want for the field_ui module to automatically ad the proper routes workarounds like this need to be applied.
https://git.drupalcode.org/project/menu_item_fields/blob/8.x-1.x/modules...

I feel we should not change field_ui to support the menu_link_content entity but to be consistent in the way mono bundle entities are implemented in core.

To illustrate more this is the preCreate method on the entity that implements its own way of being mono bundle.
https://git.drupalcode.org/project/drupal/blob/8.8.x/core/modules/menu_l...

As far as I know menu_item_extras is the only contrib module that implements bundles for menu items and for them the disruption would be to add a check for the existence of the bundle key to add it themselves. In fact for menu_item_extras the column they actually use for the bundle is not bundle but menu_name so when we get rid of:

 *   entity_keys = {
...
 *     "bundle" = "bundle",
 *     "published" = "enabled",
 *   },

They can do something like:

 *   entity_keys = {
...
 *     "bundle" = "menu_name",
 *     "published" = "enabled",
 *   },

I am open to the possibility of having bundles for menu_link_content entities in the future but I feel having fields + view modes in core for menu_link_content entities is simpler and will solve many use cases that bundles can also solve. That is why I started the menu_item_fields module.

We discussed a bit also about bundles in menu items on the following issue
#2311295: Introduce MenuLinkContent bundles

So the general idea is that menu_link_content shouldn't be different than user at having a single bundle to play nice with field_ui and also be a good example for other single bundle entities. In the current situation we kind of half-care for contrib modules that might want to implement bundles since they can add the bundle key themselves but also force them to override the entity class to get rid of that preCreate().

Let see what the testbot thinks about removing the bundle from the module.

The patch was missing the update of the schema. Let's hope we get less errors.

I also updated the issue summary.

As expected there is more tests that assume there is a bundle

+++ b/core/modules/menu_link_content/menu_link_content.install
@@ -42,3 +44,25 @@ function menu_link_content_update_8601() {
+  if (\Drupal::entityTypeManager()->getStorage('menu_link_content') instanceof SqlEntityStorageInterface) {
+    if ($schema->tableExists($entity_type->getBaseTable())) {
+      $schema->dropField($entity_type->getBaseTable(), 'bundle');
+    }
+  }

So I was wondering why this is necessary. I guess the entity update manager will not uninstall the field if there are existing field values. And I guess we cannot replace the field values with NULL because the column is marked as NOT NULL. Is that the reasoning? Still a bit unfortunate, but I cannot come up with a better solution then, either... :-/

@tstoeckler It is necessary to delete the column even when the table is empty. I don't really know why. I expected the column to be deleted without that piece of code you highlight.

This piece of code is based on the function media_update_8700() .

Working on this at DCScot19.

So I was wondering why this is necessary. [..] Is that the reasoning?

No. The reason appeared to be the updateEntityType() before the uninstallFieldStorageDefinition(). The update manager needs the right info from the entity type in order to delete its field. And if the field is removed from the entity too early, then the uninstallation (is that a word? :D ) doesn't happen properly.

Also patch doesn't have test coverage for the upgrade path, which I believe is required due the issues we've seen with the 'bundle' db column hanging there at some point during the work.

Tests are still running locally, hopefully testbot will be quicker... and green.

#25 works for me

^

Confirming bundle references removed on update. Updated test cases look fine.

I review these aspects for the patch:
At new installations:

• menu_link_content entity doesn't have the bundle field

At d8 instance already installed and working.

• menu_link_content entity loss the bundle field

Running menu tests:

• core/modules/menu_link_content/tests/src/Functional/Rest/MenuLinkContentResourceTestBase.php Works fine without bundle field
• core/modules/jsonapi/tests/src/Functional/MenuLinkContentTest.php Works fine without bundle field
• core/modules/menu_link_content/tests/src/Functional/Update/MenuLinkContentUpdateTest.php Test correctly if bundle is deleted
• core/modules/menu_link_content/tests/src/Kernel/MenuLinksTest.php Works fine without bundle field Works fine without bundle field

About this point:


Also patch doesn't have test coverage for the upgrade path, which I believe is required due the issues we've seen with the 'bundle' db column hanging there at some point during the work.

This patch doesn't work over path value and all incidences at core/modules/menu_link_content/tests/src/Kernel/MenuLinksTest.php for bundle are covered.

Setting RTBC as failure doesn't seem related, probably caused by #3055648: Frequent random fail in \Drupal\Tests\media_library\FunctionalJavascript\MediaLibraryTest

Can we get a change record here?

I checked some contrib modules that provide extra entity features such as menu_item_extras, and they won't be disrupted by this change:

https://git.drupalcode.org/project/menu_item_extras/blob/8.x-2.x/menu_it...

I started the draft for the change record
https://www.drupal.org/node/3065722

What do you think?

That works. Thank you for keeping this moving.

Thanks @rodrigoaguilera . I made some changes on the CR, mainly rephrasing some parts, clearly stating what we are changing and linking some examples and alternatives.

Does anything need to happen for configurable fields added to the menu link entity?

@catch
This issue is not strictly needed for that to happen, it will just make the enabling of field_ui for menu link entities a one-liner and we will clean a core mess.
I proposed the idea for configurable fields here
#3047131: Enable the field interface for creating fields and view modes on content menu items

But there is also a contrib module that demonstrates the workarounds needed to enable field UI without this change.
https://www.drupal.org/project/menu_item_fields

Basically the field_ui routes need to be added explicitly

According to #14 this would disrupt at least one contributed module. Would be good to get feedback from @volega, and that might mean this needs to be a Drupal 9 patch.

Tagging for explicit sign-off due to the API change.

Technically, changes and update function looks solid, so +1 to that from an entity API maintainer perspective.

That said, agreed about this being tricky to change. One example would be any kind clients using rest/json api to create menu links, AFAIK several if not all normalizers will fail hard if you pass field data for non-existing fields, so it is possible that this would break things.

It would arguably be a bit weird, but one option would be to just stop defining bundle as a bundle key but keeping it as a bogus field, maybe even explicitly deprecating it in some way.

> and that might mean this needs to be a Drupal 9 patch.

So the fun part is that doing it in 9.0 would break another rule that you yourself recently agreed on with @amateescu and others.. that there are no schema changes/entity definition updates between 8.8 (or 8.9?) and 9.0, so how would we remove this in D9?

Removing the novice tag as there are no longer any novice-related tasks.

There is an issue to migrate menu link bundles #3070495: Migration of menu links does not set the appropriate bundle but if they are going to be removed should that be done or not?

Anyone care to comment?

I think menu_item_extras does some interesting things with bundles. What does that mean, it means changing the bundle from menu_link_content is troublesome. I've already ran into it because of the use of that module with another little module I write recently: group_content_menu. It would make things a lot easier for group_content_menu if it was removed. But the BC implications are a little complicated and I don't think we can do that until 9.1 for full removal in 10.

Rerolled the latest patch as a MR.

From what I understand from the comments Drupal 9.2 is a good moment to do this kind of update.

I am not sure I understand @heddn comment. Can the modules that rely on the bundle field being there can just recreate it in their own update functions?

My point in #44 is that removing it from core into contrib is fine. But we'd have to do some fancy BC work around it and probably couldn't fully remove it until 10.0 Because while contrib could take it on, they wouldn't know to do so until they have a lengthy enough BC phase where we tell them to do that via deprecations. And I'm not sure with fields if that is possible. As in, how do we know where the field is created from? Core or contrib? There's not a really good way to deprecate the old data model.

Isn't this going to break things (and delete data!!) for any site that has made use of different bundles for menu link entities?

I also think this will break things: https://git.drupalcode.org/project/menu_item_extras/-/blob/8.x-2.x/src/E...

From the IS:

So the idea is that menu_link_content shouldn't be different than user at having a single bundle to play nice with field_ui and also be a good example for other single bundle entities.

For the record, I'm actually a fan of going the other direction and user following this pattern of specifying a bundle key, I have a use case for user bundles and it would mean the user_bundle module wouldn't need to hack the user annotation, giving it a slightly more supported foundation.

Together with this, we should also fix the menu_ui module to start filling the value to the bundle property, because without it we have an error on the creation of a new menu item via the Node edit form:

The website encountered an unexpected error. Please try again later.

Drupal\Core\Entity\EntityStorageException: Missing bundle for entity type menu_link_content in Drupal\Core\Entity\ContentEntityStorageBase->doCreate() (line 125 of core/lib/Drupal/Core/Entity/ContentEntityStorageBase.php).

Drupal\Core\Entity\ContentEntityStorageBase->create(Array) (Line: 1143)
Drupal\Core\Entity\ContentEntityBase::create(Array) (Line: 109)
_menu_ui_node_save(Object, Array) (Line: 338)
menu_ui_form_node_form_submit(Array, Object)

How to reproduce:
1. Get the default clean Drupal installation.
2. Apply the patch from this issue.
3. Go to /node/add/page
4. Check the "Provide a menu link".
5. Press "Save" and see the fatal error.

So the fix is to fill the bundle in the submit alter like this:

    // Create a new menu_link_content entity.
    $entity = MenuLinkContent::create([
      'link' => ['uri' => 'entity:node/' . $node->id()],
      'langcode' => $node->language()->getId(),
+     'bundle' => $values['menu_name'],
    ]);

I've updated the MR with this fix, and also have rebased on the fresh 10.1.x branch.

Menu Items Extras aims to use menu entities as a bundle for menu_link_content. But there are some historical limitations to that approach:
- fieldable menu items were not possible before drupal:8.4.x due to an issue #2903161: Fix incorrect FieldFormatter id for "weight" field in base field definition in display options.
- in core menu items in menu is portable across menu entities. With an entity bundle, the user can't move the menu_link_content entity across different menus without data loss in custom fields. Menu Item Extras introduce that limitation.
- some code did introduce the hardcoded bundle definition at the creation of menu_link_content entities. But this is redundant and removed where it was possible to do. For example #2923429: Remove 'bundle' key from menu link create method in 'Drupal\menu_link_content\Controller\MenuController' and #3322209: Menu Item Extras adding links integration issue
- the same menu can be rendered multiple times in the blocks in the same page layout. That requires the additional need to handle different view modes usage for menu_link_content entities in other blocks. That situation becomes more complicated if a specific menu block has to render different levels of menu items in different view modes.

Fieldable menu items + layout builder creates a new experience in managing the reach content of menu items without losing integration with sitemap functionality. Just check the usage of the project https://www.drupal.org/project/usage/menu_item_extras .

Fill the bundle value on MenuLinkContent creation

I added a rebase for Drupal 10.1.x in a new MR although this change will enter into Drupal 11 the earliest.

@murz
looks like you applied the patch and didn't run the database updates. Menu links should be created without bundle with this applied.

@voleger
Thanks for the details about how is to deal with the menu link entity and its weird mono-bundle.

@joachim @alexverb
Yes, the change proposed in its current state can break things for sites that use the bundle column for some purpose, mainly sites with menu_item_extras installed.
For solving that I'm thinking about adding more code to the hook_update_N to check before removing the bundle column if all values in it are "menu_link_content". In this case it should be safe.

For the the sites with menu_item_extras the module can be made responsible for maintaining the entity_keys with the "bundle" in them via hook_entity_type_build or hook_entity_type_alter. When there is a hook that adds back the bundle key to the entity definition the update_N can stop the deleting of the field. This can be achieved getting the entity definition, removing the bundle key, invoking the hooks and when it is not added back then the bundle column can be safely removed. When it is not removed is the responsibility of the hooks in contrib or custom modules to add it back when needed

That is two checks to add before actually running the part of the update that deletes data: All the links have the same bundle and there is no code adding a bundle key to the menu_link_content entity.

There would be two cases in which the update can't perform the delete. We can fail the the update by throwing an exception but I'm not sure that is a possibility.
Or some status variable can be set that enables a special mode in which the bundle is removed from the definition but added back by a hook.
This way we can later check for that special mode in upgrade_status or the status page to inform the user about the actions needed and finally remove that special mode in Drupal 12.

@AaronMcHale
user and menu_link_content being two different kinds of mono-bundle entities is a pattern that leads to many odd situations. I considered the other route of having the user entity have a fake but then the bundle becomes some kind of mandatory thing for any entity with no good examples of mono-bundle entities in core.

The key here is what @heddn wrote

There's not a really good way to deprecate the old data model.

so maybe this issue can pave the way so we are able to remove quirks from Drupal core.

Re #63: I'm not sure I'm comfortable with the idea that it should be left to contrib modules to add the bundle field when needed, I could quickly see that leading to all sorts of conflicts between modules. For instance, if two modules both attempt to add the bundle field/key but take slightly different approaches with that could cause problems with them conflicting. I think it makes more sense for Core to provide the field so that we can facilitate contrib using it and being able to do so consistently and in a reliable way.

When there is a hook that adds back the bundle key to the entity definition the update_N can stop the deleting of the field.

That also seems risky to me, can we really be sure that the field isn't being used on some sites in a way that we are not anticipating. To me it seems that there is a risk that some edge cases could come up and sites may break that we previously relying on this field, there could be sites with custom implementations which we cannot account for here.

I think the current approach is fine - we don't have to accommodate all possible contirb code that might have used this unintended behavior.

Also - if you want to define a kind of menu link content with bundle you can do that with another entity type - it doesn't have to leverage the core entity. For example: https://www.drupal.org/project/menu_link_config

@pwolanin Do you mean the current approach in core or the current approach of the MR ?

Left some comments on the MR.

Also it had a failure that seems legit.