Add route generation handlers for entities

Not sure whether this is a normal to be honest?

.

Do we have a reasonably fast and simple way to get all the routes for a particular entity or entity type once they are generated?

Let's add a small test.

@pwolanin
On the longrun we should either use linkTemplates or uriRelationships but yeah this is not really part of the discussion around this particular issue.

1. +++ b/core/lib/Drupal/Core/Entity/EntityManager.php
   @@ -311,19 +325,7 @@ public function getHandler($entity_type, $handler_type) {
   +      $this->handlers[$handler_type][$entity_type] = $this->doInstantiateHandler($class, $definition);
   

   Why do... ? That's a pattern used when having internal helper methods to a public method, but there is no public instantiateHandler(), so why not just name so?

2. +++ b/core/lib/Drupal/Core/Entity/EntityManager.php
   @@ -1206,4 +1208,31 @@ protected function deleteLastInstalledFieldStorageDefinition(FieldStorageDefinit
   +   * @return mixed
   

   @return object? Do we now no longer need a description?

3. +++ b/core/lib/Drupal/Core/Entity/EntityTypeInterface.php
   @@ -280,6 +280,15 @@ public function setFormClass($operation, $class);
   +   * @return array
   

   string[] is I think how we document this now?

4. +++ b/core/lib/Drupal/Core/Entity/Routing/EntityRouteProviderInterface.php
   @@ -0,0 +1,28 @@
   +  public function provideRoutes(RouteCollection $route_collection, EntityTypeInterface $entity_type);
   

   Wondering if provideRoutes() is really a good method name here?

5. +++ b/core/lib/Drupal/Core/EventSubscriber/EntityRouteProviderSubscriber.php
   @@ -0,0 +1,57 @@
   +  public function onDynamic(RouteBuildEvent $event) {
   +    foreach ($this->entityManager->getDefinitions() as $entity_type) {
   +      foreach ($this->entityManager->getRouteProviders($entity_type->id()) as $route_provider) {
   +        $route_provider->provideRoutes($event->getRouteCollection(), $entity_type);
   +      }
   +    }
   

   Wondering how fast this is for 50, 100 or however many entity types you will have. Would it make sense to optimize this to check if $entity_type has any router provider classes first?

6. +++ b/core/modules/node/src/Entity/NodeRouteProvider.php
   @@ -0,0 +1,48 @@
   +    $route = (new Route('/node/{node}'))
   +      ->addDefaults([
   +        '_content' => '\Drupal\node\Controller\NodeViewController::view',
   +        '_title_callback' => '\Drupal\node\Controller\NodeViewController::title',
   +      ])
   +      ->setRequirement('_entity_access', 'node.view');
   +    $route_collection->add('entity.node.canonical', $route);
   

   We're obviously not going to change that, but I kind of like the pattern we use for BaseFieldDefinition, with the ::create() method, then you don't need the () trick.

   I'm wondering when it makes sense to use constructor arguments, when multiple and when single methods. Seems like in cases like this, calling setDefault() would be easier to read? Or use the constructor, which has $defaults as second argument, and $requirements as third..

7. +++ b/core/modules/user/src/Entity/UserRouteProvider.php
   @@ -0,0 +1,51 @@
   +    $route = (new Route('/user/{user}/edit'))
   ...
   +    $route = (new Route('/user/{user}/cancel'))
   

   Those are not removed from user.routing.yml

Also, should we move routes that are specific to an entity type but will likely not be autogenerated too? things like node.add, entity.node.preview, entity.node.version_history

And, should we include node_type routes too in this issue? config entities are probably usually easier to generalize.

Why do... ? That's a pattern used when having internal helper methods to a public method, but there is no public instantiateHandler(), so why not just name so?

Fixed

@return object? Do we now no longer need a description?

Improved the docs a bit.

Wondering if provideRoutes() is really a good method name here?

Let's improve it properly, let's it return stuff and rename it to just routes?

Wondering how fast this is for 50, 100 or however many entity types you will have. Would it make sense to optimize this to check if $entity_type has any router provider classes first?

Well, let's add the check directly.

We're obviously not going to change that, but I kind of like the pattern we use for BaseFieldDefinition, with the ::create() method, then you don't need the () trick.

HAHAHHAHAHAA, did you really suggested to add a method to symfonys route object? Hilarious, just hilarious :)

I'm wondering when it makes sense to use constructor arguments, when multiple and when single methods. Seems like in cases like this, calling setDefault() would be easier to read? Or use the constructor, which has $defaults as second argument, and $requirements as third..

Well, you know everyone can do what he things is the best, but I would agree with you if php would have named parameters ...

Those are not removed from user.routing.yml

Good catch ...

Also, should we move routes that are specific to an entity type but will likely not be autogenerated too? things like node.add, entity.node.preview, entity.node.version_history

I don't think so we should ... these routes are specific to the link templates. Can we discuss that later?

HAHAHHAHAHAA, did you really suggested to add a method to symfonys route object? Hilarious, just hilarious :)

Yeah, should be easy, right? ;)

+++ b/core/modules/node/src/Entity/NodeRouteProvider.php
@@ -20,7 +20,8 @@ class NodeRouteProvider implements EntityRouteProviderInterface {
-  public function provideRoutes(RouteCollection $route_collection, EntityTypeInterface $entity_type) {
+  public function routes( EntityTypeInterface $entity_type) {

Hm, I would have gone with getRoutes() then, not sure.

Let's check if we can get some other opinions, from alexpott maybe?

I'm fine with discussing the non-link-template-specific routes later, but for example the version_history one *is* a link template, but it will not be possible to autogenerate it. So it is currently very hard to see what should be done there and what in routing.yml. IMHO, it would make sense to pull all routes that are for a specific entity type in the route provider. But separate issue is fine. Are we going to do a single one to move all node types over as a follow-up? doing per-module issues seems a bit overkill, as it is very easy for now. Or do you want to look into generating them first, before we move more of them around?

Well, because there's nothing standardized about that route?

entity.node.version_history:
  path: '/node/{node}/revisions'
  defaults:
    _title: 'Revisions'
    _content: '\Drupal\node\Controller\NodeController::revisionOverview'
  requirements:
    _access_node_revision: 'view'
  options:
    _node_operation_route: TRUE

Custom controller, access, options.. there's nothing that we could generate?

Hm. Sounds like a potential place for further standardization as time allows then. :-)

It's true that we won't be able to auto-generate every single route; since we cannot control what links an entity may define we can't guarantee that there's code ready for generating everything. That's fine, as long as we can get all of the major/easy/common ones.

Well, beside real life this statement is maybe right.

This certainly works much better in case you just have to care about REST routes.

This should hopefully fix it.

Anyone wants to step forward and review it?

The code is fine but the primary question is this way makes entity system easy?
The old approach require devs to use annotations for routes (link templates) and yml file so it's easy to find the route.
The new way require to implement additional class to provide routes for none-standard ones.

I really sure that we need to make it easy for devs to provide entity routes, but current code looks like complication.

+++ b/core/modules/node/node.routing.yml
@@ -48,24 +39,6 @@ entity.node.preview:
-entity.node.canonical:
-  path: '/node/{node}'
-  defaults:
-    _content: '\Drupal\node\Controller\NodeViewController::view'
-    _title_callback: '\Drupal\node\Controller\NodeViewController::title'
-  requirements:
-    _entity_access: 'node.view'

+++ b/core/modules/node/src/Entity/NodeRouteProvider.php
@@ -0,0 +1,51 @@
+    $route = (new Route('/node/{node}'))
+      ->addDefaults([
+        '_content' => '\Drupal\node\Controller\NodeViewController::view',
+        '_title_callback' => '\Drupal\node\Controller\NodeViewController::title',
+      ])
+      ->setRequirement('_entity_access', 'node.view');
+    $route_collection->add('entity.node.canonical', $route);

The old one is more readable

I asked in IRC but you didn't answer, so I will ask again here :p Why do we not have a base

EntityRouteProvider

implementation here that can be used?

@damiankloip
My computer of myself was probably offline at that point ... for now the point is to enable the machinery which will
allow us in future to auto-generate the common routes for entities. Writing this auto-generation as part of this issue just defers the process.

The old one is more readable

Well, you can't deny that for sure, but on the longrun you, especially for custom code, not have to write the code at all.

... Reroll.

The old one is more readable

Well, you can agree with that point, on the other hand that is not the point of this issue.

Readability is not the primary question here...

in future to auto-generate the common routes for entities

I really trying to get that future. The big picture is not clear.
In my POV entity should provide a canonical link prefix (path: /node, /taxonomy/term)
Then #16 EntityRouteProvider collects a link names (probably entity-operations) that exposed via entity annotation and build routes (maybe route collections per entity type).

#8

Are we going to do a single one to move all node types over as a follow-up?

@dawehner suppose 2 issues for config and content would be enough, is not it?

I'm rather surprised this didn't exist already. :-)

Although the method_exists() calls really really should e interfaces. Probably OT for this issue but who were those ever added as method_exists()s?

I just copied that over from the other handler code ... nothing new in this patch ...

I assume the idea is that this line changes to use the canonical link value once #2281645: Make entity annotations use link templates instead of route names is in? (And similar elsewhere.)

Right, and for obvious reasons we can't do it here.

The generic version of this class is also waiting on #2281645: Make entity annotations use link templates instead of route names, I take it?

its as simple as that.

So do you want to update the issue status of this issue?

+++ b/core/lib/Drupal/Core/Entity/Routing/EntityRouteProviderInterface.php
@@ -0,0 +1,29 @@
+  public function routes(EntityTypeInterface $entity_type);

getRoutes() would be more inline with our standards IMO. Especially in the entity/field system we generally only static methods don't have the "get" prefix.

Thank you tobias.

getRoutes() would be more inline with our standards IMO. Especially in the entity/field system we generally only static methods don't have the "get" prefix.

I'll don't complain here, but using "getters" for things which aren't getters let's you stop thinking about domain knowledge, but instead
just always expose internal data structures and be done. Sadly this antipattern is hard to stop, and requires work.

Thanks for the quick fix!

using "getters" for things which aren't getters let's you stop thinking about domain knowledge, but instead
just always expose internal data structures and be done

Wow, that's very interesting. I'll have to think about that some before being able to respond :-)

Btw. We went with this method name, because all the existing route callbacks in the $module.routing.yml files follow that pattern already.

Hmm... that's true. I guess it's up to the committers to commit either #19 or #26.

This conflicts with #2335879: Change SqlContentEntityStorageSchema::requiresEntityDataMigration() to ask the old storage handler if it has data rather than assuming yes unless NULL storage which just got committed, that added a public method of the method here called initiateHandler().

This conflicts with #2335879: Change SqlContentEntityStorageSchema::requiresEntityDataMigration() to ask the old storage handler if it has data rather than assuming yes unless NULL storage which just got committed, that added a public method of the method here called initiateHandler().

Cool, exactly what we need here.

Given that I think we are back to RTBC?

Yup, reroll looks good.

Worked on this issue on the train, so this justifies the tag.

This is good work, but I don't want to commit this during the Ghent sprint. On its own it makes the DX worse and while that can get fixed (via a base class) in follow-up issues, I don't feel that should be our focus. I'd rather have us focus on critical issues at this stage of the release cycle.

Let's just link to another related issue for now.

We seem to have zero documentation of what can go in the handlers key in an entity type annotation:

  /**
   * An array of handlers.
   *
   * @var array
   */
  protected $handlers = array();

(from \Drupal\Core\Entity\EntityType)

Whilst fixing this is beyond the scope of this issue. Adding a @todo here and creating the issue to add the documentation is not. Once this is done I don't see why we should hold up making progress here on the ghent sprint.

I think this change is overall positive for DX. If you are creating a new entity type you have to do less, if you looking for an entity's routes the current expectations would be to look in the routing file and entity type definition - now you need to look in a different place in entity type definition - but once you've learnt this then you're good. What we should have is a D8 to D8 change record to tell everyone.

As discussed in Amsterdam, the route generation handler is designed to be opt-in, defining the routes manually in routing.yml will still work.

I'm also not sure if we should even document this already in a change record, because it is only to prepare for the more important next step: Generating default routes. Not sure if it makes sense for contrib to already switch to this pattern as they will have to switch again if they want (part of) their entity routes to be automatically generated.

Cool, we are allowed to work on this issue again.

I'm also not sure if we should even document this already in a change record, because it is only to prepare for the more important next step: Generating default routes. Not sure if it makes sense for contrib to already switch to this pattern as they will have to switch again if they want (part of) their entity routes to be automatically generated.

This is a tough question ... its not adding advantage for contrib to use it.

Anyone wants to review it?

This issue still needs change record. Some minor nits. Please ignore if seems irrelevant other then that RTBC.

1. +++ b/core/lib/Drupal/Core/EventSubscriber/EntityRouteProviderSubscriber.php
   @@ -0,0 +1,74 @@
   +   * Provide routes on route rebuild time.
   

   Provides

2. +++ b/core/lib/Drupal/Core/EventSubscriber/EntityRouteProviderSubscriber.php
   @@ -0,0 +1,74 @@
   +  public function onDynamic(RouteBuildEvent $event) {
   ...
   +    $events[RoutingEvents::DYNAMIC][] = ['onDynamic'];
   

   Can we please give it some better name? onDynamic is not helpful a lot.

This issue still needs change record.
Right, ... we argued before, why we don't want to add one already.

ok then

Committed 1601b08 and pushed to 8.0.x. Thanks!

Thanks for adding the beta evaluation for to the issue summary.

I'm also not sure if we should even document this already in a change record, because it is only to prepare for the more important next step: Generating default routes.

I still think we should be working in on draft change record for this and the more important next step...