Part of meta-issue #1788918: [GTD] [META] Prepare for Twig main engine core inclusion until December 1

Problem/Motivation

This patch is being developed in the CMI sandbox: config-list-1781372-tim branch

export ui.png
One of the big things that Views needs CTools for is the "Export UI":

Obviously we have to remove that dependency for Views to go into core, but more than that, this would be useful for all configuration entities, e.g., the vocabulary listing at admin/structure/taxonomy.

We've rewritten Views to use that already, and the above screenshot show the new code, not CTools :)

Proposed resolution

Introduce EntityListControllerInterface and a list controller class key for hook_entity_info().

Implementation

With the current patch (which removes non-essential functionality and defers it to the followup issues below), you can implement the list for a given my_entity entity type provided by my_module with the folllowing steps:

  1. Define $return['my_entity']['list controller class'] in the my_module_entity_info() hook implementation.

    The class must implement EntityListControllerInterface. For a theme_table() listing of entities similar to the Views screenshot at right or the current admin/structure/taxonomy page, use or extend EntityListController. For configuration entities, use or extend ConfigEntityListController.

  2. Create a page callback that calls entity_list_controller('my_entity') and renders the result.
  3. Define a path for the listing in the my_module_hook_menu().
  4. (optional) Define a MENU_LOCAL_ACTION menu item with a page callback that adds entities of the given type.

See #1783964: Allow entity types to provide menu items for followup discussion on how the path definitions for CRUD operations might be simplified.

Followup questions

  1. #20: The possible operations for an entity are actually bound to the entity, not the list [controller]. It would make much more sense to have that as a concept and method on the Entity class. The per-entity operations can be re-used in many other situations. See #1783964: Allow entity types to provide menu items.

  2. #20: We need to figure out how to dynamically enhance the output/operations for BundleConfigEntity objects; i.e., Configurables that represent bundles for another entity type (e.g., NodeType entities are the bundles for Node). These entity types typically have additional "manage fields" and "manage display" links in their operations. The 'fieldable' property in entity info cannot be leveraged, because that property is not set for the entity we're listing, but instead is a property of the "other" entity type that ours provides bundles for. Since it becomes more and more clear that we'll need a BundleConfigEntityInterface for these kind of entities, we could check the implemented interface though.

  3. The current config entity listing pages in HEAD only output the label of the Configurable, but not the machine name. The node/content type listing page is the only exception; it outputs @label <small>(Machine name: @id)</small> in the Label column. Exposing the machine name is definitely beneficial for users that are seeking for a particular one, but an entire (table) column looks and feels too dominant.

  4. The operations links (e.g. edit, delete, etc.) are related to each individual entity and defined in the list controller class for rendering, but in response to feedback (#16), the listing path itself and the path to add a new entity of the given type (which are related to the entity type rather than specific entities) are set separately in hook_menu(). As a result, the "Add" link is not displayed within the empty text of the listing table, because this text cannot be set through hook_menu(). #1783964: Allow entity types to provide menu items should provide a more complete solution. See #102.

Related issues

CommentFileSizeAuthor
#113 1781372-config-list-113.patch20.58 KBandypost
#113 1781372-interdiff-111vs113.txt4.04 KBandypost
#114 1781372-config-list-114.patch18.71 KBandypost
#114 1781372-config-list-111vs114.txt1.24 KBandypost
#111 config-1781372-111.patch18.75 KBxjm
#111 interdif-111.txt1.22 KBxjm
#108 config-1781372-108.patch18.89 KBxjm
#108 interdiff-108.txt4.96 KBxjm
#102 config-1781372-102.patch18.55 KBxjm
#102 interdiff-102.txt10.86 KBxjm
#102 list-controller-add-link.jpg34.06 KBxjm
#102 list-controller-no-add-link.jpg34.41 KBxjm
#92 1781372-91.patch15.77 KBdamiankloip
#92 interdiff.txt5.23 KBdamiankloip
#91 ListPageController.php_.test8.8 KBpounard
#85 listing-1781372-85-do-not-test.patch16.01 KBLars Toomre
#74 listing-1781372-74.patch12.66 KBtim.plunkett
#74 interdiff.txt897 bytestim.plunkett
#72 listing-1781372-72.patch12.82 KBtim.plunkett
#70 listing-1781372-69.patch12.76 KBtim.plunkett
#70 interdiff.txt5.02 KBtim.plunkett
#58 interdiff-58.txt8.42 KBxjm
#56 interdiff-1781372-42.txt688 bytesxjm
#47 config-1781372-47.patch12.68 KBxjm
#47 interdiff-47.txt8.75 KBxjm
#42 listing-1781372-42.patch12.01 KBJelle_S
#37 i1781372-37.png20.5 KBattiks
#37 i1781372-37-dpm.png49 KBattiks
#35 listing-1781372-35.patch12 KBtim.plunkett
#35 interdiff.txt3.9 KBtim.plunkett
#32 listing-1781372-32.patch12.63 KBtim.plunkett
#32 interdiff.txt7.36 KBtim.plunkett
#29 listing-1781372-29.patch11.94 KBtim.plunkett
#29 interdiff.txt1.41 KBtim.plunkett
#25 config.list_.23.patch11.59 KBsun
#23 config.list_.23.patch65.86 KBsun
#23 interdiff.txt2.29 KBsun
#22 config.list_.22.patch11.74 KBsun
#22 interdiff.txt6.15 KBsun
#20 config.list_.20.patch11.29 KBsun
#20 interdiff.txt22.05 KBsun
#11 drupal-1781372-11.patch13.63 KBtim.plunkett
#11 interdiff.txt3.05 KBtim.plunkett
#8 1781372-8.patch13.98 KBdamiankloip
#8 interdiff.txt4.51 KBdamiankloip
#6 listing-1781372-6.patch13.82 KBdamiankloip
#6 interdiff.txt4.94 KBdamiankloip
#5 interdiff.txt2.96 KBtim.plunkett
#5 listing-1781372-5.patch13.33 KBtim.plunkett
config-entity-list.patch13.44 KBtim.plunkett
export ui.png124.46 KBtim.plunkett
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

tim.plunkett’s picture

Tagging.

Status: Needs review » Needs work
Issue tags: -Configuration system, -VDC, -Configurables

The last submitted patch, config-entity-list.patch, failed testing.

tim.plunkett’s picture

Status: Needs work » Needs review
Issue tags: +Configuration system, +VDC, +Configurables
tim.plunkett’s picture

Issue tags: +CTools dependency

Last tag from me, I swear.

tim.plunkett’s picture

FileSize
13.33 KB
2.96 KB

c0febd0 Type-hint with EntityInterface instead of ConfigEntityInterface.
acc2db1 Remove redundant reloading of the controller.
9c22a9a Simplify EntityListControllerBase::__construct()

damiankloip’s picture

FileSize
4.94 KB
13.82 KB

A few doc tweaks/additions from todo's etc.. I also changed:

-    if (isset($type_info['list controller class'])) {
+    if (!empty($type_info['list controller class'])) {

Seems we might as well just use empty instead? Just incase someone wants to just set the key or something dumb. I don't think this is babysitting code too much.

Gábor Hojtsy’s picture

Works pretty well! I'm using this now in http://drupal.org/sandbox/goba/1782014 to work out a very simple module for D8 to support layouts with a list of common regions that layouts can share. The edit/delete/add operations are not yet implemented, need to figure out the best practices for entity form controllers now, but the list controller works flawlessly.

One UX note, IMHO we should default to exposing the 'id' as 'Machine name' and the 'Actions' as 'Operations' in the table header. This is the terminology used by Views and most of the rest of Drupal as well :) It is pretty odd that the proposed patch uses ID and Actions as user facing table headers.

damiankloip’s picture

FileSize
4.51 KB
13.98 KB

OK, that seems like a good point! Glad to hear it is working for you already.

Status: Needs review » Needs work

The last submitted patch, 1781372-8.patch, failed testing.

Gábor Hojtsy’s picture

That is the sanest default, and it let me loose some custom code :) http://drupalcode.org/sandbox/goba/1782014.git/commitdiff/8e41b7cf6aa36a...

tim.plunkett’s picture

Status: Needs work » Needs review
FileSize
3.05 KB
13.63 KB

Status: Needs review » Needs work

The last submitted patch, drupal-1781372-11.patch, failed testing.

catch’s picture

If #1782244: Convert image styles $style array into ImageStyle (extends ConfigEntity) or a similar patch lands before this, it'd be good to do one conversion of a real ConfigEntity object here. If not then I guess those issues should also be converting the listings? I assume we want to use this for the listings of all ConfigEntity objects (thanks xjm for the least offensive pluralization so far) in core and save about 12 custom listings that way which'd be lovely. Haven't reviewed the patch here yet.

Gábor Hojtsy’s picture

(I also use this now for the Drupal 8 port of http://drupal.org/project/gridbuilder :).

Dave Reid’s picture

I do not like from a DX aspect that we are defining hook_menu() items in the controller. Menu callbacks are tied to modules, so why are we creating more work and harder to find items by defining them there?

Dave Reid’s picture

As an owner of the module I would find doing this MUCH MUCH easier than having to manage hook_menu() in a plugin. I can easily see what menu path I define, makes it easy to extend it with local tasks and actions since they're all in the same place.

function config_test_menu() {
  $items['config-listing-test'] = array(
    'title' => 'Config test',
    'description' => 'Config test listing page.',
    'page callback' => 'config_entity_listing_page',
    'page arguments' => array(config_get_list_controller('config_test')),
    'access arguments' => array('administer config test'),
  );

  return $items;
}

I also pointed out in IRC that if we go with a hook_menu() method then likely we need to patch whatever localize.drupal.org uses to scan for translations as having menu callbacks not in hook_menu() will mean that the strings will be untranslatable until we fix it.

Jose Reyero’s picture

Sorry to interrupt, but is it too late for doing lists of configurable things that are not entities?
See issues with Config Entities at the end of this thread, #1754246: Languages should be configuration entities

Gábor Hojtsy’s picture

On hook_menu(), based on the two modules I ported today to Drupal 8, indeed the add/edit and delete items would still be in hook_menu because entity form controllers so far have procedural mappers to retrieve and use the proper entity form controller + the deletion forms are not even handled in the entity form controllers, so what you get is:

defined in hook_menu:
- add item: function that uses the entity form controller
- edit item: function that uses the entity form controller
- delete item: drupal_get_form with regular confirm form

defined via hookMenu in the list controller:
- list items: assembled based on data from the methods

Depending on where hook menu is heading I don't necessarily agree that moving stuff out of there is bad. Based on the extent of changes planned, localize.drupal.org will need to adapt either way I think.

My sandbox where I have the list controller and form controller and delete confirms combined for a simple config entity with a machine name and label is at http://drupalcode.org/sandbox/goba/1782014.git/tree - hopefully all using current D8 best practices.

tim.plunkett’s picture

Status: Needs work » Needs review

The controller needs to know the path, which is why it's currently in hook_entity_info.

The listing path should be added to hook_menu by the config module, as it currently is. And therefore, the implementing class needs to override the title, callbacks, etc. It would be a huge WTF if you specify a path and a controller and there is no menu item added.

If we want to somehow restrict the ability to add further items in the controller we could, but it will limit functionality.

Based on the extent of changes planned, localize.drupal.org will need to adapt either way I think.

If that's true, I think we can forge on with this approach.

Also, this is CNR, that was a random failure.

sun’s picture

Title: Add an API for listing configuration entities » Add an API for listing (configuration) entities
Issue tags: +Entity system
FileSize
22.05 KB
11.29 KB

I completely agree with @Dave Reid above. However, I also see that the current situation with regard to proper encapsulation and separation between router definitions and entity controllers is very unclear. E.g., the EntityFormController somehow manages to get around the problem (similar situation with having to specify $form_state['redirect'] there), though I didn't have time to study how exactly it does that yet. Anyway, let's create a separate issue to discuss that.

Aside from that hook_menu() binding, the current code also adds a hard dependency on config.module for the entity list controller abstraction, which we should avoid...[snip]

Alright, I reviewed this and found too many issues, so I created a new config-list-1781372-sun branch in the CMI sandbox based on the latest patch here. Everyone has git access to that sandbox already.

Fixed minor code style issues.
Moved EntityListController* into entity system.
Fixed ConfigEntityListTest does not verify exact ConfigTest entity type.
Removed ::hookMenu() and moved list controller factory into entity.inc.
Removed all Ajax functionality. (Later, guys. :))
Fixed bogus 'add' link in operations.
Removed getList() method, as it duplicates EntityStorageControllerInterface::load().
Re-ordered header/row/operations builder methods.
Renamed methods into buildHeader(), buildRow(), buildOperations(), and render().
Restored getList() as load() method.
Fixed getOperations() does not respect/use Entity::uri().
Fixed entity label should appear first in lists.
Improved maintainability of render array definitions.

Still many open issues with this:

- First and foremost, the possible operations for an entity are actually bound to the entity, not the list [controller]. I think it would make much more sense to have that as a concept and method on the Entity class. The per-entity operations can be re-used in many other situations.

- I dislike how the buildHeader() and buildRow() methods are enforcing the concept and notion of a table output onto the list controller. I think this should be simplified; e.g., by merging buildHeader() into render(), and possibly renaming buildRow() to something else. In turn, there'd be no notion of a table baked in.

- We likely want to re-introduce a ConfigEntityListController base class specialized for ConfigEntity objects. (A first common operation for them is the ConfigTestListController::load() override.)

- We need to figure out how to dynamically enhance the output/operations for BundleConfigEntity objects; i.e., Configurables that represent bundles for another entity type (e.g., NodeType entities are the bundles for Node). These entity types typically have additional "manage fields" and "manage display" links in their operations. I was playing with the idea of leveraging the 'fieldable' property in entity info, but alas, that property is not set for the entity we're listing, but instead a property of the "other" entity type that ours provides bundles for. Since it becomes more and more clear that we'll need a BundleConfigEntityInterface for these kind of entities, we could check the implemented interface though.

The interdiff is not much of a help, but attaching it anyway.

Status: Needs review » Needs work

The last submitted patch, config.list_.20.patch, failed testing.

sun’s picture

Status: Needs work » Needs review
FileSize
6.15 KB
11.74 KB

Added weights to operations to allow them to be altered/enhanced.
Fixed missing empty data handling.
Fixed ConfigEntityListTest failures.
Replaced ConfigTest entity specific list controller with generic ConfigEntityListController.

sun’s picture

FileSize
2.29 KB
65.86 KB

Leverage native render arrays for operations instead of theme_link() specific data. (this gives built-in handling for stuff like #weight, #access, #ajax, etc for each operation)

Also, @tim.plunkett moved the original branch into CMI as config-list-1781372-tim, and I just rebased mine onto that (replacing my remote branch).

Status: Needs review » Needs work

The last submitted patch, config.list_.23.patch, failed testing.

sun’s picture

Status: Needs work » Needs review
FileSize
11.59 KB

d'oh, sorry, the git diff for the patch in #23 went completely wrong... ;)

tim.plunkett’s picture

There were a boatload of changes between #11 and #25, which completely break the Views implementations, as well as @Gábor Hojtsy's.

I'm going through the changes and updating the Views implementation, commit by commit, to see if everything still works, and to look for things to push back on.

If this weren't assigned to me, I'd assign it to myself now :)

sun’s picture

Issue summary: View changes

Updated issue summary.

sun’s picture

Issue summary: View changes

Updated issue summary.

sun’s picture

I started a list of architectural discussion topics in the issue summary (essentially some items from #20), so we can keep track of them.

Also, @tim.plunkett really wants to have a discussion on the idea of hookMenu() methods in entity controllers, and I agree we should discuss that. However, if we start to introduce that, then we should do so on all entity controller levels; i.e., including the entity forms + edit page (EntityFormController), the primary entity view page (soon EntityRenderController), and with this also the list page (EntityListController). We definitely should not hold up this issue on that; that's why I deliberately removed those methods. Let's make sure to create an issue for that. (Leaving to @tim.plunkett, since he appears to be very passionate about the topic anyway already ;))

tim.plunkett’s picture

I've added #1783964: Allow entity types to provide menu items for the follow-up about menu items.
----

I've gone through the changes sun made in his patch, and I was able to adapt the Views implementation to all of them except one:
http://drupalcode.org/sandbox/heyrocker/1145636.git/commitdiff/c0bf36d

This changes the operations links from a render array prepared for theme_links(), to just an array render arrays for each operation, with '#type' => 'link'.
We're currently using theme_links__ctools_dropbutton(), and are working towards #1608878: Add CTools dropbutton to core.

I've brought config-list-1781372-tim all the way up to one commit behind sun's.

---

Of the 4 items added to the issue summary, I agree that #1 and #2 should be done, and I'm going to work on those.
I think #3 is a follow-up issue, possibly also blocked on http://drupal.org/node/1782460.
#4 is fine, I think it was only added for now as an example.

tim.plunkett’s picture

FileSize
1.41 KB
11.94 KB

Here's a new patch, also with
5b2f210 Use entity_get_info() to provide a default controller instead of hardcoding it in entity_list_controller().

webchick’s picture

#29: listing-1781372-29.patch queued for re-testing.

sun’s picture

To make EntityListController a default implementation, we need to:

1) Move the ::getOperations() method into Entity (so nothing is left to override in EntityListControllers by default).

2) Remove the Base suffix.

Regarding the left-out commit mentioned in #29, it's most probably best to detach the operations entirely from any kind of render element type. I think the reasoning provided for the commit makes sense though:

- we need to determine user access for each operation, and

- each operation should also have a weight assigned to it, so other modules can inject further operations in between others (e.g., Field UI adding manage fields/display before delete), etc.

tim.plunkett’s picture

FileSize
7.36 KB
12.63 KB

ebbcba6 Remove the obsolete part of config_test_list_page().
1a9f20d Make EntityListControllerBase a default implementation instead of an abstract class.
c66a096 Move getOperations() from the list controller to the entity itself.

Status: Needs review » Needs work

The last submitted patch, listing-1781372-32.patch, failed testing.

fago’s picture

- each operation should also have a weight assigned to it, so other modules can inject further operations in between others (e.g., Field UI adding manage fields/display before delete), etc.

Yes, having modular extensibility for operations would be a big plus.

I'm not so sure about adding the operations to the entity class though. It should be possible to use the controller to provide listings at various different places in the admin UI, but having operations defined in the entity class makes them go back to the "main UI" always. As of this would be problematic as breadcrumbs and various context is derived from the menu system (not sure whether that's supposed to change?). But also, it makes access checking and keeping control easier if everything goes through *your* menu items.

1) Move the ::getOperations() method into Entity (so nothing is left to override in EntityListControllers by default).

Why not just leave them in the ListController? The implementation could still default to the same operations, so it still could be as used as is.

tim.plunkett’s picture

Status: Needs work » Needs review
FileSize
3.9 KB
12 KB

This moves the getOperations back to the list controller, this time to the new generic implementation.

The operations DO have weights assigned, and that works as is right now.

I also removed the hardcoded table ID.

tim.plunkett’s picture

The tests still currently belong to config.module, but I'm not going to move them now unless #1785974: Move ConfigEntity into a Core component lands first.

Assuming #35 passes tests, I think it adequately addresses @sun and @fago's concerns, and should be ready.

attiks’s picture

FileSize
49 KB
20.5 KB

Great work all, I tried this, but I guess I'm missing something

Patch applies cleanly and works, I get a list of my config objects with an edit and a delete link, yeah!

i1781372-37.png

I'm using the following code (which might be wrong):

function breakpoints_entity_info() {
  // Breakpoint
  $types['breakpoint'] = array(
    'label' => 'Breakpoint',
    'entity class' => 'Drupal\breakpoints\Breakpoint',
    'controller class' => 'Drupal\config\ConfigStorageController',
    'form controller class' => array(
      'default' => 'Drupal\breakpoints\BreakpointFormController',
    ),
    // 'list controller class' => 'Drupal\breakpoints\BreakpointListController',
    'list controller class' => 'Drupal\config\ConfigEntityListController',
    'list path' => 'admin/config/media/breakpoints/breakpoint',
    'uri callback' => 'breakpoints_breakpoint_uri',
    'config prefix' => 'breakpoints',
    'entity keys' => array(
      'id' => 'id',
      'label' => 'label',
      'uuid' => 'uuid',
    ),
  );
  return $types;
}

function breakpoints_menu() {
  $items = array();

  $items['admin/config/media/breakpoints/breakpoint'] = array(
    'title' => 'Breakpoints',
    'page callback' => 'breakpoints_breakpoint_page',
    'description' => 'Manage breakpoints',
    'access arguments' => array('administer breakpoints'),
    'weight' => 10,
    'file' => 'breakpoints.breakpoint.admin.inc',
  );
  $items['admin/config/media/breakpoints/breakpoint/add'] = array(
    'title' => 'Add breakpoint',
    'page callback' => 'breakpoints_breakpoint_page_add',
    'access callback' => 'user_access',
    'access arguments' => array('administer breakpoints'),
    'type' => MENU_LOCAL_ACTION,
    'file' => 'breakpoints.breakpoint.admin.inc',
  );
  $items['admin/config/media/breakpoints/breakpoint/%breakpoints_breakpoint'] = array(
    'title' => 'Edit breakpoint',
    'page callback' => 'breakpoints_breakpoint_page_edit',
    'page arguments' => array(5),
    'access callback' => 'user_access',
    'access arguments' => array('administer breakpoints'),
    'type' => MENU_CALLBACK,
    'file' => 'breakpoints.breakpoint.admin.inc',
  );
  $items['admin/config/media/breakpoints/breakpoint/%breakpoints_breakpoint/edit'] = array(
    'title' => 'Edit breakpoint',
    'page callback' => 'breakpoints_breakpoint_page_edit',
    'page arguments' => array(5),
    'access callback' => 'user_access',
    'access arguments' => array('administer breakpoints'),
    'type' => MENU_CALLBACK,
    'file' => 'breakpoints.breakpoint.admin.inc',
  );
  $items['admin/config/media/breakpoints/breakpoint/%breakpoints_breakpoint/delete'] = array(
    'title' => 'Delete',
    'page callback' => 'drupal_get_form',
    'page arguments' => array('breakpoints_breakpoint_delete_confirm', 5),
    'access callback' => 'user_access',
    'access arguments' => array('administer breakpoints'),
    'type' => MENU_CALLBACK,
    'file' => 'breakpoints.breakpoint.admin.inc',
  );
  return $items;
}

The problem is that breakpoints_breakpoint_load receives a mix of arguments, but I have no idea why, so I had to change the load function to the following, to suppress all warnings, but if anyone know how to fix it, great!

function breakpoints_breakpoint_load($id) {
if (is_object($id)) {
return $id;
}
if (is_array($id)) {
return $id;
}
return entity_load('breakpoint', $id);
}

i1781372-37-dpm.png

tim.plunkett’s picture

@attiks, you don't specify any page arguments for half of those menu items. That's your problem. See #1783964: Allow entity types to provide menu items for the follow-up

tim.plunkett’s picture

Issue summary: View changes

.

attiks’s picture

@tim thanks, I'll have a look.

Jelle_S’s picture

Status: Needs review » Needs work
+++ b/core/lib/Drupal/Core/Entity/EntityListController.phpundefined
@@ -0,0 +1,134 @@
+  /**
+   * Implements Drupal\Core\Entity\EntityListControllerInterface::render();
+   */
+  public function render() {
+    $build = array(
+      '#theme' => 'table',
+      '#header' => $this->buildHeader(),
+      '#rows' => array(),
+      '#empty' => t('There is no @label yet. <a href="@add-url">Add one</a>.', array(
+        '@label' => $this->entityInfo['label'],
+        '@add-url' => $this->getPath() . '/add',
+      )),
+    );
+    foreach ($this->load() as $entity) {
+      $build['#rows'][$entity->id()] = $this->buildRow($entity);
+    }
+    return $build;
+ }

Should be changed to

/**
 * Implements Drupal\Core\Entity\EntityListControllerInterface::render();
 */
public function render() {
  $build = array(
    '#theme' => 'table',
    '#header' => $this->buildHeader(),
    '#rows' => array(),
    '#empty' => t('There is no @label yet. !add-link.', array(
      '@label' => $this->entityInfo['label'],
      '!add-link' => l(t('Add one'), $this->getPath() . '/add'),
    )),
  );
  foreach ($this->load() as $entity) {
    $build['#rows'][$entity->id()] = $this->buildRow($entity);
  }
  return $build;
}

because right now the link for the empty table isn't right. For example, with our breakpoints module getPath() returns admin/config/media/breakpoints/breakpoint. With the current code, the link turns in to admin/config/media/breakpoints/breakpoint/admin/config/media/breakpoints/breakpoint/add. Using the l() function solves this issue and returns admin/config/media/breakpoints/breakpoint/add as expected.

Gábor Hojtsy’s picture

Wrapping $this->getPath() . '/add', in a url() is in fact better for translatability, vs. moving the whole link out to an l(), because it keeps the text with the 'Add one' properly together.

Jelle_S’s picture

Status: Needs work » Needs review
FileSize
12.01 KB

This should do it then.

Jelle_S’s picture

Issue summary: View changes

update issue summary

Lars Toomre’s picture

When this is next re-rolled, the following documentation issues hopefully can be addressed too.

+++ b/core/includes/entity.incundefined
@@ -530,3 +531,33 @@ function entity_form_submit_build_entity($entity_type, $entity, $form, &$form_st
+ *
+ * @see hook_entity_info()

@see directives should go after @return directive in this docblock.

+++ b/core/lib/Drupal/Core/Entity/EntityListController.phpundefined
@@ -0,0 +1,134 @@
+
+  public function __construct($entity_type) {

Are we missing a docblock for this constructor? I think so since we need a @param documenting what $entity_type is.

+++ b/core/lib/Drupal/Core/Entity/EntityListController.phpundefined
@@ -0,0 +1,134 @@
+  }
+
+  public function getPath() {
+    return $this->entityInfo['list path'];
+  }

Need a complete docblock for this method.

+++ b/core/lib/Drupal/Core/Entity/EntityListControllerInterface.phpundefined
@@ -0,0 +1,75 @@
+
+  /**
+   * Renders a list of operation links.
+   *
+   * @return array
+   *   A renderable array of operation links.
+   */
+  public function buildOperations(EntityInterface $entity);

Missing @param directive here.

Gábor Hojtsy’s picture

Updated regions module for the latest patch at http://drupalcode.org/project/region.git/commitdiff/6e83304b660453820213... - let me loose the list controller which is great IMHO, although I needed to add the URI callback which might be useful for other things.

xjm’s picture

Assigned: tim.plunkett » xjm

Exciting!

@timplunkett asked me to take a close look at the documentation in the patch. Incoming!

Architectural and functional questions

  1. +++ b/core/modules/config/lib/Drupal/config/ConfigEntityListController.phpundefined
    @@ -0,0 +1,27 @@
    +  /**
    +   * Overrides Drupal\Core\Entity\EntityListController::load().
    +   */
    +  public function load() {
    +    $entities = parent::load();
    +    uasort($entities, 'Drupal\config\ConfigEntityBase::sort');
    +    return $entities;
    +  }
    

    Interesting. This seems kind of odd to me and I had to read these twice before it made sense to me because the relationship between EntityListController::load() and ConfigEntity isn't really explicitly here in this class.

    Is there a reason that the list controller interface and/or base class doesn't have its own sort() method? (I'll look through the issue to see if this was discussed already).

    At the least, I'd add another paragraph to the docblock, something like: "ConfigEntity objects are sorted for listing according to their own sorting method" or something. But it seems like this will need to be overridden or copypastaed constantly.

  2. How tightly coupled is the listing interface to the Render not-an-API? It includes a lot of methods ostensibly for building render arrays for an HTML table. We should do a better job either on the interface or on the base class of clarifying the fact that there's a group of these methods that are specifically tied to building the render array. I don't know in which place, and I don't know if we want more explicit names or just clearer docs or what. (Bunch more specific points about the docs below.)
  3. I guess I share some of @Dave Reid's apprehension about the path/routing/menu item stuff, but it's mostly out of ignorance (and possibly incomplete docs). Though I see that there's #1783964: Allow entity types to provide menu items as a followup.

Test coverage

  1. +++ b/core/modules/config/lib/Drupal/config/Tests/ConfigEntityListTest.phpundefined
    @@ -0,0 +1,66 @@
    +    // Verify that the default ConfigTest configuration appears.
    +    $this->assertText('default');
    +    $this->assertText('Default');
    

    Okay, this is kind of weird. Why are we asserting the text appears in both lower and sentence case? I have no idea from the code. At the least, a more specific inline comment might be good, though maybe an xpath assertion would be better. What elements are we actually checking for here?

  2. +++ b/core/modules/config/lib/Drupal/config/Tests/ConfigEntityListTest.phpundefined
    @@ -0,0 +1,66 @@
    +    // Verify that operation links appear.
    +    foreach (array('Edit', 'Delete') as $link) {
    +      $this->drupalSetContent($page);
    +      $this->assertLink($link);
    +      $this->clickLink($link);
    +      $this->assertResponse(200);
    

    I guess we're verifying that the operation links both appear and are functional. Are there separate tests for what's actually on the edit and delete pages?

Incomplete or confusing docs

  1. +++ b/core/includes/entity.incundefined
    @@ -530,3 +531,33 @@ function entity_form_submit_build_entity($entity_type, $entity, $form, &$form_st
    + * Since there might be different scenarios in which an entity is edited,
    + * multiple form controllers suitable to the different operations may be defined.
    + * If no controller is found for the default operation, the base class will be
    + * used. If a non-existing non-default operation is specified an exception will
    + * be thrown.
    

    I kinda don't understand this at all from the docs. Where does this exception get thrown and what kind of exception? I didn't see any @throws anywhere in the patch. And what's the "default operation"? Meow? Maybe some @see would help? Not sure. :)

    (Also the wrapping is a little off; one line is 81 chars.)

  2. +++ b/core/lib/Drupal/Core/Entity/EntityListControllerInterface.phpundefined
    @@ -0,0 +1,75 @@
    +   * Loads entities of this type.
    

    This docblock seems kinda weird because I'd think that EntitiesOfThisType::load() does that. It's loading them for listing them, right? And that's different from their native load() because why? (This also ties in to my earlier question about sorting.)

  3. +++ b/core/lib/Drupal/Core/Entity/EntityListController.phpundefined
    @@ -0,0 +1,134 @@
    +  public function __construct($entity_type) {
    +    $this->entityType = $entity_type;
    +    $this->storage = entity_get_controller($this->entityType);
    +    $this->entityInfo = entity_get_info($this->entityType);
    +  }
    

    Need a docblock for the constructor here.

  4. +++ b/core/lib/Drupal/Core/Entity/EntityListController.phpundefined
    @@ -0,0 +1,134 @@
    +  public function getPath() {
    +    return $this->entityInfo['list path'];
    +  }
    

    This guy needs a docblock, most especially since it doesn't seem to be on the interface?

  5. So, regarding the render array for the list table: I couldn't understand how to use these methods from reading the interface. Again, I'm not sure if it's inappropriate to specifically mention render array stuff on the interface or not, but if it isn't appropriate then these methods seem kinda not generic enough and maybe some belong on the base class. And if it is appropriate to specify that it's render array stuff, let's do so more explicitly:
    • +++ b/core/lib/Drupal/Core/Entity/EntityListControllerInterface.phpundefined
      @@ -0,0 +1,75 @@
      +   * @return array
      +   *   An array of operation link data to use in buildOperations.
      

      This should also document the expected array structure. Edit: Looks like it's a sorted renderable array of #links in the base class implementation.

    • +++ b/core/lib/Drupal/Core/Entity/EntityListControllerInterface.phpundefined
      @@ -0,0 +1,75 @@
      +   * Builds the header row.
      +   *
      +   * @return array
      +   *   An array of header strings.
      

      The header row for what? What's a header string? (Looking at the implementation on the base class, it's actually an array of table header cell labels, keyed by column ID, used for the #header in a render array for an HTML table. Right?)

    • +++ b/core/lib/Drupal/Core/Entity/EntityListControllerInterface.phpundefined
      @@ -0,0 +1,75 @@
      +   * Builds an array of data for each row.
      

      An array of what data? Each row of what?

    • +++ b/core/lib/Drupal/Core/Entity/EntityListControllerInterface.phpundefined
      @@ -0,0 +1,75 @@
      +   * @param Drupal\Core\Entity\EntityInterface $entity
      +   *
      ...
      +  public function buildRow(EntityInterface $entity);
      ...
      +  public function buildOperations(EntityInterface $entity);
      

      These methods both need complete parameter documentation so that the API module can make sense of them.

    • +++ b/core/lib/Drupal/Core/Entity/EntityListControllerInterface.phpundefined
      @@ -0,0 +1,75 @@
      +   * @return array
      +   *   An array of fields to use for this entity.
      

      What kind of fields? To use for this entity how?

  6. +++ b/core/lib/Drupal/Core/Entity/EntityListControllerInterface.phpundefined
    @@ -0,0 +1,75 @@
    +   * Gets the entity storage controller.
    +   *
    +   * @var Drupal\Core\Entity\EntityStorageControllerInterface
    +   */
    +  public function getStorageController();
    

    Eh? This isn't a member variable, it's a method. So I think this docblock is wrong; it wants an @param sorry @return. :)

  7. +++ b/core/modules/config/lib/Drupal/config/Tests/ConfigEntityListTest.phpundefined
    @@ -0,0 +1,66 @@
    +   * Tests entity list controller functionality.
    

    Maybe something less vague here? Looks to me like it tests that the entity list controller loads the correct list of entities.

  8. +++ b/core/modules/config/lib/Drupal/config/Tests/ConfigEntityListTest.phpundefined
    @@ -0,0 +1,66 @@
    +    // Get a list of ConfigTest entities.
    

    "...and confirm that it contains the expected results," maybe?

Style nitpicks

  1. +++ b/core/lib/Drupal/Core/Entity/EntityListController.phpundefined
    @@ -0,0 +1,134 @@
    +  /**
    +   * The entity info.
    +   *
    +   * @var array
    +   */
    +  protected $entityInfo;
    

    I think we discussed this before in another patch, but let's be a little more explicit here about what the entity info is ("as returned by entity_get_info()" or whatever, and/or with an @see).

  2. +++ b/core/modules/config/lib/Drupal/config/Tests/ConfigEntityListTest.phpundefined
    @@ -0,0 +1,66 @@
    + * Tests listing of configuration entities.
    ...
    +      'description' => 'Tests listing of configuration entities.',
    

    "Tests the listing..."

  3. +++ b/core/modules/config/lib/Drupal/config/ConfigEntityListController.phpundefined
    @@ -0,0 +1,27 @@
    + * Default list controller for ConfigEntity objects.
    

    This is technnically supposed to start with a third-person verb; we can say "Defines the default list controller for ConfigEntity objects."

  4. +++ b/core/lib/Drupal/Core/Entity/EntityListController.phpundefined
    @@ -0,0 +1,134 @@
    +   * Implements Drupal\Core\Entity\EntityListControllerInterface::getStorageController();
    ...
    +   * Implements Drupal\Core\Entity\EntityListControllerInterface::load();
    ...
    +   * Implements Drupal\Core\Entity\EntityListControllerInterface::buildHeader();
    ...
    +   * Implements Drupal\Core\Entity\EntityListControllerInterface::buildRow();
    ...
    +   * Implements Drupal\Core\Entity\EntityListControllerInterface::buildOperations();
    ...
    +   * Implements Drupal\Core\Entity\EntityListControllerInterface::render();
    

    I do this all the time myself, but these are English sentences rather than code "sentences" and so should end in periods rather than semicolons. ;)

  5. +++ b/core/includes/entity.incundefined
    @@ -530,3 +531,33 @@ function entity_form_submit_build_entity($entity_type, $entity, $form, &$form_st
    + * @see hook_entity_info()
    

    Like Lars said, this should go down at the bottom of the docblock.

I need to go home before it gets dark, but I'll fix up some of these things once I get home and then talk to @timplunkett about the rest once I've read over the whole issue.

xjm’s picture

+++ b/core/includes/entity.incundefined
@@ -530,3 +531,33 @@ function entity_form_submit_build_entity($entity_type, $entity, $form, &$form_st
+/**
+ * Returns an entity list controller for a given entity type.
+ *
+ * Since there might be different scenarios in which an entity is edited,
+ * multiple form controllers suitable to the different operations may be defined.
+ * If no controller is found for the default operation, the base class will be
+ * used. If a non-existing non-default operation is specified an exception will
+ * be thrown.
+ *
+ * @see hook_entity_info()
+ *
+ * @param string $entity_type
+ *   The type of the entity.
+ *
+ * @return Drupal\Core\Entity\EntityListControllerInterface
+ *   An entity list controller instance.

Ahh, I figured out what is going on here when I opened up the file. This was copied from the docblock for entity_form_controller(), which is why it makes absolutely no sense. :)

xjm’s picture

Assigned: xjm » tim.plunkett
FileSize
8.75 KB
12.68 KB

Cleaned up what I could and handing back to @tim.plunkett. I addressed all the style and docs questions I had in #45 except for many of the points about the various render methods on the interface. I think everything in there could be documented more clearly, but I see #1788918: [GTD] [META] Prepare for Twig main engine core inclusion until December 1 linked in the summary here so I'm going to wait for someone who understands the render process better than I do to tell me if the interface is coupled to the Render not-an-API. :)

My questions above under "Architectural and functional questions" and "Test coverage" are still open.

xjm’s picture

+++ b/core/lib/Drupal/Core/Entity/EntityListController.phpundefined
@@ -73,12 +81,21 @@ class EntityListController implements EntityListControllerInterface {
+   * @todo What is this method for, other than fetching the list path? Is this
+   *  for http://drupal.org/node/1783964 ? Should it be on the interface?
+   */
   public function getPath() {

Oh, forgot to point this out in my previous comment. This is also an open question. :)

xjm’s picture

+++ b/core/modules/config/lib/Drupal/config/Tests/ConfigEntityListTest.phpundefined
@@ -25,18 +25,20 @@ class ConfigEntityListTest extends WebTestBase {
-   * Tests entity list controller functionality.
+   * Tests that entity list controller loads a valid list of entities.

Oops, typo to fix here. "Tests that the entity list controller..."

pounard’s picture

Each time I see procedural code into a class my eyes bleed:

+  public function __construct($entity_type) {
+    $this->entityType = $entity_type;
+    $this->storage = entity_get_controller($this->entityType);
+    $this->entityInfo = entity_get_info($this->entityType);
+  }

Could be replaced by something such as:

+  public function __construct(EntityStorageControllerInterface $storage) {
+    $this->storage = $storage;
+  }

Which would untangle almost completly this code of global state.
Even better it could be done in a setter instead, thus allowing to create list instances without the storage (for usage with the prototype pattern or if you fetch a new instance from a factory).

I think that operations (where 'edit' and 'delete' are hardcoded) could be derived from the storage or whatever else controller, or could be just derived from menu entries (I just saw another path that proposes to declare menu entries within the controller => then we can automatically derivate the operations and compute the access rights in live).

I think the instance doesn't need to carry the entity info, only the getPath() method uses it, it would be better if the path could be looked up from another source (storage controller?) instead, thus we wouldn't need to copy those huge array in here (I now that PHP does copy on write, but that's not a good reason to keep those array copies everywhere!).

catch’s picture

I think #1788918: [GTD] [META] Prepare for Twig main engine core inclusion until December 1 is only linked here because I pointed out this issue should mean several custom theme functions get removed - i.e. if/when vocabularies get converted to ConfigEntity objects the custom theme function for vocabulary admin goes bye bye when you combine that issue with this one.

fago’s picture

+++ b/core/lib/Drupal/Core/Entity/EntityListControllerInterface.php
@@ -0,0 +1,81 @@
+  /**
+   * Loads entities of this type from storage for listing.
+   *
+   * @return array
+   *   An array of entities implementing Drupal\Core\Entity\EntityInterface.
+   */
+  public function load();
+

I don't think load() belongs into the interface.
If we want our implementation to wrap load() into another method, that's fine. But for outside use, load() is something that would be called on the storage controller.

tim.plunkett’s picture

#50 would be fine if ConfigStorageController::$entityType and ConfigStorageController::$entityInfo were public or had getters, but they don't. We could change that, of course.

#52, EntityListController::load() is just return $this->storage->load();, however it gives the entity an opportunity to sort the entities in a specific way.
Otherwise, every place the entities were loaded, they'd need to be explicitly sorted.

pounard’s picture

@#53 It would greatly reinforce object responsabilities and encapsulation if we could fix that IMHO. That's not a blocker, but if we can do this while the patch is being discussed it would be time saved.

xjm’s picture

#52, EntityListController::load() is just return $this->storage->load();, however it gives the entity an opportunity to sort the entities in a specific way.
Otherwise, every place the entities were loaded, they'd need to be explicitly sorted.

This ties back into my question about sorting above in #45.

xjm’s picture

Issue summary: View changes

Updated issue summary.

xjm’s picture

FileSize
688 bytes

Adding my changes above to the sandbox. For convenience/clarity, here's an interdiff for @Jelle_S's patch in #42.

xjm’s picture

Issue tags: +Needs tests

We should also add test coverage for the link paths. And lots of other test coverage too. :)

xjm’s picture

FileSize
8.42 KB

So for some reason I can't push to the sandbox right now, but here's an interdiff that should apply on top of the branch tip + #56.

tim.plunkett’s picture

I've updated the branch with the last couple patches.

There is still other parts of xjm's review I haven't addressed, hopefully I'll have time tonight.

fago’s picture

Status: Needs review » Needs work

#52, EntityListController::load() is just return $this->storage->load();, however it gives the entity an opportunity to sort the entities in a specific way.
Otherwise, every place the entities were loaded, they'd need to be explicitly sorted.

Yep, that's fine. However that's missing in the functions docs. Anyway, I questioned not the existence of the method but whether it should be part of the interface.

+++ b/core/modules/config/tests/config_test/config_test.module
@@ -176,36 +178,8 @@ function config_test_delete($id) {
+  $controller = entity_list_controller('config_test');
+  return $controller->render();

It looks to me that everything that really needs to be in the interface would be __construct() and render()? There is a lot of implementation specific stuff in there though - why do we need this in the interface?

+++ b/core/lib/Drupal/Core/Entity/EntityListControllerInterface.php
@@ -0,0 +1,81 @@
+   * @return array
+   *   A array of operation link data to use in
+   *   EntityListControllerInterface::buildOperations().
+   */
+  public function getOperations(EntityInterface $entity);

E.g. this here does not sound like it's actually built for outside use?

Although - I'd see getOperations (and buildOperations()) being useful so one can fetch specific operation paths from the outside. But then it needs proper docs of the return array.

andypost’s picture

Working on with #1552396: Convert vocabularies into configuration I get troubles with getting a list of vocabularies because of sorting issue - config entities are always sorted by the name

xjm’s picture

@andypost -- For vocabularies, we'd want to override the sorting to be based on vocab weight, I think. Vocab will want to either override ConfigEntityBase::sort() and/or the sort method on ConfigEntityListController once we convert it. (@tim.plunkett and I just talked about replacing the load() method with a sort() method.)

Also, agreed on all @fago's points in #60.

sun’s picture

ConfigEntityBase::sort() already sorts by weight (if there is a weight property) first and label second (natural language). It's a very smart sorting algorithm that I originally invented for admin_menu. It should work for 99% of all config entities, hence located on ConfigEntityBase.

I guess the true topic of the discussion on ::load() is the question of separation of concerns. If we'd strictly separate stuff, then we'd actually have to inject the list of loaded entities into the list controller, instead of loading it from within. But then again, the original intention of ::load() [at least the way I see it] was to possibly enhance it later on with pager functionality or the like. (OTOH, that reminds of EFQ...) /me shrugs

andypost’s picture

Sorting throws warning uasort(): Array was modified by the user comparison function so just prefixed with @ - as suggested in https://bugs.php.net/bug.php?id=50688

Sylvain Lecoy’s picture

Can we fix #60 as well for better encapsulation ?

Also, for the sake of testing, I would rather prefer to have to pass the EntityStorageControllerInterface by constructor rather than get it in the constructor. In the former case you can easily pass a mocked object, while in the second case, you have no way of overriding the entity_get_controller method, or you have to override the __construct method.

pounard’s picture

Agree with #65, this particular controller should not use any procedural accessors.

damiankloip’s picture

#64 and #65: Guys, please feel free to write the code and provide a patch for this...

pounard’s picture

@#67 The issue seems to be already handled by 2 or more developers, I won't do this at the expense of what are doing the others. I'm suggesting ideas, if they like it they will do it. If they don't like it, they won't accept my patches anyway.

Lars Toomre’s picture

I am tring to get my head around all of the various comments in this issue to figure out how to proceed. From a high level, what appears still needs to be done is as follows:

As per #52 and #53, modify ConfigStorageController and their ilk with:

  /**
   * Returns the Entity Type.
   *
   * @return Drupal\...
   *   The entity type of this instance.
   */
  public function getEntityType();

  /**
   * Returns the Entity Information array
   *
   * @return Drupal\...
   *   The entity information array for this instance.
   *
  public function getEntityInfo();

As per #53, #63, #64 and #65, modify EntityListController.php by deleting load() and adding:

  /**
   * An array of entities of this type for listing.
   *
   * @var array
   *   An array of entities implementing Drupal\Core\Entity\EntityInterface.
   */
  protected $entityItems;

  /**
   * Constructs a new EntityListController object.
   *
   * @param string $entity_type.
   *   The type of entity to be listed.
   * @param array $entity_items
   *   (optional) An array of entities implementing Drupal\Core\Entity\EntityInterface.
   * @param Drupal\Core\Entity\EntityStorageControllerInterface $entity_storage
   *   (optional) The storage controller used by this list controller.
   */
  public function __construct($entity_type, $entity_items = NULL, $entity_storage = NULL) {
    $this->entityType = $entity_type;
    $this->storage = isset($entity_storage) ? $entity_storage : entity_get_controller($this->entityType);
    $this->entityItems = $this->setItems($entity_items);
    $this->entityInfo = $this->storage->getEntityInfo;
  }

  /**
   * Retrieves the list of entity items.
   *
   * @return array
   *   An array of entities implementing Drupal\Core\Entity\EntityInterface.
   */
  public function getItems(){
    return $this->entityItems;
  }

  /**
   * Sets the list of entity items.
   *
   * @param array $entity_items
   *   (optional) An array of entities implementing Drupal\Core\Entity\EntityInterface.
   *   Defaults to NULL which causes the default list of items to be loaded from
   *   the storage controller.
   *
   * @return array
   *   An array of entities matching the entity type and implementing
   *   Drupal\Core\Entity\EntityInterface.
   */
  public function setItems($entity_items = NULL) {
    // If NULL, retrieve default list via the storage controller.
    if (!isset($entity_items) || !is_array($entity_items) || empty($entity_items) {
      $this->entityItems = $this->storage->load();
    }
    // Check that each item implements EntityInterface and of correct entityType.
    else {
      $items = array();
      foreach ($entity_items as $key => $entity) {
        if ($entity instanceof EntityType && $entity->??? === $this->entityType) {
          $items[] = $entity;
        }
      }
      $this->entityItems = $items;
    }
    return $this->entityItems;
  }

  /**
   * Sorts the array of entity items.
   *
   * @param callable|null $custom_sort
   *   (optional) A custom function to use when sorting the array of entities
   *   with uasort().  If no value is provided, no sorting will be performed
   *   and list will be presented as set in setItems().
   *
   * @return array
   *   The sorted array of entity items.
   */
  public function sort($custom_sort = NULL) {
    // Check that the custom sort function exists.
    if (isset($custom_sort) && function_exists($custom_sort)) {
      uasort($this->entityItems, $custom_sort);
    }
    return $this->entityItems;
  }

Finally, I am lost about what methods belong in the EntityListControllerInterface. In #60, @fago suggests only __construct() and render() are needed, but getOperations() and buildOperations() would be useful.

Does the above pseudo code make sense to roll into a patch?

tim.plunkett’s picture

Status: Needs work » Needs review
FileSize
5.02 KB
12.76 KB

I've moved non-essential methods out of the interface.
I also documented the return of getOperations(), as I agree it is useful.

Both DatabaseStorageController and ConfigStorageController call entity_get_info() in the constructor, we can debate that in a follow-up.
I did however change the constructor to take the EntityStorageControllerInterface, that's a good idea.

The more I think about EntityListControllerInterface::load(), the more I think it's a good idea and we should keep it.
As sun points out in #63, it's a great entry point for further manipulation of the list. I've added a comment to that effect.

Code pushed to the sandbox, but here's a patch and interdiff for those following along.

Lars Toomre’s picture

Status: Needs review » Needs work

Thanks @tim. We were working on this at the same time.

I am curious what you think about what I suggested instead of load(). In #69, I was suggeting to add entity_items to the cunstructor as well as a getter and setter. A new sort function then would operate on the protected entityItems property.

I think that what I suggested adds flexibility while maintaining what already is in the patch. Hence, one could create a listing of all config objects of 'foo*'. Does it that make sense?

tim.plunkett’s picture

Status: Needs work » Needs review
FileSize
12.82 KB

#69 doesn't make any sense to me. Maybe provide a patch I can try out?

Updating the code for the ConfigEntity move.

moshe weitzman’s picture

Great patch

+ $controllers = &drupal_static(__FUNCTION__, array());. Please justify static caches with performance benchmarks or something a bit less analytical if you must. Why do we have to cache here? I think caching could be done further down in the call stack by whatever is slow. FYI, an non-compelling justification is "I static cache here because FOO has a static cache."

tim.plunkett’s picture

FileSize
897 bytes
12.66 KB

I have a static cache here because entity_get_controller() has a static cache :)

Hilariously enough, it was so much copy/paste that it wasn't even being used.

Status: Needs review » Needs work
Issue tags: -Needs tests, -Entity system, -Configuration system, -VDC, -CTools dependency, -Configurables

The last submitted patch, listing-1781372-74.patch, failed testing.

tim.plunkett’s picture

Status: Needs work » Needs review
Issue tags: +Needs tests, +Entity system, +Configuration system, +VDC, +CTools dependency, +Configurables

#74: listing-1781372-74.patch queued for re-testing.

attiks’s picture

Never mind, need coffee I doesn't apply anymore :/

webchick’s picture

Issue tags: +Spark

Layouts, Gridbuilder, Bunnypoint, etc. are all using this so tagging for Spark.

Dave Reid’s picture

I'm uncomfortable with the fact that URI callbacks are required for entities that don't actually use URI callbacks as publicly-visible 'view' paths as evidenced by Gabor needing to implement URI callback for region entities. See #1332058: Need a way to distinguish "public/final" URIs for entities from admin and redirected ones.

tim.plunkett’s picture

@Dave Reid, that makes sense to me as a new policy/standard, but I think your issue (which I followed) is the best place to codify that change. This patch doesn't make URI callbacks any ore required than they are already.

@damiankloip said he'd be working on the tests more today, yay!

Dave Reid’s picture

Tim, if I'm looking at the patch correctly, it seems it would force configuration entities to have an URI callback in order to get the edit and delete links when most of them aren't something that is 'visible' and has a view callback. It's not really something we can undo if this gets in.

tim.plunkett’s picture

@Dave Reid, well, we could solve it in #1783964: Allow entity types to provide menu items, or subclass EntityListController for "public/visible" entities and override getOperations().

Gábor Hojtsy’s picture

Indeed, for me the easiest way was to define a URI callback, but I could have defined my own list controlled and overriden the operations. It was comfier to provide a URI callback even though it might not be correct from a technical purity standpoint.

Dave Reid’s picture

Ok great I wanted to make sure we have a plan in place to prevent abuse of the URI callback and not further regress. This is complicated since the config_test entity type implements a non-view URI callback, but that will be handled separately, it's just that this issue exposes it.

Lars Toomre’s picture

@tim This untested patch is what I was trying to explain in #69.

It would allow one (with no sub-classing) to pick items 2, 4 and 6 from the default list and to present the list in 4, 6, and 2 order. We also could use this EntityListController implementation, for instance, to display all taxonomy terms starting with 'V' or items 11-20 (a la a pager). My thought that the function presenting the list would do something like:

$listing = new EntityList($entity_type, EntityStorageControllerInterface $storage);
(The default list of entites are are already loaded by constructor via the load() method.)
$items = $listing->getItems();
// Do something with list like filtering or cherry picking items 2, 4 and 6 resulting in $filtered_items.
$listing->setItems($filtered_items);
// Sort the list with uasort() with custom comparison function 'custom_sort'.
$listing->sort('custom_sort');
$listing->render();

My one thought to address @Dave Reid's concern is do we want to add a method setOperations(EntityInterface $entity) ?

Gábor Hojtsy’s picture

@Lars Toomre: Looks to me like it would be better to focus this on the use case we built it for and then refactor as needed / wanted later once we can use the functionality, instead of leaving this lingering in the queue not solving the initial use case either.

damiankloip’s picture

Assigned: tim.plunkett » damiankloip

Assigning to me for the tests. I got a few done; should have a finished patch by tomorrow morning!

andypost’s picture

Working on #1552396-50: Convert vocabularies into configuration & #1588422: Convert contact categories to configuration system
I found that there's no implementation of EntityStorageControllerInterface::loadByProperties() which could be useful
Also there's bug with sorting #1552396-60: Convert vocabularies into configuration https://bugs.php.net/bug.php?id=50688

Gábor Hojtsy’s picture

As for the bug with sorting, as per the PHP bug report (https://bugs.php.net/bug.php?id=50688) this sounds like an error/exception down the line in the sorting function which might be related to your test data related to label(). That is really the only "action" that the sort function executes on the config entity. If there is no error down in the label(), it should not modify the array in any way. So not sure how could we fix it, calling label() with @ in this patch does not seem like a great solution. It seems like pretty much a nasty edge case.

Gábor Hojtsy’s picture

One more data point: the reason I'm clearly thinking there is probably something wrong with your source data is that I'm using this controller in the Region module which has a pretty complete test suite (http://drupal.org/project/region) and all regions sharing the same weight, and therefore ordered by label, hitting the label() methods, but no issues experienced (in manual or automated testing). I have automated testing there to edit default shipped regions, add new ones, delete both types, etc. So I don't see a reason to hold back this patch on that.

pounard’s picture

There is something that bothers me with this patch. When I read it, I see that the EntityListController acts as a real controller in the MVC sense of the "controller" word: it reads data from the model controller by input arguments and dispatches the result into the theme layer. It is a specific business callback therefore doesn't really need the EntityListControllerInterface interface.

Something really important that we absolutely need are paging and filtering capacities. In order to achieve that properly, this should be natively handled by the EntityListController generic instance. While the storage controller cannot, I'm enclined to use a LimitIterator instead, and leaving the load() method overridable for adding better paging handling depending on specific implementations.

Operations should be handled outside of buildHeader() and buildRow(), those can be operations agnostic (and thus does not depend on getPath() which then become optional).

#85's proposal is conceptually wrong: your class is already doing the controller (in the MVC sense of it) job, adding the setItem() method on top of it is like stacking multiple controllers onto each others, it feels wrong. We have a controller here, we should use directly the Request object for getting filters and/or paging information.

I like the idea of this controller, and a lot of code in it is good, but I am tempted to adapt it and write something that would look like this (see attached file), which is more encapsuled, a bit more extensible, and provide primitive paging and filter capabilities following what can do the entity storage interface.

damiankloip’s picture

FileSize
5.23 KB
15.77 KB

Ok, here is some revamped test coverage. This should cover most things now, I hope.

@tim.plunkett: I created a new branch, config-list-1781372-damian, from your branch.

pounard’s picture

I opened #1797740: Introduce the EntityTypeAwareInterface interface for entity operations uniformisation that would help cleaning up a bit the list controller constructor and some bits of the internals.

Berdir’s picture

Coming a bit late to this, ignore me if something has already been discussed and I missed it.

I agree that a number of things could be refactored like the entity operation stuff but that can be tackled later. I like the idea of this.

The conflicts between content and config entities are a bit tricky here, the focus of this is for config entities but it's enabled by default for all entities. No idea what's going to happen for an entity type that doesn't properly support this, considering e.g. the missing paging support. Maybe not much right now, but I guess we're going to further extend this later on. Wondering if this could be an optional feature instead that needs to be declared. Modules that attempt to use this in generic way would just need to check if there is actually a controller returned...

It's also a pity that there is a default value for the list controller but every config entity (those that will actually use it) need to override it anyway.

+++ b/core/modules/config/lib/Drupal/config/Tests/ConfigEntityListTest.phpundefined
@@ -0,0 +1,144 @@
+    $this->drupalSetContent($page);
+
+    // Verify that the expected operation links work.
+    foreach (array('Edit', 'Delete') as $link) {
+      $this->drupalSetContent($page);
+      $this->assertLink($link);
+      $this->clickLink($link);
+      $this->assertResponse(200);

The first setContent() calls seems to be unecessary here?

+++ b/core/modules/config/tests/config_test/config_test.moduleundefined
@@ -82,6 +82,8 @@ function config_test_entity_info() {
     'entity class' => 'Drupal\config_test\ConfigTest',
+    'list controller class' => 'Drupal\Core\Config\Entity\ConfigEntityListController',
+    'list path' => 'admin/structure/config_test',
     'uri callback' => 'config_test_uri',

hook_entity_info() should be updated and these two option should be added to the documentation there, once the approach has been agreed on. What the todo in the issue summary says :)

xjm’s picture

Assigned: damiankloip » xjm

Thanks @Berdir! Not sure how everyone missed that we didn't update the hook docks... duh!

And I agree about the extra drupalSetContent(); I actually almost asked for that to be changed on my first review.

Fixing that now, and adding a bit more additional test coverage.

damiankloip’s picture

DrupalSetContent should be removed, its a clear mistake. I'm not sure it was there before though. I think I added that I my last patch...

pounard’s picture

I think paging should be added to this patch, without it sounds not usable for anything else than types with less than 20 entities. It shouldn't be that hard to implement? May be the entity storage needs some love to allow this more easily; Anyway I think that in order to have something generically useful, we need this. Add filtering to it and you have the UI everyone wants.

tim.plunkett’s picture

#97, I opened #1798332: Add paging to the EntityListBuilder
#96, no worries, xjm is going to fix it :)

pounard’s picture

Oh nice, thanks!

xjm’s picture

++ b/core/lib/Drupal/Core/Entity/EntityListController.phpundefined
@@ -0,0 +1,168 @@
+   *
+   * @todo What is this method for, other than fetching the list path? Is this
+   *   for http://drupal.org/node/1783964 ? Should it be on the interface?

Oh. My @todo is still in here and needs a followup.

xjm’s picture

Issue summary: View changes

Updated issue summary.

sun’s picture

+++ b/core/lib/Drupal/Core/Entity/EntityListController.php
@@ -0,0 +1,168 @@
+    $row['operations'] = drupal_render($operations);

Can we restore this to 'operations']['data' => $operations and remove the too early drupal_render(), please?

Aside from that, I'd be happy to move forward here. That is not to say that this would be set in stone. Quite the contrary, I am confident that we will revisit each and every part of this code, tear it apart, rip it out, revamp, and remix it back in. Working on this revealed a couple of very interesting questions and challenges; I not only foresee us injecting a (fancy new) Route object into the list controller, but I also see a very high potential for an entirely new concept of Entity Operations (view/edit/delete/duplicate/enable/disable/etc, and yes, including view), not only for the UI purposes here, but also for typical Entity Links on regular view (buried into ENTITY_build_content() right now), but even more so defining the corresponding callbacks and quite potentially more metadata (» Entity Actions, anyone?). We should also investigate how to make the output more themeable, which inherently, most likely means "swappable", but not from a developer/entity system perspective but a themer perspective. All of that will be very exciting to work on!

xjm’s picture

Assigned: xjm » Unassigned
Issue tags: -Needs tests
FileSize
34.41 KB
34.06 KB
10.86 KB
18.55 KB

Updated patch from the latest in this issue's branch in the CMI sandbox. Attached:

  • Adds more functional test coverage to confirm the user can interact with the list controller as expected to add, edit, and delete an entity. This should also provide coverage for the bug @Jelle_S found.

  • Adds the hook_entity_info() docs (thanks @Berdir).

  • Removes the getPath() method per protracted IRC discussion with @tim.plunkett. Currently it's not really used. We can revisit this in #1783964: Allow entity types to provide menu items.

  • Clarifies the documentation for the render helper methods in EntityListController.

  • Removes the 'list path' entity info key.

    In the recent version of the patch, which uses the hook_menu() implementation to define the list path, the 'list path' key was only a workaround to tell the controller what link path to use for an "Add" link inside the table when no entities are listed. This workaround was not very robust and kind of weird and confusing (seemed like the worst of both worlds), so after discussion with Tim I removed it in favor of a complete solution from #1783964: Allow entity types to provide menu items. The screenshots below illustrate the change. I'll file a followup shortly to add back the link for better UX once #1783964: Allow entity types to provide menu items is resolved.

  • Finally, Tim also added a commit addressing sun's point in #101 that's rolled into the patch here.

List controller knows its path
list-controller-add-link.jpg

Pure hook_menu() path definition (current)
list-controller-no-add-link.jpg

xjm’s picture

Status: Needs review » Needs work
Issue tags: -VDC

The last submitted patch, config-1781372-102.patch, failed testing.

sun’s picture

Status: Needs work » Needs review
Issue tags: +VDC

Thanks!

Final remarks:

+++ b/core/lib/Drupal/Core/Entity/EntityListController.php
@@ -148,16 +141,21 @@ class EntityListController implements EntityListControllerInterface {
+   * @todo Add a link to add a new item to the #empty text once
+   *   http://drupal.org/node/1783964 is resolved.

The @todo is sufficient, no URL needed.

+++ b/core/lib/Drupal/Core/Entity/EntityListController.php
@@ -148,16 +141,21 @@ class EntityListController implements EntityListControllerInterface {
-      '#empty' => t('There is no @label yet. <a href="@add-url">Add one</a>.', array(
-        '@label' => $this->entityInfo['label'],
-        '@add-url' => url($this->getPath() . '/add'),
-      )),
+      '#empty' => t(
+        'There is no @label yet.',
+        array('@label' => $this->entityInfo['label'])
+      ),

OK with removing @add-url for now, but that's a strange coding style change. Please revert to the previous/regular.

+++ b/core/modules/config/lib/Drupal/config/Tests/ConfigEntityListTest.php
@@ -117,28 +118,60 @@ class ConfigEntityListTest extends WebTestBase {
+    $this->assertFieldByXpath('//td', 'Antelope', "Label found for added 'Antelope' entity.");
+    $this->assertFieldByXpath('//td', 'antelope', "Machine name found for added 'Antelope' entity.");

I don't quite get why we need XPath queries here and elsewhere -- testing for the expected string contents in the columns should be sufficient. We're not testing #theme table here.

+++ b/core/modules/config/lib/Drupal/config/Tests/ConfigEntityListTest.php
@@ -117,28 +118,60 @@ class ConfigEntityListTest extends WebTestBase {
+    // Confirm that the 'Edit' link is present and works as expected.
...
+    $this->assertTitle('Edit test configuration | Drupal');
...
+    // Confirm that the 'Delete' link is works as expected.
...
+    $this->assertTitle('Are you sure you want to delete Albatross | Drupal');

This is not testing list controller functionality. CRUD operations for ConfigTest entities are tested elsewhere already and should not be duplicated. It's not trivial to draw a line here, but let's make sure to remove all unnecessary assertions here.

xjm’s picture

OK with removing @add-url for now, but that's a strange coding style change. Please revert to the previous/regular.

I disagree somewhat; the previous version is hard to scan and confusing. It makes more sense for each parameter to have its own line, the same way that array elements get their own line. This is related to the <editorial>silly, bikeshed-a-licious</editorial> #1539712: [policy, no patch] Coding standards for breaking function calls and language constructs across lines, but I don't think it's worth nitpicking when there's no standard and both patterns for wrapping the assertion are used elsewhere in core. However, I don't actually care, so I'll just put it on one line. :)

I don't quite get why we need XPath queries here and elsewhere -- testing for the expected string contents in the columns should be sufficient. We're not testing #theme table here.

To differentiate it from the text of the page title and/or confirmation messages.

This is not testing list controller functionality. CRUD operations for ConfigTest entities are tested elsewhere already and should not be duplicated. It's not trivial to draw a line here, but let's make sure to remove all unnecessary assertions here.

I also disagree with this. This tests that the links lead to the correct, actual, functional entity forms (see #40 for a related bug) and follows the expected interaction of the user. I could replace the drupalPost() with API calls for the CRUD and then reload the list... but why, since we're already on those forms anyway?

xjm’s picture

Also, the current format of the functional test coverage codifies that the user is by default returned to the list after CRUD operations from it. Which is also worthwhile IMO.

xjm’s picture

Attached addresses #105.

chx’s picture

Status: Needs review » Reviewed & tested by the community

Short of dedicating ourselves to get VBO in when Views gets in, I can't really see how else this can happen so go for it.

xjm’s picture

I went through the test by morning (well predawn) light and I suppose these two assertions in particular could be removed:

+++ b/core/modules/config/lib/Drupal/config/Tests/ConfigEntityListTest.phpundefined
@@ -128,49 +128,59 @@ class ConfigEntityListTest extends WebTestBase {
     $this->assertText('Antelope configuration has been created.');
...
     $this->assertText('Albatross configuration has been updated.');

For the rest:

  • The assertLink(), assertResponse(), and assertTitle() together confirm that the operations links work and lead to the correct pages.
  • The xpath assertions for the table cells confirm that the list responds to CRUD operations on the listed entities.
xjm’s picture

FileSize
1.22 KB
18.75 KB

Attached removes the two assertions in #110, which hopefully should do a better job of addressing @sun's feedback. Should still be RTBC since this is really a quite minor change. :)

xjm’s picture

Issue summary: View changes

Updated issue summary.

xjm’s picture

Issue summary: View changes

Updated issue summary.

xjm’s picture

I updated the issue summary to better reflect the current status of the issue.

xjm’s picture

Issue summary: View changes

Updated issue summary.

xjm’s picture

Issue summary: View changes

Updated issue summary.

xjm’s picture

Issue summary: View changes

There aren't actually any user interface changes from this patch. Conversions to it will introduce those.

xjm’s picture

Issue summary: View changes

Updated issue summary.

andypost’s picture

Patch exposes a trouble with sorting, just adds a weight to form and test the order

EDIT: filed #1798944: Convert config_test entity forms to EntityFormController

EDIT2: Troubles with sorting explained in #1552396-72: Convert vocabularies into configuration

andypost’s picture

I think we should not stop this patch and find a solution for sorting in follow-up

Patch with just minor clean-up

damiankloip’s picture

The only thing I don't like is that this can be used for any entity type. I like how this was originally formed, when we first created this; for listing config entities only.

chx’s picture

That's a valid question. Why this doesn't live in the Config\Entity namespace and uses ConfigEntityInterface where typehinting is used? The scope of this patch is utterly non-useful for generic entities IMO because while we expect config entities to be few we equally expect content entities to be numerous, needing filters, needing pagers etc.

And maybe I was wrong, there's no form in there so if this page would be, you know, a View (!) then all we would need to do is to provide the operation links? Am I very crazy thinking this?

tim.plunkett’s picture

Well, there is #1798332: Add paging to the EntityListBuilder.

I don't like the idea of purposefully restricting this to ConfigEntity.
Yes, a view would be better suited to this, by why force it? If we want to change the namespacing/typehinting later, we can.

andypost’s picture

Also we still have no implementation for EntityStorageControllerInterface::loadByProperties() for config entities I think it could be useful for lists with filters thats why we have commented out some tests in "vocabulary-config" issue

webchick’s picture

Status: Reviewed & tested by the community » Needs review

I can't tell from the recent discussion... is this still RTBC? Or do we need to refactor it? Could further improvements be done in follow-ups?

damiankloip’s picture

Status: Needs review » Reviewed & tested by the community

@andypost, I don't think this is the issue for what the configstoragecontroller doesn't implement.

@webchick, I would say this is still RTBC. My concerns were about people abusing this for any entity (as it lives in entity module now and not config). The patch itself is good though.

sun’s picture

Yes, this looks ready to me, except for the added test lines by @andypost in #113:

+++ b/core/modules/config/lib/Drupal/config/Tests/ConfigEntityListTest.php
@@ -88,6 +88,23 @@ class ConfigEntityListTest extends WebTestBase {
+    // Test sorting.
+    $entity = entity_create('config_test', array(
+      'id' => 'albatros',
+      'label' => 'Albatros',
+      'weight' => 1,
+    ));
+    $entity->save();
+    $entity = entity_create('config_test', array(
+      'id' => 'antelope',
+      'label' => 'Antelope',
+      'weight' => 1,
+    ));
+    $entity->save();
+    $list = $controller->load();
+    $entity = end($list);
+    $this->assertEqual($entity->weight, 1);

The final assertion does not assert anything.

I'm not 100% sure, but I guess the intention was to

1) make the first entity use an ID starting with "a", but a label starting with "Z" and a weight of 2.

2) make the second entity use an ID starting with "z", but a label starting with "A" and a weight of 1.

3) assert that the first item in the list is the second entity, and the last list item is the first entity. This should actually assert the $entity, not properties within.

tim.plunkett’s picture

@sun, thankfully that hunk is only in #113, and not #114.

#114 is the patch to commit.

andypost’s picture

My patch just points that if there's a items with a same weight then sorting fails

andypost’s picture

+++ b/core/lib/Drupal/Core/Config/Entity/ConfigEntityListController.phpundefined
@@ -0,0 +1,27 @@
+  public function load() {
+    $entities = parent::load();
+    uasort($entities, 'Drupal\Core\Config\Entity\ConfigEntityBase::sort');
+    return $entities;

I meant this place! this one uses uasort and this place has no test

webchick’s picture

Title: Add an API for listing (configuration) entities » Change notice: Add an API for listing (configuration) entities
Category: feature » task
Priority: Major » Critical
Status: Reviewed & tested by the community » Active
Issue tags: +Needs change record
+++ b/core/modules/config/tests/config_test/config_test.module
@@ -176,36 +177,8 @@ function config_test_delete($id) {
-  $entities = entity_load_multiple('config_test');
-  uasort($entities, 'Drupal\Core\Config\Entity\ConfigEntityBase::sort');
-
-  $rows = array();
-  foreach ($entities as $config_test) {
-    $uri = $config_test->uri();
-    $row = array();
-    $row['name']['data'] = array(
-      '#type' => 'link',
-      '#title' => $config_test->label(),
-      '#href' => $uri['path'],
-      '#options' => $uri['options'],
-    );
-    $row['delete']['data'] = array(
-      '#type' => 'link',
-      '#title' => t('Delete'),
-      '#href' => $uri['path'] . '/delete',
-      '#options' => $uri['options'],
-    );
-    $rows[] = $row;
-  }
-  $build = array(
-    '#theme' => 'table',
-    '#header' => array('Name', 'Operations'),
-    '#rows' => $rows,
-    '#empty' => format_string('No test configuration defined. <a href="@add-url">Add some</a>', array(
-      '@add-url' => url('admin/structure/config_test/add'),
-    )),
-  );
-  return $build;
+  $controller = entity_list_controller('config_test');
+  return $controller->render();

Well now, THAT is pretty damn sexy. :)

I reviewed this code and it seems really well documented and clear. It's also nice that there are a bevvy of contributed 8.x modules atm making use of it, so it seems to be pretty rock solid. I agree with andypost that the bug he uncovered in the sorting functionality is definitely worth a follow-up, but since it was not introduced by this patch, should not hold it up.

So.... Committed and pushed to 8.x! YEAH! This is AWESOME stuff, folks!

I believe this needs a change notice, so that people porting their modules know to use this new API. Marking to a critical task for that. If you could turn that around quickly, it would be much appreciated, so it doesn't block other peoples' D8 features.

webchick’s picture

Issue summary: View changes

Add 'add link' issue to the summary.

tim.plunkett’s picture

Status: Active » Needs review

I've posted a change notice here: http://drupal.org/node/1799642

plach’s picture

Title: Change notice: Add an API for listing (configuration) entities » Add an API for listing (configuration) entities
Component: config.module » entity system
Category: task » feature
Priority: Critical » Major
Status: Needs review » Fixed
Issue tags: -Entity system

Looks pretty clear, thanks. I guess it will need to be updated for the follow-up about pagers and stuff.

Moving this to the entity system queue, where it belongs.

plach’s picture

Issue tags: -Needs change record
xjm’s picture

Is there a followup issue for the sorting issue? I thought @andypost said he was going to post it but I couldn't find it. Can we link it here?

pcambra’s picture

YesCT’s picture

#1810350: Remove menu_* entity keys and replace them with entity links was postponed on this issue. (just adding this info so they are linked. also updated issue summary to add this to related issues)

YesCT’s picture

Issue summary: View changes

Updated issue summary.

xjm’s picture

Well, this issue is fixed, so...

Gábor Hojtsy’s picture

I'm trying to introduce weights (tabledrag) into my existing entity list controller at #1787942: Allow assigning layouts to pages (so that pages can have weights edited sensibly in the admin list instead of their own number weight widgets on their edit form which is mostly useless). I've implemented tabledrags plenty of times before but I'm pretty puzzled as to how would I best integrate with entity list controllers if at all. To introduce this, I need a form, not a simple list render, I need submission handling, etc, and need to separate the form rendering/theming from the form itself (which the entity list controller does not in itself allow). I'd love to get some "best practice" feedback as to how it should be approached. Currently it feels like I might need to abandon entity list controllers in favor of my own tabledrag listing implementation (possibly in classic PHP functions fashion). Thanks!

xjm’s picture

@Gábor Hojtsy, that sounds like something that would be generally useful--maybe open a feature request?

andypost’s picture

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

Anonymous’s picture

Issue summary: View changes

Updated issue summary. adding info that #1810350: Remove menu_* entity keys and replace them with entity links was postponed on this issue.