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

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.

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.

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.

FWIW, I've traced an issue in which saving a page was leading to a WSOD to stuff that seems to be related to this issue. In the site, which also uses menu_item_extras (though I don't think that's relevant), we were seeing an attempt at updating a node resulting in:

Drupal\Core\Entity\EntityStorageException: SQLSTATE[23000]: Integrity constraint violation: 1048 Column 'bundle' cannot be null: UPDATE "menu_link_content" SET "revision_id"=:db_update_placeholder_0, "bundle"=:db_update_placeholder_1, "uuid"=:db_update_placeholder_2, "langcode"=:db_update_placeholder_3 WHERE "id" = :db_condition_placeholder_0; Array ( [:db_update_placeholder_0] => 56 [:db_update_placeholder_1] => [:db_update_placeholder_2] => 3e5d49ee-3087-4200-a9bc-caf63f09dd06 [:db_update_placeholder_3] => en [:db_condition_placeholder_0] => 18 ) in Drupal\Core\Entity\Sql\SqlContentEntityStorage->save() (line 817 of /var/www/html/docroot/core/lib/Drupal/Core/Entity/Sql/SqlContentEntityStorage.php).

Looking at what mapToStorageRecord in SqlContentEntityStorage is doing, I saw that $entity->bundle->value == 'main' but the code is trying to retrieve the bundle value from $entity->bundle->target_id. The difference seems to be due to ContentEntityBase having the following logic:

    if ($entity_type->hasKey('bundle')) {
      if ($bundle_entity_type_id = $entity_type->getBundleEntityType()) {
        $fields[$entity_type->getKey('bundle')] = BaseFieldDefinition::create('entity_reference')
          ->setLabel($entity_type->getBundleLabel())
          ->setSetting('target_type', $bundle_entity_type_id)
          ->setRequired(TRUE)
          ->setReadOnly(TRUE);
      }
      else {
        $fields[$entity_type->getKey('bundle')] = BaseFieldDefinition::create('string')
          ->setLabel($entity_type->getBundleLabel())
          ->setRequired(TRUE)
          ->setReadOnly(TRUE);
      }
    }

During the call above, the field definition is a string but the table mapping is for an entity reference. Clearing the caches makes saving work again so I guess something the bundle entity type is somehow getting set and the definition cached somewhere along the road. (I've spent long enough on this already, so won't chase this down further right now).

In any case, the patch helps, though it looks like I will also need to patch menu_item_extras if someone hasn't done that already.

I've updated the merge request for the 10.x branch. This included removing the last commit, which sought to revert the changes removing setting the bundle value.

nigelcunningham → changed the visibility of the branch 2987537-custom-menu-link-11.x to hidden.

nigelcunningham → changed the visibility of the branch 2987537-custom-menu-link-10x to hidden.

nigelcunningham → changed the visibility of the branch 2987537-custom-menu-link-11.x to active.

nigelcunningham → changed the visibility of the branch 2575577-d11-comment-11-approach to hidden.

Sorry - pushed branch to wrong issue fork.