Problem/Motivation

Right now, entity lists are not standardized as a link template. We have an entity_list route thingy, we have a list builder handler, but they are not connnected. Route names are not standardized and there is no link template for it, which means that there is now way to get the list route for an entity type in a generic way.

We have two immediate use cases for this:
* Provide a useful default delete confirmation form that entity types can use without having to add a custom class. The patch over there already removes hundreds of lines of code, this will make it even better.
* The ability to generate the route definition in #2350509: Implement auto-route generation for all core entities and convert all of the core entities. (although we still need solve the problem of the missing plural label)

Proposed resolution

* Add a 'collection' link template. (As defined by http://www.iana.org/assignments/link-relations/link-relations.xml)
* Rename all routes to the now enforced route name pattern: entity.$entity_type.collection

Remaining tasks

None.

User interface changes

None.

API changes

The affected routes are renamed, direct references to them need to be updated. Existing entity types can opt in to this link template, they are not forced to use it.

Beta phase evaluation

Reference: https://www.drupal.org/core/beta-changes
Issue category Task, enables us to make generic forms and route generation code more useful.
Issue priority Major, because I think this is a missing part in our link template API, and the entity route generation is major as well.
Disruption See API changes, contrib modules that reference any of the renamed routes need to be updated. References to those from other modules are fairly uncommon, however.

Note: Credit @larowlan for his work in #2401793: Rename entity list routes to entity.$entity_type.list to make autogeneration possible..

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Berdir’s picture

Status: Active » Needs review
FileSize
1.95 KB

No idea if this is enough to define it :)

dawehner’s picture

+++ b/core/modules/node/src/Entity/NodeType.php
@@ -36,7 +36,8 @@
+ *     "list" = "node.overview_types"

Well, if we do that, we should rename the routes as well.

Berdir’s picture

We could do that, and we probably should if we want to be able to generate them, but having a link template means that it doesn't matter how it is named :)

Adding this is an API addition, renaming existing routes an API change, that's why I'm not sure..

dawehner’s picture

We could do that, and we probably should if we want to be able to generate them, but having a link template means that it doesn't matter how it is named :)

This is not TRUE.

Link templates already supposed to be named entity.$entity_type.$link_template. Breaking this on purpose is a nogo.

dawehner’s picture

Status: Needs review » Reviewed & tested by the community
Berdir’s picture

Status: Reviewed & tested by the community » Needs review

Oh, I didn't mean this to be done already, just started with one example, want to add it for other entity types as well and probably this needs to be mentioned somewhere in the documentation?

Berdir’s picture

FileSize
34.48 KB

These are all that I could find, including a few usages of it.

Doesn't make much of a difference right now, but it will for #1728804: Introduce (Content)EntityDeleteForm and children to handle entity deletions.

Status: Needs review » Needs work

The last submitted patch, 7: entity-list-link-template-2401505-7.patch, failed testing.

Berdir’s picture

Status: Needs work » Needs review
FileSize
35.24 KB
4.71 KB

Fixing some tests.

amateescu’s picture

Looking at http://www.iana.org/assignments/link-relations/link-relations.xml, this looks more like a 'collection' link template to me :)

Status: Needs review » Needs work

The last submitted patch, 9: entity-list-link-template-2401505-9.patch, failed testing.

Berdir’s picture

That's fine with me as well ;)

larowlan’s picture

Berdir’s picture

Status: Needs work » Needs review
FileSize
35.03 KB

Renamed to collection. No interdiff, as every line in the patch changed ;)

amateescu’s picture

+++ b/core/lib/Drupal/Core/Entity/Entity.php
@@ -273,9 +273,12 @@ public function url($rel = 'canonical', $options = array()) {
+    if ($rel != 'list') {

+++ b/core/modules/action/action.module
@@ -45,5 +45,6 @@ function action_entity_type_build(array &$entity_types) {
+    ->setLinkTemplate('list', 'entity.action.edit_form');

+++ b/core/modules/field_ui/field_ui.module
@@ -302,6 +303,7 @@ function field_ui_entity_type_alter(array &$entity_types) {
+  $form_mode->setLinkTemplate('list', 'field_ui.entity_form_mode_list');

@@ -311,4 +313,5 @@ function field_ui_entity_type_alter(array &$entity_types) {
+  $view_mode->setLinkTemplate('list', 'field_ui.entity_view_mode_list');

+++ b/core/modules/menu_ui/menu_ui.module
@@ -66,7 +66,8 @@ function menu_ui_entity_type_build(array &$entity_types) {
+    ->setLinkTemplate('list', 'menu_ui.overview_page');

+++ b/core/modules/shortcut/shortcut.module
@@ -373,7 +373,7 @@ function shortcut_toolbar() {
+          '#url' => $shortcut_set->urlInfo('list'),

+++ b/core/modules/views_ui/views_ui.module
@@ -61,7 +61,8 @@ function views_ui_entity_type_build(array &$entity_types) {
+    ->setLinkTemplate('list', 'views_ui.list');

Missed a few :P

Berdir’s picture

Noticed that myself and had it already fixed but forgot to re-create the patch...

:P yourself!

The last submitted patch, 14: entity-list-link-template-2401505-14.patch, failed testing.

dawehner’s picture

  1. +++ b/core/lib/Drupal/Core/Entity/Entity.php
    diff --git a/core/modules/action/action.module b/core/modules/action/action.module
    index f1cde81..87148de 100644
    
    index f1cde81..87148de 100644
    --- a/core/modules/action/action.module
    
    --- a/core/modules/action/action.module
    +++ b/core/modules/action/action.module
    
    +++ b/core/modules/action/action.module
    +++ b/core/modules/action/action.module
    @@ -45,5 +45,6 @@ function action_entity_type_build(array &$entity_types) {
    
    @@ -45,5 +45,6 @@ function action_entity_type_build(array &$entity_types) {
         ->setFormClass('delete', 'Drupal\action\Form\ActionDeleteForm')
         ->setListBuilderClass('Drupal\action\ActionListBuilder')
         ->setLinkTemplate('delete-form', 'entity.action.delete_form')
    -    ->setLinkTemplate('edit-form', 'entity.action.edit_form');
    +    ->setLinkTemplate('edit-form', 'entity.action.edit_form')
    +    ->setLinkTemplate('collection', 'entity.action.edit_form');
     }
    

    We use the same route for a collection as for editing an action? This can't be right

  2. +++ b/core/modules/block_content/src/Entity/BlockContent.php
    @@ -42,6 +42,7 @@
    + *     "collection" = "block_content.list",
    

    I still highly dislike that we don't adapt the route names if we introduce them here. I regret that we haven't enforced that somehow in our code earlier when we did all the conversions.

  3. +++ b/core/modules/search/src/Controller/SearchController.php
    @@ -191,7 +191,8 @@ public function performOperation(SearchPageInterface $search_page, $op) {
    +    $url = $search_page->urlInfo('collection');
    +    return $this->redirect($url->getRouteName(), $url->getRouteParameters());
    

    Don't we have to cary over the options as well? you never know

Wim Leers’s picture

Status: Needs review » Needs work

+1 for dawehner's feedback, but adding one nuance:

+++ b/core/modules/block_content/src/Entity/BlockContent.php
@@ -42,6 +42,7 @@
+ *     "collection" = "block_content.list",

'collection' versus 'list', consistency would be nice.

amateescu’s picture

'collection' versus 'list', consistency would be nice.

See #10 for why the current patch uses 'collection' for the link template. Are you suggesting that we should rename the entity list handlers at this point? :)

Wim Leers’s picture

#20: just renaming the routes would already be nice. Renaming entity list handlers is indeed probably not ok at this point :)

Berdir’s picture

Title: Add an entity link template for lists » Add an entity collection template for lists
Status: Needs work » Needs review

Re-rolled,
# 18
1. Fixed as part of the rebase.
3. Fixed, a redirectUrl() would be neat I guess ;)

2. Well, you won now, as you got the link issue in in time ;)

route names are now "validated", as in they completely break if they do not follow the pattern.

Did not do that yet, means that we have to merge #2401793: Rename entity list routes to entity.$entity_type.list to make autogeneration possible. into this. This will then obviously fail horribly, but wanted to upload it as a starting point.

Berdir’s picture

Forgot the patch. not bothering with the interdiff, the only thing that I didn't do as part of the reroll was the options thing.

Status: Needs review » Needs work

The last submitted patch, 23: entity-list-link-template-2401505-23.patch, failed testing.

Berdir’s picture

You leave me no choice ;)

Merged #2401793: Rename entity list routes to entity.$entity_type.list to make autogeneration possible. into this, did my best to revert the search.settings configuration strings, and renamed to collection.

Status: Needs review » Needs work

The last submitted patch, 25: entity-list-link-template-2401505-25.patch, failed testing.

Berdir’s picture

Status: Needs work » Needs review
FileSize
188.32 KB
25.07 KB

Fixing those test fails. My regex search & replace didn't work on some strings, no idea why and looks like @larowlan missed the node_type route in the other issue :) Also a few other fixes.

Status: Needs review » Needs work

The last submitted patch, 27: entity-list-link-template-2401505-27.patch, failed testing.

dawehner’s picture

It looks pretty good! One small thing I noticed while reading the patch.

+++ b/core/modules/image/src/Plugin/Field/FieldFormatter/ImageFormatter.php
@@ -122,7 +122,7 @@ public function settingsForm(array $form, FormStateInterface $form_state) {
-        '#markup' => $this->linkGenerator->generate($this->t('Configure Image Styles', array('@url' => $this->urlGenerator->generateFromRoute('image.style_list'))), new Url('image.style_list')),
+        '#markup' => $this->linkGenerator->generate($this->t('Configure Image Styles', array('@url' => $this->urlGenerator->generateFromRoute('entity.image_style.collection'))), new Url('entity.image_style.collection')),

Huch, we don't use @url in the string. Not sure whether its in scope of this issue to fix it

Berdir’s picture

Status: Needs work » Needs review
FileSize
190.05 KB
3.37 KB

Fixed that test fail and removed the strange @url there, nice find. Also means we no longer need urlGenerator in there.

dawehner’s picture

Status: Needs review » Reviewed & tested by the community

Great work, and sorry for the extra work, which nevertheless would had to be done.

Berdir’s picture

I'll update the issue summary ASAP.

alexpott’s picture

Status: Reviewed & tested by the community » Needs work
Issue tags: +Needs issue summary update

This issue is a major task. To evaluate whether we should make this change during the beta, we need to update the issue summary according to https://www.drupal.org/core/beta-changes to explain what improvements it makes and what the disruption of the change would be. Can someone add Drupal 8 beta phase evaluation template to the issue summary.

Berdir’s picture

Issue summary: View changes
Status: Needs work » Reviewed & tested by the community
Issue tags: -Needs issue summary update

Updated the issue summary.

Not sure if and what we need in regards to change records.

Crossposted with @alexpott :) Back to you, your decision now.

Berdir’s picture

Issue summary: View changes

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 30: entity-list-link-template-2401505-30.patch, failed testing.

jibran’s picture

fago’s picture

The patch looks great in general, but I'm not sure on how the collection vs list wording is best.

So the entity system goes with "list builder" already, but the link template standard imposes "collection" wording on us. Right now, we take over the collection wording on the route names, which then uses the list builders for implementation.

I'm wondering, whether it would be better to have the link-template collection already implementing to entity.node_type.list instead of entity.node_type.collection? That way, Drupal internally would use list everywhere, it's just using collection where it's directly going with links (where collection is specified). That seems to be a bit more consistent to me, what do you think?

fago’s picture

As berdir pointed out on IRC, entity.node_type.collection is directly connected with the link relation name and cannot easily be separed. As having parity of route names with the link relations is a plus as well, one could say entity.node_type.list is really more consistent than entity.node_type.collection. Given that, I think it's ok to move on with it as it is.

alexpott’s picture

Status: Reviewed & tested by the community » Fixed

Given that we want to auto-generate routes I think we want to proceed with this change.

Committed 3d14a7d and pushed to 8.0.x. Thanks!

Thank you for adding the beta evaluation to the issue summary.

diff --git a/core/modules/migrate_drupal/src/Tests/d6/MigrateSearchConfigsTest.php b/core/modules/migrate_drupal/src/Tests/d6/MigrateSearchConfigsTest.php
index f87c013..e3c1c0d 100644
--- a/core/modules/migrate_drupal/src/Tests/d6/MigrateSearchConfigsTest.php
+++ b/core/modules/migrate_drupal/src/Tests/d6/MigrateSearchConfigsTest.php
@@ -13,7 +13,7 @@
 use Drupal\migrate_drupal\Tests\MigrateDrupalTestBase;
 
 /**
- * Upgrade variables to search.settings.yml.
+ * Upgrade variables to search.settings.
  *
  * @group migrate_drupal
  */
@@ -43,7 +43,7 @@ protected function setUp() {
   }
 
   /**
-   * Tests migration of search variables to search.settings.yml.
+   * Tests migration of search variables to search.settings.
    */
   public function testSearchSettings() {
     $config = $this->config('search.settings');

Reverted these changes as they are out-of-scope.

  • alexpott committed 3d14a7d on 8.0.x
    Issue #2401505 by Berdir, jibran, larowlan: Add an entity collection...
andypost’s picture

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.