Updated: Comment #0

Problem/Motivation

In many implementations of list controllers, the buildHeader() and buildRow() go to ridiculous lengths to reuse the parent implementation.

Take this example from \Drupal\user\RoleListController:

  /**
   * {@inheritdoc}
   */
  public function buildHeader() {
    $row = parent::buildHeader();
    $row['label'] = t('Name');
    unset($row['id']);
    $row['weight'] = t('Weight');
    return $row;
  }

This is only slightly better in buildRow(), this from ImageStyleListController:

  /**
   * {@inheritdoc}
   */
  public function buildRow(EntityInterface $entity) {
    $row = parent::buildRow($entity);
    unset($row['id']);
    return $row;
  }

This makes any upstream changes harder to make.

Proposed resolution

Make it easier to reuse the generic buildHeader() and buildRow() implementation by only returning 'operation'.
This is the only key that is used across the board, and it is always last.

This way you could build your row and then do

return $row + parent::buildRow();

without fear of clashes or need to unset anything.

Remaining tasks

Write a patch

User interface changes

N/A

API changes

No true API change, but expectations need to be adjusted

CommentFileSizeAuthor
#12 list-2064557-12.patch28.1 KBtim.plunkett
#10 interdiff.txt514 bytestim.plunkett
#10 list-2064557-10.patch28.2 KBtim.plunkett
#8 list-2064557-8.patch28.19 KBtim.plunkett
#8 interdiff.txt2.05 KBtim.plunkett
#6 list-2064557-6.patch26.87 KBtim.plunkett
#6 interdiff.txt1.07 KBtim.plunkett
#5 list-2064557-5.patch25.8 KBtim.plunkett
#5 interdiff.txt22.24 KBtim.plunkett
#1 list-2064557-1.patch14.45 KBtim.plunkett

Comments

tim.plunkett’s picture

tim.plunkett
he/him
Primary language English
Location Philadelphia
commented
Status: Active » Needs review
StatusFileSize
new list-2064557-1.patch14.45 KB

Yes, this requires the list controllers to do $row['label'] = String::checkPlain($entity->label());, but it prevents us from having to do stuff like this to preserve ordering:

return $row = array_slice($row, 0, 2, TRUE) + array(
  'weight' => t('Weight'),
) + array_slice($row, 0, NULL, TRUE);

#1855402: Add generic weighting (tabledrag) support for config entities (DraggableListController) will be able to just add modify buildRow and buildHeader, and it will Just Work™.

andypost’s picture

andypost
he/him
Primary language Russian
commented

Suppose better get rid of local variables in favour of plain array()

  1. @@ -127,8 +127,6 @@ public function getOperations(EntityInterface $entity) {
   public function buildHeader() {
-    $row['label'] = t('Label');
-    $row['id'] = t('Machine name');
     $row['operations'] = t('Operations');
     return $row;

    why not return array('operations' => t('Operations')) 

  2. @@ -145,10 +143,7 @@ public function buildHeader() {
+    $row['operations'] = $this->buildOperations($entity);
     return $row;

    same

tim.plunkett’s picture

tim.plunkett
he/him
Primary language English
Location Philadelphia
commented

Core does both in different places. Definitely on multiple lines either way, for adding stuff later.
I don't care either way, I'm going to wait for the test results and more reviews before changing.

Status: Needs review » Needs work

The last submitted patch, list-2064557-1.patch, failed testing.

tim.plunkett’s picture

tim.plunkett
he/him
Primary language English
Location Philadelphia
commented
Status: Needs work » Needs review
StatusFileSize
new interdiff.txt22.24 KB
new list-2064557-5.patch25.8 KB

I'm going to leave it as is, no reason to make the patch bigger.

tim.plunkett’s picture

tim.plunkett
he/him
Primary language English
Location Philadelphia
commented
StatusFileSize
new interdiff.txt1.07 KB
new list-2064557-6.patch26.87 KB

Missed a spot.

Since this now covers the intention of #2027117: Always use parent::buildRow($entity) in EntityListController, I'm marking that as a duplicate (even though it has a completely opposite approach).

Status: Needs review » Needs work

The last submitted patch, list-2064557-6.patch, failed testing.

tim.plunkett’s picture

tim.plunkett
he/him
Primary language English
Location Philadelphia
commented
Status: Needs work » Needs review
StatusFileSize
new interdiff.txt2.05 KB
new list-2064557-8.patch28.19 KB

Fixes for contact and picture.

andypost’s picture

andypost
he/him
Primary language Russian
commented 
  1. @@ -92,6 +92,19 @@ public function load() {
+   * Returns the escaped title of an entity.
...
+  protected function getLabel(EntityInterface $entity) {

    I think better replace title with label in doc-block too 

  2. @@ -21,7 +21,7 @@
- *     "list" = "Drupal\Core\Config\Entity\ConfigEntityListController",
+ *     "list" = "Drupal\config_test\ConfigTestListController",

    So all entities must implement their own list controller? 

  3. @@ -84,6 +84,7 @@ public function load() {
+    $row = parent::buildRow($view);

@@ -96,9 +97,7 @@ public function buildRow(EntityInterface $view) {
+        'operations' => $row['operations'],

    Suppose this should be:

    return array(
'data' => array(
...
) + parent::buildForm($view),
...
);
tim.plunkett’s picture

tim.plunkett
he/him
Primary language English
Location Philadelphia
commented
StatusFileSize
new list-2064557-10.patch28.2 KB
new interdiff.txt514 bytes

1) Sure thing

2) Yes. I see no real way around it. Also the only 3 that didn't have them were two test entity types and Picture Mapping, which isn't actually used in core... So in reality everyone was overriding this anyway.

3) No, the array structure here is pretty complex already, I don't think we should overcomplicate it. Plus it's the Views UI, which was very very specific in D7 already.

andypost’s picture

andypost
he/him
Primary language Russian
commented
Status: Needs review » Reviewed & tested by the community

Makes sense, so let's get commiter's feedback (supposing patch is green)

tim.plunkett’s picture

tim.plunkett
he/him
Primary language English
Location Philadelphia
commented
StatusFileSize
new list-2064557-12.patch28.1 KB

Rerolled for the Plugin/Core/Entity move

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented
Status: Reviewed & tested by the community » Fixed

Committed d663924 and pushed to 8.x. Thanks!

Status: Fixed » Closed (fixed)

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