Set MenuLinkContent getEntity to public visibility

Looks great!

Patch works as expected!
Thanks!

Sorry, my patch was not necessary.
I recreated the patch because the original failed, but the fail was not related to the patch.

Seems to be working, removing the duplicate patch from @StijnStroobants and making the original one from @Grimreaper the default one.

Putting it back to reviewed and tested since it's still working

Can you elaborate on what the use-case is here? We like to limit the API we expose so want to make sure there is a genuine need for this.

Also, if we're making it public - having at least one usage of it in core would be useful - so a test would probably be the easist thing there, because otherwise, someone might change it back without realising

Hello,

here's an updated patch for an automated test, thanks for review!

+++ b/core/modules/menu_link_content/tests/src/Unit/MenuLinkPluginTest.php
@@ -28,4 +28,13 @@ public function testGetInstanceReflection() {
 
+  /**
+   * @covers ::getEntity
+   */
+  public function testVisibilityGetEntity() {
+    $class = new \ReflectionClass(MenuLinkContent::class);
+    $instance_method = $class->getMethod('getEntity');
+    $this->assertEquals(true, $instance_method->isPublic());
+  }
+

This test isn't necessary, we know PHP visibility works. Do we have an existing test in core that checks that this actually returns an entity already?

Also if the method is going to be public it should probably be added to MenuLinkContentInterface.

Tagging for issue summary update too, and Needs Change Record because this is an API addition - the change record should include an example use-case.

This time with the tags.

This could be very usefull. Here is the patch with the method added in the interface and the comments

And here is the same patch that should work for 8.6.x

Edit : woops, not the right interface

Here is the patch for Drupal 8.6.x (without the interface changes, I needed it for a project)

This is still at NW for all the reasons mentioned above.

I have re-rolled the patch #25

+1

The translatable_menu_link_url module could also use this to reduce a bunch of entity loads they've manually coded.
We would also use it for some custom menu rendering.

In a project of mine, menu link content should not be visible if $entity->access('view') is not allowed. Therefore, I needed to access the menu link content's entity.

+1 for opening this issue and thank you for the work.

I confirm patch works as expected.

Thanks for proposing this issue!

This still needs a change record and an issue summary update. It may also need automated tests to confirm the method works properly when called externally. We should check for existing test coverage of the method and document what already exists. If the method doesn't already have test coverage, then we need to add a simple test for it.

Finally, we probably want subsystem maintainer approval to increase the visibility of this part of the API, so tagging for that. (If we don't receive subsystem maintainer review in a timely fashion, the framework managers can evaluate it instead.)

Thanks!

Also see #18 and #12. Three committers have now requested these things. :) Let's not mark it RTBC again until those things are addressed. Thanks!

Not sure why the interface changes got removed, but I've added it back.

I'm tagging this as DX because it is a bit weird that you need to load the entity when needed (i.e. how do you retrieve a translated title of a menu from the plugin?) when it is already on the object.

I've tried to find a good usage for core so we can add a ad-hoc test, but haven't been able to find any, however, as some have pointed out, contrib modules could benefit of this visibility increase.

Wrong, patch, this is the right one.

Updating the summary shortly.

Ah wrong interface, we need to create one I think

Replying to @pcambra for a real use case in core is, for instance, if you want to check access by language.

+++ b/core/modules/menu_link_content/src/Plugin/Menu/MenuLinkContentInterface.php
@@ -0,0 +1,24 @@
+interface MenuLinkContentInterface extends MenuLinkInterface, ContainerFactoryPluginInterface {

A bit of a nitpick, but I don't think CFPI should be on here. It should stay on the class.

If a plugin class has DI, then that's an implementation detail of that particular class. It's not a behaviour we expect menu link plugins to have in general.

Hi
I was faced this issue, in our context, we are using graphql-4.x producers to fetch the menu via GraphQl calls, in a multilanguale website, we must return the current language of the menu_link_content properties, and in a decouled Drupal website the language context is not easy to deal with, the graphQl Producer method had to access to the menu_link_content entity to be able to get correct field languages.
Now our problem and after updating to last Drupal Core 9.4.8, the pathes here, are not applied any more onour Drupal Deployement pipeline (that uses composer install)
I agree with @joachim, " I don't think CFPI should be on here. It should stay on the class."

Added reroll of patch #46.

I have fixed CS error.

Fixed failed commands on #48
Attached interdiff patch

Re-rolled patch against #41 in drupal 10.1.x

Re-rolled patch against #41 in drupal 10.1.x-dev

The Needs Review Queue Bot tested this issue. It either no longer applies to Drupal core, or fails the Drupal core commit checks. Therefore, this issue status is now "Needs work".

Apart from a re-roll or rebase, this issue may need more work to address feedback in the issue or MR comments. To progress an issue, incorporate this feedback as part of the process of updating the issue. This helps other contributors to know what is outstanding.

Consult the Drupal Contributor Guide to find step-by-step guides for working with issues.

Question: Per #18

Also if the method is going to be public it should probably be added to MenuLinkContentInterface.

Is this accurate? We're looking at the menu link plugin class, which extends MenuLinkBase which implements MenuLinkInterface

MenuLinkContentInterface is about a content entity class interface.

Should it be on MenuLinkInterface or MenuLinkContentInterface?

I think the patch in #46 #47 #48 #49 #50 #51 seems incorrect re-rolls.
All are missing the new interface added in patch #40
Hiding all 5 patches.

- Adding a new test case to validate the getEntity method and validate the return value.
- CR added here https://www.drupal.org/node/3361300

Been 3 years without submaintainer review so moving to framework.

To get this in front of a committer for their thoughts.

+++ b/core/modules/menu_link_content/src/Plugin/Menu/MenuLinkContent.php
@@ -9,11 +9,12 @@
 use Drupal\Core\Plugin\ContainerFactoryPluginInterface;
 use Symfony\Component\DependencyInjection\ContainerInterface;
+use Drupal\menu_link_content\MenuLinkContentInterface as MenuLinkContentEntityInterface;
 
 /**
  * Provides the menu link plugin for content menu links.
  */
-class MenuLinkContent extends MenuLinkBase implements ContainerFactoryPluginInterface {
+class MenuLinkContent extends MenuLinkBase implements MenuLinkContentInterface, ContainerFactoryPluginInterface {
 

It's a bit confusing having two classes called MenuLinkContentEntityInterface. I don't think I realised this would happen when I asked for the method to be added to the interface.

Sorry to contradict myself from 2019, but I think we should not add this interface and just make the method on the plugin class public instead.

I checked for duplicate test coverage and couldn't find any, so that bit is fine.

Done, removed the additional interface and test is passing on local.

Oops, accidentally removed keyword.
Putting back.
Interdiff is against #54
Hiding #59

OK this looks good.

It just being a public method on a plugin means there's no official bc guarantee, but in practice unless we do a major reworking of the menu links system again I can't see this changing dramatically, and for core I think it makes more sense than adding an interface for a plugin implementation.

Committed 7961aba and pushed to 11.x. Thanks!

10.2.x will be branched from 11.x later this year. I’m not sure if the introduced in branch should 11.x or 10.2.x but the version is correct.