D7 Backport: theme_table() should take an optional footer variable and produce <tfoot>

The d8 version needs to be committed first. Once that happens d7 patch needs to be attatched to that issue.

D8 patch was committed, Pol has an updated patch in that issue, copy that patch here as this is the new policy.

Here's the patch.

Here's the updated patch.

Here's a better patch with an improved coding style.

New patch, including tests.

It's already in D8, passes tests, has test, looks good.
Although I've never actually had to use this yet, others are.

Bumping to 7.70, this didn't make it into 7.60

Just re-rolling the excellent patch from @Pol at comment #11 to match current state of drupal 7 (7.63) as the old patch no longer applies.

Endless rework of good work while waiting for the merge to master.

The patch still apply on my side, without any troubles.

Updating credits.

+++ b/docroot/includes/theme.inc
@@ -2069,55 +2084,66 @@ function theme_table($variables) {
+  // Rows and footer have the same structure.
+  $sections = array(
+    'rows' => 'tbody',
+    'footer' => 'tfoot',
+  );
+  $variables['rows'] = $rows;
+  foreach ($sections as $section => $tag) {
+    if (!empty($variables[$section])) {

I'd like a little restructuring here:

$sections = [];
$sections['tbody'] = $rows;
if (!empty($variables['footer']) {
  $sections['tfooter'] = $variables['footer'];
}

That will simplify the code and make it easier to ready.

---

I delayed committing that as the changes looked massive, but it was overall just structure changes, which is okay this time, but next time let's please do cleanup separate from feature development as it makes things harder to review than necessary.

Hi Fabian,

Thanks for replying.

We cannot do the change you requested.

The $sections variable is a mapping of keys from $variables array into their respective HTML tags and they are processed using the same mechanism, this is why this is abstracted into this variable, to avoid code duplication.

rows => <tbody>
footer => <tfoot>

If we implement your logic, we won't be able to map the $rows to <tbody> tag and $footer to <tfoot> tag.

After a discussion in IRC, I got the Fabian's point.

Here's an updated patch and the interdiff.

1. +++ b/includes/theme.inc
   @@ -2069,42 +2086,54 @@ function theme_table($variables) {
   +  $sections = array(
   +    'tbody' => $rows,
   +  );
   

   My bad - we need to check for !empty() like before to populate 'rows' in sections.

2. +++ b/includes/theme.inc
   @@ -2069,42 +2086,54 @@ function theme_table($variables) {
   -      else {
   -        $cells = $row;
   -        $attributes = array();
   -        $no_striping = FALSE;
   ...
   +      if (!$no_striping) {
   +        $class = $flip[$class];
   +        $attributes['class'][] = $class;
          }
   -      if (!empty($cells)) {
   -        // Add odd/even class
   -        if (!$no_striping) {
   -          $class = $flip[$class];
   -          $attributes['class'][] = $class;
   -        }
    
   -        // Build row
   +      // Build row.
   +      if (!empty($cells)) {
   

   Is there a reason for moving the no_striping out of the `!empty($cells)`?

   Looks wrong - or rather needs a justification before that can go in.

The nice thing about this new approach is that the indentation almost remains the same, which makes this much simpler to review than before :D.

Updated patch.

I moved back the no_stripping logic into the condition as it was before.

Updating credits - adding Fabian.

1. +++ b/includes/theme.inc
   @@ -2069,42 +2086,56 @@ function theme_table($variables) {
   +  if (!empty($variables['rows'])) {
   +    $sections['tbody'] = $variables['rows'];
   +  }
   

   Should be !empty($rows) as that is the weird header / rows structure.

2. +++ b/includes/theme.inc
   @@ -2069,42 +2086,56 @@ function theme_table($variables) {
   +      $cells = $row;
   +      $attributes = array();
   +      $no_striping = $tag === 'tfoot';
   +
   ...
   -      else {
   -        $cells = $row;
   -        $attributes = array();
   -        $no_striping = FALSE;
   -      }
   

   This change is problematic because it changes the current behaviour.

   What would work for me is to keep the code as-is, but initialize the $no_stripiing once before the for loop if the tag is tfoot.

   That would change the default just once.

3. +++ b/includes/theme.inc
   @@ -2069,42 +2086,56 @@ function theme_table($variables) {
   +      // Build row.
   ...
   -        // Build row
   

   This move is unnecessary.

Thanks, updating the patch, interdiff provided.

+++ b/includes/theme.inc
@@ -2069,42 +2088,56 @@ function theme_table($variables) {
+    $no_striping = $tag === 'tfoot';
+
+    foreach ($content as $number => $row) {
+      $cells = $row;
+      $attributes = array();
+
+      // Check if we're dealing with a simple or complex row.
...
-      else {
-        $cells = $row;
-        $attributes = array();
-        $no_striping = FALSE;
-      }
+

Let's not remove the else - the additional line is fine though.

Updating patch.

1. +++ b/includes/theme.inc
   @@ -1948,6 +1948,8 @@ function theme_breadcrumb($variables) {
   + *   - footer: An array of table rows which will be printed within a <tfoot>
   + *     tag, in the same format as the rows element (see above).
   

   I think it might be good to say that no_striping defaults to FALSE for 'rows' and TRUE for 'footer'.

2. +++ b/includes/theme.inc
   @@ -2069,23 +2088,38 @@ function theme_table($variables) {
   +    $no_striping = $tag === 'tfoot';
   +
   
   @@ -2097,14 +2131,15 @@ function theme_table($variables) {
            $no_striping = FALSE;
          }
   

   Let's do this differently and put:

   $default_no_striping = ($tag === 'tfoot');

   Then use $default_no_striping both in the ternary operator as well as in the else block.

just nits remaining!

Looks very great already :).

1. +++ b/includes/theme.inc
   @@ -1948,6 +1948,11 @@ function theme_breadcrumb($variables) {
   + *     that the no_stripping boolean has no effect, there is no rows stripping
   

   s/stripping/striping/

2. +++ b/includes/theme.inc
   @@ -2049,17 +2057,31 @@ function theme_table($variables) {
   -  // Format the table header:
   +  // Assigning back the $rows variable into $variables as we won't modify it
   +  // anymore.
   +  $variables['rows'] = $rows;
   

   This is no longer needed as it's unused.

3. +++ b/includes/theme.inc
   @@ -2069,23 +2091,38 @@ function theme_table($variables) {
    
   -  // Format the table rows:
   ...
   +
   

   I think we can change that to '// Format the table rows and footer'

Forget this one, I forgot to fix a typo.

RTBC - let's get that in

Thanks all!

Nice work! Thanks!

Merci Pol
Dankeschön FabianX