Convert theme_table() to Twig

Going ahead and creating separate issues for each theme function/template now, so re-titling this one.

This issue will now be for converting theme_table().

Related issue that maybe we can incorporate as part of this conversion issue:

#1868212: Notice: Indirect modification of overloaded element of Drupal\Core\Template\Attribute has no effect (theme_table)

I've updated my table twig testing module to work with current Drupal 8.x:

https://github.com/psynaptic/twig-tables

I'm blocked on this as I can't get the twig template to be used, despite the registry having the correct path to the twig template file in the template_file property.

@psynaptic - thanks for looking at this!

See #1939066: Convert theme_breadcrumb() to Twig for a simple theme.inc patch example. You should define 'template' in drupal_common_theme(), add the Twig template to core/modules/system/templates/table.html.twig, remove the theme() function and possibly add template_preprocess_table().

Also, check out #drupal-twig on IRC :)

I did all of these things, except I removed theme_table() and not theme() ;)

Sorry, by that I meant remove the theme_*() function :)

I'm not sure what the template_file you mentioned in #5 is.

Me either, I meant the "template" property of hook_theme().

I just checked my stashed changes and the template file is now recognised. At least I can start working on this now.

@psynaptic - whats your status on this? I'd be glad to help out if you'd like.

@tlattimore: feel free to take it if you like. I've been working on something else in the meantime and was expecting to get to this soon but my time is running out as I'm going on holiday next week and I have work commitments this week.

I took the sandbox conversion + c4rl's last patch and merged it against core. Then touched up a few little things, added a few @todos, tweaked the 2 tests so whitespace and empty classes weren't failing them unfairly. Tried it out on a few pages to see if it's doing ok and seemed to be fairing pretty well.

I have my doubts if this will actually pass all the tests but here goes nothing.

Also, would really like to get rid of 'data' and 'attributes' and replace them with '#attributes' element_children() for everything else. They may be much more involved but my hopes it would make things less 'special case' and more consistent. Please let me know if you are against this idea.

Since the way the data was entered in the old _theme_table_cell() auto cast integers as strings I had to add in a little check and cast to help 0's.

missing ending tr and some whitespace mods.

activate

darn wrong patch....

Nice, happy to see a green patch on this issue. Fantastic work @joelpittet, I'll be back for a review.

Again, great work on this @joelpittet. Revised patch to address documentation and coding standards. Some notes/further followup:

+++ b/core/modules/system/templates/table.html.twigundefined
@@ -0,0 +1,94 @@
+ * // Note: Drupal currently supports only one table header row.
+ * // @see http://drupal.org/node/893530
+ * // @see http://api.drupal.org/api/drupal/includes!theme.inc/function/theme_table/7#comment-5109

* // should only be used for comments within @code…@endcode blocks. I’m not sure if what I’ve changed it to is totally correct either so I'm mentioning it specifically here.

+++ b/core/includes/theme.incundefined
@@ -2083,52 +2085,65 @@ function drupal_pre_render_table(array $element) {
+  // Flags.
...
+  // Preprocess header.
...
+  // Preprocess colgroups.

@@ -2136,25 +2151,26 @@ function theme_table($variables) {
+      // Set colgroup attributes.
...
+      // Set cols.
...
+  // Preprocess Empty message.

@@ -2162,97 +2178,180 @@ function theme_table($variables) {
+  // Preprocess table header.
...
+  // Preprocess table rows.
...
+          // Create cell attributes

Some of these comments could probably be removed, but this is a pretty huge function, so I’d be okay with keeping them.

+++ b/core/includes/theme.incundefined
@@ -2083,52 +2085,65 @@ function drupal_pre_render_table(array $element) {
+  // Table headers. (Drupal supports only one, that's why it's called 'header').
+  // Headers needed for reference of active sorting
+  // header after it's cells are processed
+  // array_values indexes for the table sort to work

This comment needs some love, this should be a paragraph or two using complete sentences.

Adding logic from #1868212-8: Notice: Indirect modification of overloaded element of Drupal\Core\Template\Attribute has no effect (theme_table) but in a slightly different manner. Should do the trick.

Actually in my testing #23 didn't fix the exceptions. Tested in conjunction with #1898034-39: block.module - Convert PHPTemplate templates to Twig. May need to come back to this.

The bug described and solved in #1959110: theme_table outputs the no_striping option as an HTML attribute (followups) which is RTBC and can be committed soon, still appears in this patch. So it will need to be rerolled after the other issue has been committed.

@fietserwin - thank you for the heads up, I don't think that means the patch needs work just yet though :)

• Simplified the change in #23.
• Deleted most of the comment noted in #22, I don't think it adds too much here, suggestions welcome.

Also tagging for manual testing.

Adding sandbox issue to related issues

This todo: "Look into removing 'data' and treating this like any other renderable array with element_children and #attributes."
... maybe something worth discussing in #1965214: Improve the theme_table API

This will need a quick reroll now that #1961886: Remove 'help' definition from drupal_common_theme() is in.

And a reroll for #1959110: theme_table outputs the no_striping option as an HTML attribute (followups) now that one is in as well.

Unassigning, anyone can pick this up and do the re-roll. Instructions: http://drupal.org/patch/reroll

Reroll version of #27

A fixed version of my previous patch.

After to finish the reroll i found a bug with the "header" attribute in the cells.

With the old theme_table the header attribute in the cells change the "td" for "th", but in the new table.html.twig not exists a condition for replicate that behavior:

  {# Rows #}
  {% if rows %}
    <tbody>
      {% for row in rows %}
        <tr{{ row.attributes }}>
          {% for i in row.cells|keys -%}
            <td{{ row.cells[i].attributes }}>
                {{- row.cells[i].data -}}
            </td>
          {%- endfor %}
        </tr>
      {% endfor %}
    </tbody>
  {% endif %}

I decided before to try to fix this new bug, make sure to my reroll no longer have issues.

David.

@gnuget - thanks for your work here. The reroll still looks a bit off - I think @joelpittet might need to take a look at this one since he has worked a lot on this issue. Would that be alright? There are other areas you can help with :)

As for the header logic…

+++ b/core/modules/system/templates/table.html.twigundefined
@@ -0,0 +1,96 @@
+  {# Header #}
+  {% if header %}
+    <thead>
+      <tr>
+        {% for i in header|keys -%}
+          <th{{ header[i].attributes }}>
+              {{- header[i].data -}}
+          </th>
+        {%- endfor %}
+      </tr>
+    </thead>
+  {% endif %}

Isn't that covered here?

the problem with the header, is not the header in self, the problem is the attribute "header" in the cells.

For example, i took this table from the test module of psynaptic (#4, https://github.com/psynaptic/twig-tables)

function twig_tables_page() {
  $table1 = array(
    'rows' => array(
      array(
        array('data' => t('Foo'), 'header' => TRUE),
        t('Bar'),
        t('Baz'),
      ),
      array(
        array('data' => t('Foo'), 'header' => TRUE),
        t('Bar'),
        t('Baz'),
      ),
      array(
        array('data' => t('Foo'), 'header' => TRUE),
        t('Bar'),
        t('Baz'),
      ),
    ),
  );

on this table the first cell of each row has the attribute "header", in the 8.x branch the /includes/theme.inc file have this function:

/**
 * @} End of "addtogroup themeable".
 */

/**
 * Returns HTML output for a single table cell for theme_table().
 *
 * @param $cell
 *   Array of cell information, or string to display in cell.
 * @param bool $header
 *   TRUE if this cell is a table header cell, FALSE if it is an ordinary
 *   table cell. If $cell is an array with element 'header' set to TRUE, that
 *   will override the $header parameter.
 *
 * @return
 *   HTML for the cell.
 */
function _theme_table_cell($cell, $header = FALSE) {
  $attributes = '';

  if (is_array($cell)) {
    $data = isset($cell['data']) ? $cell['data'] : '';
    // Cell's data property can be a string or a renderable array.
    if (is_array($data)) {
      $data = drupal_render($data);
    }
    $header |= isset($cell['header']);
    unset($cell['data']);
    unset($cell['header']);
    $attributes = new Attribute($cell);
  }
  else {
    $data = $cell;
  }

  if ($header) {
    $output = "<th$attributes>$data</th>";
  }
  else {
    $output = "<td$attributes>$data</td>";
  }

  return $output;
}

which before to return the $output check if the cell has the attribute $header, then instead of print a td tag print a th tag, this condition not exists in the twig version right now, and this attribute in the cell is different of the "header" of the table.


About to let to @joelpittet take a look on this, i'm not have any problem, i think to my last reroll just need a bit of more work because that patch passed all the automated tests, i will looking forward for check what i was missing.


Thank you for your help on this.

Updating tags. @gnuget, thanks for looking into this issue! This is one of our most complex conversion issues so might not be the best place to jump in :) I should have realized the reroll would not be an easy one. Please drop by IRC when you can.

@gnuget I did the merge too. I don't think you missed anything, nice work. And yes you are right, that is a regression for the table cells to not be able to swap between td/th. We should fix that and also write a test to make sure it doesn't happen again. If you want to tackle the fix, I could write the test?

Cheers and nice work. The interdiff is nice to see the differences, probably a bit trickier with a rebase re-roll with merge conflicts:) This patch is just some touchups.

I should have let @joelpittet review the reroll, my apologies @gnuget. Keep up the great work folks :)

@joelpittet

I fixed the regression, interdiff and new patch attached.

Closing tag needs to match.

+++ b/core/modules/system/templates/table.html.twig
@@ -85,7 +85,7 @@
                 {{- row.cells[i].data -}}
             </td>
           {%- endfor %}

May need the same tag for the ending tag(see the answer to the universe #42). But great work so far!

Here's the fix for #42, though I haven't given the rest of this a good once-over. I'll post a further revision if I encounter anything else.

I also changed the proposed syntax a bit to be somewhat friendlier to novices.

duh! this is embarrassing, sorry.

New patch and interdiff attached.

Regards

Resetting status. Looks like gnuget and I cross-posted :)

We took a slightly different approach to how to determine the tag. It's a bit of a bikeshed I suppose; the only real indication of which is better I guess necessitates a performance review -- that is, does setting a variable in twig with a ternary take more time than calculating an existing if statement twice?

IMHO, the double if is more grokkable, so I might fire up apache bench to see if there's a clear winner.

#45: twig-table-1939008-44.patch queued for re-testing.

I don't understand why my patch fail the automatic tests :/

errr, that test failure seems completely unrelated... awkward

#45: twig-table-1939008-44.patch queued for re-testing.

@gnuget just whitespace issue. Added modifiers see interdiff.txt. Quite interested to see @c4rl's performance tests!

Okay, so after some basic performance testing (building 100 3x3 tables on a given page callback, 100 requests via ab), the double-if approach acknowledged in #46 is about 3% slower. Let's stick with the cell_tag approach.

Thanks @c4rl! That is good to know, I'll try to keep my ifs down to a minimum, especially in the loops.

Could someone give that nested for loop issue a go with this patch?
http://drupal.org/node/1867090#comment-7317014

re-rolled as #1867090: Nested Twig 'for' loops behaving strange landed, and fixed some whitespace and redundancies in the twig template.

#58: 1939008-58-theme-table.patch queued for re-testing.

Update remaining, add testing steps

I added the missing test for check if the header option in cells works correctly.

Nice cleanup here.

+++ b/core/modules/system/templates/table.html.twig
@@ -84,10 +74,10 @@
+          {% for cell in row.cells -%}
+            {%- set cell_tag = cell.header ? 'th' : 'td' -%}
+            <{{ cell_tag }}{{ cell.attributes }}>

I wonder about inlining that ternary for cell tag and losing the {% set?

@steveoliver #63, See #44, #46 and #55.

ah, yep. two tags. duh.

Add related issue

Whats the ETA on this? Anything I can do to help

@Bojhan This one is fairly dialed, but won't get committed until all the rest of twig conversions get in all at once. It couldn't hurt to do one more round of manual tests to make sure we didn't miss anything because it is one of the trickier conversions.

@Bojhan - a patch review would be great too :)

I don't know *anything* about Twig, so I'm not reviewing the correctness of the code. Instead, I'm reviewing code style and understandability.

+++ b/core/includes/theme.incundefined
@@ -2091,52 +2093,63 @@ function drupal_pre_render_table(array $element) {
+     $variables['header'] = array_values($variables['header']);

Indented too far.

+++ b/core/includes/theme.incundefined
@@ -2091,52 +2093,63 @@ function drupal_pre_render_table(array $element) {
+  // Set to new variable for convenient checks later.

We never add comments for this sort of thing. This line can be removed.

+++ b/core/includes/theme.incundefined
@@ -2091,52 +2093,63 @@ function drupal_pre_render_table(array $element) {
+  // @todo API change, rename this variable from 'empty' to 'empty_message'.

We're approaching code freeze, nothing may be deferred to follow-ups — unless it's an independent change.

+++ b/core/includes/theme.incundefined
@@ -2091,52 +2093,63 @@ function drupal_pre_render_table(array $element) {
+  if (!empty($variables['header'])) {

A similar check is at the beginning of the function. Weird.

+++ b/core/includes/theme.incundefined
@@ -2091,52 +2093,63 @@ function drupal_pre_render_table(array $element) {
+      // Add 'sticky-enabled' class to the table to identify it for JS.

AFAIK we always write "JavaScript", not "JS" in comments.

+++ b/core/includes/theme.incundefined
@@ -2091,52 +2093,63 @@ function drupal_pre_render_table(array $element) {
+    // If the table has headers and it should react responsively to columns
+    // hidden with the classes represented by the constants
+    // RESPONSIVE_PRIORITY_MEDIUM and RESPONSIVE_PRIORITY_LOW, add the
+    // tableresponsive behaviors.

This is way too much repeated information.
Something like this should be sufficient:
"If the table is responsive, set a class and add the necessary library."

+++ b/core/includes/theme.incundefined
@@ -2091,52 +2093,63 @@ function drupal_pre_render_table(array $element) {
+      // Add 'responsive-enabled' class to the table to identify it for JS.

s/JS/JavaScript/

+++ b/core/includes/theme.incundefined
@@ -2091,52 +2093,63 @@ function drupal_pre_render_table(array $element) {
+      // This is needed to target tables constructed by this function.

Again too much info, this line can be deleted.

+++ b/core/includes/theme.incundefined
@@ -2091,52 +2093,63 @@ function drupal_pre_render_table(array $element) {
+  // Preprocess colgroups.

Again excessive, delete.

+++ b/core/includes/theme.incundefined
@@ -2091,52 +2093,63 @@ function drupal_pre_render_table(array $element) {
+      // Support simple or complex colgroups.

s/or/and/

+++ b/core/includes/theme.incundefined
@@ -2144,25 +2157,26 @@ function theme_table($variables) {
+      // Set colgroup attributes.

Excessive, remove.

+++ b/core/includes/theme.incundefined
@@ -2144,25 +2157,26 @@ function theme_table($variables) {
+      // Set cols.

Better comment needed.

+++ b/core/includes/theme.incundefined
@@ -2144,25 +2157,26 @@ function theme_table($variables) {
+  // Preprocess Empty message.

Excessive, remove.

+++ b/core/includes/theme.incundefined
@@ -2144,25 +2157,26 @@ function theme_table($variables) {
+  // data rows and the empty message is set.

s/is set/is not empty/

+++ b/core/includes/theme.incundefined
@@ -2170,96 +2184,174 @@ function theme_table($variables) {
+    // @todo Leave rows empty, let template decide how to handle no rows, using
+    //   empty_message variable and optionally counting the columns to generate
+    //   an "empty" row spanning all columns like this.

See remark for earlier @todo

+++ b/core/includes/theme.incundefined
@@ -2170,96 +2184,174 @@ function theme_table($variables) {
+  // Build up an associative array of responsive classes keyed by column.

s/Build up/Build/

+++ b/core/includes/theme.incundefined
@@ -2170,96 +2184,174 @@ function theme_table($variables) {
+  // Preprocess table header.
+  if (!empty($variables['header'])) {

Third (!!!) time there's an if-test for this.

+++ b/core/includes/theme.incundefined
@@ -2170,96 +2184,174 @@ function theme_table($variables) {
+
+    // Initialize table sorting.
+    $ts = tablesort_init($variables['header']);
+
+    foreach ($variables['header'] as $col_key => $cell) {
+
+      // If the cell is not an array, make it one.

What's with all the blank lines?

+++ b/core/includes/theme.incundefined
@@ -2170,96 +2184,174 @@ function theme_table($variables) {
+      // Create cell attributes.

Excessive, remove.

+++ b/core/includes/theme.incundefined
@@ -2170,96 +2184,174 @@ function theme_table($variables) {
+      // @todo Find out why we can use $variables['header'] here but must use
+      //   $headers for tablesort_cell() below.

See previous @todo remarks.

+++ b/core/includes/theme.incundefined
@@ -2170,96 +2184,174 @@ function theme_table($variables) {
+      // Conversion to Attribute() happens last.

Excessive, remove.

+++ b/core/includes/theme.incundefined
@@ -2170,96 +2184,174 @@ function theme_table($variables) {
+      // @todo Make sure we're not overwriting or losing any header element
+      //   variables.

Again @todo.

+++ b/core/includes/theme.incundefined
@@ -2170,96 +2184,174 @@ function theme_table($variables) {
+  // Preprocess table rows.

Again excessive.

+++ b/core/includes/theme.incundefined
@@ -2170,96 +2184,174 @@ function theme_table($variables) {
+      // Add odd/even class

Missing trailing period.

+++ b/core/includes/theme.incundefined
@@ -2170,96 +2184,174 @@ function theme_table($variables) {
+      if (!empty($cells)) {
+        // Build row.

Comment should be above the if-test.

+++ b/core/includes/theme.incundefined
@@ -2170,96 +2184,174 @@ function theme_table($variables) {
+          // Create cell attributes.

Excessive, remove.

+++ b/core/includes/theme.incundefined
@@ -2170,96 +2184,174 @@ function theme_table($variables) {
+          if (!isset($cell['attributes'])) {
+            $cell['attributes'] = array();

Similar code exists earlier, consider creating a helper function.

+++ b/core/includes/theme.incundefined
@@ -2170,96 +2184,174 @@ function theme_table($variables) {
+          // Move cell properties that are not data, attributes or header property

80 cols exceeded?

+++ b/core/includes/theme.incundefined
@@ -2170,96 +2184,174 @@ function theme_table($variables) {
+          // Conversion to Attribute() happens last.

Excessive.

+++ b/core/modules/system/lib/Drupal/system/Tests/Theme/TableTest.phpundefined
@@ -81,4 +81,20 @@ function testThemeTableWithNoStriping() {
+   * Tests that the 'header' option in cells works correctly

Trailing period.

@Wim Leers, I have made the edits you requested.

I will comment on the more notable changes:

+++ b/core/includes/theme.incundefined
@@ -2091,52 +2093,63 @@ function drupal_pre_render_table(array $element) {
+  if (!empty($variables['header'])) {

I looked at the 2 blocks of code and I didn't see anything that lead me to believe that they couldn't be combined so I took the two conditionals and combined them.

+++ b/core/includes/theme.incundefined
@@ -2144,25 +2157,26 @@ function theme_table($variables) {
+      // Set cols.

I changed to "// Build colgroups and assign attributes"

+++ b/core/includes/theme.incundefined
@@ -2170,96 +2184,174 @@ function theme_table($variables) {
+    // @todo Leave rows empty, let template decide how to handle no rows, using
+    //   empty_message variable and optionally counting the columns to generate
+    //   an "empty" row spanning all columns like this.

Seems like a good @todo, I removed in the patch but do we need a new ticket for this or should we just move on?

+++ b/core/includes/theme.incundefined
@@ -2170,96 +2184,174 @@ function theme_table($variables) {
+  // Preprocess table header.
+  if (!empty($variables['header'])) {

I only counted 2 and I combined the two code blocks.

+++ b/core/includes/theme.incundefined
@@ -2170,96 +2184,174 @@ function theme_table($variables) {
+      // @todo Find out why we can use $variables['header'] here but must use
+      //   $headers for tablesort_cell() below.

I looked through the code and $headers was only used once. I just removed the variable and replaced it's one usage with $variables['header'] which I think solves the @todo. $headers was really serving no purpose.

+++ b/core/includes/theme.incundefined
@@ -2170,96 +2184,174 @@ function theme_table($variables) {
+          // Move cell properties that are not data, attributes or header property

I did a char count and it was less than 80

I have not yet tested these changes.

@drupalninja99 something's funky with your interdiff format it won't apply and dreditor doesn't syntax highlight it. Also, #62 Needed a re-roll first so that may be why your patch isn't applying.

Here is the reroll from #62

I'll try to get your patch on top of it or do an interdiff between the branches at the least.

Ok yeah, had to roll back to apply your patch @drupalninja99, commit it and rebase forward.

Attached is the interdiff between #72 and your patch re-rolled.

Here is some further cleanup:

• Line length
• Twig whitespace
• Revert empty_message back to empty (because twig is smarter than I thought)
• Let attributes do it's thing, lazy as of #1982024: Lazy-load Attribute objects later in the rendering process only if needed

@drupalninja99 thank you very much for the patch btw!

Tagging

Seems like these may not be good enough? But there is quite a few more function calls, so maybe some refactoring could happen?

Scenario 1

/admin/people/permissions, stark

=== 8.x..8.x compared (51b0309e0d24b..51b0311a4e34a):

ct  : 269,267|269,267|0|0.0%
wt  : 1,105,275|1,104,666|-609|-0.1%
cpu : 1,052,388|1,051,309|-1,079|-0.1%
mu  : 30,745,176|30,745,176|0|0.0%
pmu : 32,209,728|32,209,728|0|0.0%

http://www.lionsad.de/xhprof-kit/xhprof/xhprof_html/?run1=51b0309e0d24b&...

=== 8.x..1939008-theme-table compared (51b0309e0d24b..51b031de38383):

ct  : 269,267|281,479|12,212|4.5%
wt  : 1,105,275|1,138,786|33,511|3.0%
cpu : 1,052,388|1,088,512|36,124|3.4%
mu  : 30,745,176|30,920,432|175,256|0.6%
pmu : 32,209,728|33,057,608|847,880|2.6%

http://www.lionsad.de/xhprof-kit/xhprof/xhprof_html/?run1=51b0309e0d24b&...

Scenario 2

/admin/modules, stark

=== 8.x..8.x compared (51b032b631491..51b0336932f18):

ct  : 226,611|226,611|0|0.0%
wt  : 985,847|985,619|-228|-0.0%
cpu : 919,876|907,702|-12,174|-1.3%
mu  : 31,548,344|31,548,344|0|0.0%
pmu : 31,851,120|31,851,120|0|0.0%

http://www.lionsad.de/xhprof-kit/xhprof/xhprof_html/?run1=51b032b631491&...

=== 8.x..1939008-theme-table compared (51b032b631491..51b033ccbc666):

ct  : 226,611|233,348|6,737|3.0%
wt  : 985,847|1,004,964|19,117|1.9%
cpu : 919,876|940,223|20,347|2.2%
mu  : 31,548,344|31,729,200|180,856|0.6%
pmu : 31,851,120|32,038,464|187,344|0.6%

http://www.lionsad.de/xhprof-kit/xhprof/xhprof_html/?run1=51b032b631491&...

Is this just profiling the patched files against 8.x HEAD?

@drupalninja99 yes, it's theme_table vs table.html.twig battle royal.

I had to re-create patch #74 because it wouldn't apply to head as of 15 minutes ago. I think I was able to get it reapplied to head just now.

#79 @drupalninja99 I just applied #74 with no problems.

Again, something wrong with your interdiffs. Jump on IRC so we can discuss your workflow.

You may not use IRC, so the channel is #drupal-twig if you decide to join.

In the meantime:

git pull --rebase
git checkout origin/8.x -b 1939008-twig-theme-table
curl https://drupal.org/files/1939008-74-theme-table.patch | git apply 
git add -A . 
git commit -m "#74"

Your changes go here.

git add -A . 
git commit -m "changed some stuff"
git show > interdiff.txt
git diff origin/8.x > https://drupal.org/files/1939008-81-theme-table.patch

It'd be good for me to know how you got that strange interdiff though your patch seems to be the right format.

Hi Joel, sorry I was just doing a lazy diff btwn patches. I am new to Drupal patches. You workflow makes sense, I will start doing that from now on.

My analysis of the profiling report is that the underlying Twig calls/inefficiency are what's causing the WT increase. The Attribute method/object is getting called a ton and so if we can fix any efficiencies there (already have a ticket for this) then we should be fine. I didn't see anything in template_preprocess_table that would be low hanging fruit as far as general refactoring goes.

Also another note, do we need to replace all doc references to "theme_table()" with template_preprocess_table()? I saw 9 references to theme_table() just in the /core/includes folder. Since there is no more theme_table I would think we'd want to refer people to template_preprocess_table().

Patch in #79 no longer applies, needs to be rerolled.

Re-rolled. Already profiled in #76

Thanks @drupalninja99 and @joelpittet for keeping this going :) Patch no longer applies, needs a reroll.

Re-roll and remove @see template_preprocess() from twig template.

Bot! go

Re-roll and also note it would be really nice to have forum as #type table so the @todo from here for the forum table headers can be removed from this patch.

#1938906: Convert forum theme tables to table #type Re-opened the #type table issue because it still needs to be done.

Made a followup issue to remove _theme_table_cell from forum.
@see #2092343: Consolidate forum.module and remove call to _theme_table_cell() within template_preprocess_forum_topic_list()

Also re-closed that #type=>table for forum which I thought would be a good fix to that but it was tricky and possibly not worth the effort.

This issue is blocked on that new issue so we can remove _theme_table_cell() as we have in this patch and to keep this issue as a conversion patch as it's big enough as it is.

I'm assigning this to me to fix those two errors and remove the _theme_table_cell removal from this patch. Something to do on the plane.

Remove sandbox link

Well unpostponing because that little quick issue got taken over and became more complicated. Here's a re-roll.

94: 1939008-94-theme-table.patch queued for re-testing.

Removed and simplify tablesort_cell as a micro-optimization and remove the zebra for zoo todo.

Latest profiling of Scenario 1 from #76

=== 8.x..8.x compared (52c2677cc22b0..52c2691acb42f):

ct  : 322,426|322,426|0|0.0%
wt  : 1,327,571|1,334,169|6,598|0.5%
cpu : 1,320,793|1,327,127|6,334|0.5%
mu  : 31,027,224|31,027,224|0|0.0%
pmu : 32,612,592|32,612,592|0|0.0%

http://www.lionsad.de/xhprof-kit/xhprof/xhprof_html/?run1=52c2677cc22b0&...

=== 8.x..1939008-theme-table compared (52c2677cc22b0..52c2694eecde3):

ct  : 322,426|331,781|9,355|2.9%
wt  : 1,327,571|1,369,723|42,152|3.2%
cpu : 1,320,793|1,362,487|41,694|3.2%
mu  : 31,027,224|31,103,040|75,816|0.2%
pmu : 32,612,592|32,792,848|180,256|0.6%

http://www.lionsad.de/xhprof-kit/xhprof/xhprof_html/?run1=52c2677cc22b0&...

Tagging for reroll.

Rerolled.

In general I think that 3% overhead for tables is not bad. Most tables are rendered on the admin backend and never on the frontend.

1. +++ b/core/includes/theme.inc
   @@ -1546,52 +1548,28 @@ function drupal_pre_render_table(array $element) {
   +function template_preprocess_table(&$variables) {
   

   It is impressive how much logic we need to render a table :(

2. +++ b/core/includes/theme.inc
   @@ -1624,96 +1602,171 @@ function theme_table($variables) {
   +      drupal_add_library('system', 'drupal.tableheader');
   ...
   +      drupal_add_library('system', 'drupal.tableresponsive');
   

   Can we get rid of the direct call to drupal_add_library?

Ok it's mildly better now that twig 1.15 is in there. 2.7% slower instead of 3-3.3% slower.

=== 8.x..8.x compared (52ed72044139b..52ed72fd446c1):

ct  : 330,732|330,732|0|0.0%
wt  : 1,921,607|1,922,583|976|0.1%
cpu : 1,912,169|1,914,073|1,904|0.1%
mu  : 36,465,856|36,465,856|0|0.0%
pmu : 37,081,904|37,081,904|0|0.0%

http://www.lionsad.de/xhprof-kit/xhprof/xhprof_html/?run1=52ed72044139b&...

=== 8.x..1939008-theme-table compared (52ed72044139b..52ed7448d018c):

ct  : 330,732|340,082|9,350|2.8%
wt  : 1,921,607|1,972,596|50,989|2.7%
cpu : 1,912,169|1,963,810|51,641|2.7%
mu  : 36,465,856|36,690,016|224,160|0.6%
pmu : 37,081,904|37,321,448|239,544|0.6%

http://www.lionsad.de/xhprof-kit/xhprof/xhprof_html/?run1=52ed72044139b&...

@dawehner yes it was quite a challenging conversion. I'll convert those to #library.

@dawehner seems that it's awkward to do the #attached thing in theme functions... there may be a solution in place in #2171071: Rename drupal_add_library() to _drupal_add_library() and remove its uses which is critical and will cause this to be re-rolled as soon as it's in so I'll wait on that.

This is a re-roll because #2092343: Consolidate forum.module and remove call to _theme_table_cell() within template_preprocess_forum_topic_list() is now in and doesn't need to be part of this conversion.

107: 1939008-theme-table-107.patch queued for re-testing.

Reviewing my own patch:D I'll post the changes soon. I'm glad a bit of the forum stuff is out of this. Considering moving the tablesort_cell and _theme_table_cell out to their own cleanup patches so lighten this patch up a bit more... thoughts?

1. +++ b/core/includes/theme.inc
   @@ -1641,96 +1619,171 @@ function theme_table($variables) {
              // Add active class if needed for sortable tables.
   -          $cell = tablesort_cell($cell, $header, $ts, $i);
   +          if (isset($variables['header'][$i]['data']) && $variables['header'][$i]['data'] == $ts['name'] && !empty($variables['header'][$i]['field'])) {
   +            $cell['attributes']['class'][] = 'active';
   +          }
   +
   +          // Move cell properties that are not data, attributes or header
   +          // property into attributes.
   +          foreach ($cell as $key => $value) {
   +            if ($key != 'data' && $key != 'attributes' && $key != 'header') {
   +              $cell['attributes'][$key] = $value;
   +            }
   +          }
   

   This move cell properties needs to happen first or it will clobber the sort attributes. Swap the blocks around.

2. +++ b/core/includes/theme.inc
   @@ -1641,96 +1619,171 @@ function theme_table($variables) {
   +          // Make sure that zeros get printed as '0'.
   +          if (isset($cell['data']) && is_int($cell['data'])) {
   +            $cell['data'] = (string) $cell['data'];
              }
   

   This could use a test.

3. +++ b/core/modules/forum/forum.module
   @@ -6,6 +6,7 @@
   +use Drupal\Core\Template\Attribute;
   

   This isn't needed here.(leftover)

4. +++ b/core/modules/system/lib/Drupal/system/Tests/Theme/TableTest.php
   @@ -81,4 +81,20 @@ function testThemeTableWithNoStriping() {
   +    $this->content = theme('table', array('rows' => $rows));
   

   use drupal_render() instead of theme() because theme calls are being deprecated.

5. +++ b/core/modules/system/templates/table.html.twig
   @@ -0,0 +1,86 @@
   + *     @todo Deprecate no_striping?
   

   Remove @todo, we aren't deprecating it.

Here is a patch with all of the changes joelpittet suggested above, except for the addition of the test in item 2, which he said he would post separately, possibly a sub-issue.

+++ b/core/modules/system/lib/Drupal/system/Tests/Theme/TableTest.php
@@ -27,7 +27,7 @@ public static function getInfo() {
+    $this->content = drupal_render('table', array('header' => $header, 'rows' => $rows, 'sticky' => TRUE));

if only they were the same like that. drupal_render() takes a renderable array with all the keys as #.

so drupal_render(array('#theme' => 'table', '#header' => $header, ... etc.));

Everything else looks good:)

This should fix the issue with the drupal_render arrays, along with the previous fixes. Interdiff provided too.

@mdrummond sorry my bad you can't pass in an array directly because it's a referenced value. I always forget that until I run the tests.

Here's the fix and also I made sure the xpath is a bit closer to what it was. before that conversion.

Since the tests are passing, what's next with this conversion?

Needs a bit more manual tests. You could get the tables from here and put them in a controller or preprocess to get them rendering (module is really out of date but table render arrays should still be good).

https://github.com/joelpittet/twig-tables

And if the manual testing goes well enough then maybe RTBC?

I'm taking a look at this now

Ok, so the twig tables module didn't work for me, i added the correct .info.yml file but still says page not found on /twig-tables - i think because it needs a routing.yml file ???

But i added the example table render array to bartik_preprocess_page, then printed my new variable in page.html.twig

here's the source code

<table id="twig-table2" class="responsive-enabled">
<caption>Colgroup Examples</caption>
 <colgroup> <col class="funky" /> </colgroup>
 <colgroup class="jazzy"> <col class="funky" /> </colgroup>
 <thead><tr><th>Foo</th><th colspan="2">Operations</th><th>test</th> </tr></thead>
<tbody>
 <tr class="odd"><td>Bar</td><td><a href="/lorem">ipsum</a></td><td><a href="/ipsum">lorem</a></td><td>Biz</td> </tr>
 <tr class="even"><td>Baz</td><td><a href="/lorem">ipsum</a></td><td><a href="/ipsum">lorem</a></td><td>Buzz</td> </tr>
</tbody>
</table>
<table id="twig-table2" class="responsive-enabled">
<caption>Colgroup Examples</caption>
 <colgroup> <col class="cg1c1" /> </colgroup>
 <colgroup class="cg2"> <col class="cg2c1" /> <col class="cg2c2" /> </colgroup>
 <colgroup class="cg3"> <col class="cg3c1" /> <col class="cg3c2" /> <col class="cg3c3" /> <col class="cg3c4" /> </colgroup>
 <thead><tr><th>Foo</th><th colspan="2">Operations</th><th>test</th> </tr></thead>
<tbody>
 <tr class="odd"><td>Bar</td><td><a href="/lorem">ipsum</a></td><td><a href="/ipsum">lorem</a></td><td>Biz</td> </tr>
 <tr class="even"><td>Baz</td><td><a href="/lorem">ipsum</a></td><td><a href="/ipsum">lorem</a></td><td>Buzz</td> </tr>
</tbody>
</table>
<table id="twig-table2" class="responsive-enabled">
<caption>Colgroup Examples</caption>
 <colgroup> <col class="cg2c1" /> <col /> </colgroup>
 <colgroup class="cg2"> <col class="cg2c1" /> <col class="cg2c2" /> </colgroup>
 <colgroup> <col 0="cg3" /> </colgroup>
 <thead><tr><th>Foo</th><th colspan="2">Operations</th><th>test</th> </tr></thead>
<tbody>
 <tr class="odd"><td>Bar</td><td><a href="/lorem">ipsum</a></td><td><a href="/ipsum">lorem</a></td><td>Biz</td> </tr>
 <tr class="even"><td>Baz</td><td><a href="/lorem">ipsum</a></td><td><a href="/ipsum">lorem</a></td><td>Buzz</td> </tr>
</tbody>
</table>

And i've attached a screen shot (Mac chrome)

When i went to apply the most recent patch it didn't apply...

error: patch failed: core/modules/system/lib/Drupal/system/Tests/Theme/TableTest.php:27

reroll

ok i've applied the patch from #121 and have made a screengrab, and copied the source, and diffed the original output to patched output.

However it's really hard to see what's going on because the formatting is very different

So I reformatted both the source code outputs, put everything onto separate lines, removed all whitespace, diffed the 2... and they are 100% identical, which leaves to one of 2 conclusions, either this patch is perfect, or I did something wrong and i'm comparing the same thing twice.

I've attached all my files, see what you think

The patch was not applying.

I am adding the Re-roll.

#2171071: Rename drupal_add_library() to _drupal_add_library() and remove its uses was committed.

Please note that the responsive and sticky header libraries are added in drupal_pre_render_table now. We should no longer call _drupal_add_library in theme functions.

Thanks!

I'm postponing this again on #2216407: Remove _theme_table_cell(). which splits out the removal of _theme_table_cell and tablesort_cell. As well as the header test addition.

Hope this makes this patch easier to read after that is in.

+++ b/core/modules/system/templates/table.html.twig
@@ -0,0 +1,85 @@
+  {% if rows %}
+    <tbody>
+      {% for row in rows %}

Wasn't there support for multiple TBODYs at some point?

Or am I misguided and that feature is not in core yet...?

@sun re #127 not in D7 or D8 yet there hasn't been that feature. Though at issue is really old and over here #31535: Allow table row groups in table.html.twig and template_preprocess_table()

1. Applied latest green patch by @joelpittet in #121.
2. Merged 8.x + resolved merge conflicts.
3. Updated for latest HEAD; polished template to produce tidy HTML.

@sun awesome, thanks for taking a look at this! All the changes look great, just some minor notes below.
I really like the {{ cell.tag }} change:)

1. Did you maybe want to move that removeWhitespace and render methods onto the base class? And just curious were they nessasary to get things working?

2. +++ b/core/includes/theme.inc
   @@ -1495,26 +1497,14 @@ function drupal_pre_render_table(array $element) {
          // Check if we're dealing with a simple or complex column
   

   Might as well put a period at the end of this sentance as well.

3. +++ b/core/includes/theme.inc
   @@ -1523,55 +1513,56 @@ function theme_table($variables) {
   +      // Build colgroups and assign attributes
   

   Needs a .

4. +++ b/core/modules/system/templates/table.html.twig
   @@ -0,0 +1,85 @@
   +  {% for colgroup in colgroups -%}
   +    {% if colgroup.cols %}
   +    <colgroup{{ colgroup.attributes }}>
   +      {% for col in colgroup.cols -%}
   +      <col{{ col.attributes }} />
   +      {%- endfor %}
   +
   +    </colgroup>
   +    {% else %}
   +    <colgroup{{ colgroup.attributes }} />
   +    {% endif %}
   +  {%- endfor %}
   +  {%- if header %}
   

   The indentation of the template was following the coding standards.
   The template readability is a bit more important than the HTML output indenting, IMO. @see bottom of http://twig.sensiolabs.org/doc/coding_standards.html
   and @see https://drupal.org/node/1823416

1. For now, those methods are one-off implementations on this test class. FWIW, my patch in #2243575: Add #button_type support for #type link introduces a dedicated base class for Render element tests. The TableTest can and should be moved/merged into that new concept later on. But laters.
2. Not going to touch code that I don't need to touch here.
3. Simplified colgroups processing + fixed code style for touched/changed comments (only).
4. Reformatted template according to Twig coding standards + removed almost all whitespace control.

Fixed NodeTypeRenameConfigImportTest relies on exact HTML markup.

I've created a follow up to clean up doc standards. #2256595: Clean up remaining doc standards in template_preprocess_table() after it's converted to twig template.

I think this is now RTBC. Thanks for your work on it @sun! If someone else needs to take a look at it that's cool but it's been around for a while now. Lots of eyes coming and going and was previously in the twig sandbox.

• This issue has spawned some new test coverage.
• Test fixes.
• Huge cleanup including removal of _theme_table_cell() #1842326: Merge _theme_table_cell() into theme_table()
• Profiling in #76 and #100
• Markup comparison in #122

Added commit message to IS with people from the sandbox added.

Just FYI: Due to the test code in this + some other issues, I worked on enabling native support for rendered content assertions in DUTB tests:

#2257519: Move content assertion methods into a trait, so DUTB can consume it, too

The changes of that issue will cause conflicts with this patch, so whichever lands first will require the other to be re-rolled. If the other lands first, then the removeWhitespace() method from this patch here should be moved into the new AssertContentTrait.

1. +++ b/core/modules/system/lib/Drupal/system/Tests/Theme/TableTest.php
   --- /dev/null
   +++ b/core/modules/system/templates/table.html.twig
   

   This is such an unbelievably huge improvement over the status quo (which afaics requires like 11 years of experience in PHP debugging to theme a table) I can't even explain. :) Great work.

2. +++ b/core/modules/node/lib/Drupal/node/Tests/NodeTypeRenameConfigImportTest.php
   @@ -119,4 +124,27 @@ public function testConfigurationRename() {
   +  protected function assertTextPattern($pattern, $message = NULL) {
   

   This seems generally useful (if a bit terrifying ;)), so why is it in NodeTypeRenameConfigImportTest.php and not in a test base class somewhere?

   Can the docs also be expanded slightly to explain a short sentence on why someone might want to use this helper? I can't quite figure it out. Which leads to...

3. +++ b/core/modules/node/lib/Drupal/node/Tests/NodeTypeRenameConfigImportTest.php
   @@ -106,8 +107,12 @@ public function testConfigurationRename() {
   +      $id_key = $entity_type->getKey('id');
   +      $text = "$id_key: $old_id";
   +      $this->assertTextPattern('/\-\s+' . preg_quote($text, '/') . '/', "'-$text' found.");
   +      $text = "$id_key: $new_id";
   +      $this->assertTextPattern('/\+\s+' . preg_quote($text, '/') . '/', "'+$text' found.");
   

   No idea what these tests are testing. :( We need a comment here.

4. +++ b/core/modules/system/lib/Drupal/system/Tests/Theme/TableTest.php
   @@ -79,9 +80,9 @@ function testThemeTableNoStickyHeaders() {
   -      t('Header 1'),
   +      'Header 1',
   ...
   -        'data' => t('Header 2'),
   +        'data' => 'Header 2',
   
   @@ -89,11 +90,12 @@ function testThemeTableWithEmptyMessage() {
   -      '#empty' => t('No strings available.'),
   +      '#empty' => 'Empty row.',
   

   Hm. That looks wrong. How/where is the translation being handled, if not here? Or are you only doing this because it's in a test and tests won't be translated?

Issue #1939008 by sun,,, Cottser, drupalninja99, mdrummond, sphism, c4rl,, andypost, Fabianx, Gokul N K, longwave | jenlampton: Convert theme_table() to Twig.

Ok, Joel and I spent another hour or so on this patch and came to the following conclusions:

- The unrelated t() removal is indeed because they're in test files which are not translated.
- The reason we need to move from assertText() to assertTextPattern() is because the config UI's diff output now puts the "-" and the "thing" on two separate lines, which assertText() can't deal with. Added a comment to that effect.
- I still think assertTextPattern() is needs a) some clarification as to its purpose a) to be moved to a test base class, and/or c) the changes made "upstream" in the WebTestBase method. But I don't think it's worth holding up this important twig patch just because sun decided to be fancy. ;) But opened a follow-up here: #2257945: Move NodeTypeRenameConfigImportTest::assertTextPattern() to AssertContentTrait

Given that the "meat" of this still looks nice and solid, going to go ahead and get 'er done.

Committed and pushed to 8.x. Thanks! GREAT work, everyone!

SRSLY needs a change record. :-)

@Mile23 We discussed this briefly at last weeks Thursday twig hangout. Which is recorded and on youtube https://www.youtube.com/watch?v=sj8BxZLhVb4&list=UUhHSH9ZDWsfpHrhYLam8qIg. Gah I hate hearing my voice.

It's a bit strange to do conversion issues as we've done so many and the main goal behind them to keep the same API(if the API does change we provided change records) and convert the theme_* functions that provides HTML output to the *.html.twig template that provides the same output. We even diligently compare markup to make sure that hasn't change either before RTBC.

• If we were to do it for this conversion, should we do it for all previous conversions? There are over 160+ theme functions converted to twig. And ~60 left to convert @see #1757550: [Meta] Convert core theme functions to Twig templates
• What would it say? "theme.inc's theme_table() function is now at template file at /core/modules/system/templates/table.html.twig"
• Maybe there should be one big change record at the end of the conversions? For the entire list of the theme function conversions?
• Do we need to do the *.tpl.php to *.html.twig files as well?

I agree - that's a much larger scope, and it doesn't make any sense to create one-off change notices for every single theme function that is converted to a Twig template. A (really long) list of individual change notices would not help anyone at all.

Let's figure that out in a separate issue — also, I can't imagine that this is the first time the question comes up, so I can only assume that there must be an issue already.

It is the first time I've seen it come up and @webchick asked me about it last weekend that's why it ended up the meeting.

@Mile23 I'd recommend just adding a "Needs change record" tag #1757550: [Meta] Convert core theme functions to Twig templates and making any further recommendations on what the draft should say. And maybe start a draft CR of what you think it should say.