Extract DraggableListBuilderTrait from DraggableListBuilder for use with content entities

How is that going to work when subclasses need to

+++ b/core/lib/Drupal/Core/Entity/DraggableListBuilderTrait.php
@@ -0,0 +1,183 @@
+   * Name of the entity's weight field or FALSE if no field is provided.

Does it make sense to allow FALSE here?

Thank you tstoeckler, this is a great idea.

I first reviewed #4, and had the thought of "but wait, $this->weightKey isn't actually used".
I do prefer the approach of #2, but realize that it's too risky. Even Commerce uses the unset($this->weightKey) trick which caused the VocabularyListBuilder breakage.

My only remaining question is here:

+  protected function formBuilder() {
+    if (!$this->formBuilder) {
+      $this->formBuilder = \Drupal::formBuilder();
+    }
+    return $this->formBuilder;
+  }

Is it odd for a trait to reach into \Drupal?
If this was a Commerce trait I'd just expect the parent class to set $this->formBuilder in the constructor, with no fallback or method, but core is usually more tolerant.

+++ b/core/lib/Drupal/Core/Config/Entity/DraggableListBuilder.php
@@ -61,123 +35,17 @@ public function __construct(EntityTypeInterface $entity_type, EntityStorageInter
+  protected function setWeight(EntityInterface $entity, $weight) {
+    /* @var \Drupal\Core\Config\Entity\ConfigEntityInterface $entity */
+    $entity->set($this->weightKey, $weight);
   }

Maybe return $this in setter?

+1 to this issue overall.

+1 to #7. The cost of possible BC breakage outweighs the tech debt of keeping this API.

+1 to #8. I agree that if you use this trait, you should properly set $this->formBuilder in your constructor via good DI, not this. However, the existing code "does it wrong", so I'm not sure if fixing this is scope creep here. I'd probably just try to fix it now, but maybe a follow-up is better.

+1 to #9.

Thanks,
-Derek

#8 I went with the smallest change possible to make it easier to get this issue committed.

#9 The setter now does return $this

Fixed code style

LGTM, though follow-ups need to be filed for the various suggestions for follow-ups in the comments.

Back to RTBC per #18 after #3255836: Test fails due to Composer 2.2 solved the unrelated test failure.

+1 to release.

1. +++ b/core/lib/Drupal/Core/Entity/DraggableListBuilderTrait.php
   @@ -0,0 +1,185 @@
   +  abstract protected function getWeight(EntityInterface $entity);
   

   Can use a return tyephint.

2. +++ b/core/lib/Drupal/Core/Entity/DraggableListBuilderTrait.php
   @@ -0,0 +1,185 @@
   +  abstract protected function setWeight(EntityInterface $entity, $weight);
   

   We can and should use scalar typehints - plus we can use return typehints too ...not sure what it would be.

3. +++ b/core/lib/Drupal/Core/Entity/DraggableListBuilderTrait.php
   @@ -0,0 +1,185 @@
   +  /**
   +   * {@inheritdoc}
   +   */
   +  public function buildHeader() {
   ...
   +  /**
   +   * {@inheritdoc}
   +   */
   +  public function buildRow(EntityInterface $entity) {
   ...
   +  /**
   +   * {@inheritdoc}
   +   */
   +  public function render() {
   

   These inheritdocs are from \Drupal\Core\Entity\EntityListBuilder and it's interface

4. +++ b/core/lib/Drupal/Core/Entity/DraggableListBuilderTrait.php
   @@ -0,0 +1,185 @@
   +  /**
   +   * {@inheritdoc}
   +   */
   +  public function buildForm(array $form, FormStateInterface $form_state) {
   ...
   +  /**
   +   * {@inheritdoc}
   +   */
   +  public function validateForm(array &$form, FormStateInterface $form_state) {
   ...
   +  /**
   +   * {@inheritdoc}
   +   */
   +  public function submitForm(array &$form, FormStateInterface $form_state) {
   

   These are from \Drupal\Core\Form\FormInterface

   ... not sure what to do but @inheritdoc here is incorrect. We're not inheriting any docs.

Seems points #26 have been addressed in MR 5154. Since this was previously RTBC before that going to restore

Started leaving comments about adding more type hints on the MR, then I realised I should have read #29 properly, of course it will affect subclasses.

Committed a993304 and pushed to 11.x (which will also become 10.3.x). Thanks!

I was going to file a follow-up for the injection of formBuilder, but #3123209: Replace non-test usages of \Drupal::formBuilder() with IoC injection should cover it.

It missing change notice

The commit from #33 expects a return value int|float for the getWeight method in DraggableListBuilder. I just came across a config entity, that has a weight property, but that may be NULL. In that case, this throws an exception.

Should the getWeight method maybe fall back to 0, if the weight property has no value?

Marking RTBC for the CR. Would like a committer to also see before publishing it.

CR looks great. Publishing.

> The commit from #33 expects a return value int|float for the getWeight method in DraggableListBuilder. I just came across a config entity, that has a weight property, but that may be NULL. In that case, this throws an exception.

Similar issue -- I've just tried to use DraggableListBuilder on a config entity, and got a crash because the weight is a string not an int.

Follow-up -- and I wonder whether we should fix this rather than document it: #3425848: DraggableListBuilder / DraggableListBuilderTrait need to document that buildRow() behaves completely differently from the parent class.

Ah yeah, the bug is that environment_indicator has declared this incorrectly:

    weight:
      type: string
      label: 'Name'

> But I guess it wouldn't hurt to add a (float) type-cast to getWeight(). I can open a follow-up for that, if people think that makes sense.

On second thoughts, we shouldn't babysit that, but we should instead add something to the trait docs to say the weight field must be an int.