Updated: Comment #0

Problem/Motivation

\Drupal\Core\Config\Entity\DraggableListController contains numerous checks such as if (!empty($this->entityInfo['entity_keys']['weight'])) {* and falls back to the parent (\Drupal\Core\Config\Entity\ConfigEntityListController) if the condition is FALSE.

Because DraggableListController is explicitly opt-in (i.e. not the default) we should enforce a weight key. Without a weight key DraggableListController is identical to ConfigEntityListController, so the whole class becomes pointless. Conceptually this makes sense, as well: In order for a list to be draggable, you need someplace to store the order (== weights) of the respective rows.

* This specific check is in fact done only once in DraggableListController, but in that check $this->weightKey is set conditionally and then that is checked in the rest of the class.

Proposed resolution

1. Document in the class PHPDoc that a 'weight' key is required.
2. Remove the checks for the weight key.

Remaining tasks

Discuss whether failing with a notice (e.g. "Undefined index 'weight' in line ...") or with a proper Exception ("You must declare a weight key in order to use DraggableListController) is better.

API changes

DraggableListController is only usable for entities with a 'weight' key. It's already pointless to do so, though, so that API change is only theoretical.

DraggableListController is being introduced in #1855402: Add generic weighting (tabledrag) support for config entities (DraggableListController) on which this issue is currently postponed.

Files: 
CommentFileSizeAuthor
#6 2070621-6-draggable-list-weight-required.patch5.1 KBtstoeckler
FAILED: [[SimpleTest]]: [MySQL] 58,073 pass(es), 53 fail(s), and 789 exception(s). View
#6 interdiff-3-6.txt1.68 KBtstoeckler
#3 2070621-3-draggable-list-weight-required.patch3.29 KBtstoeckler
FAILED: [[SimpleTest]]: [MySQL] 58,455 pass(es), 1 fail(s), and 30 exception(s). View
#3 2070621-3-draggable-list-weight-required_for-review_do-not-test.patch2.52 KBtstoeckler

Comments

tstoeckler’s picture

Re-posting the relevant discussion from the parent issue:

#102 tstoeckler:

+++ 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.

#103 timplunkett:

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.

#104 tstoeckler:

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.

#105 timplunkett:

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

#107 tstoeckler:

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

tstoeckler’s picture

Assigned: Unassigned » tstoeckler
Status: Postponed » Active

The parent is in (yay!), so we can do this now.

Having thought about this, I think throwing an exception on an undefined weight key actually makes a lot of sense, IMO.

Will work on this.

tstoeckler’s picture

Status: Active » Needs review
FileSize
2.52 KB
3.29 KB
FAILED: [[SimpleTest]]: [MySQL] 58,455 pass(es), 1 fail(s), and 30 exception(s). View

Here we go. Also attaching a version with git diff -w.

Status: Needs review » Needs work

The last submitted patch, 2070621-3-draggable-list-weight-required.patch, failed testing.

tim.plunkett’s picture

Title: Make the existance of a weight key mandatory in DraggableListController » Make the existence of a weight key mandatory in DraggableListController
Component: base system » configuration entity system
Issue tags: +Configuration system, +Configurables
tstoeckler’s picture

Status: Needs work » Needs review
FileSize
1.68 KB
5.1 KB
FAILED: [[SimpleTest]]: [MySQL] 58,073 pass(es), 53 fail(s), and 789 exception(s). View

Yeah, I now realize why you brought up VocabularyListController. :-)

Also thanks @tim.plunkett for following this after I got on your nerves with it. Very much appreciated.

This patch removes the one-off for the vocabulary list to disable the weighting if there's only one vocabulary. No other draggable table (that I'm aware of) does this and we already have a follow-up issue to generalize the capability. So technically, this is a usability regression but it also removes a one-off, both in code and in the interface. I don't think that having only one vocabulary is much more common than having only image effect, one term in a vocabulary, ...

Also the change in EntityListController is technically not necessary, but I had fixed this to get rid of some notices during debugging, and it's an improvement, so...

Status: Needs review » Needs work

The last submitted patch, 2070621-6-draggable-list-weight-required.patch, failed testing.

tim.plunkett’s picture

I'm not sure we should purposefully cause a usability regression when the issue to fix it has no momentum.

andypost’s picture

Suppose we should unify entity_ids storage for the forms and have helper method ConfigListController::isSortable()

tim.plunkett’s picture

What does sorting have to do with dragging?

andypost’s picture

if entity isSortable then draggable list controller applies automatically

tim.plunkett’s picture

That is out of scope. Like, completely. isSortable() doesn't even exist yet, and is not related to dragging.

tstoeckler’s picture

Assigned: tstoeckler » Unassigned
tim.plunkett’s picture

Status: Needs work » Closed (won't fix)

I'm going to close this, we still depend on this for VocabularyListBuilder, who knows who else is in contrib/custom.