Allow modules to inject columns into tables more easily

I came up with similar options @sun in the first iterations of #1833716: WYSIWYG: Introduce "Text editors" as part of filter format configuration (when we were manually injecting columns before the #type = table). I went with option #2 in the early versions of that patch.

I prefer the #2 approach for multiple reasons:
- Finding a header key is no longer dependent on a translated string (since it will now have a machine key).
- The header key having a consistent name means it's easy to find and other modules could easily change the translated string for a header column. i.e. $form['table']['#header']['title'] = t('Some other title');.
- The named approach makes it easy to handle situations where you either want your column to always be in a static position (say the second column) or when you want your column to always be after/before another column.

Even though array_slice() is probably the more efficient way to handle slotting in a column, with the second approach developers may be more comfortable with a significantly easier to understand typical foreach() loop.

$new_header = array();
foreach($element['#header'] as $key => $label) {
  $new_header[$key] = $label;
  if ($key === 'title') {
    $new_header['my_new_column'] = t('My new column');
  }
}
$element['#header'] = $new_header;

Here's alternative, more-efficient (but more confusing) code for splicing an item into the header. Assuming we went with option 2. @sun might be able to come up with something more readable or further efficient. Given the choice, I'd probably fallback to the "easy" approach using the foreach loop().

$position = array_search('name', array_keys($element['#header'])) + 1;
$start = array_splice($element['#header'], 0, $position);
$element['#header'] = array_merge($start, array('my_new_column' => t('My new column')), $element['#header']);

+    // Sort columns by #header.
+    $element[$first]['#sorted'] = FALSE;
+    $weight = 0;
+    foreach ($element['#header'] as $key => $value) {
+      if (is_string($key) && isset($element[$first][$key])) {
+        $element[$first][$key]['#weight'] = $weight = $weight + 10;
+      }
+    }

This is an interesting approach that I wouldn't have thought of. My attempt would have been to use the keys from the $element['#header'] array to determine the order, rather than using element_children() to resort the columns. However the downside of that approach (but solved by the one in the patch) is that elements would have required a matching key in the header. This approach doesn't require a matching key, which should be useful in situations where the table doesn't have a header at all.

However if the developer were to make a mistake and skip a key, or a column was added without a header, the new column would end up in an unpredictable location, possibly throwing off the column matching.

Additionally, if the columns have weights already, the order could come out different than intended also.

Both of these seem like minor inconveniences, outweighed by the ability to easily inject a new column. In *every* place I've ever done a table like this, the column order is established by definition order (that is, they always have a weight of 0). Tables are always screwy if you don't have enough headers to match the columns and shouldn't be allowed anyway. Unless there are other objections, this looks good to go.

I am for mandating matching keys, and mandating a #header structure, even if we can give the option not to display it (probably something along the lines of #title_display).

Regarding existing #weight properties on columns, yeah, those would be unconditionally overwritten, but in the end, that's the entire point of mapping the order from #header into table body row columns, right?

Ah, you're correct. I read this line incorrectly:

$element[$first][$key]['#weight'] = $weight = $weight + 10;

I read it as:

$element[$first][$key]['#weight'] = $weight = $element[$first][$key]['#weight'] + 10;

Which of course doesn't make any sense. I thought the existing #weight property was factoring in somehow, but it's not. Works for me.

One more thing to consider, what effect does this have on use of colspan? Prior to having the Operations column use a dropbutton, using a colspan of 3-4 was common on the operations column. However if you have 3 columns at the end, they obviously can't all have the same parent key in the $element['#table'] array, so the header column won't be able to group all of them together.

Patch so far looks good to me. I don't have an opinion yet on latest colspan discussion.

I totally like the idea of mapping columns to header keys, because of its intuitiveness in declaring, i.e. I can do
$form['table'][5]['title'] = 'Foo'; and have have 'Foo' actually appear in the correct column. I don't understand, though, why we have given up on weights totally. We already need to support to specify header elements as arrays for anything non-trivial, so from the declaration perspective, I would find it weird for the following not to work:

$form['table']['#header'] = array(
  'operations' => array('data' => t('Operations'), 'weight' => 10),
  'title' => array('data' => t('Title'), 'weight' => 5),
);

Above the argument was made that you cannot possibly find default values for weights that are flexible enough for each possible use-case. But that is already the case with regular form elements, as well. It is the most common thing in the world if module B alters module A's form that it has to adjust the form elements' weights to make everything appear in the correct order. In core we can just go and change the original weight directly, but in contrib this is extremely common. And, especially think for things like 'operations' and tabledrag's 'weight' I do think we can find defaults that satisfy the 80% use-case. I definitely like setting a couple weights a lot more than array_splice()ing, even if it's just the header. And, again, I have never heard anyone argue for the same thing on the top-level $form level. Neither have I ever seen anyone array_splice()ing the whole $form. So I don't understand why here we are settling for something that seems sub-optimal from the start.

For supporting colspans I would suggest #weight as well. For example: (I know we don't do operations like this anymore, but bear with me)

$form['table']['#header']  = array(
  'title' => 'Title',
  'operations' => array('data' => 'Operations', 'colspan' => 2),
);
$form['table'][0]['title']['#markup'] = 'Foo';
$form['table'][0]['operations'] = array(
  'delete' => array(
    '#markup' => 'Delete',
    '#weight' => 10,
  ),
  'edit' => array(
    '#markup' => 'Edit',
    '#weight' => 5,
  ),
);

Of course, if you don't specify any weights, the declaration order would simply be taken over.

I think that would be the most intuitive way to declare this. I don't find specifying sub-keys in the header that great, but if we do want to do that, I think it should be explicitly declared instead of implied by additional array nesting. I.e. I would vote for @sun's second option (only that I would leave 'colspan' as is and name this bikeshed('sub-keys')).

@tstoeckler: You're second example uses an interesting but non-traditional way of declaring the operations column, because it defines multiple items inside what would normally be considered a single cell. If we adopted that structure officially, you wouldn't really need to specify colspan in the #header at all, because it would be apparent how big it needs to be based on the number of "subcells" that are specified in the matching key. Then we could keep our #header declaration simple to declare and just deduce that information from the matching key. However this would mean the logic for theme_table() would become even *more* messy, retroactively adjusting the header colspan based on the contents of cells.

Well, I think if we do go with the concept of 'named columns' (which, to be clear, I totally think we should), I think the following would be extra weird:

$form['table']['#header']  = array(
  'title' => 'Title',
  'operations' => array('data' => 'Operations', 'colspan' => 2),
);
$form['table'][0]['title']['#markup'] = 'Foo';
$form['table'][0][] = array(
  '#markup' => 'Delete',
  '#weight' => 10,
);
$form['table'][0][] = array(
  '#markup' => 'Edit',
  '#weight' => 5,
);

I don't really see how we can reasonably place all elements in the same array level in the case of colspans.

Thinking about that, the example code in #12 might actually turn into a huge DX WTF on its own, because element_children($form['table'][0]) returns things which are maybe table cells, but not necessarily.

Hmm...

If you'd replace/_alter that #type from 'colspan' into 'dropbutton'/'operations', then the 'operations' column turns into a single and there's no more colspan involved.

Is this what we're doing already for operations (a #type = 'dropbutton')? If so, this solution looks ideal, that you could switch between using a dropbutton or multiple columns simply by changing a single word, and the structure of the form continues to stay the same between them. This approach has great advantages from an "alterability" standpoint. It's easy not only to add an additional "normal" column, but it's also easy to add into an operations column and have the header colspan automatically adjust.

You can NOT have each operation in its own column (i.e., no colspan). At least not without changing the structure of all rows, and manually defining the new columns in #header.

That sounds like a fine trade-off. And it's still possible, just difficult. Considering dropbuttons are already what we're using in most places and we already have this problem, this isn't introducing a new problem (just the same problem in a different format). I expect it to be uncommon enough it doesn't need any further consideration.

So, +1 #15. I'd love to have it.

We got away with array_splice() and manual changes in #1833716: WYSIWYG: Introduce "Text editors" as part of filter format configuration. So editor.module is another place in core that we can "fix" in this issue's patch.

This may have impact on this issue:
#1948374: #type 'table' allow attributes on table cells

#5: drupal8.table-column-sort.5.patch queued for re-testing.

In the above, the 'Operations' header column automatically gets a 'colspan' => 2 assigned to it, since the 'operations' column is of #type 'colspan' and contains two child elements.

Look really bad that #header depends on data columns.

I'd prefer more strict as #7 suggests! the only questionable part - should we drop data cells that does not have keys in #header?

#5: drupal8.table-column-sort.5.patch queued for re-testing.

Looks like the patch no longer applies:

error: core/modules/filter/filter.admin.inc: No such file or directory

I was looking through the remaining @todos for editor.module and leveraging the changes of this issue to simplify some code is one of them.

Can this still happen in D8? Or is it D9 material?

This issue is about approach and DX but there's no conclusion, suppose we should fix this in D8

Updated issue summary.

We worked around this so far, except editor.module.

Reroll.

Changes in filter.admin.inc was overridden by #1987124: Convert filter_admin_format_page() and filter_admin_overview() to a Controller in 93ea9a472b988ac8d1aa56f947bc916c24f8ea73, so doesn't included.

#30 didn't have a patch

Thank you for creating this issue to improve Drupal.

We are working to decide if this task is still relevant to a currently supported version of Drupal. There hasn't been any discussion here for over 8 years which suggests that this has either been implemented or is no longer relevant. Your thoughts on this will allow a decision to be made.

Since we need more information to move forward with this issue, the status is now Postponed (maintainer needs more info). If we don't receive additional information to help with the issue, it may be closed after three months.

Thanks!

This looks like a valid feature request still.