DraggableListBuilder / DraggableListBuilderTrait need to document that buildRow() behaves completely differently from the parent class

Actually, looking at this some more, I wonder if this is just a downright WTF in DraggableListBuilderTrait which we should fix before DraggableListBuilderTrait makes it to a stable release.

Functionally, child render elements in the table are the same as #rows -- \Drupal\Core\Render\Element\Table::preRenderTable() converts child render elements into #rows.

Sounds like a WTF. What’s the proposed change to the API to make it more consistent? I don’t think this should be fixed by docs alone, but I’m just reading the summary on a phone, not studying the code.

> I don’t think this should be fixed by docs alone, but I’m just reading the summary on a phone, not studying the code.

I agree -- I thought it was docs problem at first, wrote up the issue, then realised that since DraggableListBuilderTrait is new we have the opportunity to fix it. Though DraggableListBuilder would need BC handling.

The fix would be to change DraggableListBuilderTrait to set the result of buildRow() as #data rather than child elements.

I know that some times work as #data that don't work as child elements, but I don't know if the reverse is also true. In other words, does one method have features that are a subset of the other, or are they intersection features?

This is going to cause problems with modules that might want to start using this.

For example, ✨ use DraggableListBuilder Active -- Environment Indicator module has weighted entities, and should really switch to using a draggable list. But it's currently returning rows which expect to go into #data -- for example, there's a #style attribute on the entire row. That totally fails to work when they're used as render elements.

Are you certain any of that is really due to the *trait*?

What I mean is that I'm fairly certain that all of this logic isn't new, it "always" existed in DraggableListBuilder, we just made it reusable with the trait now for non-config entity types. But we can't change it, because existing entity types already relied on the behaviour as it is.

Which means that this already is in a stable release and is at best a documentation issue to explain that better.

The fix would be to change DraggableListBuilderTrait to set the result of buildRow() as #data rather than child elements.

Completely disagree. If we do want to change anything here, it should be the other way around. In fact, originally when '#type' => 'table' was introduced the plan was to convert all tables over to using child elements instead of '#rows'. That never happened, but the fact that most tables in core use '#rows' is simply a remnant of them previously using '#theme' => 'table' instead of '#type' => 'table' before the latter existed, not that using child elements is somehow a problem.

The reason that using child elements is in fact preferable is that as child elements they actually proper participate in form building, and can, in particular, have '#process' callbacks. Using '#rows' bypasses the form handling and just goes through the render system. On the other hand, there is nothing that using '#rows' brings over using child elements.

In particular,

for example, there's a #style attribute on the entire row. That totally fails to work when they're used as render elements.

is completely untrue, as far as I can tell. In fact DraggableListBuilderTrait itself sets the draggable class on the entire row. I don't know what you mean by a "#style attribute" but you can certainly set '#attributes' => ['style' => $style] on the row element, if you want to do that for whatever reason.

Thanks for the explanation and the history @tstoeckler! :) I've wondered why there's both theme=>table and type=>table which are subtly different!

> If we do want to change anything here, it should be the other way around.

Works for me! I should have emphasised the need for consistency being the important thing.

> I don't know what you mean by a "#style attribute" but you can certainly set '#attributes' => ['style' => $style] on the row element, if you want to do that for whatever reason.

Aha! Yes, got it working (the use case is environment_indicator module which wants to colour each row for the environment entity's colour). Thanks for the tip.

Ok, so I suggest we keep this issue about documentating the difference, and open a new issue to think about making the two list builders consistent?

Glad that you got it to work, nice.

Sounds like a good plan regarding the issues.

Filed 📌 standardize DraggableListBuilder / DraggableListBuilderTrait and ConfigEntityListBuilder's handling of rows returned by buildRows() Active .