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

Created on 23 July 2018, almost 6 years ago
Updated 6 April 2023, about 1 year ago

The menu_link_content entity has only one bundle in drupal core (with the same name as the entity).

During the development cycle before Drupal 8 stable there was an implementation for bundles with this entity but it ended up having one single bundle just like the user entity.

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. 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 somehow override the preCreate() method on the current class entity.

πŸ“Œ Task
Status

Needs work

Version

10.1 ✨

Component
Menu systemΒ  β†’

Last updated 4 days ago

Created by

πŸ‡·πŸ‡ΊRussia Chi

Live updates comments and jobs are added and updated live.
  • Needs release manager review

    It is used to alert the release manager core committer(s) that an issue significantly affects the overall technical debt or release timeline of Drupal, and their signoff is needed. See the governance policy draft for more information.

Sign in to follow issues

Comments & Activities

Not all content is available!

It's likely this issue predates Contrib.social: some issue and comment data are missing.

  • @rodrigoaguilera opened merge request.
  • Status changed to Needs review over 1 year ago
  • πŸ‡ͺπŸ‡ΈSpain rodrigoaguilera Barcelona

    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 make 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 update: 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.

  • πŸ‡¬πŸ‡§United Kingdom AaronMcHale Edinburgh, Scotland

    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.

  • πŸ‡ΊπŸ‡ΈUnited States pwolanin

    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 β†’

  • πŸ‡ͺπŸ‡ΈSpain rodrigoaguilera Barcelona

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

  • Status changed to Needs work about 1 year ago
  • πŸ‡ΊπŸ‡ΈUnited States smustgrave

    Left some comments on the MR.

    Also it had a failure that seems legit.

Production build 0.69.0 2024