Add generic weighting (tabledrag) support for config entities (DraggableListController)

Should be piece of cake once #80855: Add element #type table and merge tableselect/tabledrag into it has landed.

Awesome!

+++ b/core/lib/Drupal/Core/Config/Entity/ConfigEntityListController.phpundefined
@@ -15,7 +17,29 @@
+   * Overrides EntityListController::__construct().
...
+   * Overrides EntityListController::load().

@@ -23,4 +47,119 @@ public function load() {
+   * Overrides EntityListController::buildHeader().
...
+   * Overrides EntityListController::buildRow().
...
+   * Overrides EntityListController::render().

Needs the full namespace \Drupal\Core\Entity\EntityListController

+++ b/core/lib/Drupal/Core/Config/Entity/ConfigEntityListController.phpundefined
@@ -23,4 +47,119 @@ public function load() {
+    if (empty($this->weightKey)) {
...
+    if (empty($this->weightKey)) {

Each of these empty() checks feel like they should just be !$this->weightKey

+++ b/core/lib/Drupal/Core/Config/Entity/ConfigEntityListController.phpundefined
@@ -23,4 +47,119 @@ public function load() {
+    if (empty($this->weightKey)) {
+      return parent::buildRow($entity);
+    }
+
+    $row['label']['#markup'] = $entity->label();
+    $row['id']['#markup'] = $entity->id();
+    $row['operations']['data'] = $this->buildOperations($entity);

This could just be

$row = parent::buildRow($entity);
if (empty($this->weightKey)) {
  return $row;
}

Tagging.

Let's see how it goes.

Fixed broken test by moving sorting support exactly to ConfigEntityListController
This shows that #theme(table) and #type(table) still different in render

Patch now green again so here doc-block fixes following current http://drupal.org/coding-standards/docs#classes

There's a some suggestions to fix JS for tabledrag
#1869328-32: Restore simplicity of language list

Does not mean to change a status

Not straightforward to do from the JS side: #1016056: Hide tabledrag handle and "Show row weights" when there is only one item in list

+++ b/core/lib/Drupal/Core/Config/Entity/ConfigEntityListController.php
@@ -2,12 +2,14 @@
- * Definition of Drupal\Core\Config\Entity\ConfigEntityListController.
+ * Definition of \Drupal\Core\Config\Entity\ConfigEntityListController.

+++ b/core/lib/Drupal/Core/Entity/EntityListController.php
@@ -2,7 +2,7 @@
- * Definition of Drupal\Core\Entity\EntityListController.
+ * Definition of \Drupal\Core\Entity\EntityListController.

If we're already changing this, it should be "Contains" instead of "Definition of"

+++ b/core/lib/Drupal/Core/Config/Entity/ConfigEntityListController.php
@@ -23,4 +47,122 @@ public function load() {
+    drupal_set_message(t('Your settings have been saved.'));

To be consistent with system_config_form_submit() this should be 'The configuration options have been saved.'

Leaving at needs review, because those are minor, and since I think this is blocking stuff elsewhere, might be adjusted in a follow-up.

This looks awesome to me.

Let's move forward with this patch, and perform any possible improvements in follow-up issues.

Existing test coverage should be sufficient here, I think.

+++ b/core/lib/Drupal/Core/Config/Entity/ConfigEntityListController.phpundefined
@@ -2,12 +2,14 @@
- * Definition of Drupal\Core\Config\Entity\ConfigEntityListController.
+ * Contains \Drupal\Core\Config\Entity\ConfigEntityListController.

@@ -15,7 +17,29 @@
-   * Overrides Drupal\Core\Entity\EntityListController::load().
...
+   * Overrides EntityListController::__construct().
...
+   * Overrides EntityListController::load().

+++ b/core/lib/Drupal/Core/Entity/EntityListController.phpundefined
@@ -2,7 +2,7 @@
- * Definition of Drupal\Core\Entity\EntityListController.
+ * Contains \Drupal\Core\Entity\EntityListController.

@@ -15,7 +15,7 @@ class EntityListController implements EntityListControllerInterface {
-   * @var Drupal\Core\Entity\EntityStorageControllerInterface
+   * @var \Drupal\Core\Entity\EntityStorageControllerInterface

@@ -40,7 +40,7 @@ class EntityListController implements EntityListControllerInterface {
-   * @param Drupal\Core\Entity\EntityStorageControllerInterface $storage.
+   * @param \Drupal\Core\Entity\EntityStorageControllerInterface $storage.

@@ -50,21 +50,21 @@ public function __construct($entity_type, EntityStorageControllerInterface $stor
-   * Implements Drupal\Core\Entity\EntityListControllerInterface::getStorageController().
+   * Implements EntityListControllerInterface::getStorageController().
...
-   * Implements Drupal\Core\Entity\EntityListControllerInterface::load().
+   * Implements EntityListControllerInterface::load().
...
-   * Implements Drupal\Core\Entity\EntityListControllerInterface::getOperations().
+   * Implements EntityListControllerInterface::getOperations().

@@ -89,7 +89,7 @@ public function getOperations(EntityInterface $entity) {
-   * @see Drupal\Core\Entity\EntityListController::render()
+   * @see \Drupal\Core\Entity\EntityListController::render()

@@ -101,32 +101,31 @@ public function buildHeader() {
-   * @param Drupal\Core\Entity\EntityInterface $entity
+   * @param \Drupal\Core\Entity\EntityInterface $entity
...
-   * @see Drupal\Core\Entity\EntityListController::render()
+   * @see \Drupal\Core\Entity\EntityListController::render()

@@ -140,7 +139,7 @@ public function buildOperations(EntityInterface $entity) {
-   * Implements Drupal\Core\Entity\EntityListControllerInterface::render().
+   * Implements EntityListControllerInterface::render().

Absolutely none of this is in scope.

+++ b/core/modules/contact/lib/Drupal/contact/CategoryListController.phpundefined
@@ -41,10 +41,12 @@ public function getOperations(EntityInterface $entity) {
+    // @see http://drupal.org/node/1876718

@@ -52,11 +54,17 @@ public function buildHeader() {
+    // @see http://drupal.org/node/1876718

Can we have an actual comment here about what the array_slice is doing, beyond linking to that issue?

+++ b/core/modules/contact/lib/Drupal/contact/Plugin/Core/Entity/Category.phpundefined
@@ -28,7 +28,8 @@
+ *     "weight" = "weight"

This needs to be documented in EntityManager, under entity_keys

Let's fix the stuff in the review.

One problem is that currently we just have a single entityManager so weight just for on config entities.

Thanks! Let's get this in now.

With this, the need for #1876718: Allow modules to inject columns into tables more easily becomes more apparent. (already linked in the @todos)

Once filter formats are converted (#1779026: Convert Text Formats to Configuration System), we can leverage it there, which means that we can remove even more unnecessary custom code.

+++ b/core/modules/contact/lib/Drupal/contact/CategoryListController.phpundefined
@@ -52,11 +56,19 @@ public function buildHeader() {
   public function buildRow(EntityInterface $entity) {
-    $row['category'] = check_plain($entity->label());
-    $row['recipients'] = check_plain(implode(', ', $entity->recipients));
     $default_category = config('contact.settings')->get('default_category');
-    $row['selected'] = ($default_category == $entity->id() ? t('Yes') : t('No'));
-    $row['operations']['data'] = $this->buildOperations($entity);
+    $row = parent::buildRow($entity);
+    // The two array_slice work together to put additional columns after the
+    // first ones.
+    // @see http://drupal.org/node/1876718
+    $row = array_slice($row, 0, 2, TRUE) + array(
+      'default' => array(
+        '#markup' => ($default_category == $entity->id() ? t('Yes') : t('No')),
+      ),
+      'recipients' => array(
+        '#markup' => check_plain(implode(', ', $entity->recipients)),
+      ),
+    ) + array_slice($row, 0, NULL, TRUE);

This looks a bit messy to me. It's both slow (un-doing work) and pretty ugly to read. Is there a way we can improve this? Sounds like #1876718: Allow modules to inject columns into tables more easily could help with this, although I'm skeptical it would make it a lot easier.

Related #1887582: update #type=table for language list to simplify tables

Re #22: Actually #1876718: Allow modules to inject columns into tables more easily will (almost certainly) allow to completely remove/revert that hunk entirely. What is done in addition to the array_splice()ing in that hunk is the usage of render elements (albeit #type => 'markup') instead of a pure string. That might (or might not) be retained, but that can be discussed independently.

Yes, that's a known issue (hence the @todos). #1876718: Allow modules to inject columns into tables more easily will clean that up, and we're actively discussing the best solution over there already.

I'd prefer if we could move forward here and accept the array_slice() ugliness as a temporary thing only.

Maybe better to store columns & operations separate? so we can inject weight column only when needed

Updated issue summary.

Another list that needs this added to summary #1891690: Use EntityListController for vocabularies

I think this issues should investigate most common needs for configurable entities, probably some code could be moved to EntityListController. We have a lot of implementations in Core that could be generalized also taking into account translatability of the entities.

Current implementations are Views, PictureMappingListController, Menus, Contact, vocabularies, imagestyles

For vocabularies, contact, filters, languages, layout-pages we need forms with weight.
For menus, shortcuts, image-styles - we use simple render-tables
For forums, menu-items, image-effects - sortable trees

For most of fieldable entities we need 2 menu-items + edit and delete operations

Also we need a way to remove Save button and table-weight behavior when there's only 1 item in list.
There's a lot of issues waiting for this and more menu-routers could be simplified

Related issues:
#1787942-41: Allow assigning layouts to pages
#599770-15: Clean up the contact forms listing UI: Allow to set the default category and weights on the listing page
#1809376-15: Use EntityListController for image styles
#1783964-7: Allow entity types to provide menu items
#1891690: Use EntityListController for vocabularies
#1798540: Add link to add a new entity in an empty entity list controller table
#1803586: Give all entities their own URI
#1479454: Convert user roles to configurables

Updated issue summary.

Updated

Updated issue summary.

Updated issue summary.

I don't see the reason to do anything more in this issue. Furthermore, we'd ideally not use EntityListController for non-config, using Views. See #1864980: [meta] Figure out how to integrate Views into core

This patch changes a contact listing by adding a machine name which seems wrong. see screens attached

Before

After

EDITED
Also I tried to implement controller for vocabularies and it works #1891690-5: Use EntityListController for vocabularies

+++ b/core/lib/Drupal/Core/Config/Entity/ConfigEntityListController.php
@@ -23,4 +47,122 @@ public function load() {
+      'weight' => array(
+        '#type' => 'textfield',
+        '#title' => t('Weight for @title', array('@title' => $entity->label())),
+        '#title_display' => 'invisible',
+        '#default_value' => $entity->get($this->weightKey),
+        '#size' => 4,
+        '#attributes' => array('class' => array('weight')),

Why not use #type=weight for weight field?

Patch limits the scope of "column-injection" to buildHeader() only by adding sorting for each row by table header so no matter in order the buildRow() adds additional data.

The weight field added at the end of table because mostly they hidded in UI.

This affects all configurables, vocabulary part could be out of scope but issue should care all cases.

Let's see how many tests is broken by this.

Fix broken tests

Can we please go back to #20 and get that in first?

Regarding the "regression" of an additional machine name being output in the contact category listing (as depicted in #31), IMHO that's perfectly acceptable for now. I think we want to re-evaluate what the default listing for config entities should contain and look like either way. Quite potentially, we actually want to output the machine name (but not necessarily in an own column).

#20: drupal-1855402-20.patch queued for re-testing.

So first here is an interdiff from 34 to 20. Thanks andypost for improving the entity list controllers in all that places.

The changes for vocabularies seems to be fine, but maybe out of scope, as we can improve the actual instances later?
The same can be said for pieces like the menu list controller etc.
Let the patch size be as small as possible, and iterate on it. That's the proper way to get progress at the end.

I don't see the point why adding the machine name in the tabledrag issue makes any sense.

So ok, I moved mack machine name but into responsive column and no taxonomy now to minimize a patch

Patch adds back #20 way to insert columns with @todo for follow-up
Also conversion columns to render is the same for label and id because we need a way to play easily with css rules like Menu does.

Also let's get reviews from usability and mobile team!

PS: there's no reason to rush this patch, we need a generic implementation

@andypost:
I've the impression you're mistaking this issue as the issue that wants to introduce a super-generic/catch-all ConfigEntityListController implementation for all config entities... but that's not the case. We're trying to focus on the tabledrag/weighting aspect only. (As the issue title states.) Evaluating what the generic default implementation for config entity listings should be is left for a separate issue. The original patch in #20 does not affect any config entity listings, unless they specifically opt-in for the weight/tabledrag handling. That's really all we want to achieve in this issue.

@sun #39 is the same as #20 but fixes regressions in UI and tests.

And the main difference that better to remove array_slice() code from render() see code below. Custom List controllers are just add own columns in buildRow() but the order is defined in header so this change should improve DX.

Changes in default controller are affects current implementations so patch must fix this regressions.

+++ b/core/lib/Drupal/Core/Config/Entity/ConfigEntityListController.php
@@ -23,4 +47,140 @@ public function load() {
+    // Save header's order of columns for sorting data-rows.
+    $header_sort = array_flip(array_keys($form['entities']['#header']));
+    foreach ($this->load() as $entity) {
+      $row = $this->buildRow($entity);
+      // Sort row columns by header's order.
+      $form['entities'][$entity->id()] = array_merge($header_sort, $row);

this

+++ b/core/lib/Drupal/Core/Entity/EntityListController.php
@@ -153,8 +152,12 @@ public function render() {
+    // Save header's order of columns for sorting data-rows.
+    $header_sort = array_flip(array_keys($build['#header']));
     foreach ($this->load() as $entity) {
-      $build['#rows'][$entity->id()] = $this->buildRow($entity);
+      $row = $this->buildRow($entity);
+      // Sort row columns by the header's order.
+      $build['#rows'][$entity->id()] = array_merge($header_sort, $row);
     }

this

I dont get what you want me to review

@Bojhan the UI changes in #31 #39 for contact category listing. Menus in #39 gets new Machine name column. Is there any suggestions for machine names?

#39: 1855402-config-sort-39.patch queued for re-testing.

EDIT can't reproduce translation test failure locally

I see no reason to have a machine name column, I'm sure there are loads of usecases that you would need them - I do not see a 80% usecase. I would side against them for now, because 1) its relatively easy to have them show up, through a custom view, or just adjusting this listing through a module, 2) you can get the machine name, if you click edit, 3) the consistent pattern we employ in core currently and received no negative feedback is to edit the thing to see the machine name (albeit not on content types, but the Field UI is a one off).

Updated issue summary.

Should be postpone this for #1876718: Allow modules to inject columns into tables more easily ?

Or implement a generic and use it for a all form-tables like #1876712: [meta] Convert all tables in core to new #type 'table'

patch without machine names

Contact categories

Menus

Andypost asked me to weigh in on the machine name issue and I pretty much agree with everything Bojhan said.

Fixed broken test

Merge HEAD after #1826602: Allow all configuration entities to be enabled/disabled

Now we should wait for #1913084: Introduce a Form interface for building, validating, and submitting forms to land and probably #1913618: Convert EntityFormControllerInterface to extend FormInterface

I think EntityFormController is not related to ListController so any reviews are welcome because other issues depends on this one

Build on top of FormInterface like BlockListController does

Remove copy/paste error

#56: 1855402-config-sort-56.patch queued for re-testing.

#56: 1855402-config-sort-56.patch queued for re-testing.

Current doc blocks

Re-roll

+++ b/core/lib/Drupal/Core/Config/Entity/ConfigEntityListController.phpundefined
@@ -9,14 +9,38 @@
+  protected $weightKey;
...
+    else {
+      $this->weightKey = FALSE;

You could just start with protected $weightKey = FALSE; and skip the else

+++ b/core/lib/Drupal/Core/Config/Entity/ConfigEntityListController.phpundefined
@@ -51,4 +75,133 @@ public function getOperations(EntityInterface $entity) {
+    $entities = entity_load_multiple($this->entityType, array_keys($values));

Should switch to

$entities = \Drupal::service('plugin.manager.entity')->getStorageController($this->entitytType)->load(array_keys($values));
to make it easier to inject later.

+++ b/core/modules/config/lib/Drupal/config/Tests/ConfigEntityListTest.phpundefined
@@ -79,21 +79,24 @@ function testList() {
+    $entity_info = entity_get_info('config_test');

@@ -119,24 +122,25 @@ function testListUI() {
+    $entity_info = entity_get_info('config_test');

$entity_info = $this->container->get('plugin.entity.manager')->getDefinition('config_test');

Here it is

Default value moved out of constructor

copy/paste error :(

#66: 1855402-sort-config-66.patch queued for re-testing.

+++ b/core/lib/Drupal/Core/Config/Entity/ConfigEntityListController.phpundefined
@@ -51,4 +72,133 @@ public function getOperations(EntityInterface $entity) {
+    // Configurable entities could have default link to their edit pages.
+    if ($uri = $entity->uri()) {
+      $row['label'] = array('data' => array(
+        '#type' => 'link',
+        '#title' => $row['label'],
+        '#href' => $uri['path'],
+        '#options' => $uri['options'],
+      ));
+    }
+    else {
+      $row['label'] = array('data' => array(
+        '#markup' => check_plain($row['label']),
+      ));

Also currently in core we provide links only to "Content Entities"

+++ b/core/lib/Drupal/Core/Entity/EntityListController.phpundefined
@@ -153,8 +152,12 @@ public function render() {
+    // Save header's order of columns for sorting data-rows.
+    $header_sort = array_flip(array_keys($build['#header']));
     foreach ($this->load() as $entity) {
-      $build['#rows'][$entity->id()] = $this->buildRow($entity);
+      $row = $this->buildRow($entity);
+      // Sort row columns by the header's order.
+      $build['#rows'][$entity->id()] = array_merge($header_sort, $row);

Is strict but #1876718-7: Allow modules to inject columns into tables more easily seems better

+++ b/core/modules/contact/lib/Drupal/contact/CategoryListController.phpundefined
@@ -38,26 +38,33 @@ public function getOperations(EntityInterface $entity) {
+    // @todo Simplify this http://drupal.org/node/1876718
+    return array_slice($row, 0, 1, TRUE) + array(
+      'default' => t('Default'),
+      'recipients' => t('Recipients'),
+    ) + array_slice($row, 0, NULL, TRUE);

still depends on it

#66: 1855402-sort-config-66.patch queued for re-testing.

re-roll

Related issues #2027117: Always use parent::buildRow($entity) in EntityListController and #2019071: EntityListController::buildRow() should return secure label

reroll

Fix tests

patch

Remove debug

Render needed to alter properly

Fix broken tests

Fix taxonomy test

#84: 1855402-sort-config-84.patch queued for re-testing.

merge HEAD

@@ -60,4 +82,124 @@ public function getOperations(EntityInterface $entity) {
+    // @todo Simplify this http://drupal.org/node/1876718
+    return $row = array_slice($row, 0, 2, TRUE) + array(
+      'weight' => t('Weight'),
+    ) + array_slice($row, 0, NULL, TRUE);

I still don't see how this is reasonable. If EntityListController changes its header, we'll have plenty of other problems. As a base class, why not just dictate the header and row data?

@@ -60,4 +82,124 @@ public function getOperations(EntityInterface $entity) {
+      '#markup' => check_plain($row['label']),

Double check_plain'd now.

@@ -60,4 +82,124 @@ public function getOperations(EntityInterface $entity) {
+    return drupal_get_form($this);

Are we sure we want to go this direction, and not just have a separate base class? This certainly convolutes this class.

@@ -44,33 +45,33 @@ public function getOperations(EntityInterface $entity) {
+    // @todo Simplify this http://drupal.org/node/1876718
+    return array_slice($row, 0, 1, TRUE) + array(
+      'recipients' => t('Recipients'),
+      'selected' => t('Selected'),
+    ) + array_slice($row, 0, NULL, TRUE);

This seems more reasonable, since we *don't* want to overwrite the parent.

Fixed double escape after

@tim.plunkett I've no idea how to implement this in base class, also please point the files you are referring in dreditor

+++ b/core/lib/Drupal/Core/Config/Entity/ConfigEntityListController.php
@@ -9,14 +9,36 @@
+    // Check if the entity type supports weighting.
+    if (!empty($this->entityInfo['entity_keys']['weight'])) {

The change applies to config entities only, so this is a base class that dictates behavior

Not all config entities need tabledrag. And some content entities do.

The 1st and 3rd hunks are both in ConfigEntityListController, the 2nd you fixed and the 4th was an observation.

I think it should be a separate class, WeightedConfigEntityListController or something.

#2064557: Improve strange coupling in EntityListControllers by improving buildRow() and buildHeader() will make this code way way way simpler.

Working on this.

Since #2064557: Improve strange coupling in EntityListControllers by improving buildRow() and buildHeader() went in, this didn't apply at all.

I also took a different approach, in order to keep ConfigEntityListController as is.

Oops!

Also, I'm not expecting this to pass the first time, I'll update for any failures.

+++ b/core/modules/editor/editor.module
@@ -156,19 +156,19 @@ function editor_menu() {
-  foreach (element_children($form['formats']) as $format_id) {
+  foreach (element_children($form['entities']) as $format_id) {

+++ b/core/modules/taxonomy/lib/Drupal/taxonomy/Tests/VocabularyTest.php
@@ -90,7 +90,7 @@ function testTaxonomyAdminChangingWeights() {
-      $edit['vocabularies[' . $key . '][weight]'] = $weight;
+      $edit['entities[' . $key . '][weight]'] = $weight;

If we're really concerned about backward compatibility, I can introduce a key like DefaultPluginBag::$pluginKey has, something like DraggableListController::$entitiesKey, set the default to 'entities', and let these two override it...

Yep, #95 works nicely and should resolve most if not all of those failures.

Looks awesome!

--- a/core/lib/Drupal/Core/Config/Entity/DraggableListController.php
+++ b/core/lib/Drupal/Core/Config/Entity/DraggableListController.php

@@ -91,15 +98,12 @@ public function buildForm(array $form, array &$form_state) {
-    // Save header's order of columns for sorting data-rows.
-    $header_sort = array_flip(array_keys($form['entities']['#header']));
...
-      // Sort row columns by header's order.
-      $form['entities'][$entity->id()] = array_merge($header_sort, $row);

This needs follow-up at least to display only columns that have header (allowing to re-sort table columns by weight too)

I only tested the language list without locale on, whoops.

Not sure what you mean in #98, that seems out of scope.

Yep, follow-up in #1876718: Allow modules to inject columns into tables more easily
I found no nitpicks with code

+++ b/core/lib/Drupal/Core/Config/Entity/DraggableListController.php
@@ -0,0 +1,147 @@
+    // Check if the entity type supports weighting.
...
+      $this->weightKey = $this->entityInfo['entity_keys']['weight'];
+    }
...
+      if (isset($this->entities[$id]) && $this->entities[$id]->get($this->weightKey) != $value['weight']) {
...
+        $this->entities[$id]->set($this->weightKey, $value['weight']);

Couldn't we just document that entities using this need a weight key? I like the split classes but it's sad to have all those checks in there, as without a weight key the whole class is pointless.

Also it seems $this->weightKey is already used unconditionally in submitForm() so...

Since this has been sitting for so long, I'm not downgrading this. I'm fine with doing this as a follow-up. I couldn't find anything substantially wrong with this patch and my proposal would merely be a minor code clean-up not actual technical debt in any sense of the word.

We can't and shouldn't assume that the key will be called weight, it could be order or position.
Also, submitForm() is only called if the weightKey is set during render(), hence the lack of check for it.

Ahh, I totally missed render(). Yes, that makes sense.

But I still think we should just remove the checks. No, we can't enforce that weightKey == 'weight', but IMO we can enforce that !empty(weightKey) == TRUE. Then we just set $this->weightKey in __construct() and be done with it.

And what if weightKey isn't set? Random notices? A thrown exception (of what type)?

Also, VocabularyListController has the pre-existing functionality of NOT being a draggable form when there is only one vocab.
It relies on these weightKey checks to function.

Re #105: Since the DraggableListController is explicitly opt-in I think a "Undefined index 'weightKey'" would actually be quite helpful.

Re #106: I think that should be handled generically in #1016056: Hide tabledrag handle and "Show row weights" when there is only one item in list

But apparently my suggestion is more controversial than I had anticipated so I opened a follow-up issue. Let's discuss there and finally get this one in:
#2070621: Make the existence of a weight key mandatory in DraggableListController

Sure, #1016056: Hide tabledrag handle and "Show row weights" when there is only one item in list can block #2070621: Make the existence of a weight key mandatory in DraggableListController, that's fine.

Committed 9e01ce3 and pushed to 8.x. Thanks!

This should probably get a change notice? I can use this in contrib modules and others probably too.

Using $this->entities seems to be pretty weird here? That means we serialize all of them and then save the version that was previously serialized, so that could be an older version that was already changed in the meantime. Wouldn't be hard to just load them again? This was introduced in #100, without further discussion on it... I see that it's also used in one other method, that probably makes sense there, but not so sure about the submit method.

About $entities in submit - each time when submit should be called the build happens, there's no serialization
By the same time 100 introduces

+++ b/core/modules/language/lib/Drupal/language/LanguageListController.php
@@ -81,6 +81,7 @@ public function buildRow(EntityInterface $entity) {
+    $form[$this->entitiesKey]['#languages'] = $this->entities;

so we have 2 lists of languages and probably other types affected.

PS: seems there's a room for follow-up to clean-up things for better contrib DX ("entities" stored in form array could be changed via form alter so we need a competent list of entities)

Filed follow-up #2074875: Reload entities in DraggableListController::submitForm()

Taxonomy page should be updated when there's only one vocabulary to hide weight header column

#2083415: [META] Write up all outstanding change notices before release

https://drupal.org/node/2089283

Looks fine, added a bit example code as an entity type always needs a custom subclass. Also changing the issue title to contain the name of the class that was added.