Implement auto-route generation for all core entities and convert all of the core entities.

We already had an issue with a patch, any reason for new one?

I do agree that the other issue had valuable discussions already, but it is quite outdated especially in terms what we plan now.

AFAICT this is blocked on #2281645: Make entity annotations use link templates instead of route names.

Yes it is.

Update the status.

Here is a basic start.

+++ b/core/lib/Drupal/Core/Entity/Routing/DefaultRouteProvider.php
@@ -0,0 +1,60 @@
+    $route = (new Route(sprintf("/%s/{%s}", $entity_type_id, $entity_type_id)));
...
+    $route = (new Route(sprintf("/%s/{%s}/delete", $entity_type_id, $entity_type_id)));
...
+    $route = (new Route(sprintf("/%s/{%s}/edit", $entity_type_id, $entity_type_id)));

Shouldn't this use the link templates now? And be conditional based on that, e.g. don't generate a canonical route if the entity has no link template

Also thinking about patterns/ways to make it easy to override/customize, I think we talked about having a method per route or so..

Shouldn't this use the link templates now? And be conditional based on that, e.g. don't generate a canonical route if the entity has no link template

You are absolute right!

Also thinking about patterns/ways to make it easy to override/customize, I think we talked about having a method per route or so..

It would be indeed good to think about it. Do you think it makes it easier requiring a new method compared to altering the collection directly?

I think at least having a getBasePath() - which defaults to return $entity_type; would be quite useful. Especially for config entities...

@tstoeckler: #2281645: Make entity annotations use link templates instead of route names landed, so now the link templates need to have the URL anyway, there's not much point in having an admin path or base path then?

Oh, yes, sorry that was stupid. I had seen the commit, but it hadn't quite sunk in. Ignore me.

Just thinking out loud, what about looping over link templates and then have a method name pattern based on that?

foreach ($this->entityType()->getLinkTemplates() as $name => $url) {
  $method = 'build' . camelize($name) . 'Route';
  if (method_exists($this, $method) && $route = $this->$method($url)) {
    $collection->add("entity.{$entity_type_id}." . magic_conversion($name), $route);
  }
}

My idea for this is that we can easily standardize the route name pattern. Subclasses can then decide if they want to override a method (to e.g. build a specific route completely custom), or they can still override getRoutes() and do whatever they want there.

Also, EntityTestRoutes might be a good example to convert to this instead of a route provider. My goal is still to have most of the entity test stuff being able to rely on default code. No idea why we have a custom controller for add/edit there, for example..

@tsteockler: See also #2401505: Add an entity collection template for lists , that will come pretty close to the idea of a base path for many, but not all entity types. But some are quite different, e.g. the collection url for users is now /admin/people

• Fixed user edit forms
• Ensured that routes are just created when the corresponding link template exists
• Converted some of the TestRoutes over
• While doing so, ensured that the parameter conversions are set up properly.

Fixed some failures and converted taxonomy over.

...

This was closed as a dupe of #2089757: Auto generate routing entries for entities based on an 'admin_path' and 'admin_permission' annotation which was major.

I would consider this a critical bug after #2281645: Make entity annotations use link templates instead of route names, but that's just me.

I would consider this a critical bug after #2281645: Make entity annotations use link templates instead of route names, but that's just me.

Can you clarify why this blocks the release? Do you talk about the CRY argument of requiring the paths in multiple places? I am
not sure wether this is such a valid argument, given that previously it has been two places where the route name had to be entered.

Fixed some issues and converted the vocabulary entity type.

1. +++ b/core/lib/Drupal/Core/Entity/Controller/EntityViewController.php
   @@ -98,4 +100,32 @@ public function view(EntityInterface $_entity, $view_mode = 'full', $langcode =
   +   * @return string The title.
   +   * The title.
   

   Not really correct :)

2. +++ b/core/lib/Drupal/Core/Entity/Routing/DefaultRouteProvider.php
   @@ -0,0 +1,73 @@
   +    $route = (new Route($entity_type->getLinkTemplate('canonical')));
   +    $route
   +      ->addDefaults([
   +        '_entity_view' => "{$entity_type_id}.full",
   +        '_title_callback' => '\Drupal\Core\Entity\Controller\EntityViewController::title',
   +      ])
   +      ->setRequirement('_entity_access', "{$entity_type_id}.view")
   +      ->setOption('parameters', [
   +        $entity_type_id => ['type' => 'entity:' . $entity_type_id],
   +      ]);
   

   The canonical route needs to be optional as well, most config entities don't have one I think, they can't be viewed.

3. +++ b/core/modules/node/src/Entity/NodeRouteProvider.php
   @@ -15,37 +16,24 @@
   +    $collection->get('entity.node.canonical')
   +      ->setDefault('_entity_view', NULL)
   +      ->setDefault('_controller', '\Drupal\node\Controller\NodeViewController::view')
   

   This is a bit weird that we have to set it to NULL. Can't we make the thing that converts those routes intelligent enough to not overwrite an existing _controller?

4. +++ b/core/modules/system/tests/modules/entity_test/src/Entity/EntityTest.php
   @@ -28,10 +28,14 @@
   + *     "route_provider" = {
   + *       "html" = "Drupal\Core\Entity\Routing\DefaultRouteProvider",
   + *     },
   

   Should we rename the route provider to something like DefaultHtmlRouteProvider?

   Also, in #2364157: Replace most existing _url calls with Url objects, I noticed that we have a canonical route in entity_test.routing.yml, but just for this entity type, not generic, no idea way. Anyway, you can remove that one too.

5. +++ b/core/modules/taxonomy/src/Entity/TermRouteProvider.php
   @@ -0,0 +1,39 @@
   +    $collection->get('entity.taxonomy_term.canonical')
   +      ->setDefault('_title', 'Taxonomy term');
   

   I might have asked this before, but what's the point of this _title, we also have a _title callback.

6. +++ b/core/modules/taxonomy/src/Entity/VocabularyRouteProvider.php
   @@ -0,0 +1,32 @@
   +    $collection->get('entity.taxonomy_vocabulary.edit_form')
   +      ->setDefault('_entity_form', 'taxonomy_vocabulary.default')
   +      ->setDefault('_title_callback', 'Drupal\Core\Entity\Controller\EntityViewController::title');
   

   view controller title callback on the edit form? Should we have a useful default for this as well?

The canonical route needs to be optional as well, most config entities don't have one I think, they can't be viewed.

Total valid point.

This is a bit weird that we have to set it to NULL. Can't we make the thing that converts those routes intelligent enough to not overwrite an existing _controller?

Does that mean we should that uri relationships just one level up? I still don't understand why we have to special case node here at all.

Should we rename the route provider to something like DefaultHtmlRouteProvider?

Sold.

I might have asked this before, but what's the point of this _title, we also have a _title callback.

Given that the entity ID is required there is none.

view controller title callback on the edit form? Should we have a useful default for this as well?

I think the best we should do is to provide a editTitle method and set it by default? This is related to [#2403359]l

I might have asked this before, but what's the point of this _title, we also have a _title callback.

It might have been important in earlier times, but I don't think its helpful any longer.

view controller title callback on the edit form? Should we have a useful default for this as well?

Moved it to EntityController, as a more generic place.

Also converted views, and tried to increase the failure rate again.

Converted roles and comment_type as well, and maybe fixed some failures.

What does the bot say?

Bot says no :(

1. +++ b/core/lib/Drupal/Core/Entity/EntityController.php
   @@ -0,0 +1,135 @@
   +/**
   + * Provides generic entity controllers, especially for titles.
   + */
   +class EntityController implements ContainerInjectionInterface {
   

   This will then clash with #2403359: Use _title and _title_callback where possible, but that's fine :) It should remove a large, large portion of that patch. I bet many of the test fixes you will need to borrow from there.

2. +++ b/core/lib/Drupal/Core/Entity/EntityController.php
   @@ -0,0 +1,135 @@
   +   * {@inerhtdi
   

   .

3. +++ b/core/lib/Drupal/Core/Entity/EntityController.php
   @@ -0,0 +1,135 @@
   +   * @return string The title.
   +   * The title.
   

   .

4. +++ b/core/lib/Drupal/Core/Entity/EntityController.php
   @@ -0,0 +1,135 @@
   +  public function title(RouteMatchInterface $route_match, EntityInterface $_entity = NULL) {
   ...
   +  public function editTitle(RouteMatchInterface $route_match, EntityInterface $_entity = NULL) {
   ...
   +  public function deleteTitle(RouteMatchInterface $route_match, EntityInterface $_entity = NULL) {
   

   Why not also addTitle() or createTitle()?

   And perhaps renaming title() to viewTitle() would make sense, too?

5. +++ b/core/lib/Drupal/Core/Entity/EntityController.php
   @@ -0,0 +1,135 @@
   +   *   The edit entity label.
   ...
   +   *   The delete entity label.
   

   These look strange. What about The 'edit entity' title. or The entity editing route title.?

6. +++ b/core/lib/Drupal/Core/Entity/EntityController.php
   @@ -0,0 +1,135 @@
   +      return $this->t('Edit %label', $entity->label());
   

   Wouldn't Edit %entity_type %label be more appropriate?

7. +++ b/core/lib/Drupal/Core/Entity/EntityController.php
   @@ -0,0 +1,135 @@
   \ No newline at end of file
   

   .

8. +++ b/core/lib/Drupal/Core/Entity/Routing/DefaultHtmlRouteProvider.php
   @@ -0,0 +1,83 @@
   + * Provides a default implementation of an empty route provider.
   

   s/empty/entity/? :D

9. +++ b/core/lib/Drupal/Core/Entity/Routing/DefaultHtmlRouteProvider.php
   @@ -0,0 +1,83 @@
   +        ]);
   +
   +      $collection->add("entity.{$entity_type_id}.canonical", $route);
   ...
   +        ]);
   +      $collection->add("entity.{$entity_type_id}.edit_form", $route);
   ...
   +        ]);
   +      $collection->add("entity.{$entity_type_id}.delete_form", $route);
   

   Inconsistent whitespace.

10. +++ b/core/lib/Drupal/Core/Entity/Routing/DefaultHtmlRouteProvider.php
    @@ -0,0 +1,83 @@
    +      // Use the "edit" form handler, otherwise default.
    +      $operation = 'default';
    

    I wonder if it'd be better to change those who are using default to using edit instead?

11. +++ b/core/modules/comment/src/CommentHtmlRouteProvider.php
    @@ -0,0 +1,32 @@
    + * Provides html routes for the comment entity type.
    
    +++ b/core/modules/comment/src/CommentTypeHtmlRouteProvider.php
    @@ -0,0 +1,34 @@
    + * Provides routes for the comment_type entity type.
    
    +++ b/core/modules/node/src/Entity/NodeHtmlRouteProvider.php
    @@ -0,0 +1,39 @@
    + * Provides routes for nodes.
    
    +++ b/core/modules/user/src/Entity/UserHtmlRouteProvider.php
    @@ -0,0 +1,45 @@
    + * Provides routes for the user entity.
    
    +++ b/core/modules/views_ui/src/ViewHtmlRouteProvider.php
    @@ -0,0 +1,34 @@
    + * Provides routes for the view html routes.
    

    Should be made consistent. Should all mention "HTML routes".

12. +++ b/core/modules/comment/src/CommentTypeHtmlRouteProvider.php
    @@ -0,0 +1,34 @@
    +    $collection->get('entity.comment_type.edit_form')
    +      ->setOption('_admin_route', TRUE);
    +    $collection->get('entity.comment_type.delete_form')
    +      ->setOption('_admin_route', TRUE);
    

    If we'd detect bundle entity types in DefaultHtmlRouteProvider, then we could set _admin_route: TRUE automatically.

13. +++ b/core/modules/comment/src/CommentTypeHtmlRouteProvider.php
    @@ -0,0 +1,34 @@
    \ No newline at end of file
    

    .

14. +++ b/core/modules/node/node.routing.yml
    @@ -34,7 +34,7 @@ entity.node.preview:
    -    _title_callback: '\Drupal\node\Controller\NodePreviewController::title'
    +    _title_callback: '\Drupal\node\Controller\NodePreviewController::previewTitle'
    

    Why not change it to use EntityController::title() instead? (And then delete the custom title callback.)

15. +++ b/core/modules/node/src/Entity/NodeHtmlRouteProvider.php
    @@ -0,0 +1,39 @@
    +      ->setDefault('_entity_view', NULL)
    

    Why?

16. +++ b/core/modules/node/src/Entity/NodeHtmlRouteProvider.php
    @@ -0,0 +1,39 @@
    +      ->setDefault('_controller', '\Drupal\node\Controller\NodeViewController::view')
    +    ;
    

    Dangling semi-colon.

    Also: the only value of the custom controller is to set a bunch of HTML <HEAD> links. Why not just do that for all entities automatically? Then the custom code for Nodes can go away. No harm in that.

17. +++ b/core/modules/node/src/Entity/NodeHtmlRouteProvider.php
    @@ -0,0 +1,39 @@
    +    $collection->get('entity.node.delete_form')->setOption('_node_operation_route', TRUE);
    +
    +    $collection->get('entity.node.edit_form')->setOption('_node_operation_route', TRUE);
    

    Inconsistent code formatting, compared with the other route providers.

18. +++ b/core/modules/system/tests/modules/entity_test/src/Entity/EntityTestBaseFieldDisplay.php
    @@ -21,15 +21,24 @@
    + *     "route_provider" = {
    + *       "html" = "Drupal\Core\Entity\Routing\DefaultHtmlRouteProvider",
    + *     },
    

    Hm, I thought this would automatically be the default? I.e. I thought it wouldn't be necessary to define it in the annotation if you wanted to use the default HTML routes?

19. +++ b/core/modules/taxonomy/src/Entity/TermHtmlRouteProvider.php
    @@ -0,0 +1,34 @@
    +      ->setDefault('_title', 'Delete term')
    

    This line can be deleted; the default title is superior. We also don't use the hardcoded title for editing terms anymore; no need to do it for deleting then.

20. +++ b/core/modules/user/src/Entity/UserHtmlRouteProvider.php
    @@ -0,0 +1,45 @@
    +    $collection->get('entity.user.canonical')->setDefault('_title_callback', 'Drupal\user\Controller\UserController::userTitle');
    

    Inconsistent code formatting.

21. +++ b/core/modules/user/src/Entity/UserHtmlRouteProvider.php
    @@ -0,0 +1,45 @@
    +      ->setDefault('_title_callback', 'Drupal\user\Controller\UserController::userTitle')
    

    Why keep this?

22. +++ b/core/modules/views_ui/src/ViewHtmlRouteProvider.php
    @@ -0,0 +1,34 @@
    +      ->setDefault('_entity_form', NULL)
    

    ?

23. +++ b/core/modules/views_ui/views_ui.module
    @@ -61,6 +62,7 @@ function views_ui_entity_type_build(array &$entity_types) {
         ->setLinkTemplate('break-lock-form', '/admin/structure/views/view/{view}/break-lock');
    +
     }
    

    Accidental whitespace.

Thank you for your long

Why not also addTitle() or createTitle()?

And perhaps renaming title() to viewTitle() would make sense, too?

Mh, IMHO we don't have that link template defined yet, so its a little bit pointless for this issue to add it.

These look strange. What about The 'edit entity' title. or The entity editing route title.?

Went with the first one.

Wouldn't Edit %entity_type %label be more appropriate?

Well, this one seemed to be the one which is used most of the time, so I went with it. As we make things more consistent here, we can adapt it later, if needed.

Inconsistent whitespace.

Fixed

I wonder if it'd be better to change those who are using default to using edit instead?

There is clearly no actual strategy for existing code, so I would vote to keep it like that an decide in an issue whether the concept of a default form makes sense. I guess it doesn, as
we need a form for the add case as well.

Should be made consistent. Should all mention "HTML routes".

fixed.

If we'd detect bundle entity types in DefaultHtmlRouteProvider, then we could set _admin_route: TRUE automatically.

I won't do that, because I don't get why we manually add it here. The AdminRouteSubscriber should take care of it, if possible. Not sure why its added here,
but there is probably a reason.

Why not change it to use EntityController::title() instead? (And then delete the custom title callback.)

Well, its tricky as the NodePreviewController uses a different param converter.

Why?

Well, we have a custom controller, which I wanna use here (maybe not anymore in the future, but for now the amount of changes should be limited, if the patch should keep reviewable).

Also: the only value of the custom controller is to set a bunch of HTML links. Why not just do that for all entities automatically? Then the custom code for Nodes can go away. No harm in that.

Well, I think if we want to change the behaviour, we should do that separate, as it requires test coverage and what not. Its not in scope of this issue IMHO, which is about improving DX
for defining routes.

Hm, I thought this would automatically be the default? I.e. I thought it wouldn't be necessary to define it in the annotation if you wanted to use the default HTML routes?

Well, if you look at ContentEntityType and EntityType you won't see many handlers added by default. I kinda prefer to opt IN the behaviour and not require a way, to opt out of that behaviour. I don't
see a strict requirement why every entity type would require it.

This line can be deleted; the default title is superior. We also don't use the hardcoded title for editing terms anymore; no need to do it for deleting then.

fixed

Why keep this?

Oh, I first they do something different, but they do the same, just way more complex.

Alright, let's also fix stuff.

Add tag

From 2460 fails to 117! Nice! :)

Mh, IMHO we don't have that link template defined yet, so its a little bit pointless for this issue to add it.

Oh, ok, never mind then :)

As we make things more consistent here, we can adapt it later, if needed.

Fair!

so I would vote to keep it like that an decide in an issue whether the concept of a default form makes sense

I see that angle, but the code introduced here is made more complicated because of that concept. I think we want input from more people about that.

The AdminRouteSubscriber should take care of it, if possible. Not sure why its added here,
but there is probably a reason.

Because:

  protected function alterRoutes(RouteCollection $collection) {
    foreach ($collection->all() as $route) {
      if (strpos($route->getPath(), '/admin') === 0 && !$route->hasOption('_admin_route')) {
        $route->setOption('_admin_route', TRUE);
      }
    }
  }

and the routes in this patch with _admin_route set don't have paths that begin with /admin.

Well, we have a custom controller, which I wanna use here (maybe not anymore in the future, but for now the amount of changes should be limited, if the patch should keep reviewable).

Ok. Could you document this? It was very puzzling to me why you'd set something to NULL.

Well, I think if we want to change the behaviour, we should do that separate, as it requires test coverage and what not.

Hrm, ok. Because it's so tightly related it feels sufficiently in scope, but: fair.

I kinda prefer to opt IN the behaviour and not require a way, to opt out of that behaviour. I don't see a strict requirement why every entity type would require it.

Right; there could be content entities that don't have HTML routes. Makes sense; thanks!

I have tested this on the Content Entity Example from the examples Module with https://www.drupal.org/node/2409195 applied. Removed view, edit and delete route from the annotation.

The View, Edit and Delete links work perfectly well.

@cgalli
Thank you for the manual testing. Good to know that it actually works :P

and the routes in this patch with _admin_route set don't have paths that begin with /admin.

Right, but there are exceptions, for whatever reason( admin/structure/comment/*,admin/structure/block/* )

Ok. Could you document this? It was very puzzling to me why you'd set something to NULL.

Sure.

Fixed all failures and later converted all the other entity types.

Ha, annotations <3

.

Opened #2409691: EntityAccessControlHandlers miss entity admin access, which this issue can leverage once its there.

Looks likes tests failed had exceptions due to incorrect namespace for FeedHtmlRouteProvider in annotation.

Good catch @mglaman

For now just a reroll.

Some more work, not really happy about some of the required changes.

Reroll.

This fixes some fails, hopefully.

So, I realize that this patch isn't done (tests don't pass), but I'm not convinced this is a step forward.

It looks like most entities covered in the patch are losing a few lines in the routing.yml file, at the expense of having to define their own route provider class and reference in in entity annotation, apparently just to override some defaults that would otherwise just be part of the route definitions. This doesn't seem like great DX. It's easier to provide the defaults in the routing file than to define a class, I think, and it's way more lines being added than lost.

There are a few entities (like Feed in Aggregator module) that also don't seem to have lost anything in the routing.yml file and have only added lines in the annotation and the new class. This may be a problem in the patch, but is definitely not great DX.

And the patch will eventually need to update the Entity topic docs, so that developers know about this. I added a note about this to the issue summary so it doesn't get forgotten.

Re @jhodgdon: The overrides that these entity types are just to be consistent with the current behavior: They will not be necessary for the majority of use-cases. Also the fact that this patch is uncovering all sorts of inconsistencies and weirdnesses between the different entity type is proof IMO that some form of generic implementation is needed or at least very helpful. We can always improve the base implementation in follow-ups so that maybe in the future less follow-ups will be necessary, but this is certainly a big step forward. I've been using this patch on a site I'm developing for a while now and it's a *big* timesaver.

I share some of jhodgdon's DX concerns. Let me repeat a few points that I made in #34:

• 12.
  

  +++ b/core/modules/comment/src/CommentTypeHtmlRouteProvider.php
  @@ -0,0 +1,34 @@
  +    $collection->get('entity.comment_type.edit_form')
  +      ->setOption('_admin_route', TRUE);
  +    $collection->get('entity.comment_type.delete_form')
  +      ->setOption('_admin_route', TRUE);
  

  If we'd detect bundle entity types in DefaultHtmlRouteProvider, then we could set _admin_route: TRUE automatically.

  to which dawehner replied

  I won't do that, because I don't get why we manually add it here. The AdminRouteSubscriber should take care of it, if possible. Not sure why its added here,
  but there is probably a reason.

• 18.
  

  +++ b/core/modules/system/tests/modules/entity_test/src/Entity/EntityTestBaseFieldDisplay.php
  @@ -21,15 +21,24 @@
  + *     "route_provider" = {
  + *       "html" = "Drupal\Core\Entity\Routing\DefaultHtmlRouteProvider",
  + *     },
  

  Hm, I thought this would automatically be the default? I.e. I thought it wouldn't be necessary to define it in the annotation if you wanted to use the default HTML routes?

  to which dawehner replied:

  Well, if you look at ContentEntityType and EntityType you won't see many handlers added by default. I kinda prefer to opt IN the behaviour and not require a way, to opt out of that behaviour. I don't
  see a strict requirement why every entity type would require it.

  Which is a pretty good reason.

Perhaps revisiting those things could help the DX?

Which is a pretty good reason.

If you look at the entity API you will see that its an explicit API, you specify what is done, and don't rely on the default
instead of just one single place.

It looks like most entities covered in the patch are losing a few lines in the routing.yml file, at the expense of having to define their own route provider class and reference in in entity annotation, apparently just to override some defaults that would otherwise just be part of the route definitions. This doesn't seem like great DX. It's easier to provide the defaults in the routing file than to define a class, I think, and it's way more lines being added than lost.

If you would be extreme, you could say, let's skip whatever core its doing, but for custom entity types you REALLY want to use it, and have a consistent behaviour.

OK, I'll bow to your greater wisdom and tstoeckler's experience on this being better DX for defining your own entity type.

Regarding the complaint about having to specify using the default route builder, I think that in most other areas of the Entity API, such as the Access controller, we've made it so if you're using the default class, you don't have to specify it. So I think that should be the default here too? But that is not a huge deal, as it's only one line in the annotation.

Also, one note about the "admin_route" designation of routes... It seems to me that the default controller should and could choose more sensible defaults. For instance, it could designate either all routes, or just edit/add/delete routes (omitting "canonical"), for Config entities as admin_route. Alternatively, Core could provide two default classes, the first as it is now, and the second having this admin_route default set, so that entities could choose one or the other as their route provider without the necessity of defining their own class, and we wouldn't have to have a bunch of little classes floating around whose sole purpose is to set the admin_route flag on their routes. That would cut the size of this patch down considerably -- there are at least 4 classes in there whose sole purpose is to set admin_route defaults I think.

Another reason entities in this patch seem to be overriding the default class is to specify the title, but I think these overrides can actually be omitted. Most controllers for these things override the title for the page anyway, so the title in the current routing files is rarely if ever even used.

Anyway... just a couple of thoughts on how to possibly improve the DX in this patch.

That would cut the size of this patch down considerably -- there are at least 4 classes in there whose sole purpose is to set admin_route defaults I think.

+1

Another reason entities in this patch seem to be overriding the default class is to specify the title, but I think these overrides can actually be omitted.

+1

OK, I'll bow to your greater wisdom and tstoeckler's experience on this being better DX for defining your own entity type.

Let's try to be friends :)

Also, one note about the "admin_route" designation of routes... It seems to me that the default controller should and could choose more sensible defaults. For instance, it could designate either all routes, or just edit/add/delete routes (omitting "canonical"), for Config entities as admin_route. Alternatively, Core could provide two default classes, the first as it is now, and the second having this admin_route default set, so that entities could choose one or the other as their route provider without the necessity of defining their own class, and we wouldn't have to have a bunch of little classes floating around whose sole purpose is to set the admin_route flag on their routes. That would cut the size of this patch down considerably -- there are at least 4 classes in there whose sole purpose is to set admin_route defaults I think.

That sounds like a good thing to experiment with!

Regarding the complaint about having to specify using the default route builder, I think that in most other areas of the Entity API, such as the Access controller, we've made it so if you're using the default class, you don't have to specify it. So I think that should be the default here too? But that is not a huge deal, as it's only one line in the annotation.

Well, we do it for 2 cases: a) the storage controller and b) the access control handler ... For everything else we are explicit. I think one of the entity subsystem group should
tell us their opinion, given that they actually know the intended behavior of those annotations.

Another reason entities in this patch seem to be overriding the default class is to specify the title, but I think these overrides can actually be omitted. Most controllers for these things override the title for the page anyway, so the title in the current routing files is rarely if ever even used.

Note: we need the title on the routes for breadcrumbs.

Anyway... just a couple of thoughts on how to possibly improve the DX in this patch.

Its good that you care about it?

RE: friends, of course! I really was not trying to be snarky, and I apologize if it came across that way. I see the wisdom of what you said and agree with it.

RE: route titles and breadcrumbs -- oh interesting. I hadn't thought of that. I guess that is important only for routes that are the base for local task/action groups then?

+1 for making it explicit, that you have to specify it. That was the condition that we agreed on in Amsterdam to allow introducing this so late, that it is an opt-in mechanism. Otherwise it is going to mess up existing modules.

As @dawehner mentioned, we only do it for just storage, which just works for all entities, and entity access, which doesn't do anything else but checking the admin permission as defined. This is too experimental to enable it by default, at least for a first version.

Also, as @dawehner and @tstoeckler mentioned, almost all our entity types have some special cases which is why the defaults don't work for them and they do need overrides.

1. +++ b/core/modules/aggregator/src/FeedHtmlRouteProvider.php
   @@ -0,0 +1,31 @@
   +    $collection->get('entity.aggregator_feed.edit_form')
   +      ->setDefault('_title', 'Configure')
   +      ->setOption('_admin_route', TRUE);
   

   I'm not sure if Configure is really something that we need to keep, but I wouldn't be surprised if we do have a test for this somewhere?

   Agreed that we could consider to set _admin_route to TRUE by default for edit/delete, this for example does not set it for delete, which is very likely an oversight, possibly in HEAD as well. _admin_route TRUE for edit/delete seems like a good default. That said, that just means that node will have to override more, as that part is configurable there, because nodes like to be special.

2. +++ b/core/modules/comment/src/CommentTypeHtmlRouteProvider.php
   @@ -0,0 +1,32 @@
   +    $collection->get('entity.comment_type.edit_form')
   +      ->setOption('_admin_route', TRUE);
   +    $collection->get('entity.comment_type.delete_form')
   +      ->setOption('_admin_route', TRUE);
   

   I don't really see why this should be necessary though, those are /admin paths anyway? All config entities should be But setting it by default won't hurt either.

3. +++ b/core/modules/file/src/Tests/SaveUploadTest.php
   @@ -69,7 +69,7 @@ protected function setUp() {
   -  function testNormal() {
   +  function ptestNormal() {
   

   ups.

4. +++ b/core/modules/menu_link_content/src/Plugin/Menu/MenuLinkContent.php
   @@ -187,14 +187,14 @@ public function getDeleteRoute() {
      public function getTranslateRoute() {
   -    return $this->getEntity()->urlInfo('content-translation-overview');
   +    return $this->getEntity()->urlInfo('drupal:content-translation-overview');
      }
   

   Looks like you found that problem here as well with the wrong prefix, that was afaik just fixed in HEAD.

5. +++ b/core/modules/rest/src/Tests/AuthTest.php
   +++ b/core/modules/rest/src/Tests/AuthTest.php
   @@ -91,6 +91,7 @@ protected function basicAuthGet(Url $url, $username, $password) {
   
   @@ -91,6 +91,7 @@ protected function basicAuthGet(Url $url, $username, $password) {
            CURLOPT_URL => $url->setAbsolute()->toString(),
            CURLOPT_NOBODY => FALSE,
            CURLOPT_HTTPAUTH => CURLAUTH_BASIC,
   +        CURLOPT_HTTPHEADER => array('Accept: ' . $this->defaultMimeType),
            CURLOPT_USERPWD => $username . ':' . $password,
          )
   

   Why is this suddenly necessary? entity_test route changes?

6. +++ b/core/modules/system/src/Tests/Entity/EntityViewControllerTest.php
   @@ -41,6 +41,9 @@ protected function setUp() {
   +    $account = $this->drupalCreateUser(['view test entity']);
   +    $account->save();
   

   save is not necessary, drupalCreateUser() saves the user already.

I noticed some more routes and other things that can be removed. Did not tackle the tests yet.

Because Wim Leers' happiness is of ample importance to me, I wondered if it would make sense to introduce a HtmlRouteProviderThatUsesTheAdminRouteOptionForEditAndDeleteFormRoutesByDefault in order to consolidate the different route providers that set _admin_route to TRUE. (I think that is along the lines of what you proposed above, right?) It turns out, though, that each of those (but for one) also does something special like setting a different title or something. So it wouldn't actually be a net reduction of code (although it might be helpful for contrib?).

Edit: The paragraph above is bollocks (except, of course, the first subclause <3 :-)), sorry. New patch coming up.

Ok, here we go.

I just now saw #64, so this includes the fixes for this as well. I did not fix 1., though. I think when this is green we can see what fails if we remove the FeedHtmlRouteProvider, and then evaluate.

So this one - I think - will make @Wim Leers happy :-)

It introduces an AdminHtmlRouteProvide which is used directly by BlockContent and Term and extended by Feed and User. I think that is more explicit than simply making it the default. Then we would have to disable it for e.g. comments as well.

Berdir was right and CommentType in fact does not need the override at all.

...

Well, that was not too smart.

This installs at least.

Now postponed #2403359: Use _title and _title_callback where possible on this.

Just a reroll for now.

Some fixes here and there.

More fixes.

Reroll.

I would have loved to do it that way, but I'm not sure who, but I remember there was a strong push forward to do them all ...

In what circumstances would the entity not already be loaded in the parameter bag and therefore available for the reflection-based parameter passing? That seems like a bug; if there's a legit non-bug reason for it, it should be documented.

Yeah I don't remember at all, but it was kinda needed for some failures, this is what I remember.

That way getRoutes() becomes easier to read, and each discrete task is in its own (reusable!) method.

Solved that.

Additionally, if entity.entitytype.canonical is manually defined via YAML already we want to skip this entirely, IIRC. Is that check already happening somewhere else, and if not, how do we make it happen?

Do you mind adding this into a follow up? We aren't doing that yet.

This seems unrelated? (I'm not against it, jut just seems unrelated.)

Alright, let's try to drop and see what fails.

`Can't this be automated? We automate most entity permissions already, I believe...

OUT OF SCOPE, and no, this is like asking for automating the path for a canonical link template.

This might be scope creep, but shouldn't we keep the route names/behavior as similar as possible? Account cancelation may involve deletion, so should it reuse the same route name for consistency, even if we change some of the parameters on it?

No, user deletion is NOT user cancelation, see https://www.drupal.org/node/8

My bigger issue, though, is the heavy use of inheritance. Default->Admin->Specific seems like a bad way to do it, since there's no is-a relationship. What we actually want here is a way to mix and match common auto-routes.

Seriously, this is what you worry about? Seen worse things in my life and IMHO inheritance makes sense. Well, default should extend from a base, but then IMHO it totally makes sense

Instead, perhaps we could have a CommonRoutesTrait and AdminRoutesTrait. Then a DefaultRouteHandler could use both of those, and its getRoutes method just calls the methods out of those traits and returns. (As noted above, that should be a lot of little methods rather than 1-2 big ones.) Then instead of extending and extending, each entity-specific implementation could use those same traits and call the methods it wants. If it doesn't have anything custom to do, it can just use Default which is fine. That allows, say, admin routes without the default routes. (Since "has admin routes" is not a strict special-case of "has the common routes", conceptually.)

I think using traits would make it harder to read, because you'd still have a admin one, which calls out to the trait methods, which then do pretty much nothing. Feel free to rewrite the code though, but I think using traits would make it actually harder in terms of DX, which is IMHO the point of this issue.

Forgot to add one file.

1. +++ b/core/modules/block_content/src/Tests/PageEditTest.php
   @@ -62,7 +62,7 @@ public function testPageEdit() {
   -    $this->drupalGet("block/" . $revised_block->id());
   +    $this->drupalGet($block->urlInfo('edit-form'));
   

   $revised_block->urlInfo()

2. +++ b/core/modules/user/src/Entity/UserHtmlRouteProvider.php
   @@ -0,0 +1,44 @@
   +  public function getRoutes(EntityTypeInterface $entity_type) {
   +    $collection = parent::getRoutes($entity_type);
   +
   +    $collection->get('entity.user.canonical')->setDefault('_title_callback', 'Drupal\user\Controller\UserController::userTitle');
   
   +++ b/core/modules/views_ui/src/ViewHtmlRouteProvider.php
   @@ -0,0 +1,34 @@
   +  protected function getEditFormRoute(EntityTypeInterface $entity_type) {
   +    $route = parent::getEditFormRoute($entity_type);
   +
   +    // Replace the _entity_form with a custom _controller.
   +    $route
   

   Why use this approach for some and the other for others?

+++ b/core/lib/Drupal/Core/Entity/Routing/AdminHtmlRouteProvider.php
@@ -0,0 +1,37 @@
+/**
+ * Provides routes for the taxonomy_term entity.
+ */
+class AdminHtmlRouteProvider extends DefaultHtmlRouteProvider {

What should this comment really say? Provides the default admin routes for config entities?

$revised_block->urlInfo()

Good catch!!

Why use this approach for some and the other for others?

I went with a single overridden method on the user case, because its extending the list in multiple other ways at the same time.
Feel free to disagree.

What should this comment really say? Provides the default admin routes for config entities?

Ha, improved the comment.

There we go.

Maybe now ...

manually tested shortcuts after #96 and this is the result for canonical route (empty page)

patch in #96 passes because it simply bypass checks in ShortcutLinksTest

looking at shortcut.routing.yml

+++ b/core/modules/shortcut/shortcut.routing.yml
@@ -54,22 +54,6 @@ shortcut.link_add:
-entity.shortcut.canonical:
-  path: '/admin/config/user-interface/shortcut/link/{shortcut}'
-  defaults:
-    _entity_form: 'shortcut.default'
-    _title: 'Edit'
-  requirements:
-    _entity_access: 'shortcut.update'

entity.shortcut.canonical coincides with entity.shortcut.edit_form so probably we should create a dedicated class to handle it

reverted changes in ShortcutLinksTest and added a dedicated class for shortcut

@willzyx
Well, I think shortcuts are pretty stupid, in the way how the configured routes look like,
so I'm not sure whether its really the best idea to let the canonical route point to the edit route as well, but sure, feel free to do this here.

So now we need some proper reviews ... its green!

ping crell!

+1 rtbc, looks solid

NW for:
1) needs consistency in having of entity route providers - admin_route is not a default
2) change record (NodeRouteProvider renamed)
Probably follow-up for entity translation routes

Also we need to link here #2407505: [meta] Finalize the menu links (and other user-entered paths) system

+++ b/core/lib/Drupal/Core/Entity/Routing/AdminHtmlRouteProvider.php
@@ -0,0 +1,37 @@
+class AdminHtmlRouteProvider extends DefaultHtmlRouteProvider {

+++ b/core/lib/Drupal/Core/Entity/Routing/DefaultHtmlRouteProvider.php
@@ -0,0 +1,130 @@
+class DefaultHtmlRouteProvider implements EntityRouteProviderInterface {

+++ b/core/modules/aggregator/src/Entity/Feed.php
@@ -30,7 +30,10 @@
+ *       "html" = "Drupal\aggregator\FeedHtmlRouteProvider",

+++ b/core/modules/aggregator/src/FeedHtmlRouteProvider.php
@@ -0,0 +1,29 @@
+class FeedHtmlRouteProvider extends AdminHtmlRouteProvider {

+++ b/core/modules/block_content/src/Entity/BlockContent.php
@@ -34,7 +34,10 @@
+ *       "html" = "Drupal\Core\Entity\Routing\AdminHtmlRouteProvider",

+++ b/core/modules/comment/src/CommentHtmlRouteProvider.php
@@ -0,0 +1,31 @@
+class CommentHtmlRouteProvider extends DefaultHtmlRouteProvider {

+++ b/core/modules/comment/src/Entity/Comment.php
@@ -35,7 +35,10 @@
+ *       "html" = "Drupal\comment\CommentHtmlRouteProvider",

+++ b/core/modules/comment/src/Entity/CommentType.php
@@ -24,7 +24,10 @@
+ *       "html" = "Drupal\Core\Entity\Routing\DefaultHtmlRouteProvider",

+++ b/core/modules/menu_link_content/src/Entity/MenuLinkContent.php
@@ -31,7 +31,10 @@
+ *       "html" = "Drupal\Core\Entity\Routing\DefaultHtmlRouteProvider",

+++ b/core/modules/node/src/Entity/NodeHtmlRouteProvider.php
@@ -0,0 +1,40 @@
+class NodeHtmlRouteProvider extends DefaultHtmlRouteProvider {

+++ b/core/modules/taxonomy/src/Entity/Term.php
@@ -31,7 +31,10 @@
+ *       "html" = "Drupal\Core\Entity\Routing\AdminHtmlRouteProvider",

+++ b/core/modules/taxonomy/src/Entity/Vocabulary.php
@@ -24,7 +24,10 @@
+ *       "html" = "Drupal\taxonomy\Entity\VocabularyRouteProvider",

+++ b/core/modules/taxonomy/src/Entity/VocabularyRouteProvider.php
@@ -0,0 +1,31 @@
+class VocabularyRouteProvider extends DefaultHtmlRouteProvider {

+++ b/core/modules/taxonomy/taxonomy.routing.yml
@@ -14,26 +14,6 @@ entity.taxonomy_term.add_form:
-entity.taxonomy_term.edit_form:
...
-    _admin_route: TRUE
...
-entity.taxonomy_term.delete_form:
...
-    _admin_route: TRUE

CommentHtml should be inherited from Admin provider
Same for comment type, menu link, node - we have mostly all edit operations in "admin" so maybe add "_admin_route: TRUE" in default?

CommentHtml should be inherited from Admin provider
Same for comment type, menu link, node - we have mostly all edit operations in "admin" so maybe add "_admin_route: TRUE" in default?

Good point!

2) change record (NodeRouteProvider renamed)

I'm sorry but I don't think that this is an API.

I don't think it makes sense for the admin one to be the default as then we would have to undo the _admin_route option for a bunch of entity types. I think having to undo a previous implementation is a sign of over-generalization and bad design.

I am also not a fan of huge inheritance chain, but IMO it's not fair to complain about that here when we have enormous inheritance chains all over core (Typed Data, anyone?). If we want to avoid that, let's do the following: Let's add a getRouteOptions() (or similar) method which is returns an empty array by default and then let's add a trait which implements this method and returns ['_admin_route' => TRUE].

the patch needs a reroll

What bunch of entities do we not want the edit-et-al pages to be admin-marked? Do you disagree with andypost that most if not all of them are/should be?

Let's stop talking BS and look at actual data:

The following ones aren't using the AdminHtmlRoutesProvider (for example extend the Html one because of other reasons):

• comment
• comment_type
• menu_link_content
• node
• shortcut_html

The following are:

• feed
• block_content
• taxonomy_term
• user

I think given that the default should not be the admin one, node and comment aren't a special case, if you ask me.

The outer-most parentheses here seem extraneous. Left-over from trying to chain everything, maybe?

(Same in other methods.)

OH yeah this is annoying.

Hm. Something else I just realized. "route provider" is already the name of a class, and a rather important one. Using this key here is self-descriptive, but also likely to be confusing. :-(

Unfortunately, the key there and the class names are not introduced in this patch, so it may be too late to change it. Blast!

Well, the FQCN for itself should describe things. We should have put the route provider into the namespace ...

This ought to have a link defined in the annotation to piggy back off of, no? That would be consistent with all of the other routes. That we then generate the route for it custom here doesn't change that.

Not sure what to respond, honestly.

For the last point, I mean that we should declare a "cancel" link relation on User, give it a URI in the annotation, and then use that data to generate the route just like we do for all of the other routes.

Sounds quite out of scope of this issue!

Ha, I think given that we actually have a link relation already, not using it, is actually wrong :)

As someone making a lot of entity types in contrib right now, I'm loving this patch :)
I'm also loving the consistency it adds to the core entity routes. The current _admin_path approach for edit/delete makes sense.

+  protected function doGetEntity(RouteMatchInterface $route_match, EntityInterface $_entity = NULL) {
+    if ($_entity) {
+      $entity = $_entity;
+    }

It's a bit weird that the $_entity parameter has an underscore in front, telling me it's somehow special, but no comment explaining why.

        // Let's look up in the route object for the name of upcasted values.
+      foreach ($route_match->getParameters() as $parameter) {
+        if ($parameter instanceof EntityInterface) {
+          $entity = $parameter;
+          break;
+        }
+      }

This will probably be triggered in Commerce, since we've opted out of the magical automatic entity upcasting.
Would it make more sense to take the last entity parameter instead of the first?
Sometimes you nest UIs, in which case taking the last parameter would cover more use cases.

+    $collection->get('entity.node.canonical')
+      ->setDefault('_entity_view', NULL)

Why do this? A comment might help.

Docs nitpicking:

+/**
+ * Provides generic entity controllers, especially for titles.
+ */

Odd comment for a single class. Perhaps simply "Provides a generic entity controller."? Not sure if we want to focus on the title aspect since the class might grow in the future.

+ * It provides:
+ * - A view route with title callback.
+ * - An edit route with title callback.
+ * - A delete route with title callback.

...with a title callback?

+          foreach ($routes as $route_name => $route) {
+            // Don't override static defined routes.
+            if (!$route_collection->get($route_name)) {

Don't override existing routes? Or already defined routes?

Thank you @bojanz!!

It's a bit weird that the $_entity parameter has an underscore in front, telling me it's somehow special, but no comment explaining why.

Nothing introduced in that patch, but sure, let's add some documentation

This will probably be triggered in Commerce, since we've opted out of the magical automatic entity upcasting.
Would it make more sense to take the last entity parameter instead of the first?
Sometimes you nest UIs, in which case taking the last parameter would cover more use cases.

Well, I think because IMHO its more likely to do so. But honestly, I don't give a shit that you are doing things the wrong way.
All you need is to add the entity interface onto your controller, its not that hard.

Perhaps simply "Provides a generic entity controller."?

...with a title callback?

Sure, let's do that.

Don't override existing routes? Or already defined routes?

Went with the first one.

One other thing... I don't have time to look at this in more detail right now, but there is no update to a .api.php file in this patch. I think that some Entity topic on api.drupal.org most likely needs an update about this change, and this should be part of the patch?
https://api.drupal.org/api/drupal/core!lib!Drupal!Core!Entity!entity.api...

It seems like probably several parts of this topic need to be updated.

reroll first

Ugh, I feel like an idiot and I don't understand why patch in #121has INSTALL.txt, MAINTAINERS.txt, composer and so on. Patch file itself look fine on my local and don't have the above files. I was going to continue work on addressing concerns from #119 and #120but I think I need to wait until I or someone else can figure out what's going on.

another reroll. let see.....

I was working on this issue, but didn't mean to post this here. Please ignore this patch.

Here's a reroll of #118, with the #119 items. If #127 passes, it might be the solution to some remaining test fails.

Try this.

Revert the route change.

123 was a reroll.
127 can be ignored.

Some changes in head regarding caching caused some test fails. There were 3 in #129. Now there are 2.

Todo:
1.

+++ b/core/modules/menu_link_content/src/Tests/MenuLinkContentTranslationUITest.php
@@ -20,7 +20,7 @@ class MenuLinkContentTranslationUITest extends ContentTranslationUITestBase {
+  protected $defaultCacheContexts = ['languages:language_interface', 'theme'];

I think we need to add 'languages:language_content' here.

2. For some reason, the MenuLinkContentDeleteForm does not have "/edit" on the Cancel Link (which it should according to the test.)

Try to get the cache contexts right.

I looked through the latest patch and I have some comments:

1. Still missing a docs update... Adding tag. Needs to have the Entity topic on api.drupal.org updated.
   https://api.drupal.org/api/drupal/core!lib!Drupal!Core!Entity!entity.api...

2. +++ b/core/lib/Drupal/Core/Entity/Controller/EntityController.php
   @@ -0,0 +1,142 @@
   + * It provides:
   + * - A view route together with a title callback.
   + * - An edit route together with a title callback.
   + * - A delete route together with a title callback.
   + */
   +class EntityController implements ContainerInjectionInterface {
   +
   

   Does this provide controllers or routes? I think it provides controllers, so don't document that it provides routes, right?

3. +++ b/core/lib/Drupal/Core/Entity/Controller/EntityController.php
   @@ -0,0 +1,142 @@
   +   * @return \Drupal\Core\Entity\EntityInterface|NULL
   +   *   Returns the entity, otherwise NULL.
   +   */
   +  protected function doGetEntity(RouteMatchInterface $route_match, EntityInterface $_entity = NULL) {
   +    if ($_entity) {
   

   Looking at the code for this method, I think the @return could use some more documentation about what it is returning. It seems to be returning either the entity in $_entity, or any other entity object that is found anywhere in the route parameters. Right?

4. +++ b/core/lib/Drupal/Core/Entity/Routing/AdminHtmlRouteProvider.php
   @@ -0,0 +1,37 @@
   +/**
   + * Provides admin routes for entities.
   + */
   +class AdminHtmlRouteProvider extends DefaultHtmlRouteProvider {
   +
   

   It might be helpful in this class's doc block to explain how this differs from its parent class, because I had to read the code to figure it out.

   I think I'd add a line saying something like:

   These routes are the ones provided by \Drupal\Core\entity\Routing\DefaultHtmlRouteProvider, but with the _admin_route route option set for the edit and delete routes.

5. +++ b/core/lib/Drupal/Core/Entity/Routing/DefaultHtmlRouteProvider.php
   @@ -0,0 +1,130 @@
   + * It provides:
   + * - A view route with title callback.
   + * - An edit route with title callback.
   + * - A delete route with title callback.
   + */
   +class DefaultHtmlRouteProvider implements EntityRouteProviderInterface {
   +
   

   This class is just providing routes, not title callbacks, right?

   Oh... I see. The route definition has a title callback.

   How about "including a" instead of "with" here? And it also provides an access callback...

   So maybe this could be changed to say:

   This class provides the following routes for entities, each with title and access callbacks defined:
   - canonical
   - edit_form
   - delete_form

   (I think it would be clearer to use the actual machine names of the routes, like canonical rather than 'view'.)

6. +++ b/core/modules/aggregator/src/Entity/Feed.php
   @@ -30,7 +30,10 @@
   + *     "route_provider" = {
   + *       "html" = "Drupal\aggregator\FeedHtmlRouteProvider",
   + *     },
     *   },
   

   See, this definitely needs to be documented in the Entity topic.

7. +++ b/core/modules/block_content/src/Entity/BlockContent.php
   @@ -42,7 +45,7 @@
     *     "canonical" = "/block/{block_content}",
     *     "delete-form" = "/block/{block_content}/delete",
   - *     "edit-form" = "/block/{block_content}",
   + *     "edit-form" = "/block/{block_content}/edit",
     *     "collection" = "/admin/structure/block/block-content",
   

   Is this introducing a new path for custom blocks? It looks like previously the canonical and edit routes both went to block/ID, and now "canonical" is some kind of a view route at that URL, while eidt is now at block/ID/edit? This doesn't seem correct to me.

8. +++ b/core/modules/content_translation/src/Tests/ContentTranslationUITestBase.php
   @@ -75,13 +75,6 @@ protected function doTestBasicTranslation() {
   -    $this->drupalGet($entity->urlInfo());
   -    $this->assertResponse(200, 'Entity URL is valid.');
   -
   

   Why were these two lines removed from the test?

9. +++ b/core/modules/menu_link_content/src/Entity/MenuLinkContent.php
   @@ -45,7 +48,7 @@
   - *     "canonical" = "/admin/structure/menu/item/{menu_link_content}/edit",
   + *     "canonical" = "/admin/structure/menu/item/{menu_link_content}",
     *     "edit-form" = "/admin/structure/menu/item/{menu_link_content}/edit",
     *     "delete-form" = "/admin/structure/menu/item/{menu_link_content}/delete",
   

   Same question here as on the Block Content entity: you're introducing a new URL here? Seems wrong.

   As another note, the code for generating these routes seems to have logic in it that says "Generate canonical if it exists", so maybe you don't need "canonical" at all? In any case I don't think it makes sense to view a block or menu link entity on a page?

10. +++ b/core/modules/menu_link_content/src/Tests/MenuLinkContentTranslationUITest.php
    @@ -95,7 +95,7 @@ function testTranslationLinkTheme() {
    -    $this->drupalGet('admin/structure/menu/item/' . $entityId . '/edit/translations');
    +    $this->drupalGet('admin/structure/menu/item/' . $entityId . '/translations');
         $this->assertRaw('core/themes/seven/css/base/elements.css', 'Translation uses admin theme as well.');
    

    This URL is also changing???

11. +++ b/core/modules/node/src/Entity/NodeHtmlRouteProvider.php
    @@ -0,0 +1,41 @@
    +    $collection->get('entity.node.canonical')
    +      // Use a custom controller. Sadly, there is no removeDefault method.
    +      ->setDefault('_entity_view', NULL)
    +      ->setDefault('_controller', '\Drupal\node\Controller\NodeViewController::view');
    +
    

    Maybe you could just remove the entity.node.canonical entry from the collection instead of being so sad here? :)

12. +++ b/core/modules/shortcut/src/Entity/Shortcut.php
    @@ -46,7 +49,7 @@
      *     "canonical" = "/admin/config/user-interface/shortcut/link/{shortcut}",
      *     "delete-form" = "/admin/config/user-interface/shortcut/link/{shortcut}/delete",
    - *     "edit-form" = "/admin/config/user-interface/shortcut/link/{shortcut}",
    + *     "edit-form" = "/admin/config/user-interface/shortcut/link/{shortcut}/edit",
      *   },
    

    Another introduction of a new URL here?

13. +++ b/core/modules/system/tests/modules/entity_test/src/Controller/EntityTestController.php
    @@ -63,26 +63,6 @@ public function testAdd($entity_type_id) {
    -  public function testEdit(RouteMatchInterface $route_match, $entity_type_id) {
    

    So you've removed this test... are we not needing to test this functionality any more, or could you just not get it to pass?

14. +++ b/core/modules/taxonomy/src/Entity/Vocabulary.php
    @@ -35,11 +38,12 @@
    + *     "canonical" = "/admin/structure/taxonomy/manage/{taxonomy_vocabulary}",
      *     "add-form" = "/admin/structure/taxonomy/manage/{taxonomy_vocabulary}/add",
    + *     "edit-form" = "/admin/structure/taxonomy/manage/{taxonomy_vocabulary}",
      *     "delete-form" = "/admin/structure/taxonomy/manage/{taxonomy_vocabulary}/delete",
    

    See, here you've set the canonical and edit-form routes to go to the same URL. If the canonical route is absolutely necessary to have defined (and that needs to be documented!), can't we also use the same URL as we had before in the block and menu link entity classes, rather than introducing new URLs?

15. +++ b/core/modules/taxonomy/src/Tests/VocabularyLanguageTest.php
    @@ -55,8 +56,10 @@ function testVocabularyLanguage() {
    -    $this->drupalGet('admin/structure/taxonomy/manage/' . $vid);
    +    $this->drupalGet($vocabulary->urlInfo('edit-form'));
    

    It actually scares me that this kind of change might have been necessary to get the test to pass. Did the URL change or not? I would prefer the test to be testing that the URL is what is expected, rather than calling the urlInfo() function to get the URL, because probably as a regression test, something that introduces a change in URLs ought to be caught as a test failure, right?

    This was also done in some of the other tests above. Really I think it's better for the tests to have a hard-wired drupalGet() URL so that we verify the URL is what we think it is.

Reverting the cache context changes that I made. There is just something wrong with the MenuLinkContent conversion.

1. Got the documentation started.
2. We already know it's a controller by the class name, so how about just "provides: A title callback". Updated in patch.
3. How about: "The entity from the controller or the first entity found in the route." Although I'm not really sure "from the controller" is right; It's the entity passed to the route generator, but I'm not completely sure where the passed entity comes from.
4. Updated.
5. Agree that the rewording is more concise, but actually think view, edit, delete is more readable for a comment.
6. Duplicate of #1.
7, 9, 10, 11, 12,14: All related. I assumed that it was some standardization the the edit-form path should be /edit. But, now don't see any consistent pattern. Needs more discussion.
8. I assumed that it wasn't needed anymore. Adding that piece back (add the cachecontexts assert after it that I deleted in the reroll) to see what happen.
Results:
Get an error in the MenuLinkContent conversion:
Canonical route used to be: /admin/structure/menu/item/1/edit
With the patch it is: admin/structure/menu/item/1 (without the /edit) which gets Access Denied.
13. This isn't a test, but just code in a test module that isn't needed after the conversion to use the routeProvider.
15. Maybe also part of a conversion.

Looks like the tests do not pass... will wait to review most of the patch until they do.

Regarding the docs, This is the only change I'm seeing to an api.php file:

+++ b/core/lib/Drupal/Core/Entity/entity.api.php
@@ -385,6 +385,12 @@
  *     "form" = {
  *       "default" = "Drupal\block\BlockForm",
  *   @endcode
+ * - Optionally, utilize an auto-route generator that provides typical routes:
+ *   @code
+ *   handlers = {
+ *     "route_provider" = {
+ *       "html" = "Drupal\Core\Entity\Routing\DefaultHtmlRouteProvider",
+ *   @endcode

Some questions/comments:

a) Is this auto-route generation really optional?

b) This topic needs more updates:
- In the "Defining" section, it talks about needing to define routes, which needs some modification.
- The "Entity Routes" section also needs some modification
I think both of these should mention that in place of defining your own routes, you can use a route provider.

c) If this auto-route generation *isn't* optional... or even if it is optional... I am failing to see why someone would want to do this (define a class) in place of just defining the routes in a routing.yml file? The routing.yml file seems much easier. ??!?

Looks like the tests do not pass... will wait to review most of the patch until they do.

IMHO you really don't have to.

a) Is this auto-route generation really optional?

Yes its opt in, as most every other handler in there, like the views data.

. I am failing to see why someone would want to do this (define a class) in place of just defining the routes in a routing.yml file? The routing.yml file seems much easier. ??!?

Well, the point of this issue is in the title. You want to not have to care about it ... It is quite an effort to get view + edit + delete + access + etc. right, the automation is an advantage.

OK, fair enough. So #144(b) still applies - we need to mention this in two other places in the docs in the entity.api.php file.

Anyway, I took a look at the code as well, and it looks like my previous review comments were not addressed. They still exist. Here are a few; I gave up about half way through the patch because I think there are some real problems that have been overlooked.

1. +++ b/core/lib/Drupal/Core/Entity/Controller/EntityController.php
   @@ -0,0 +1,142 @@
   +/**
   + * Provides generic entity controllers.
   + *
   + * It provides:
   + * - A view title callback.
   + * - An edit title callback.
   + * - A delete title callback.
   + */
   +class EntityController implements ContainerInjectionInterface {
   +
   

   This isn't really a generic entity controller and I'm a bit confused about the name... it really is just something you could use to get these title callbacks, but it doesn't do any route controller stuff by itself.

   Maybe it should be called EntityTitleController and the first line should say something like:

   Provides title callbacks for entity routes.

   or something like that?

2. +++ b/core/lib/Drupal/Core/Entity/Controller/EntityController.php
   @@ -0,0 +1,142 @@
   +   * @return \Drupal\Core\Entity\EntityInterface|NULL
   +   *   Returns the entity, otherwise NULL.
   +   */
   +  protected function doGetEntity(RouteMatchInterface $route_match, EntityInterface $_entity = NULL) {
   +    if ($_entity) {
   

   This method could use a bit more detail... maybe something like:

   Returns the entity if it is either passed in directly in $_entity, or if there is an entity that is one of the parameters of the route.

   or something like this? I had to read the code to figure out what this does.

3. +++ b/core/lib/Drupal/Core/Entity/Routing/AdminHtmlRouteProvider.php
   @@ -0,0 +1,39 @@
   +/**
   + * Provides admin routes for entities.
   + *
   + * Use this class if the default routes should use the admin theme.
   + */
   +class AdminHtmlRouteProvider extends DefaultHtmlRouteProvider {
   +
   

   Um... it's only the delete and edit routes that will use admin, not canonical/view routes, right? So please fix the docs.

4. +++ b/core/lib/Drupal/Core/Entity/Routing/DefaultHtmlRouteProvider.php
   @@ -0,0 +1,130 @@
   + * It provides:
   + * - A view route with title callback.
   + * - An edit route with title callback.
   + * - A delete route with title callback.
   + */
   +class DefaultHtmlRouteProvider implements EntityRouteProviderInterface {
   +
   

   Can we please call these what they are?

   canonical
   edit_form or edit-form
   delete_form or delete-form

   There is no such thing as a "view" route...

5. +++ b/core/lib/Drupal/Core/Entity/entity.api.php
   @@ -385,6 +385,12 @@
   + * - Optionally, utilize an auto-route generator that provides typical routes:
   + *   @code
   + *   handlers = {
   + *     "route_provider" = {
   + *       "html" = "Drupal\Core\Entity\Routing\DefaultHtmlRouteProvider",
   + *   @endcode
     *
   

   Actually here I think I wouldn't have just said "provides typical routes" because you can always define your own route handler to define your own, and I'd also mention the admin class.

   This just needs a bit more detail.

6. +++ b/core/modules/block_content/src/Entity/BlockContent.php
   @@ -42,7 +45,7 @@
     *     "canonical" = "/block/{block_content}",
     *     "delete-form" = "/block/{block_content}/delete",
   - *     "edit-form" = "/block/{block_content}",
   + *     "edit-form" = "/block/{block_content}/edit",
     *     "collection" = "/admin/structure/block/block-content",
   

   I mentioned this in a previous review, but I don't think it's OK really to have a new URL here that didn't exist before. Previously, canonical and edit-form were the same URL path. Now you have two, but really there shouldn't be a "view" path for BlockContent entities. This is just wrong.

7. +++ b/core/modules/block_content/src/Entity/BlockContent.php
   --- a/core/modules/block_content/src/Tests/PageEditTest.php
   +++ b/core/modules/block_content/src/Tests/PageEditTest.php
   
   +++ b/core/modules/block_content/src/Tests/PageEditTest.php
   +++ b/core/modules/block_content/src/Tests/PageEditTest.php
   @@ -37,7 +37,7 @@ public function testPageEdit() {
   
   @@ -37,7 +37,7 @@ public function testPageEdit() {
        $this->assertTrue($block, 'Custom block found in database.');
    
        // Load the edit page.
   -    $this->drupalGet('block/' . $block->id());
   +    $this->drupalGet($block->urlInfo('edit-form'));
        $this->assertFieldByName($title_key, $edit[$title_key], 'Title field displayed.');
   

   Again, in tests I think calling the urlInfo() method is wrong.

   You want to verify in a test like this that if you go to a particular URL that is defined in the test as a literal string that it will work.

   If you write the test like this you are not testing for regressions like what is introduced by the problem above, where the URL changed and it shouldn't have.

8. +++ b/core/modules/comment/src/Entity/CommentType.php
   @@ -24,7 +24,10 @@
   + *     "route_provider" = {
   + *       "html" = "Drupal\Core\Entity\Routing\DefaultHtmlRouteProvider",
   + *     },
     *   },
   

   Really, when editing CommentType entities, you don't want to be on Admin?

9. +++ b/core/modules/language/src/Tests/LanguageConfigurationElementTest.php
   @@ -235,7 +235,7 @@ public function testTaxonomyVocabularyUpdate() {
   -    $this->drupalPostForm('admin/structure/taxonomy/manage/country', $edit, t('Save'));
   +    $this->drupalPostForm($vocabulary->urlInfo('edit-form'), $edit, t('Save'));
    
   

   Again I would avoid using urlInfo() in tests like this. Probably the URL changed here and the test failed and it would have uncovered a problem in the code, so instead someone changed the test. The test failure was real...

10. +++ b/core/modules/menu_link_content/src/Entity/MenuLinkContent.php
    @@ -31,7 +31,10 @@
    + *     "route_provider" = {
    + *       "html" = "Drupal\Core\Entity\Routing\DefaultHtmlRouteProvider",
    + *     },
      *   },
    

    Again shouldn't this be using the admin route provider?

11. +++ b/core/modules/menu_link_content/src/Entity/MenuLinkContent.php
    @@ -45,7 +48,7 @@
    - *     "canonical" = "/admin/structure/menu/item/{menu_link_content}/edit",
    + *     "canonical" = "/admin/structure/menu/item/{menu_link_content}",
      *     "edit-form" = "/admin/structure/menu/item/{menu_link_content}/edit",
      *     "delete-form" = "/admin/structure/menu/item/{menu_link_content}/delete",
    

    Again you've introduced a new URL path that should not exist. There should not be a way to view menu links on their own URL.

#146.11. This also leads to the following failure with the code that jhodgdon mentioned was removed in #141.8:

+++ b/core/modules/content_translation/src/Tests/ContentTranslationUITestBase.php
@@ -75,13 +75,6 @@ protected function doTestBasicTranslation() {
-    $this->drupalGet($entity->urlInfo());
-    $this->assertResponse(200, 'Entity URL is valid.');
-

The base test for translations is testing to see if the canonical link (default argument for urlinfo() ) is valid. But, MenuLinkContent doesn't have a view controller.

Re #146 8. and 10.: Those entities (and others) are completely managed on URLs under /admin, so they automatically get the admin theme. But we should not set _admin_route explicitly, that's why the default route provider is used

Ah, OK. Maybe we should add a comment to the Admin route provider class that says this is only needed for entities whose paths are outside of /admin then?

I don't get why this was assigned to me all the time.

Will take a stab at fixing the test fails. Not yet addressing any comments since #142.

tstoeckler++

So the test fails were again related to the problem of content entities without a canonical link relation. I think I've found a fairly generic solution, which should work in most cases. In ContentTranslationUiTestBase we had an explicit reference to canonical, so I implemented a little workaround for that.

Let's see what I broke with this.

Yep, looks this "missing canonical link" becomes noisy