Allow table row groups in table.html.twig and template_preprocess_table()

LAsan, careful on setting the status. This isn't fixed.

This should be combined with column groups. http://drupal.org/node/239071

The question is how to implement this without making it too complicated.

That one has been commited, so we should work on this issue separately now.

Subscribing. Too bad we didn't get this in in D7.

Well, if you provide the patch *now*, it can get in D8 very early :)

Would be nice to have a use case of the $row_groups in Drupal core when this goes in. Maybe apply it to the permissions page in this patch? I'm all for this going into Drupal 7 if it means cleaning up user_admin_permissions() and theme_user_admin_permissions().

#125099: theme_table() should accept attributes for the THEAD and TBODY tags and header TR tag
Patch allows attributes for tbody.

Also implemented rowgroups for admin user permissions, which is a good excuse to get this in D7.

Example why this is usefull: #793478: Add instantfilter to permissions configuration form.

Reroll.

#12: theme_table.patch queued for re-testing.

...subscribing.

Now that we have editable issue summaries, I updated it a bit by removing the mention of the optional end tbody tag as per #1 above.

@casey: Care to also explain what the patch does, what neds testing/review and perhaps update the issue summary too? Thanx

...removed the comment on optional end tag of

...edited the summary once more to add the template from Issue summary standards. I left blank the sections where I cannot possibly know what needs to be added there. Hopefully this is enough help to get one started ;)

...conforming to the Issue summary standards.

Let me try to give some direction here.

1) This addition adds even more insanity to the already insane theme_table(). No themer on this earth is going to be able to override theme_table() any longer. I'd like to see the entire table structure (especially regarding colgroups and rowgroups) prepared in a template_preprocess_table() function (preprocessors are no longer limited to templates since D7). The complex code logic needs to be hidden away from themers. The resulting theme function should only (and always) get rows and columns in groups, essentially decreasing the function body to a couple of simple foreach()s in the ideal case.

2) "rowgroups" is a kinda strange variable name. The existing "colgroups" at least maps to more or less direct equivalent HTML tags. "rowgroups", however, is converted and translated into "tbody" tags.

3) We need to be careful to not break tableDrag with this change; or in other words: tableDrag potentially needs to be updated to work with rowgroups, too.

4) Speaking of, it seems like this patch could and possibly should simplify the existing "row grouping" implementations we already have in core - on Block module's administrative block overview page, and Field UI's Manage fields as well as Display fields pages.

5) The changes to the user permissions page/table need to be removed from this patch.

6) Existing unit tests for theme_table() need to be enhanced to assert expectations for this functionality.

7) Lastly, we have to discuss whether it wouldn't make more sense to make a more far-reaching change and require the $rows array to always contain groups on the top-level. I.e.,

$rows = array();
$row = array('one', 'two', 'three');
$rows[] = $row;
theme('table', array('rows' => $rows));

would become

$groups = array();
$rows = array();
$row = array('one', 'two', 'three');
$rows[] = $row;
$groups[''] = $rows;
theme('table', array('rows' => $groups));

I think this is NW for sun's feedback in #19.

API changes

We're going to need that for some JS cleanup and very likely for the tabledrag rewrite.

Point 1) from #19 is up for review #1812374: Add preprocess to theme_table function to simplify it.

...formatting link to issue with [#...] so that status is directly visible.

This needs a completely different patch because of #1939008: Convert theme_table() to Twig. Because of this change, the patch can be simpler. There is relevant code in class Table and template_preprocess_table().

I think it is reasonable to use #rowgroups; that maps onto the rowgroup value for the scope attribute. Or it could be #tbodys. It doesn't matter to me.

If both #rows and #rowgroups are set, the template could display the rows from #rows then those from #rowgroups to keep backward compatibility.

I propose that the value of #rowgroups be like this:

#rowgroups = [
  [
    'rows' => $rows, // Same as #rows.
    ..., // Attributes to put on the tbody.
  ],
  $more_rows_1, // Further rowgroups.
  $more_rows_2,
];

Making the table theming more powerful should be separate from putting the new features to use, such as in the permissions page as suggested above.

If people are agreeable with this proposal, I will start implementing it.

Sorry about "sqlite db driver"; that was unintentional.

There is no patch to review right now; all patches on this issue are pre-Twig and as such, obsolete. What is needed is discussion about the proposed solution.

Does anyone have thoughts about the proposal in #32? I'd love to hear feedback from anyone, in particular, the followers of this ticket.

I just found that I needed this functionality. Yes, #32 sounds like a solid implementation. #rowgroups seems the intuitive element to place it in.

Thanks for this!

Seems good to me too, would #rowgroups have additional properties, like #title or something? if we look at the mockups for the permissions page that this was needed for there is a sort of "label" for the group.

How would the #title be displayed?

Rowgroups can be labelled by a th with scope="rowgroup".

Here is an initial patch implementing #rowgroups. It doesn't update all the table templates that come with core but it is enough to demonstrate the approach. Any rows in #rows are added to be beginning of #rowgroups in their own group. After the rows are processed, #rows is made to contain pointers to all the rows in #rowgroups. So, a template can render either #rowgroups or #rows and it will get all the rows, so it is backwards-compatible with templates that do not support #rowgroups.

Created merge request with patch #42.

(I don't know why comment #44 was generated. I just created an issue fork and it seems to have created a merge request at the same time, before there was any commit to merge.)

The tests failed with "Custom Commands Failed". I don't think this patch uses any custom commands, so I don't think this patch is the cause of the test failure.

The failure is from a spell checker:

/var/www/html/core/includes/theme.inc:916:8 - Unknown word (rowgroups)
/var/www/html/core/includes/theme.inc:921:28 - Unknown word (rowgroups)
/var/www/html/core/includes/theme.inc:922:54 - Unknown word (rowgroups)
/var/www/html/core/includes/theme.inc:923:31 - Unknown word (rowgroups)
/var/www/html/core/modules/system/templates/table.html.twig:21:6 - Unknown word (rowgroups)
/var/www/html/core/modules/system/templates/table.html.twig:76:9 - Unknown word (rowgroups)
/var/www/html/core/modules/system/templates/table.html.twig:77:24 - Unknown word (rowgroups)
/var/www/html/core/themes/seven/templates/classy/dataset/table.html.twig:21:6 - Unknown word (rowgroups)
/var/www/html/core/themes/seven/templates/classy/dataset/table.html.twig:79:9 - Unknown word (rowgroups)
/var/www/html/core/themes/seven/templates/classy/dataset/table.html.twig:80:24 - Unknown word (rowgroups)

I added 'rowgroup' and 'rowgroups' to the dictionary. I don't know whether it's common jargon or not, but it's defined in the documentation where we're using it, so I don't see a problem adding it to the dictionary.

There's a couple issues: in core/modules/system/templates/table.html.twig. Looks like core/themes/seven/templates/classy/dataset/table.html.twig is correct though.

• There's a missing {% endif %}.
• {% if rows %} should be {% if rowgroup.rows %}

I think that's why the tests don't work right now. The last job (https://www.drupal.org/pift-ci-job/2363868) shows a ton of fails (https://dispatcher.drupalci.org/job/drupal_patches/124754/). The problem with the twig template causes a fatal error, and that'd probably explain so many broken pages.

The branch is also based off of 9.2.x, but should probably be for 10 first, then backported to 9.4.

I was creating a reroll for D10 with #51 corrections, but when I ran a PHPStan check it returned the following error:

  Line   includes/theme.inc                       
 ------ ----------------------------------------- 
  1102   Undefined variable: $ts                  
  1108   Undefined variable: $responsive_classes  

Looking deeper into the code I noticed that this piece of code:

if (isset($variables['header'][$col_key]['data']) && $variables['header'][$col_key]['data'] == $ts['name'] && !empty($variables['header'][$col_key]['field'])) {
  $row['cells'][$col_key]['active_table_sort'] = TRUE;
}
// Copy RESPONSIVE_PRIORITY_LOW/RESPONSIVE_PRIORITY_MEDIUM
// class from header to cell as needed.
if (isset($responsive_classes[$responsive_index])) {
  $cell_attributes['class'][] = $responsive_classes[$responsive_index];
}

Was moved from the template_preprocess_table() function to _template_preprocess_table_process_rows() where these variables are not defined. I don't know the right solution. I though about passing them as argument to _template_preprocess_table_process_rows() but this sounded too much hacky and wrong.

Also, regarding comment #51, I could not find the missing {% endif %} on either templates/table.html.twig or dataset/table.html.twig. Both of them have exactly 6 {% if ... and 6 {% endif %}.

I just pushed a change to fix core/modules/system/templates/table.html.twig. It's still based on 9.2.x but it should be interesting to see what happens with the tests this time.

The undefined variables that #52 pointed out are still not addressed.

The last commit fixes the undefined variables $ts and $responsive_classes by passing them in to the new function. That fixed one of the kernel tests, so I hope it fixes the functional tests too (I didn't run them, so we'll see what CI says). The other kernel test that was failing was a test of Classy assets, which requires that all themes that copy from Classy to have identical files, so I copied table.html.twig to Classy and the rest of the themes that borrow from it, and I updated the md5 sum of table.html.twig in the test (for more info about what's going on there, this is the test https://dispatcher.drupalci.org/job/drupal_patches/124891/testReport/jun...)

This would still need to be rebased on 10.x / 9.4 / etc.

The last two changes:

• Change [] array destructuring back to list(). This fixes a phpcs complaint about the trailing ,.
• Fix backwards compatibility. Previously it wasn't setting the rows variable to the processed row.

The last last commit adds rowgroups to the theme definition in drupal_common_theme and Table::getInfo, and adds a test to the TableTest kernel test.

Can the MR please be updated for 10.1