Convert theme_table to Twig

this is fixed in the latest chx sandbox

with #1770284: [META] Drupal 8 with twig issue queue cleanup
and Stark as admin theme on admin/modules

Twig_Error_Syntax: Unexpected character "$" in "core/themes/stark/templates/theme.inc/table.twig" at line 52 in Twig_Lexer->lexExpression() (line 285 of /var/www/podarok/drupal_8_theme_and_twig_sprint/core/vendor/twig/twig/lib/Twig/Lexer.php).

tagging
looks like this will be nice documentation example for #1779402: Make how-to documentation update based on sprint practice

+++ b/core/themes/stark/templates/theme.inc/table.twigundefined
@@ -66,11 +66,11 @@
-</table>
\ No newline at end of file

\ No newline at end of file

looks like we need decide and extend #1760468: [DX] Functions and filters in twig templates list of functions as described in Twig docs http://twig.sensiolabs.org/doc/advanced.html#functions

$twig = new Twig_Environment($loader);
$twig->addFunction('functionName', new Twig_Function_Function('someFunction'));

...

postponed till common decision about the way for feature adding new functions into twig #1760468: [DX] Functions and filters in twig templates

_theme_table_cell has been moved to the preprocess function - and has no business being called from within a twig template :) We Certainly should not add it.

Also, there is no $ in table.twig. Please make sure you have the latest code. I replaced the pixelmord sandbox with the chx sandbox yesterday - so if you do a git pull you should get the latest versions of everything :)

http://drupalcode.org/sandbox/pixelmord/1750250.git/blob/514cc820ff6b494...

Looks like really fixed @andypost

The preprocess code is totally broken.

Tables do not display at all currently.

That is critical, because other blocks cannot be tested.

psynaptic, could you take that one?

Sure, I'll have a go at this tomorrow.

I got this working for headers, rows, and empty. Tables are not yet 100% complete but at least they are working to a good base level now.

http://drupalcode.org/sandbox/pixelmord/1750250.git/commit/c64567f79212c...

I need to figure out what to do about attributes as currently

<table {{ attributes }}>

throws an error:

User error: "id" is an invalid render array key in element_children() (line 6251 of core/includes/common.inc).

It also needs testing on more tables, and other table features implementing. I'm happy to work on this some more but I'm done for today.

I fixed the attributes issue by converting them to the new Attribute system. I did a few test tables in a custom module and tested all the admin tables to ensure things work as expected and everything I've done so far is looking good. There are still other table features that need implementing, basically copying them from theme_table to template_preprocess_table and ensuring they work.

http://drupalcode.org/sandbox/pixelmord/1750250.git/commit/c64567f79212c...

I am adding the latest patch you applied to front-end here to use Dreditor to comment ...

+++ b/core/includes/theme.incundefined
@@ -2886,37 +2887,46 @@ function template_preprocess_table(&$variables) {
+  if (!empty($header)) {
+    foreach ($header as $cell) {
+      $variables['header']['cells'][] = _theme_table_cell($cell, TRUE);
+    }

We can either use _theme_table_cell in the preprocess or call it from within twig as sub-theme once table-cell.twig has been converted. That might be even faster, but that would need testing.

+++ b/core/includes/theme.incundefined
@@ -2886,37 +2887,46 @@ function template_preprocess_table(&$variables) {
+  if (!empty($rows)) {

It would be nice in general if you could add some test code you are using to test this.

We will most probably need it when we try to get this patches into core.

+++ b/core/themes/stark/templates/theme.inc/table.twigundefined
@@ -36,7 +36,7 @@
 #}
-<table {{ attributes }}>
+<table{{ attributes }}>
   {% if caption %}

@@ -44,13 +44,13 @@
     {% if cols|length %}
-      <colgroup {{ colgroup.attributes }}>
+      <colgroup{{ colgroup.attributes }}>
       {% for col in cols %}
...
-      <colgroup {{ colgroup.attributes }}/>
+      <colgroup{{ colgroup.attributes }}/>

@@ -68,7 +68,7 @@
       {% if row.cells|length %}
-        <tr {{ row.attributes }}>
+        <tr{{ row.attributes }}>

You should be using always:

The {- is used to not create some white space and increases readability (imho).

Please see: http://twig.sensiolabs.org/doc/templates.html#whitespace-control

Thank you very much for your work so far. Looking forward to how you'll get the tablesort things in :-).

As this is functional now, moving to "major".

We can either use _theme_table_cell in the preprocess or call it from within twig as sub-theme once table-cell.twig has been converted. That might be even faster, but that would need testing.

I was thinking that we should remove _theme_table_cell and replace it with a Twig template.

It would be nice in general if you could add some test code you are using to test this.

I haven't written any "tests" per se, I've just created a module with tables so I can manually verify things work as expected.

You should be using always:

The {- is used to not create some white space and increases readability (imho).

Please see: http://twig.sensiolabs.org/doc/templates.html#whitespace-control

Ok, sounds good. I didn't realise that hyphen was doing a trim, good to know.

Thank you very much for your work so far. Looking forward to how you'll get the tablesort things in :-).

As this is functional now, moving to "major".

To be honest, I have no idea how this works but I assume it just uses JS so it wouldn't need any structural changes.

I haven't written any "tests" per se, I've just created a module with tables so I can manually verify things work as expected.

Yes and it would be great to attach the module here to do manual testing as well.

To be honest, I have no idea how this works but I assume it just uses JS so it wouldn't need any structural changes.

Yes, only need to copy the code to the preprocess, no need for .twig changes.

Ah, of course, not JS but another query.

This commit breaks the front-end branch for me on admin/modules and admin/people. I used clean install and db.
I get the following error:

Fatal error: Class 'Drupal\Core\Template\ArrayIterator' not found in /Users/anthony/Sites/drupal_8_theme_and_twig_sprint/core/lib/Drupal/Core/Template/Attribute.php on line 132

Yes and it would be great to attach the module here to do manual testing as well.

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

Got table sorting working:

http://drupalcode.org/sandbox/pixelmord/1750250.git/commit/39d93a187f57d...

I've pushed the tablesort example table to my twig-tables test module repo.

#21: I created a core bug for it #1805776: Fatal error: Class 'Drupal\Core\Template\ArrayIterator' not found in core/lib/Drupal/Core/Template/Attribute.php

Back to major now that front-end works again.

There have been a lot of changes to theme_table in HEAD that we don't have yet in this sandbox. maybe we should hold off on doing any more here until we merge with head? See #1779136: Weekly - Merge Twig sandbox with latest HEAD

@psynaptic we're going to need the new table stuff added into twig now that we've merged back together. Should I leave this to you since you have already started on it?

This code is still under heavy work, but I'd like to reference some bugs I just found: #1843724: Preprocess table cell errors;

Sure, I'll try to find time to look at this. Right now I'm a bit swamped.

I haven't merged any of this work yet but looking at fixing some of the obvious bugs. Here's a patch that:
* Makes sure $variables is & referenced in template_preprocess_table
* does the isset() part from #1843724
* changes the first $attributes to $table_attributes so it doesn't get mangled by the other $attribute arrays
* change $responsive to $is_responsive because there is another variable later called $responsive later down the function
* Pump $table_attributes onto $variables['attributes'] for the table to get them.

This doesn't fix everything bug gets the track a little straighter and would like to commit this and keep working on this if it helps out psynaptic?

Ok this patch gets things actually working in stark, but it doesn't play nice with theme_table because I needed another array for the template to spit out the row attributes and it's cells.

{% for cell in row.cells %}

Also, I added in colgroup support to the twig template but Attribute is messing with me and adding to some classes to the cols but not all of them...

Was testing with the colgroup example from:
http://api.drupal.org/api/drupal/includes!theme.inc/function/theme_table/7

And it was the second colgroup's col class="funky" that wasn't rendering.

Give this a go and let me know your changes.

EDIT: I'm having trouble testing this, with some inconsistent behaviour. Example:

User error: "colspan" is an invalid render array key in element_children() (line 6274 of core/includes/common.inc).

I'm curious how you went about testing the table twig template against that example from the API docs, since I'd like to try that.

One super minor typo in the comment in theme.inc:2326

      // Build cols wihtin colgroup
      if (is_array($cols) && !empty($cols)) {
        $variables['colgroups'][$number]['cols'] = array();

@ry5n I just cheated & copied that example array right into the top of the preprocess function for colgroups. Should probably make some tests to help my sanity on that one! It took a while to get all the pieces gelling.

Also I think I have a logic error

if (!empty($headers) && !empty($rows)) {

should just be

if (!empty($headers)) {

To be consistent with what was there before. The sticky and responsive tests never really cared weather there are rows.

One thing that is bothering me and is taking all my urges not to change it is that if there are no rows, the empty message get's pumped into the rows. From a themer point of view it would be nice decide that the empty_message be placed instead of the table in most cases. But would still like the case available to count the columns(read by colspan count) so that the twig themers could reproduce this colspan version that is in core. That way you could check count(rows) in twig and there would sometimes be 1 because of the empty message as it is now. Does that make sense to anybody else but me? -itch fingers

Added a new feature branch to keep track of the changes:

Latest diff:
http://drupalcode.org/sandbox/pixelmord/1750250.git/blobdiff/f2937404046...

Commit:
http://drupalcode.org/sandbox/pixelmord/1750250.git/commit/f6da63dc43e31...

Branch: twig-table-1778968

Latest changes:
http://drupalcode.org/sandbox/pixelmord/1750250.git/commitdiff/5d033b90b...

Diff from the branch start
http://drupalcode.org/sandbox/pixelmord/1750250.git/commitdiff/e1d9e49ff...

Commit:
http://drupalcode.org/sandbox/pixelmord/1750250.git/commit/5d033b90bd194...

Mostly removed all the duplicate parsing from theme_table and refactored the preprocess to work with both twig and theme_table

I did quite a bit of changes here, core hasn't done any theme.inc theme_table changes so nothing to merge from there.

Could someone have a look and see I am on the right path and not wasting my time?
My next step will be writing a bunch of fake tables to see if it's generating the correct output.

So far I have been using views /admin/structure/views and people /admin/people?sort=asc&order=Member for

Next round, added theme_table_cell() to the mix to keep things consistant and make the theme_ functions act as close to the html.twig files as I can.

Now more with more stable! Please test and throw me your bugs.

First thing I notice...
Sort image source missing.

<a href="/d8tts/admin/people?sort=asc&amp;order=Member%20for" class="link">Member for
  <img src="" alt="sort ascending" title="sort ascending" height="13" width="13">
</a>

This is what "fixes" the second colgroup's <col> missing attributes issue. Seems that in side the second colgroup loop the attributes have been popped out from the first scope. This may have to do with how {{ attributes.class }} removes it's class attribute from the next evaluation of {{ attributes }}

Does anybody know where best to put this issue to make a stink about it?

+        {% for col in colgroup.cols %}
+         <col{{ col.attributes }}/>
+        {% endfor %}

is fixed by

+        {% for i,col in colgroup.cols %}
+         <col{{ colgroup.cols[i].attributes }}/>
+        {% endfor %}

diff --git a/core/themes/stark/templates/theme.inc/table.html.twig b/core/themes/stark/templates/theme.inc/table.html.twig
index 08de9bc..4a6e9ec 100644
--- a/core/themes/stark/templates/theme.inc/table.html.twig
+++ b/core/themes/stark/templates/theme.inc/table.html.twig
@@ -41,8 +41,21 @@
     <caption>{{ caption }}</caption>
   {% endif %}
 
-  {# @todo: add colgroups #}
-
+  {# Build the colgroup element. #}
+  {% if colgroups %}
+    {% for colgroup in colgroups %}
+      {% if colgroup.cols %}
+        <colgroup{{ colgroup.attributes }}>
+        {% for i,col in colgroup.cols %} 
+         <col{{ colgroup.cols[i].attributes }}/>
+        {% endfor %}
+        </colgroup>
+      {% else %}
+        <colgroup{{ colgroup.attributes }}/>
+      {% endif %}
+    {% endfor %}
+  {% endif %}
+ 
   {# Build the table header. #}
   {% if header|length %}
     <thead><tr>

re #37 @steveoliver

Related to some preprocessing of the table image sorter... there's my take/issue
http://drupal.org/node/1860858#comment-6820284

re #38 if anybody want's to try to reproduce this, here is how:
https://gist.github.com/4231241

I chucked the dump output in there to show that it's indeed in there, but getting popped off in the second round. I think this is an Attribute popping/Twig for loop context blocker.

I committed the patch from #36 (without the trailing whitespace it contained), just to get tables working for the moment.

There's no errors being thrown, but a few known issues:

1. Sort indicator image src missing (separate issue, see joel's notes above).
2. Colgroups and multiple colgroups needs work (see attributes, as that's all <col>'s contain).
3. theme('table_cell') needs cleanup (see function signature and the 'cell' variable (WTF)).

In general, there's lots of cleanup that needs to be done to make the code more clear and understandable. Also, the API probably (I recommend) needs to change to keep all attributes in an 'attributes' or '#attributes' property instead of things like 'class' and 'colspan' being in their own top level properties for things like cells and headers.

So here's where I'm at with addressing the concerns I posted in #41. Note: does not work. #needslove.

Just some notes on what I'm thinking with the code above:

+++ b/core/includes/theme.inc
@@ -2193,51 +2196,36 @@ function template_preprocess_table(&$variables) {
-  // Format the table columns:
-  if (!empty($colgroups)) {
-    $variables['colgroups'] = array();
-    foreach ($colgroups as $number => $colgroup) {
-      $variables['colgroups'][$number] = array();
-      $colgroup_attributes = array();
-
-      // Check if we're dealing with a simple or complex colgroup
-      if (isset($colgroup['data'])) {
-        foreach ($colgroup as $key => $value) {
-          if ($key == 'data') {
-            $cols = $value;
-          }
-          else {
-            $colgroup_attributes[$key] = $value;
-          }
-        }
-      }
-      else {
-        $cols = $colgroup;
-      }
+  // Prepare table columns.
+  if (!empty($variables['colgroups'])) {
+    foreach ($variables['colgroups'] as $number => $colgroup) {

Generally, I'm trying to stay away from declaring new variables, then setting their values back to the original variables unless necessary.

+++ b/core/includes/theme.inc
@@ -2303,24 +2296,18 @@ function template_preprocess_table(&$variables) {
       else {
-        $cells = $row;
+        $row_cells = $row;

I'm also trying to use unique variable names to be clear about when we're working on header cells or row cells for example.

+++ b/core/includes/theme.inc
@@ -2330,49 +2317,39 @@ function template_preprocess_table(&$variables) {
-  // Set the table attributes
-  $variables['attributes'] = new Attribute($table_attributes);
-
-  // Replace header with parsed header array

@@ -2830,44 +2807,23 @@ function theme_table_cell($variables) {
 function template_preprocess_table_cell(&$variables) {
-
-  // Initialize variables needed by template
-  $variables['header'] = isset($variables['cell']['header']) || FALSE;
-  $variables['content'] = '';
-  $variables['attributes'] = new Attribute();
-
+  if (!empty($variables['attributes'])) {
+    $variables['attributes'] = new Attribute($variables['attributes']);
+  }
+  else {
+    $variables['attributes'] = new Attribute(array());

These top level variables should already have the right data in them, the data I don't think should be nested in 'cell'. Also, casting attributes to Attribute() right out of the gate, so we don't need to (remember to) convert them at the end of the function.

+++ b/core/themes/stark/templates/theme.inc/table-cell.html.twig
--- a/core/themes/stark/templates/theme.inc/table.html.twig
+++ b/core/themes/stark/templates/theme.inc/table.html.twig

+++ b/core/themes/stark/templates/theme.inc/table.html.twig
+++ b/core/themes/stark/templates/theme.inc/table.html.twig
@@ -4,30 +4,29 @@

@@ -4,30 +4,29 @@
  * Default theme implementation to display a table.
  *
  * Available variables:
- * - header: The table headers. Each element of the array can be accessed
- *   with the following keys:
- *   - header.data: The localized title of the table column.
- *   - header.field: The database field represented in the table column
- *     (required if user is to be able to sort on this column).
- *   - header.sort: A default sort order for this column ("asc" or "desc").
- *   - header.attributes: Any html attributes, such as "colspan", to apply
- *     to the column header cell.
- * - rows: The table rows. Every row contains cells that can be accessed with
- *   the following keys:
- *   - row.data: the cells.
- *   - row.attributes: Any html attributes, such as "class", to apply to the
- *     table row.
- *   - row.no_striping: a flag indicating that the row should receive no
- *     'even / odd' styling. Defaults to FALSE.
+ * - attributes: HTML attributes to apply to the <table> tag.
+ * - caption: A localized string for the <caption> tag.
+ * - colgroups: Column groups. Each group contains the following properties:
+ *   - attributes: HTML attributes to apply to the <col> tag.
+ * // 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
+ * - header: Table header cells. Each cell contains the following properties:
+ *   - attributes: HTML attributes to apply to the <th> tag.
+ *   - data: A localized string for the title of the column.
+ *   - field: Field name (required for column sorting).
+ *   - sort: Default sort order for this column ("asc" or "desc").
+ * - sticky: A boolean flag indicating whether to use a "sticky" table header.
+ * - rows: Table rows. Each row contains the following properties:
+ *   - attributes: HTML attributes to apply to the <tr> tag.
+ *   - data: Table cells.
+ *   - no_striping: a flag indicating that the row should receive no
+ *     'even / odd' styling. Defaults to FALSE. // @todo: Deprecate no_striping?
  *     A cell can be either a string or may contain the following keys:
  *     - data: The string to display in the table cell.
  *     - header: Indicates this cell is a header.
  *     - attributes: Any html attributes, such as "colspan", to apply to the
  *       table cell.
- * - attributes: HTML attributes to apply to the table tag.
- * - caption: A localized string to use for the <caption> tag.
- * - colgroups: An array of column groups.
- * - sticky: Use a "sticky" table header.
  * - empty_message: The message to display in an extra row if table does not have any rows.
  *

Really tried to make sense of the twig docblock by cleaning up verbiage and order. I know this is kinda a later (cleanup) task, but it helped me to understand what was/should be going on in preprocess.

+++ b/core/themes/stark/templates/theme.inc/table.html.twig
@@ -36,46 +35,48 @@
 <table{{ attributes }}>
+  {# Caption #}
   {% if caption %}
     <caption>{{ caption }}</caption>
   {% endif %}
 
-  {# Build the colgroup element. #}
+  {# Columns #}
   {% if colgroups %}
     {% 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 %}
- {% endif %}
+      {% if colgroup.cols %}
+        <colgroup{{ colgroup.attributes }}>
+          {% for col in colgroup.cols %}
+            <col{{ col.attributes }}/>
+          {% endfor %}
+        </colgroup>
+      {% else %}
+        <colgroup{{ colgroup.attributes }}/>
+      {% endif %}
+    {% endfor %}
+  {% endif %}
 
-  {# Build the table header. #}
+  {# Header #}
   {% if header %}
-    <thead><tr>
-    {% for cell in header %}
-      {{ theme('table_cell', cell) }} {# no th here since this calls table_cell #}
-    {% endfor %}
-    </tr></thead>
+    <thead>
+      <tr>
+      {%- for cell in header -%}
+        {{ theme('table_cell', cell) }} {# Note: <th> returned by table_cell. #}
+      {%- endfor -%}
+      </tr>
+    </thead>
   {% endif %}
 
-  {# Build the table rows. #}
+  {# Rows #}
   {% if rows %}
     <tbody>
-    {% for row in rows %}
-      <tr{{ row.attributes }}>
-      {% for cell in row.cells %}
-        {{ theme('table_cell', cell) }} {# no td here since this calls table_cell #}
+      {% for row in rows %}
+        <tr{{ row.attributes }}>
+        {%- for cell in row.cells -%}
+          {{ theme('table_cell', cell) }} {# Note: <td> returned by table_cell. #}
+        {%- endfor -%}
+        </tr>
       {% endfor %}
-      </tr>
-    {% endfor %}
     </tbody>
   {% endif %}

Pretty much all just indentation fixes, I think.

That's about it for now.

Committed #38 for a temporary fix and added a comment to it's purpose for now.

@steveoliver Attempting to give #42 love. I agree with the direction you are aiming for. Here are some patches that work, add the cleanup, plus got rid of the // in the twig docblock and added a bit more to what you changed. Plus rolled in the temporary fix and removed some whitespace at the end of lines.

The part that doesn't work is the theme stuff so I will dig deeper there...

Getting a bit closer to your working #42, "the answer to life the universe and everything"

This fixes the th not getting responsive classes, removes ['cell'] and gets rid of that _theme_table_cell and fix the classes for the active sorted table cell

Have a peek at #45 and #46 for code cleanup and fixes

Nice work, Joel. I went as far as I could with template_preprocess_table(), building on my earlier suggestions and your work from 45 and 46.

Attached is my latest (ready to commit as far as I'm concerned) plus the interdiff against your cumulative patches up to #46.

Gonna let someone else RTBC and commit this. c4rl, jen?

+++ b/core/includes/theme.inc
@@ -2159,29 +2159,39 @@ function theme_table($variables) {
+  // Table attributes.
+  // Note: Cast to Attribute() at the bottom.
+  // @todo: Find out why we can't cast to Attribute() here.
+  // i.e. $variables['attributes'] = new Attribute($variables['attributes']);
+  $table_attributes = $variables['attributes'];

Fabianx: Maybe you might know: I tried to set $variables['attributes'] = new Attribute($variables['attributes']) here, and add classes to $variables['attributes'] directly during preprocess. The only way I was able to get the table attributes to work was to cast to Attribute() at the bottom of _preprocess. Since I expect to be able to work with Attribute as an array, another set of eyes might notice I've done something wrong with attributes.

+++ b/core/includes/theme.inc
@@ -2275,45 +2296,53 @@ function template_preprocess_table(&$variables) {
+
+      // Update the header cell with attributes and properties.
+      // @todo: Make sure we're not overwriting or losing any header element
+      // variables.
+      $variables['header'][$col_key] = $cell;

This comment touches on the point(s) I made in earlier comments about overwriting and redefining variables using temporary variables. It made things hard to follow and I wonder if it may be dangerous when we might be wiping out data structures when all we need to do is be modifying them.

+++ b/core/includes/theme.inc
@@ -2159,29 +2159,39 @@ function theme_table($variables) {
 
-  // Empty Message
+  // Empty message.
+  // Set to new variable for convenient checks later.
   $empty_message = $variables['empty'];
+  // Remove 'empty' from template variables and set message to it's own variable
+  // to make sure we don't conflict with Twig's reserved keyword of the same name.
+  unset($variables['empty']);
+  // @todo: API change, rename this variable from 'empty' to 'empty_message'.
+  $variables['empty_message'] = $empty_message;

API Change? Rename this variable from 'empty' to 'empty_message'?

+++ b/core/includes/theme.inc
@@ -2275,45 +2296,53 @@ function template_preprocess_table(&$variables) {
-        // Add classes to attributes
+        // Move cell 'class' into 'attributes'.
+        // @todo: API change so 'class' is already within 'attributes'.
         $cell['attributes']['class'] = $cell['class'];
         unset($cell['class']);

API CHANGE: Do we want to do this, so cell doesn't have just 'class', but 'attributes', which can include 'class' along with any others? If that's considered a feature, we don't need to, because it works as is.

Here is a patch from #48, it fixes colspan (and the other attributes) missing from table cell and some notices due to headers not in table or empty message missing etc.

interdiff noob...

Last API change in #49 was removed in place of a bigger loop to get other attributes. It's nasty still but just an FYI. That's why colspan was missing...

Somewhere in the patching I lost my fancy twig loop fix

+          {#
+            Temporary Fix by http://drupal.org/node/1778968#comment-6820122
+            col.attributes is getting stripped on second loop through
+            prefer this:
+            {% for col in colgroup.cols %}
+              <col{{ col.attributes }}/>
+            {% endfor %}
+          #}
+          {% for i,col in colgroup.cols %}
+            <col{{ colgroup.cols[i].attributes }}/>
+          {% endfor %}

Joel put a Colgroups test table in twig-tables module (/twig-tables path) now.

It seems <col{{ col.attributes }}/> works fine.

This patch leaves your funky loop handling out but cleans up indentation in the table.html.twig template.

Please test and lemme know.

OK, We've still got this issue to work out: #1867090: Nested Twig 'for' loops behaving strange, but I just committed this patch to front-end.

It works. :)

diff --git a/core/themes/stark/templates/theme.inc/table.html.twig b/core/themes/stark/templates/theme.inc/table.html.twig
index 8e8dfc1..4e4492b 100644
--- a/core/themes/stark/templates/theme.inc/table.html.twig
+++ b/core/themes/stark/templates/theme.inc/table.html.twig
@@ -47,6 +47,14 @@
       {% if colgroup.cols %}
         <colgroup{{ colgroup.attributes }}>
         {% for col in colgroup.cols %}
+          {#
+            Use temporary variable for column attributes.  This somehow works
+            around the issue where columns after the first column do not receive
+            attributes.  Try using col.attributes directly, and you'll see.
+            Is this a Twig or an Attribute() issue?
+            @see http://drupal.org/node/1867090
+          #}
+          {% set col_attributes = col.attributes %}
           <col{{ col.attributes }}/>
         {% endfor %}
          </colgroup>

Notice I added this comment about the outstanding issue.

This removes (2) of the (3) total instances of theme() (theme('table_cell')) called in templates, as per #1842160: Consider appropriate usage of theme() from within Twig templates and #1842326: Merge _theme_table_cell() into theme_table().

I made some improvements to #56

I have:

* removed theme_table_cell()
* removed template_preprocess_table_cell()
* fixed hook_theme to remove table_cell

I also brought in most of the changes from this patch on #1842326: Merge _theme_table_cell() into theme_table(), but now that seems to be failing tests :(

I'll try to post a follow-up tomorrow.

Oops, forgot to post patch.

Also meant to mention: Why has the original theme_table() changed so much from HEAD? I didn't understand why, so this patch brings theme_table() to the current HEAD, plus the changes from #1842326: Merge _theme_table_cell() into theme_table() (currently failing tests -- as I said, I'll have a look at this tomorrow).

@c4rl I may have missed something but the reason we have the theme_table_cell is to keep the markup out of PHP and in the template files. Did I miss a convo to this regard, while I was out?

FYI, the attached interdiff is with respect to #56, not #58.

@joelpittet #59 Can you be more specific? That is, reference lines of code whereby _theme_table_cell() is necessary, and how the attached patch does not fulfill those goals. I believe, as I have discovered, if you examine _theme_table_cell() in 8.x HEAD you will see that it is simply a helper callback and its utility outside of theme_table() is not clear nor necessary.

With some revisions, #1842326: Merge _theme_table_cell() into theme_table() has passed tests. I have included the changes from that patch in revisions from steveoliver's #56. You will notice that theme_table() (the original theme function) is modified in this patch to match 8.x HEAD. It is not clear to my why theme_table differs in the sandbox vs 8.x HEAD since we should be only creating preprocessors for the sake of conversion in the sandbox, and handling modifications to the original markup and APIs in the core project. Let me know if I am missing anything, or if this approach seems unclear.

@c4rl #17 and #18 were people were talking about it and at #31 I did the first pass.

The intent was to remove HTML tags from being built in preprocess functions or any PHP functions for that matter. It did offer some theming flexibility, albeit minor, but mostly for consistency and cleanliness.

The intent was to remove HTML tags from being built in preprocess functions or any PHP functions for that matter.

Sure, I agree with you. That's where we want to be with preprocessors. My point is this: _theme_table_cell() isn't necessary in either theme_table() (the old function) nor template_preprocess_table() (the new function). _theme_table_cell() just provided a few lines of HTML, that's it. Please review the API page and have a look look at the patch in #60, namely:

       {% for row in rows %}
-        <tr{{ row.attributes }}>
-        {%- for cell in row.cells -%}
-          {{ theme('table_cell', cell) }} {# Note: <td> returned by table_cell. #}
-        {%- endfor -%}
-        </tr>
+        <tr {{- row.attributes }}>
+        {% for i in row.cells|keys -%}
+          <td {{- row.cells[i].attributes }}>
+              {{- row.cells[i].data -}}
+          </td>
+        {% endfor %}
       {% endfor %}

_theme_table_cell() is just a shortcut to write a few lines of HTML (and, IMHO, an example of when DRY becomes an anti-pattern). So, the patch in #60 does the following:

* In theme_table() refactors calls to _theme_table_cell() to simply mirror its functionality in theme_table(). theme_table() is already concatenating strings together, and will continue to do so until we replace it outright with template_preprocess_table() in the final patch to HEAD. The sandbox has both for the sake of conversion.
* In table.html.twig, provides the markup I pasted in the codeblock above. Preprocessors, in principle, should not contain HTML, as we both acknowledged.

@c4rl, Ok I think I am getting the pieces sorted a bit better now, thanks. The reason behind the theme_table_cell() was for some backward compatibility issues when the twig engine wasn't on, I removed the internal _theme_table_cell() and just replaced it with theme_table_cell(). I noticed you removed the theme_table_cell() in your patch so I was like OH NOES it will break in phptemplate! But now I see where all the pieces landed, the theme_table has the theme_table_cell function stuff embedded and you pulled the th/td into the table. Much better then what I thought happened!

Updated issue summary.

#1939008: Convert theme_table() to Twig