Make entity annotations use link templates instead of route names

ContentTranslationRouteSubscriber will have to work around this as well.
$entity_route = $collection->get($entity_type->getLinkTemplate('canonical')) won't work anymore.

Thank you tim, for pointing this out!

There are a couple of more places of which not all of them seem straightforward.

This issue feels more like research tbh.

https://api.drupal.org/api/drupal/core%21lib%21Drupal%21Core%21Entity%21...

Here is a bit more work ... You can work around most of the instances but let's be honest, using routes is actually better
in quite some places.

Here is an alternative approach we could discuss as well:
I really think that for most link templates using routes is actually way better. There are so many admin routes
which simply never makes sense to expose to rest. Here is a thought, why don't we keep the linkTemplates as route names but define a canonical path on top of it?

This time with the proper interdiff.

The node admin theme relies 100% on the route, so content translation has to deal with the route somehow.

• Adapted all the instances to "fallback" to entity.{$entity_type_id}.link_name
• Reverted the use of the link template in $entity->urlInfo()
• Some fixes here and there, like using proper link templates in the test
• Fixed the node admin theme problem for content translation by using

I tried to go with comment module first.

We should get rid of entity_uri first because it does not work now

Well, right, but this is another kinda orthogonal problem space.

Rewrote with using proper entity route names / paths if possible.

Some progress ...

Note: this issue includes the menu and the views patch in order to not fail everywhere. It also fixes the action and contact module.

Uploading a new patch. This has not the views issue anymore applied.

Some major problem at the moment: node for example has the field_ui_display_overview_node_type. Thinking about a proper solution.
The best strategy for now could be to avoid dealing with field_ui in this conversion.

Some progress, no actual though.

This ensures that the usual field_ui form works, but especially together with config translation this is a mindfuck.
I doubt it is a perfect idea in the first place to not support custom route names in the link templates.
For those UI only routes it feels much more appreciate to have something like link templates but with routes.

Let's see for now.

Work ...

Some more fixes. Still have no idea on the DefaultConfigTest

+++ b/core/lib/Drupal/Core/Entity/EntityManager.php
@@ -315,6 +315,20 @@ public function getHandler($entity_type, $handler_type) {
+  public function getAdminRouteInfo($entity_type_id, $bundle) {
+    if (($entity_type = $this->getDefinition($entity_type_id, FALSE)) && $entity_type->getLinkTemplate('admin-form')) {
+      return array(
+        'route_name' => "entity.$entity_type_id.admin_form",
+        'route_parameters' => array(
+          $entity_type->getBundleEntityType() => $bundle,
+        ),
+      );
+    }
+  }

Was this accidently re-added? I think we removed this when admin-form was changed to field_ui_base_route?

Some work.

Let's see, I would prefer something below 50 failures.

Still issues with translation fields, other issues are fixed though.

Some more work.

More fixes.

Suppose we should extract field_ui routes conversion into separate issue to minimize a patch and focus on actual changes.
Overall changes are great, but route rename causes current test failure

1. +++ b/core/modules/content_translation/src/Plugin/Derivative/ContentTranslationContextualLinks.php
   @@ -53,9 +53,10 @@ public function getDerivativeDefinitions($base_plugin_definition) {
   +    debug($this->derivatives);
   
   +++ b/core/modules/content_translation/src/Tests/ContentTranslationContextualLinksTest.php
   @@ -131,6 +131,7 @@ public function testContentTranslationContextualLinks() {
   +    debug($json);
   

   no reason ;)

2. +++ b/core/modules/field_ui/src/FieldOverview.php
   @@ -135,7 +135,7 @@ public function buildForm(array $form, FormStateInterface $form_state, $entity_t
   +          '#route_name' => "entity.field_config.field_ui.storage_{$this->entity_type}_edit_form",
   
   @@ -410,8 +410,8 @@ public function submitForm(array &$form, FormStateInterface $form_state) {
   +        $destinations[] = array('route_name' => "entity.field_config.field_ui.storage_{$this->entity_type}_edit_form", 'route_parameters' => $route_parameters);
   +        $destinations[] = array('route_name' => "entity.field_config.field_ui.field_{$this->entity_type}_edit_form", 'route_parameters' => $route_parameters);
   

   s/fieldui.//

3. +++ b/core/modules/field_ui/src/FieldUI.php
   @@ -29,7 +29,8 @@ class FieldUI {
   +      $bundle_entity_type = $entity_type->getBundleEntityType() != 'bundle'?  $entity_type->getBundleEntityType() : $entity_type->id();
   

   space

4. +++ b/core/modules/field_ui/src/Plugin/Derivative/FieldUiLocalTask.php
   @@ -70,41 +70,43 @@ public function getDerivativeDefinitions($base_plugin_definition) {
   +        $field_entity_type = $entity_type->getBundleEntityType() != 'bundle'?  $entity_type->getBundleEntityType() : $entity_type_id;
   
   @@ -172,19 +174,24 @@ public function getDerivativeDefinitions($base_plugin_definition) {
   +      $field_entity_type = $entity_type->getBundleEntityType() ?: $entity_type_id;
   

   consistency?

5. +++ b/core/modules/field_ui/src/Plugin/Derivative/FieldUiLocalTask.php
   @@ -70,41 +70,43 @@ public function getDerivativeDefinitions($base_plugin_definition) {
   +          'route_name' => "entity.$field_entity_type.field_ui_fields",
   ...
   +          'base_route' => "entity.$field_entity_type.field_ui_fields",
   ...
   +          'route_name' => "entity.{$field_entity_type}.field_ui_form_display",
   ...
   +          'base_route' => "entity.$field_entity_type.field_ui_fields",
   ...
   +          'route_name' => "entity.{$field_entity_type}.field_ui_display",
   ...
   +          'base_route' => "entity.$field_entity_type.field_ui_fields",
   ...
   +        $this->derivatives["field_edit_$field_entity_type"] = array(
   +          'route_name' => "field_ui.field_edit_$field_entity_type",
   ...
   +          'base_route' => "field_ui.field_edit_$field_entity_type",
   ...
   +        $this->derivatives["field_edit_$field_entity_type"] = array(
   +          'route_name' => "field_ui.storage_edit_$field_entity_type",
   ...
   +          'base_route' => "field_ui.field_edit_$field_entity_type",
   

   entity.{field_entity_type}. it's confusing when the param ends

Follow-up issue is #2346883: Standardize field_ui entity route names

merged changes
fixed leftover route renames
filed new bug #2347053: "Field settings" local task from Field UI is missing

proper patch

Let's see where we are now.i

Some more.

So, I don't really care how we define these things - but we shouldn't define the path in both the link template and the route independently.

If we put the paths back were, we need to derive the routes from them.

Extracted a part of patch

Meh, less problems.

Reapply after the URL issue.

Reroll.

I think content translation routes needs separate issue too.

Another day another try.

Some less failures.

There we go.

Let's start with converting some common cases to paths. Does anyone has an opinion whether we should add paths there everywhere already or whether this can happen in some follow up?
The paths here aren't the hard to review part of the issue though to be honest.

Let's unassign to make it clear that reviews would be welcomed!

Finally, a patch that passes the tests!

So I looked through the test and I'm very confused about a few things:

a) Do modules defining entities have to both declare the paths in their entity annotation and set up routes for them in the routing.yml files? Looks like the answer is yes...

b) It looks like although you've said the setLinkTemplate() method now is supposed to be given a path, the calls in the patch from content_translation and field_ui modules -- which changed in the patch -- are still using routes. This patch would be more convincing to me if the calls to these functions that are updated in this patch were actually converted to using paths, instead of just being changed from one route to another route. If they still need things to be routes, then ... can we even make this change?

c) I guess I'm confused about why we want to do this at all actually. Most of our URL generation stuff is being moved over to using routes, and right there in the top of the patch:

+++ b/core/lib/Drupal/Core/Entity/Entity.php
@@ -164,8 +164,9 @@ public function urlInfo($rel = 'canonical', array $options = []) {
     $link_templates = $this->linkTemplates();
 
     if (isset($link_templates[$rel])) {
-      // If there is a template for the given relationship type, generate the path.
-      $uri = new Url($link_templates[$rel], $this->urlRouteParameters($rel));
+      $route_parameters = $this->urlRouteParameters($rel);
+      $route_name = "entity.{$this->entityTypeId}." . str_replace(array('-', 'drupal:'), array('_', ''), $rel);
+      $uri = new Url($route_name, $route_parameters);

This code is a lot more complicated because of this change. Why exactly do we want to do this? The two-line issue summary is not illuminating...

d)

+++ b/core/modules/book/book.module
@@ -89,7 +89,7 @@ function book_entity_type_build(array &$entity_types) {
   $entity_types['node']
     ->setFormClass('book_outline', 'Drupal\book\Form\BookOutlineForm')
     ->setLinkTemplate('book-outline-form', 'entity.node.book_outline_form')
-    ->setLinkTemplate('book-remove-form', 'entity.node.book_remove_form');
+    ->setLinkTemplate('book-remove-form', 'book.remove');
 }
 

There are a few places like this in the patch where calls to setLinkTemplate() have been changed, but I didn't see changes in corresponding routing.yml files. As far as I can see, there is no book.remove route defined anywhere in Core, nor is book.remove a path... What's going on here, and how can this work -- is there just no test coverage that is exercising this path or link template call? [applies to several pieces of the patch]

e) There is no update to the explanations in the Entity topic about entity routing. This is in core/modules/system/entity.api.php in @defgroup entity_api.

a) For now. The goal is to automate the generation of them using a new entity handler, that you can opt-in to.

See #2350503: Add route generation handlers for entities and #2259445: Entity Resource unification

b) The conversion to links is on progress I think and not done yet.

c) See parent meta issue.

I think that there were objections above about field UI and probably content translation that they needed routes. I'd really like to see that these still work without the routes in there...

@jhodgdon
Quick story: In order to autogenerate routes you need some paths, you should not start with routes as you easily get into circular dependencies. On top of that
link templates aren't a concept we invented to be with routes, they are originally paths, as the name says.

Yes sure, this is currently ongoing work ...

But the field UI module needs to have routes, in order to make field UI admin tasks/tabs/etc. for the Manage Fields and Manage Display and Manage Forms pages, right? Those have to be set up with route names... which I think was brought up early in this issue or the meta.

@dawehner I think we need separate issue for CT routes... maybe just add to #2346883: Standardize field_ui entity route names because both affected by field ui!

Let's post quite an update.

This time it should install.

It could be green, reviews are always welcomed.

Totally anal nitpick: One of these method implementations is using concatenation, the other interpolation. We should probably pick one. (I like interpolation, myself, but don't feel that strongly.)

Well, whatever ...

This is mostly a reroll for now.

So... My understanding of this issue is that the idea is that entities will no longer define routes in their annotations, and they will instead just have link templates. The 2-line issue summary says this too.

But ... it seems like they still have routes, but they are just less transparent than they used to be? Field UI, Content Translation, and Config Translation all require routes in order to work. So now they're magic instead of explicit in the entity definition? I mean, it looks like node.routing.yml doesn't have any changes in this patch, so the Node module still has to define the routes, but ... ???

Really we need an issue summary and a change notice so we can evaluate what this patch/issue is actually doing and what changes entity-providing modules will need to make in order for it to work.

So... My understanding of this issue is that the idea is that entities will no longer define routes in their annotations, and they will instead just have link templates. The 2-line issue summary says this too.

Note: this always have been about link template, which at some point switched from being paths to be route names. This issue
just forces the already existing naming scheme for entity routes on link templates.

But ... it seems like they still have routes, but they are just less transparent than they used to be? Field UI, Content Translation, and Config Translation all require routes in order to work. So now they're magic instead of explicit in the entity definition? I mean, it looks like node.routing.yml doesn't have any changes in this patch, so the Node module still has to define the routes, but ... ???

Right, so you kinda misunderstood the purpose of this issue. This is about providing the base paths, so for example REST can use them.
Once you have both this issue and #2350503: Add route generation handlers for entities in place it will be possible to autogenerate basic routes.

The issue summary on this issue is 3 lines. Perhaps a real summary would avoid this misunderstanding?

Also will need a change notice, which will help explain the changes developers will need to do.

We also need to go through the flowchart on https://www.drupal.org/contribute/core/beta-changes and justify whether this issue even can be dealt with in 8.0.x, and add this to the issue summary. For this change, I think the justification has to be the "impact vs. disruption" defense.

Added a beta evaluation. I thought we decided to do this on Drupalcon Amsterdam.
Dear 19 people on this issue, feel free to review it.

Beta phase evaluation

Adressed point 5

Come on, ... this is something just useful for FieldUI, given that it has to be done in a specific way.

Added a change record, so we can drop all the tags.

Rerolled.

For now just another reroll.

Some bug fixes.

Some less.

Some less ...

Take that!!

Great, the tests finally passed!

A few nitpick/documentation comments on the patch:

a)

    * @return array
-   *   An array of link templates containing route names.
+   *   An array of link templates containing route names (temporary) or paths.
    */
   protected function linkTemplates() {

What does this "(temporary)" really mean? Very confusing, especially as it is in the @return, so how would I know what the function is returning? And in the setLinkTemplate for the $path argument too... I think the Core code is expecting paths now in most cases, and maybe is still expecting routes in some cases, but how is a user of the linkTemplates() function supposed to know which is being returned, and how is a user of the setLinkTemplate() function expected to know which is expected to be provided? The code as it is after this patch, I think, cannot really handle both in every case.

Really this documentation is unclear about when routes can be used and when paths are necessary. I think the only solution is to finish the patch and eliminate route names completely, unless you can be more specific in the docs, because this "temporary" is not clear at all.

b)

+++ b/core/modules/comment/src/Tests/CommentTypeTest.php
@@ -108,9 +108,10 @@ public function testCommentTypeEditing() {
     $this->drupalGet('admin/structure/comment');
     $this->assertRaw('Bar', 'New name was displayed.');
     $this->clickLink('Manage fields');
-    $this->assertUrl(\Drupal::url('field_ui.overview_comment', array('comment_type' => 'comment'), array('absolute' => TRUE)), [], 'Original machine name was used in URL.');
+    $this->assertUrl(\Drupal::url('entity.comment_type.field_ui_fields', ['comment_type' => 'comment'], ['absolute' => TRUE]), [], 'Original machine name was used in URL.');
     $this->assertTrue($this->cssSelect('tr#comment-body'), 'Body field exists.');
 
+

Extra blank line added in patch.

c)

+++ b/core/modules/config_translation/config_translation.module

-        $entity_type->setLinkTemplate('drupal:config-translation-overview', 'config_translation.item.overview.');
+        $entity_type->setLinkTemplate('config-translation-overview', 'entity.' . $entity_type_id . '.config_translation_overview');

-        $entity_type->setLinkTemplate('drupal:config-translation-overview', 'config_translation.item.overview.' . $entity_type->getLinkTemplate('edit-form'));
+        $entity_type->setLinkTemplate('config-translation-overview', 'entity.' . $entity_type_id . '.config_translation_overview');
 
+++ b/core/modules/content_translation/content_translation.module
@@ -116,7 +116,7 @@ function content_translation_entity_type_alter(array &$entity_types) {
+          $entity_type->setLinkTemplate('drupal:content-translation-overview', "entity.{$entity_type->id()}.content_translation_overview");

+++ b/core/modules/content_translation/tests/src/Unit/Menu/ContentTranslationLocalTasksTest.php
@@ -28,7 +28,7 @@ protected function setUp() {
       ->method('getLinkTemplate')
       ->will($this->returnValueMap(array(
         array('canonical', 'entity.node.canonical'),
+        array('drupal:content-translation-overview', 'entity.node.content_translation_overview'),

+++ b/core/modules/field_ui/field_ui.module
+        ->setLinkTemplate('field_ui-fields', "entity.{$entity_type->id()}.field_ui_fields")
+        ->setLinkTemplate('field_ui-form-display', "entity.{$entity_type->id()}.field_ui_form_display")
+        ->setLinkTemplate('field_ui-display', "entity.{$entity_type->id()}.field_ui_display");

See (a)... Why are these chunks of code still using/expecting/setting the link template to be a route instead of a path? If they cannot be made to work with paths, then this change should not, I think, be made at all. I've been trying to ask this question throughout the life of this issue and no one can answer the question of whether the translation and field UI modules can deal with them being paths instead of routes. I am still not convinced.

d) There is information in the Entity API topic (@defgroup entity in core/modules/system/entity.api.php) about how to define entities. I don't see this updated in the patch -- it is saying to put routes in the annotation currently. So that needs to be updated, and I think it also needs to explain the magic route names that you need to set up, because this will not be obvious to the novice entity type programmer.

e) I think that information about the magic route names you need to set up should also go into the draft change record. It looks like at least one of the core entities had a new route that had to be defined due to this change (it was using the "canonical" route for both the "canonical" link type and the "edit" link type and this is apparently not allowed any more. The change notice says nothing about this, but it is something some module developers would presumably need to change if Core needed to).

1. +++ b/core/lib/Drupal/Core/Entity/Entity.php
   @@ -164,8 +164,9 @@ public function urlInfo($rel = 'canonical', array $options = []) {
   -      // If there is a template for the given relationship type, generate the path.
   -      $uri = new Url($link_templates[$rel], $this->urlRouteParameters($rel));
   +      $route_parameters = $this->urlRouteParameters($rel);
   +      $route_name = "entity.{$this->entityTypeId}." . str_replace(array('-', 'drupal:'), array('_', ''), $rel);
   +      $uri = new Url($route_name, $route_parameters);
   
   +++ b/core/modules/block_content/block_content.routing.yml
   @@ -45,6 +45,15 @@ entity.block_content.canonical:
   +entity.block_content.edit_form:
   +  path: '/block/{block_content}'
   
   +++ b/core/modules/block_content/src/Entity/BlockContent.php
   @@ -39,9 +39,9 @@
   + *     "edit-form" = "/block/{block_content}",
   

   At first, I was thoroughly confused by the first cited code block.

   But then I saw the second and third cited code blocks, and the puzzle pieces suddenly fit together.

   We're consciously choosing to violate DRY here, right?

   What happens if the path defined in the route doesn't match that in the links annotation in the entity type definition?

   EDIT: read #2259445: Entity Resource unification. The IS there says:

   As of beta1 entity definitions specify route links to route names. That will be changed in a future beta to specify uri templates (eg, "/node/{node}") instead. That will allow the HTML routes for entities to be auto-generated by the system rather than requiring module developers to redundantly define routes in a routing.yml file. Once that mechanism is in place defining entity routes manually in routing.yml will be discouraged.

   So that's how we'll fix the DRY violation: we will add code that automatically generates all entity routes, i.e. for every link relationship. This will be done in #2350509: Implement auto-route generation for all core entities and convert all of the core entities.. But this is major and that is normal. Which means it's quite likely we'll be stuck with the DRY violation. That'll be a very confusing DX. Which makes me think we should bump #2350509: Implement auto-route generation for all core entities and convert all of the core entities. to major.

2. +++ b/core/lib/Drupal/Core/Entity/Entity.php
   @@ -224,7 +225,7 @@ public function hasLinkTemplate($rel) {
   +   *   An array of link templates containing route names (temporary) or paths.
   
   +++ b/core/lib/Drupal/Core/Entity/EntityTypeInterface.php
   @@ -455,7 +455,8 @@ public function getLinkTemplates();
   +   *   The route name (temporary) or path for this link, or FALSE if it doesn't
   
   @@ -475,12 +476,12 @@ public function hasLinkTemplate($key);
   +   *   The route name (temporary) or path to use for the link.
   

   Like jhodgdon in a), I'm confused by this. Perhaps it's a remnant of what the IS currently says? ( We determined in Amsterdam that it's best to not make it temporary and just switch everything over at once.)

3. +++ b/core/modules/config_translation/config_translation.module
   @@ -83,7 +84,7 @@ function config_translation_entity_type_alter(array &$entity_types) {
   +        $entity_type->setLinkTemplate('config-translation-overview', 'entity.' . $entity_type_id . '.config_translation_overview');
   
   @@ -91,7 +92,7 @@ function config_translation_entity_type_alter(array &$entity_types) {
   +        $entity_type->setLinkTemplate('config-translation-overview', 'entity.' . $entity_type_id . '.config_translation_overview');
   
   +++ b/core/modules/content_translation/content_translation.module
   @@ -116,7 +116,7 @@ function content_translation_entity_type_alter(array &$entity_types) {
   +          $entity_type->setLinkTemplate('drupal:content-translation-overview', "entity.{$entity_type->id()}.content_translation_overview");
   
   +++ b/core/modules/field/src/Entity/FieldConfig.php
   @@ -261,12 +261,12 @@ public static function postDelete(EntityStorageInterface $storage, array $fields
   +      $link_templates["{$this->entity_type}-field-edit-form"] = 'entity.field_config.' . $this->entity_type . '_field_edit_form';
   +      $link_templates["{$this->entity_type}-storage-edit-form"] = 'entity.field_config.' . $this->entity_type . '_storage_edit_form';
   +      $link_templates["{$this->entity_type}-field-delete-form"] = 'entity.field_config.' . $this->entity_type . '_field_delete_form';
   

   +1 to what jhodgdon says in c). I guess this is why it said "temporary". But is that still truly necessary, or does this just stem from early work from a long time ago? In other words: is it necessary for these to continue to use routes?

4. +++ b/core/modules/config_translation/config_translation.module
   @@ -157,12 +159,19 @@ function config_translation_entity_operation(EntityInterface $entity) {
   +    // @todo Special case field UI.
   

   Still needs to be done here, or in a follow-up?

5. +++ b/core/modules/field_ui/src/FieldConfigListBuilder.php
   @@ -164,11 +164,24 @@ public function getDefaultOperations(EntityInterface $entity) {
   +      $operations['edit'] = array(
   +          'title' => $this->t('Edit'),
   +          'weight' => 10,
   +        ) + $entity->urlInfo("{$entity->entity_type}-field-edit-form")->toArray();
   ...
   +      $operations['delete'] = array(
   +          'title' => $this->t('Delete'),
   +          'weight' => 100,
   +        ) + $entity->urlInfo("{$entity->entity_type}-field-delete-form")->toArray();
   

   Nit: indentation problems.

6. +++ b/core/modules/field_ui/src/FieldConfigListBuilder.php
   @@ -164,11 +164,24 @@ public function getDefaultOperations(EntityInterface $entity) {
   -      'title' => $this->t('Storage settings'),
   +      'title' => $this->t('Field settings'),
   ...
   -      'attributes' => array('title' => $this->t('Edit storage settings.')),
   ...
   +      'attributes' => array('title' => $this->t('Edit field settings.')),
   

   Changing operation titles seems out of scopes?

Feedback from @jhodgdon

What does this "(temporary)" really mean? Very confusing, especially as it is in the @return, so how would I know what the function is returning? And in the setLinkTemplate for the $path argument too... I think the Core code is expecting paths now in most cases, and maybe is still expecting routes in some cases, but how is a user of the linkTemplates() function supposed to know which is being returned, and how is a user of the setLinkTemplate() function expected to know which is expected to be provided? The code as it is after this patch, I think, cannot really handle both in every case.

See (a)... Why are these chunks of code still using/expecting/setting the link template to be a route instead of a path? If they cannot be made to work with paths, then this change should not, I think, be made at all. I've been trying to ask this question throughout the life of this issue and no one can answer the question of whether the translation and field UI modules can deal with them being paths instead of routes. I am still not convinced.

Converted the other existing usecases. In general ... we will use the paths for really generic code like REST module, otherwise the path will not be used here, at least for now.

d) There is information in the Entity API topic (@defgroup entity in core/modules/system/entity.api.php) about how to define entities. I don't see this updated in the patch -- it is saying to put routes in the annotation currently. So that needs to be updated, and I think it also needs to explain the magic route names that you need to set up, because this will not be obvious to the novice entity type programmer.

Updated ...

e) I think that information about the magic route names you need to set up should also go into the draft change record. It looks like at least one of the core entities had a new route that had to be defined due to this change (it was using the "canonical" route for both the "canonical" link type and the "edit" link type and this is apparently not allowed any more. The change notice says nothing about this, but it is something some module developers would presumably need to change if Core needed to).

Just to be clear, this is not a notion introduce by this patch, it just enforces the policy existing since quite a long time. I hope you don't freak out because of that.

Feedback from @Wim Leers

Note to point 1: We just move the RY violation from the routes names to the route paths. Its mothing fundamentally new we introduce here.
Yes, we might be able to auto-generate some routes, but certainly not all,

Like jhodgdon in a), I'm confused by this. Perhaps it's a remnant of what the IS currently says? ( We determined in Amsterdam that it's best to not make it temporary and just switch everything over at once.)

Never understood that point though honestly, it just gave the patch a hard time.

+1 to what jhodgdon says in c). I guess this is why it said "temporary". But is that still truly necessary, or does this just stem from early work from a long time ago? In other words: is it necessary for these to continue to use routes?

Fixed

Still needs to be done here, or in a follow-up?

It is done right below it :)

Changing operation titles seems out of scopes?

OH that was probably caused by some of the crazy rebasing.

1. +++ b/core/lib/Drupal/Core/Entity/EntityTypeInterface.php
   @@ -455,7 +455,8 @@ public function getLinkTemplates();
   -   *   The route name for this link, or FALSE if it doesn't exist.
   +   *   The path for this link, or FALSE if it doesn't
   +   *   exist.
   

   no need in second line

2. +++ b/core/modules/field/src/Entity/FieldConfig.php
   @@ -261,12 +261,12 @@ public static function postDelete(EntityStorageInterface $storage, array $fields
   -      $link_templates['edit-form'] = 'field_ui.field_edit_' . $this->entity_type;
   -      $link_templates['storage-edit-form'] = 'field_ui.storage_edit_' . $this->entity_type;
   -      $link_templates['delete-form'] = 'field_ui.delete_' . $this->entity_type;
   +      $link_templates["{$this->entity_type}-field-edit-form"] = 'entity.field_config.' . $this->entity_type . '_field_edit_form';
   +      $link_templates["{$this->entity_type}-storage-edit-form"] = 'entity.field_config.' . $this->entity_type . '_storage_edit_form';
   +      $link_templates["{$this->entity_type}-field-delete-form"] = 'entity.field_config.' . $this->entity_type . '_field_delete_form';
   

   It's not clear why field storage edit is bound to field config.
   Suppose field_config entity suffix should not contain "_field" just "_edit_form" and "_delete_form"

3. +++ b/core/modules/field_ui/field_ui.module
   @@ -91,10 +91,14 @@ function field_ui_entity_type_build(array &$entity_types) {
   +      // To generate links in the UI we use the route names, so we don't have
   +      // to know the exact required routes.
   +      // This allows us to not require route information inside this hook, which
   +      // otherwise could result in circular dependencies.
   ...
   +        ->setLinkTemplate('field_ui-fields', "admin/{$entity_type->id()}/fields")
   +        ->setLinkTemplate('field_ui-form-display', "admin/{$entity_type->id()}/fields-form-display")
   +        ->setLinkTemplate('field_ui-display', "admin/{$entity_type->id()}/fields-display");
   

   used path looks strange

1. Note to point 1: We just move the RY violation from the routes names to the route paths. Its mothing fundamentally new we introduce here.

   Great point! Agreed.

2. +++ b/core/modules/config_translation/config_translation.module
   @@ -84,7 +84,7 @@ function config_translation_entity_type_alter(array &$entity_types) {
   -        $entity_type->setLinkTemplate('config-translation-overview', 'entity.' . $entity_type_id . '.config_translation_overview');
   +        $entity_type->setLinkTemplate('config-translation-overview', $entity_type->getLinkTemplate('edit-form') . '/translate');
   

   Isn't it unsettling that the patch was green before this change?

   We could add some validation in setLinkTemplate(), e.g. check if the first character is a slash, otherwise throw an exception?

3. +++ b/core/modules/field_ui/field_ui.module
   @@ -91,10 +91,14 @@ function field_ui_entity_type_build(array &$entity_types) {
   +        ->setLinkTemplate('field_ui-fields', "admin/{$entity_type->id()}/fields")
   +        ->setLinkTemplate('field_ui-form-display', "admin/{$entity_type->id()}/fields-form-display")
   +        ->setLinkTemplate('field_ui-display', "admin/{$entity_type->id()}/fields-display");
   

   Don't these need a leading slash like the other link templates?

Isn't it unsettling that the patch was green before this change?

We could add some validation in setLinkTemplate(), e.g. check if the first character is a slash, otherwise throw an exception?

Well yeah we could ... we indeed never used the VALUE of that link template, which is the reason why it passed.

Don't these need a leading slash like the other link templates?

Yeah we should.

Added an exception and expanded the unit test coverage for it.

Let's see whether this fixes it all.

1. +++ b/core/lib/Drupal/Core/Entity/EntityType.php
   @@ -547,6 +547,10 @@ public function hasLinkTemplate($key) {
   +    if ($path[0] !== '/') {
   +      throw new \InvalidArgumentException('Link templates takes path, which have to start with a leading slash.');
   +    }
   

   Yay!

   s/takes path/accept paths/

2. +++ b/core/lib/Drupal/Core/Entity/EntityTypeInterface.php
   @@ -455,7 +455,8 @@ public function getLinkTemplates();
   +   *   The path for this link, or FALSE if it doesn't
   +   *   exist.
   

   80 cols; could be fixed on commit.

3. +++ b/core/modules/field_ui/field_ui.module
   @@ -91,10 +91,14 @@ function field_ui_entity_type_build(array &$entity_types) {
   +      // To generate links in the UI we use the route names, so we don't have
   +      // to know the exact required routes.
   

   80 cols.

4. +++ b/core/modules/field_ui/src/FieldUI.php
   @@ -66,4 +68,18 @@ public static function getNextDestination(array $destinations) {
   +   *   The used entity type in the route name.
   

   Super nitpicky: s/entity type/entity type ID/

5. +++ b/core/modules/system/entity.api.php
   @@ -327,10 +327,11 @@
   + *   see @ref sec_routes below for some routing notes. Typical link
   

   s/see/See/

   80 cols.

RTBC because all nitpicks can easily be fixed on commit.

Fixed the nits.

Thanks!

One more minor nit, in EntityTypeInterface:

-   * @param string $route_name
-   *   The route name to use for the link.
+   * @param string $path
+   *   The route path to use for the link.
    *
    * @return $this
    */
-  public function setLinkTemplate($key, $route_name);
+  public function setLinkTemplate($key, $path);

This needs an @throws, due to:

+  public function setLinkTemplate($key, $path) {
+    if ($path[0] !== '/') {
+      throw new \InvalidArgumentException('Link templates accepts paths, which have to start with a leading slash.');
+    }

in EntityType right?

This is also garbled:

+++ b/core/modules/system/entity.api.php
@@ -327,10 +327,11 @@
  *   also need to add a corresponding route to your module's routing.yml file;
  *   see the entity.node.canonical route in node.routing.yml for an example, and see
  *   @ref sec_routes below for some notes.
- * - Define routing and links for the various URLs associated with the entity.
+ * - Define routes and links for the various URLs associated with the entity.
  *   These go into the 'links' annotation, with the link type as the key, and
- *   the route machine name (defined in your module's routing.yml file) as the
- *   value; see @ref sec_routes below for some routing notes. Typical link
+ *   the path of this link template. The corresponding route requires the
+ *   following route name: "entity.$entity_type_id.$link_template_type".
+ *   See @ref sec_routes below for some routing notes. Typical link

It now says

These go into the 'links' annotation, with the link type as the key, and the path of this link template.

Needs "... as the value".

Thank you, here is a fix for those points.

Finally we managed to got over the 100k patch size :)

AFAICT #128 addressed all concerns in #127 (good nitpicks btw!).

Thanks!

*blink* can we remove the rest of the route system in a followup as well? Since we are not using route names here I am sure it can be done elsewhere. Progress at last.

Committed 77725b6 and pushed to 8.0.x. Thanks!

I added the beta evaluation to the issue summary.

alexpott++
alexpott++
alexpott++
alexpott++
alexpott++

This seems broken:

- // If there is a template for the given relationship type, generate the path.
- $uri = new Url($link_templates[$rel], $this->urlRouteParameters($rel));
+ $route_parameters = $this->urlRouteParameters($rel);
+ $route_name = "entity.{$this->entityTypeId}." . str_replace(array('-', 'drupal:'), array('_', ''), $rel);
+ $uri = new Url($route_name, $route_parameters);

$link_templates isn't being used even though it is being tested for.

I now get this error when using a contrib module with a custom entity type:

Twig_Error_Runtime: An exception has been thrown during the rendering of a template ("Route "entity.coin_payment_type.edit_form" does not exist.") in "core/modules/system/templates/links.html.twig" at line 51. in Twig_Template->displayWithErrorHandling() (line 304 of core/vendor/twig/twig/lib/Twig/Template.php).

@jbrown: Your routes must name conform to entity.$entity_type.$link_template (with - replaced with _). This worked fine for me after renaming my routes accordingly. We should probably mention that in the change record.

If you still have a bug, please open a follow-up.

Okay thanks!

But why are the link paths specified at all in the annotations if they are also defined in the routing file?

See #2350509: Implement auto-route generation for all core entities and convert all of the core entities..

Thanks!

Updated change log regarding route names.