- @rodrigoaguilera opened merge request.
- Status changed to Needs review
over 1 year ago 12:29am 11 March 2023 - πͺπΈ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 7:46pm 6 April 2023 - πΊπΈUnited States smustgrave
Left some comments on the MR.
Also it had a failure that seems legit.