Check test coverage for menu link content entities with entity query

Let's assign to the only one who really knows about this kind of stuff.

The 'bundle' property for menu links was added in #1966298: Introduce menu link bundles per menus (a patch that I still don't agree with :)) and it's not something that should be stored or queried. It's actually a 'computed' field, but not marked as such since menu links are not yet converted to the new API.

That said, we should actually just remove it and set the 'bundle' entity key for menu links to 'menu_name' in menu_entity_info_alter(). Not sure why it wasn't done like this in the first place...

Yup :)

+++ b/core/modules/menu/lib/Drupal/menu/Tests/MenuTest.php
@@ -692,6 +692,25 @@ public function testMenuParentsJsAccess() {
+      ->condition($entity_info->getBundleKey('bundle'), 'tools');

You want getKey(), bundle_keys defines how the keys are named on the bundle objects, e.g. node_type.

It's used 3 times in core, only one of those is correct (field_extract_bundle()), and that is no longer used, so we should drop it completely.

That said, I don't really get the menu_link bundle/menu_name stuff either :)

Created #2177971: Remove bundle_keys annotation and related methods ( thanks for reminding me), that should be a fun patch to write :)

Given that both amateescu and berdir are okay with the approach.

Given that both amateescu and berdir are okay with the approach.

For the record, what I meant is "I have no clue if this is correct" :)

This was discussed for a looong time....

The whole bundle workaround was done this way so we don't make 'menu_name' the official bundle key for this entity type (based on my objections in the issue linked in #7), but it's clear that it can get confusing when you try to use EntityQuery with that 'computed' property..

I'm not really sure how to proceed here. While I still think that menus should not act as bundles for menu links, it seems that pretty much everyone else doesn't agree with that, so I'll have to go with the flow.

What would help here would be to make the bundle() method return $this->{$this->entityInfo()->getKey('bundle')};, so contrib can easily override the bundle key and not replace the MenuLink class entirely.

@amateescu: ContentEntityBase should take care of that,

So IIRC the use-case for differentiating between menu name and bundle was that you could have menu links from different bundles in one menu. Because field instances are per-bundle, this would allow to attach an image field only to second-level menu links in a menu tree, for example. Also both book and shortcut module used to use that functionality in some way which I can't remember right now.

I don't see why we can't just store menu_name and bundle separately. The default implementation that if 'bundle' is not set, then 'bundle' == 'menu_name' can stay, but then we can leave the flexibility in place for people that need it.

And if we decide that there should not even be that relation then we simply remove that default initialization. (If I understand him correctly, that is @amateescu's position?!)

Re-roll.

So this is at least major as a) menu links are a central aspect of Drupal and entity queries not working for them is pretty bad and b) we need to figure out what to do here in order for menu links to be usable in the real world during D8 cycle.

I just looked at some of the original issues and also the current implementations of menu link bundles. Since shortcuts are no longer menu links they are no longer relevant to this discussion. book.module, however, still manages a single bundle for all it's book links across multiple menus. The fact that it does that, however, is completely unused. So that part would need to be removed from the patch as well.

Basically we need to decide what to do here. I'll try to summarize the options once again in oder to try and bring this forward a bit:

A) $menu_name === $bundle

This is the intuitive concept for menu link bundles. Bundles separate entities of the same type from another and in reality menu links are mostly separated from each other by a different menu name.

Two use-cases are not supported with this however:

1) Many menus, one bundle: As changing bundles of entities is not something we generally support, A) means that you cannot move menu links between menus. Having menu links from menus have the same bundle would allow moving menu links between those links willy-nilly.

2) One menu, many bundles: If you have fieldable menu links, it might only make sense to be able to attach fields to a certain level in your menu, i.e. to your top-level menu links but not the second-level ones. Since field instances and entity displays are per-bundle it is not possible to have those differ between multiple links in one menu if they are all the same bundle.

B) $bundle != $menu_name

This would allow a great amount of flexibility including the use-cases described above. As it is currently implemented, we could keep the behavior of making the bundle the menu name by default, if no other bundle is set. That would allow contrib UIs to go crazy with A1) and A2). I'm not aware of any performance problems with this but (perhaps, arguably) this increases the maintenance and complexity burden of menu links.

Personally I certainly think the use-cases in A1) and A2) are pretty valid so I would tentatively pick B), but I'm not bent on that very much. I just think we need to do *something* here. If we want #11 (which is sort of A)) and not #1 (which is sort of B)) I would not block that in any way.

Just a quick note on the book topic, haven't read everything yet. #2084421: Phase 2 - Decouple book module schema from menu links will mean that book will no longer (mis-)use menu links directly, and that's a beta blocker.

@tstoeckler, thanks for the in-depth writeup. As I mentioned before, I'm still very much in favor of #20 B), and if that means we'll have duplicate data in the 'bundle' and 'menu_name' columns of the menu_links table, so be it :)

19: drupal_2177611_19.patch queued for re-testing.

Hm. Just retesting old NR majors, but should this be postponed on a resolution for #2256521: [META] New plan, Phase 2: Implement menu links as plugins, including static admin links and views, and custom links with menu_link_content entity, all managed via menu_ui module?

Yes, please postpone.

No need to wait on #2256521: [META] New plan, Phase 2: Implement menu links as plugins, including static admin links and views, and custom links with menu_link_content entity, all managed via menu_ui module anymore.

Add bundle column to menu_link schema.

We have a bundle column, though it defaults to 'menu_link_content' always currently.

That probably means the bug has been fixed, we should just check the test coverage in HEAD covers what's in the patch here.

The structure change completely for this feature. Module "menu_link" it does not exist anymore it replaced by "menu_link_content" with other logic and covered with several tests. So I think this patch can't be rerolled

Applied patch against 9.3.x

Rerolled the patch #45 with Drupal 9.5.x and 10.1.x.

This issue is being reviewed by the kind folks in Slack, #need-reveiw-queue. We are working to keep the size of Needs Review queue [2700+ issues] to around 200, following Review a patch or merge require as a guide.

As #40 mentioned I don't think this can be rerolled.

Is the reported issue still an problem in D10 with the menu_link_content module?

Adding related issue #2915792: MenuLinkContentAccessControlHandler does not allow "view" access without admin permission, making these entities inaccessible via REST, JSON API and GraphQL and entity reference fields.

If still a valid task please reopen updating issue summary for D10.

Thanks.