add an entity Links handler, to provide menu links/tasks/actions for entity types

The Entity API contrib has code for this that can be borrowed (written by tstoeckler)

This would be really great!

I think a test should be added, similar to the RouteProviderTest. One of the test entity types in the entity_test module should updated to use this link provider, and the new test can simply verify the menu links are created as expected.

Getting something like this into core would be awesome, the set of links in DefaultContentEntityLinksProvider are added to almost every entity type, so it would be a huge DX win.

I can't think of a better way to connect the dots between the routes and links, besides using the established naming conventions. The template method pattern used is consistent with the route providers, so +1 on consistency.

This is seriously nice work and will be a huge DX win. Adding a progress review.

As for the other outstanding points, I'm not an expert on the system module, but the use of the deriver here seems reasonable and I can't think of a good alternative.

1. +++ b/core/lib/Drupal/Core/Entity/EntityType.php
   @@ -540,6 +540,13 @@ public function hasRouteProviders() {
   +  /**
   +   * {@inheritdoc}
   +   */
   +  public function hasLinkProvider() {
   +    return !empty($this->handlers['link_provider']);
   +  }
   
   +++ b/core/lib/Drupal/Core/Entity/EntityTypeInterface.php
   @@ -274,6 +274,14 @@ public function hasRouteProviders();
   +  /**
   +   * Indicates if this entity type has a link provider.
   +   *
   +   * @return bool
   +   *   TRUE if the entity type has a link provider handler; FALSE otherwise.
   +   */
   +  public function hasLinkProvider();
   

   I think maybe the reason we have methods like hasFormClasses and hasRouteProviders is because those types of handlers have sub-elements, so a check to hasHandlerClass wouldn't be sufficient.

   Might be worth just leaning on hasHandlerClass to keep our interface tidy? This is more a preference though, feel free to disagree.

2. +++ b/core/lib/Drupal/Core/Entity/Menu/BaseEntityLinksProvider.php
   @@ -0,0 +1,240 @@
   + * \Drupal\Core\Entity\Routing\DefaultHtmlRouteProvider, and makes assumptions
   + * about the names of routes that class provides.
   

   Lets add the class as a @see as well.

   Maybe we could say instead "uses the established convention for route names" or something and maybe even go as far as to describe what those conventions are.

3. +++ b/core/lib/Drupal/Core/Entity/Menu/BaseEntityLinksProvider.php
   @@ -0,0 +1,240 @@
   +   * This is a wrapper around the route provider's getRouteByName(), which
   +   * throws an exception for routes that are not found.
   

   I dug into this again after our conversation at DE, looks like a work-around to the exception based control flow would be to call getRoutesByNames([$route_name]) and check if it was empty or not. Not sure if this is the lesser of two evils.

4. +++ b/core/lib/Drupal/Core/Entity/Menu/DefaultConfigEntityLinksProvider.php
   @@ -0,0 +1,50 @@
   +      $link['description'] = t('Create and manage fields, forms, and display settings for @plural-label.', [
   +        // TODO: need input from translation experts on whether this is right!
   +        '@plural-label' => $this->entityType->getPluralLabel(),
   +      ]);
   

   I assume this is correct, because the plural label will already be translated: label_plural = @Translation("content items"),

5. +++ b/core/lib/Drupal/Core/Entity/Menu/DefaultConfigEntityLinksProvider.php
   @@ -0,0 +1,50 @@
   +    // Config entities have just one task here, but Field UI module hangs its
   +    // own tabs off it.
   

   Does field UI really do stuff with config entities?

6. +++ b/core/modules/system/tests/modules/entity_test/src/EntityUITestListBuilder.php
   @@ -0,0 +1,35 @@
   +class EntityUITestListBuilder extends EntityListBuilder {
   
   +++ b/core/modules/system/tests/modules/entity_test/src/EntityUITestTypeListBuilder.php
   @@ -0,0 +1,31 @@
   +class EntityUITestTypeListBuilder extends EntityListBuilder {
   

   Maybe list builders are the next kind of boilerplate we need to address 😅

7. +++ b/core/modules/system/tests/modules/entity_test/src/Form/EntityUITestTypeForm.php
   @@ -0,0 +1,57 @@
   +  /**
   +   * TODO: write function documentation.
   +   */
   

   {@inheritdoc}?

8. +++ b/core/modules/system/tests/modules/entity_test/src/EntityUITestTypeListBuilder.php
   @@ -0,0 +1,31 @@
   +/**
   + * Provides the list builder handler for the Entity UI Test Type entity.
   + */
   +class EntityUITestTypeListBuilder extends EntityListBuilder {
   

   Maybe we can lean on \Drupal\entity_test\EntityTestListBuilder here?

1. I see some copy-pasted code.
   For example,

   +  protected function getEditFormTaskLink($base_plugin_definition) {
   +    // Config entities have just one task here, but Field UI module hangs its
   +    // own tabs off it.
   +    if ($this->routeExists("entity.{$this->entityTypeID}.edit_form")) {
   +      $link = $base_plugin_definition;
   +
   +      $link['title'] = $this->t('Edit');
   +      $link['route_name'] = "entity.{$this->entityTypeID}.edit_form";
   +      $link['base_route'] = "entity.{$this->entityTypeID}.edit_form";
   +
   +      return $link;
   +    }
   +  }
   

   It may be replaced to

   +  protected function getEditFormTaskLink($base_plugin_definition) {
   +    // Config entities have just one task here, but Field UI module hangs its
   +    // own tabs off it.
   +    $edit_form_link = "entity.{$this->entityTypeID}.edit_form";
   +    if ($this->routeExists($edit_form_link)) {
   +      $link = $base_plugin_definition;
   +
   +      $link['title'] = $this->t('Edit');
   +      $link['route_name'] = $edit_form_link;
   +      $link['base_route'] = $edit_form_link;
   +
   +      return $link;
   +    }
   +  }
   

   Or even can make helper function like

   private function makeLink($link) {
     return "entity.{$this->entityTypeID}.$link";
   }
   

   and then use it as

   $edit_form_link = $this->makeLink('edit_form');
   

2. Should we check $this->routeExists() for $link['base_route'] also?

   +  protected function getEditFormTaskLink($base_plugin_definition) {
   +    if ($this->routeExists("entity.{$this->entityTypeID}.edit_form") && $this->routeExists("entity.{$this->entityTypeID}.canonical")) {
   +      $link = $base_plugin_definition;
   +
   +      $link['title'] = $this->t('Edit');
   +      $link['route_name'] = "entity.{$this->entityTypeID}.edit_form";
   +      $link['base_route'] = "entity.{$this->entityTypeID}.canonical";
   +
   +      return $link;
   +    }
   +  }
   

Bumping this again today after having to go through and painfully create a stock standard set of links from scratch.

One last question I had, does this meet the needs of any of the existing entity types in core? It would be great to include a real-world use of this and demonstrate how much fiddling it solves?

I'm not sure if this needs a framework manager review or not.

I found one bug: content entities actually need to add a menu link to their collection under admin/content as well as the tab link.

I'm not sure I follow--does this patch require they do so or something breaks?

Ah, that makes sense. So is there anything left to do here other than a change record and an issue summary update?

One interesting note: This code currently always generates a Delete local task.

However, core only provides a Delete local task for content entities. Delete local tasks for config entities were explicitly removed in #1834002: Configuration delete operations are all over the place. I am guessing that we don't want to revert that conclusion. So we should either define the Delete local task only for content entity types, or never.

I am also unsure what our convention is for defining a List local task for the collection. Core seems to do it for many config entity types.

Also wanted to say that I find the current architecture confusing from a DX standpoint. I would have preferred the contrib Entity API approach of provider per link type, cause that gives me a clear mental mapping of 1 provider to 1 file. Reading through the current codebase required me to do several double-takes to figure out which link type I'm looking at.

I'm very excited about this one, let's do it!

I had a look at the latest patch, which btw applies cleanly against 8.9.x.

I had a go at the failing test, but was unable replicate the failure locally (tested using PHP 7.3, 7.2 and 7.1).

Let's see if the failure is actually still there with the test bot then, setting to needs review for now.

Hiding patches now there's an MR

Left a review, this is a huge improvement over the current setup

I wonder if its worth converting an existing entity type, e.g. taxonomy perhaps so we also get the benefit of any existing test coverage

Agree this is a great addition, also left some comments.

I wonder if its worth converting an existing entity type, e.g. taxonomy perhaps so we also get the benefit of any existing test coverage

+1 one to that, let's see how many we can do here, and open follow-ups for any that aren't a simple change.

Should we also be generating contextual links?

Good point, that feels like reason enough to forego contextual links

Added comments to MR.

> This is still blocked on the problem of how to handle menu link IDs which will change.

I would not change existing entity types here, verify on test entity types and conversion can be a follow up

Hi Joachim thanks for the work you done on this issue.

I tried to test the patch locally, but seems it need work. when I applied this patch seems the link for entity type collection is missing. It also misses the local task links for revisions and translation.

drush gen entity:content

Welcome to content-entity generator!
––––––––––––––––––––––––––––––––––––––

Module machine name [web]:
➤ entity_test

Entity type label [Entity test]:
➤

Entity type ID [entity_test]:
➤

Entity base path [/entity-test]:
➤

Make the entity type fieldable? [Yes]:
➤

Make the entity type revisionable? [No]:
➤ Yes

Make the entity type translatable? [No]:
➤ Yes

The entity type has bundle? [No]:
➤ Yes

Create canonical page? [Yes]:
➤ Yes

Create entity template? [Yes]:
➤ Yes

Create CRUD permissions? [No]:
➤ Yes

Add "label" base field? [Yes]:
➤ Yes

Add "status" base field? [Yes]:
➤ Yes

Add "created" base field? [Yes]:
➤ Yes

Add "changed" base field? [Yes]:
➤ Yes

Add "author" base field? [Yes]:
➤ Yes

Add "description" base field? [Yes]:
➤

Create REST configuration for the entity? [No]:
➤ No

The following directories and files have been created or updated:
–––––––––––––––––––––––––––––––––––––––––––––––––––––––––––––––––––
• /app/web/modules/custom/entity_test/entity_test.links.action.yml
• /app/web/modules/custom/entity_test/entity_test.links.contextual.yml
• /app/web/modules/custom/entity_test/entity_test.links.menu.yml
• /app/web/modules/custom/entity_test/entity_test.links.task.yml
• /app/web/modules/custom/entity_test/entity_test.module
• /app/web/modules/custom/entity_test/entity_test.permissions.yml
• /app/web/modules/custom/entity_test/config/schema/entity_test.entity_type.schema.yml
• /app/web/modules/custom/entity_test/src/EntityTestAccessControlHandler.php
• /app/web/modules/custom/entity_test/src/EntityTestInterface.php
• /app/web/modules/custom/entity_test/src/EntityTestListBuilder.php
• /app/web/modules/custom/entity_test/src/EntityTestTypeListBuilder.php
• /app/web/modules/custom/entity_test/src/Entity/EntityTest.php
• /app/web/modules/custom/entity_test/src/Entity/EntityTestType.php
• /app/web/modules/custom/entity_test/src/Form/EntityTestForm.php
• /app/web/modules/custom/entity_test/src/Form/EntityTestTypeForm.php
• /app/web/modules/custom/entity_test/templates/entity-test.html.twig

I used the above drush command and generated test entity. After that I deleted below four ymls and added the patch from #67 and added "link_provider" = "Drupal\Core\Entity\Menu\DefaultContentEntityLinksProvider", in my entity annotation definition and cleared cache and check the entity type collection list , but it is missing.
• /app/web/modules/custom/entity_test/entity_test.links.action.yml
• /app/web/modules/custom/entity_test/entity_test.links.contextual.yml
• /app/web/modules/custom/entity_test/entity_test.links.menu.yml
• /app/web/modules/custom/entity_test/entity_test.links.task.yml

Basically menu link for entity.entity_test_type.collection route is not available with the patch.

It is also good to add Derivative for contextual links and permissions as well.

@joachim thanks for the quick response.

Were you able to see entity_test_type link under structure? Which list bundles and manage fields?

Thats what missing after applying this patch.

@joachim that was missing in my code. Thanks After adding that menu link is visible under structure, but it still misses bundle list and add new bundle option in second level menu.

Pls refer attached images for difference between node and custom entity. You can see that second level menu links are not visible while admin toolbar module is enabled.

Got it thanks @joachim

I've tested the MR and as far as I can see it's working well.

Contextual links

I was expecting the contextual links to be part of this proposal, until I found in the issue comments the reasons why this wasn't included. Just saying from a 'consumer' point of view'.

Parent base route

I think the DefaultContentEnityLinksProvider might hardly ever be used 'out of the box' for the lack to control the weight, base route and control over the UI as too many tabs and menu items might be added to the top admin/content menu item.

It's easy enough to extend this class, however could we consider adding a key to the entity type definition that can be used, such as 'link_provider_base_route' and 'link_provider_weight'?

In regards to the weights, I just want to highlight this issue #3306388: Delete local task should be the last tab for content entities, the proposal is awaiting sign off at the Usability Group (hopefully I'll get a chance to present it today), but it's a strong case for providing sensible default weights, at least for the Delete local task.

The proposed resolution in #3306388: Delete local task should be the last tab for content entities was accepted at #3307567: Drupal Usability Meeting 2022-09-09, so that issue is good to proceed.

Would anyone here also be willing to work on that issue?

This issue should also take account of the solution in that issue, as it will directly impact the solution here.

Hi @joachim ,

Seems I observed one more issue. After applying this patch and patch from #3028213-23 I am seeing two menu links for custom entity bundle collection. Please look into the attached images.

I have just generated entities as discussed in #69 and added link_provider in both entity and entity type annotation.

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.

The Needs Review Queue Bot tested this issue. It 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.

Looking into MR 3441 I see tests were added so removing that tag.

Remaining tasks mention

Get a review from one of the managers of the plugin system. YAML plugins are not discoverable in core/lib (unlike annotation-based plugins), and so the current patch declares the plugin derivatives that are in core/lib/Core/Entity in system module's YAML files. This approach needs framework manager review.

So tagging for that.

The 1 failure in the MR seems valid.

I have a question about entity collection listing: Is there a guideline, policy or d.o issue about where the entity list should go in the admin menu?
something like:
- if you extend the ContentEntity class, then your entity listing should be under admin/content/my-entity-plural
- if you extend the ConfigEntity class, then your entity listing should be under admin/structure/my-entity-plural

Some of the examples where this "guideline" does not apply:
Shortcuts (ContentEntity from core) listing is under /admin/config/user-interface/shortcut
Taxonomy terms (ContentEntity from core) listing is under /admin/structure/taxonomy/manage/%vocabulary%/overview
Image styles (ConfigEntity from core) listing is under admin/config/media/image-styles
Workflows (ConfigEntity from core) listing is under admin/config/workflow/workflows
Languages (ConfigEntity from core) listing is under admin/config/regional/language
Entity Browsers (ConfigEntity from entity_browser module) listing is under admin/config/content/entity_browser
Newsletters (ConfigEntity from simplenews module) listing is under /admin/config/services/simplenews
QRCodes (ContentEntity from qrcode module) listing is under /admin/config/services/qrcode

I have a question about entity collection listing: Is there a guideline, policy or d.o issue about where the entity list should go in the admin menu?

While we were working on the effort to reorganise blocks in the admin UI, we were guided by structured content principles, and we applied these to the Drupal admin menu as follows:

• Content: this is where you create individual items of content (whether that be nodes, blocks, media, etc).
• Structure: this is where you build the structures that your content uses (aka bundles and fields), along with different representations of your structure (thinking Views here).
• Appearance: this is where you decide what the look and feel of your site as a whole (aka selecting a theme, and hopefully in 10.2 the global block layout).

There's a nice organisation there, from individual items up to the whole site.

This isn't formally documented anywhere that I'm aware of (maybe it should be), but we will likely use this to help inform any future reorganisation efforts of the admin UI.

In terms of how that relates to the question on this issue, it allows us to think about this a little more holistically. For collections of content entities, in general those probably fit best under Content; If it's a collection of config entities, those could live under Structure if they facilitate structured content (aka bundles and fields), but those could also live under Configuration if they are not related to structured content.