git commit -m "Issue #1939008 by sun, joelpittet, gnuget, Cottser, drupalninja99, mdrummond, sphism, c4rl, steveoliver, andypost, Fabianx, Gokul N K, longwave | jenlampton: Convert theme_table() to Twig."

Task

Use Twig instead of PHPTemplate

Remaining

  • Patch needs review
  • Manual testing

Testing steps

Visit admin/structure/types and check the markup for the table displayed.

Files: 
CommentFileSizeAuthor
#133 interdiff.txt2.37 KBsun
#133 theme.table_.133.patch19.23 KBsun
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 70,233 pass(es). View
#131 interdiff.txt3.97 KBsun
#131 theme.table_.131.patch16.86 KBsun
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 70,255 pass(es), 10 fail(s), and 0 exception(s). View
#129 interdiff.txt15.14 KBsun
#129 theme.table_.129.patch16.75 KBsun
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 69,970 pass(es). View
#123 theme_table_reroll.patch19.19 KBgokulnk
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Invalid PHP syntax in core/includes/theme.inc. View
#122 d8_tables_patch120-reformatted.html_.txt1.91 KBsphism
#122 d8_tables_patch120.html_.txt3.23 KBsphism
#122 d8_tables-reformatted.html_.txt1.91 KBsphism
#122 d8_tables.html_.txt1.79 KBsphism
#122 after_patch120.png31.05 KBsphism
#122 before_patching.png31.14 KBsphism
#121 1939008-theme-table-reroll-120.patch19.68 KBjoelpittet
PASSED: [[SimpleTest]]: [MySQL] 64,518 pass(es). View
#120 Screen Shot 2014-02-20 at 17.04.14.png31.14 KBsphism
#116 1939008-theme-table-115.patch21.61 KBjoelpittet
PASSED: [[SimpleTest]]: [MySQL] 64,169 pass(es). View
#116 interdiff.txt3.87 KBjoelpittet
#114 interdiff-1939008-111-113.txt3.16 KBmdrummond
#114 convert-theme-table-1939008-113.patch21.39 KBmdrummond
FAILED: [[SimpleTest]]: [MySQL] 64,210 pass(es), 1 fail(s), and 0 exception(s). View
#111 interdiff-1939008-107-111.txt5.23 KBmdrummond
#111 convert-theme-table-1939008-111.patch21.32 KBmdrummond
FAILED: [[SimpleTest]]: [MySQL] 64,210 pass(es), 1 fail(s), and 0 exception(s). View
#107 1939008-theme-table-107.patch19.92 KBjoelpittet
PASSED: [[SimpleTest]]: [MySQL] 63,778 pass(es). View
#102 1939008-twig-table-102.patch20.68 KBlongwave
PASSED: [[SimpleTest]]: [MySQL] 59,868 pass(es). View

Comments

jenlampton’s picture

Issue summary: View changes

top 5

Cottser’s picture

Title: Convert the most used / most important theme functions to Twig » Convert theme_table() to Twig

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

Cottser’s picture

Issue summary: View changes

This issue will now be for converting theme_table().

Cottser’s picture

psynaptic’s picture

Assigned: Unassigned » psynaptic
psynaptic’s picture

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

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

psynaptic’s picture

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.

Cottser’s picture

@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().

Cottser’s picture

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

psynaptic’s picture

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

Cottser’s picture

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

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

psynaptic’s picture

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

psynaptic’s picture

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

tlattimore’s picture

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

psynaptic’s picture

@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.

joelpittet’s picture

Assigned: psynaptic » joelpittet
Status: Active » Needs review
FileSize
21.28 KB
FAILED: [[SimpleTest]]: [MySQL] 53,293 pass(es), 3 fail(s), and 0 exception(s). View

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.

Status: Needs review » Needs work

The last submitted patch, theme-table-twig-1939008-14.patch, failed testing.

joelpittet’s picture

Status: Needs work » Needs review
FileSize
21.46 KB
FAILED: [[SimpleTest]]: [MySQL] 53,272 pass(es), 1 fail(s), and 0 exception(s). View
1013 bytes

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.

Status: Needs review » Needs work

The last submitted patch, theme-table-twig-1939008-16.patch, failed testing.

joelpittet’s picture

FileSize
1.19 KB
21.46 KB
FAILED: [[SimpleTest]]: [MySQL] 53,282 pass(es), 1 fail(s), and 0 exception(s). View

missing ending tr and some whitespace mods.

joelpittet’s picture

Status: Needs work » Needs review

activate

joelpittet’s picture

FileSize
1.19 KB
21.44 KB
PASSED: [[SimpleTest]]: [MySQL] 53,289 pass(es). View

darn wrong patch....

Cottser’s picture

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

Cottser’s picture

FileSize
9.43 KB
21.49 KB
PASSED: [[SimpleTest]]: [MySQL] 53,611 pass(es). View

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.

Cottser’s picture

FileSize
655 bytes
21.5 KB
PASSED: [[SimpleTest]]: [MySQL] 53,621 pass(es). View
Cottser’s picture

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.

fietserwin’s picture

Status: Needs review » Needs work

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.

Cottser’s picture

Status: Needs work » Needs review

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

Cottser’s picture

Issue tags: +Needs manual testing
FileSize
1006 bytes
21.17 KB
PASSED: [[SimpleTest]]: [MySQL] 54,226 pass(es). View
  • 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.

Cottser’s picture

Issue summary: View changes

Adding sandbox issue to related issues

a.ross’s picture

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

Cottser’s picture

Status: Needs review » Needs work
Issue tags: +Novice, +Needs reroll

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

fietserwin’s picture

Cottser’s picture

Assigned: joelpittet » Unassigned

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

gnuget’s picture

Assigned: Unassigned » gnuget
gnuget’s picture

Status: Needs work » Needs review
FileSize
21.18 KB
FAILED: [[SimpleTest]]: [MySQL] 54,508 pass(es), 1 fail(s), and 0 exception(s). View

Reroll version of #27

gnuget’s picture

FileSize
21.04 KB
PASSED: [[SimpleTest]]: [MySQL] 54,496 pass(es). View

A fixed version of my previous patch.

gnuget’s picture

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.

Cottser’s picture

Status: Needs review » Needs work

@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?

gnuget’s picture

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 = "$data";
  }
  else {
    $output = "$data";
  }

  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.

Cottser’s picture

Issue tags: -Novice, -Needs reroll

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.

joelpittet’s picture

Status: Needs work » Needs review
FileSize
21.11 KB
PASSED: [[SimpleTest]]: [MySQL] 54,363 pass(es). View
1.22 KB

@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.

Cottser’s picture

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

gnuget’s picture

FileSize
21.18 KB
PASSED: [[SimpleTest]]: [MySQL] 54,463 pass(es). View
1.58 KB

@joelpittet

I fixed the regression, interdiff and new patch attached.

c4rl’s picture

Status: Needs review » Needs work

Closing tag needs to match.

joelpittet’s picture

+++ 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!

c4rl’s picture

Status: Needs work » Needs review
FileSize
738 bytes
21.24 KB
PASSED: [[SimpleTest]]: [MySQL] 54,499 pass(es). View

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.

gnuget’s picture

Status: Needs review » Needs work
FileSize
1.65 KB
21.23 KB
FAILED: [[SimpleTest]]: [MySQL] 54,668 pass(es), 1 fail(s), and 0 exception(s). View

duh! this is embarrassing, sorry.

New patch and interdiff attached.

Regards

c4rl’s picture

Status: Needs work » Needs review

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.

Status: Needs review » Needs work
Issue tags: -Needs manual testing, -Twig

The last submitted patch, twig-table-1939008-44.patch, failed testing.

gnuget’s picture

Status: Needs work » Needs review

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

Status: Needs review » Needs work
Issue tags: +Needs manual testing, +Twig

The last submitted patch, twig-table-1939008-44.patch, failed testing.

gnuget’s picture

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

a.ross’s picture

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

gnuget’s picture

Status: Needs work » Needs review
Issue tags: -Needs manual testing, -Twig

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

Status: Needs review » Needs work
Issue tags: +Needs manual testing, +Twig

The last submitted patch, twig-table-1939008-44.patch, failed testing.

joelpittet’s picture

Status: Needs work » Needs review
FileSize
21.23 KB
PASSED: [[SimpleTest]]: [MySQL] 54,499 pass(es). View
635 bytes

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

c4rl’s picture

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.

joelpittet’s picture

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

joelpittet’s picture

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

joelpittet’s picture

FileSize
2.38 KB
20.73 KB
PASSED: [[SimpleTest]]: [MySQL] 54,733 pass(es). View

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

Status: Needs review » Needs work
Issue tags: -Needs manual testing, -Twig

The last submitted patch, 1939008-58-theme-table.patch, failed testing.

joelpittet’s picture

Status: Needs work » Needs review
Issue tags: +Needs manual testing, +Twig

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

joelpittet’s picture

Issue summary: View changes

Update remaining, add testing steps

c4rl’s picture

Assigned: gnuget » Unassigned
gnuget’s picture

FileSize
1.21 KB
21.45 KB
PASSED: [[SimpleTest]]: [MySQL] 55,386 pass(es). View

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

steveoliver’s picture

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?

c4rl’s picture

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

steveoliver’s picture

ah, yep. two tags. duh.

steveoliver’s picture

Issue summary: View changes

Add related issue

Bojhan’s picture

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

joelpittet’s picture

@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.

Cottser’s picture

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

Wim Leers’s picture

Status: Needs review » Needs work

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.

drupalninja99’s picture

Status: Needs work » Needs review
FileSize
5.49 KB
20.08 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch 1939008-70-theme-table.patch. Unable to apply patch. See the log in the details link for more information. View

@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.

Status: Needs review » Needs work

The last submitted patch, 1939008-70-theme-table.patch, failed testing.

joelpittet’s picture

FileSize
21.41 KB
FAILED: [[SimpleTest]]: [MySQL] 55,726 pass(es), 1 fail(s), and 4,518 exception(s). View

@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.

joelpittet’s picture

Status: Needs work » Needs review
FileSize
8.46 KB
20.04 KB
FAILED: [[SimpleTest]]: [MySQL] 55,822 pass(es), 1 fail(s), and 4,511 exception(s). View

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.

joelpittet’s picture

FileSize
19.64 KB
PASSED: [[SimpleTest]]: [MySQL] 55,386 pass(es). View
4.48 KB

Here is some further cleanup:

joelpittet’s picture

Issue tags: +Needs profiling

@drupalninja99 thank you very much for the patch btw!

Tagging

joelpittet’s picture

Status: Needs review » Needs work

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&...

drupalninja99’s picture

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

joelpittet’s picture

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

drupalninja99’s picture

Status: Needs work » Needs review
FileSize
5.49 KB
19.64 KB
PASSED: [[SimpleTest]]: [MySQL] 56,568 pass(es). View

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.

joelpittet’s picture

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

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

joelpittet’s picture

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
joelpittet’s picture

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

drupalninja99’s picture

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.

drupalninja99’s picture

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().

Cottser’s picture

Status: Needs review » Needs work
Issue tags: +Needs reroll

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

joelpittet’s picture

Status: Needs work » Needs review
Issue tags: -Needs profiling, -Needs reroll
FileSize
19.48 KB
PASSED: [[SimpleTest]]: [MySQL] 57,873 pass(es). View

Re-rolled. Already profiled in #76

Cottser’s picture

Status: Needs review » Needs work
Issue tags: +Needs reroll

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

joelpittet’s picture

Issue tags: -Needs reroll
FileSize
19.42 KB
PASSED: [[SimpleTest]]: [MySQL] 58,392 pass(es). View
19.45 KB
PASSED: [[SimpleTest]]: [MySQL] 58,468 pass(es). View

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

joelpittet’s picture

Status: Needs work » Needs review

Bot! go

joelpittet’s picture

FileSize
19.59 KB
FAILED: [[SimpleTest]]: [MySQL] 59,239 pass(es), 2 fail(s), and 0 exception(s). View

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.

joelpittet’s picture

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

Status: Needs review » Needs work

The last submitted patch, 1939008-90-reroll-theme-table.patch, failed testing.

joelpittet’s picture

Assigned: Unassigned » joelpittet
Status: Needs work » Postponed

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.

joelpittet’s picture

Issue summary: View changes

Remove sandbox link

joelpittet’s picture

Issue summary: View changes
Status: Postponed » Needs review
Parent issue: » #1757550: [Meta] Convert core theme functions to Twig templates
Related issues: +#1778968: Convert theme_table to Twig, +#1885560: [meta] Convert everything in theme.inc to Twig, +#1812374: Add preprocess to theme_table function to simplify it
FileSize
19.59 KB
FAILED: [[SimpleTest]]: [MySQL] 60,039 pass(es), 1 fail(s), and 0 exception(s). View

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

Status: Needs review » Needs work

The last submitted patch, 94: 1939008-94-theme-table.patch, failed testing.

The last submitted patch, 94: 1939008-94-theme-table.patch, failed testing.

joelpittet’s picture

Status: Needs work » Needs review

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

joelpittet’s picture

FileSize
2.33 KB
20.73 KB
PASSED: [[SimpleTest]]: [MySQL] 60,152 pass(es). View

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

The last submitted patch, 94: 1939008-94-theme-table.patch, failed testing.

joelpittet’s picture

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&...

Cottser’s picture

Status: Needs review » Needs work
Issue tags: +Needs reroll

Tagging for reroll.

longwave’s picture

Status: Needs work » Needs review
Issue tags: -Needs reroll
FileSize
20.68 KB
PASSED: [[SimpleTest]]: [MySQL] 59,868 pass(es). View

Rerolled.

dawehner’s picture

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?

joelpittet’s picture

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.

joelpittet’s picture

@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.

Cottser’s picture

Issue tags: +Twig conversion
joelpittet’s picture

FileSize
19.92 KB
PASSED: [[SimpleTest]]: [MySQL] 63,778 pass(es). View

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.

Status: Needs review » Needs work

The last submitted patch, 107: 1939008-theme-table-107.patch, failed testing.

joelpittet’s picture

Status: Needs work » Needs review

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

joelpittet’s picture

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.

mdrummond’s picture

FileSize
21.32 KB
FAILED: [[SimpleTest]]: [MySQL] 64,210 pass(es), 1 fail(s), and 0 exception(s). View
5.23 KB

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.

joelpittet’s picture

+++ 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:)

Status: Needs review » Needs work

The last submitted patch, 111: convert-theme-table-1939008-111.patch, failed testing.

mdrummond’s picture

Status: Needs work » Needs review
FileSize
21.39 KB
FAILED: [[SimpleTest]]: [MySQL] 64,210 pass(es), 1 fail(s), and 0 exception(s). View
3.16 KB

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

Status: Needs review » Needs work

The last submitted patch, 114: convert-theme-table-1939008-113.patch, failed testing.

joelpittet’s picture

Status: Needs work » Needs review
FileSize
3.87 KB
21.61 KB
PASSED: [[SimpleTest]]: [MySQL] 64,169 pass(es). View

@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.

mdrummond’s picture

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

joelpittet’s picture

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?

sphism’s picture

I'm taking a look at this now

sphism’s picture

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

joelpittet’s picture

FileSize
19.68 KB
PASSED: [[SimpleTest]]: [MySQL] 64,518 pass(es). View

reroll

sphism’s picture

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

gokulnk’s picture

FileSize
19.19 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Invalid PHP syntax in core/includes/theme.inc. View

The patch was not applying.

I am adding the Re-roll.

Status: Needs review » Needs work

The last submitted patch, 123: theme_table_reroll.patch, failed testing.

jessebeach’s picture

#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!

joelpittet’s picture

Status: Needs work » Postponed

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.

sun’s picture

+++ 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...?

joelpittet’s picture

@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: Grouping of rows in theme_table()

sun’s picture

Assigned: joelpittet » sun
Status: Postponed » Needs review
Issue tags: -Needs manual testing
FileSize
16.75 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 69,970 pass(es). View
15.14 KB
  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.
joelpittet’s picture

@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

sun’s picture

FileSize
16.86 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 70,255 pass(es), 10 fail(s), and 0 exception(s). View
3.97 KB
  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.

Status: Needs review » Needs work

The last submitted patch, 131: theme.table_.131.patch, failed testing.

sun’s picture

Status: Needs work » Needs review
FileSize
19.23 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 70,233 pass(es). View
2.37 KB

Fixed NodeTypeRenameConfigImportTest relies on exact HTML markup.

joelpittet’s picture

Issue summary: View changes
Status: Needs review » Reviewed & tested by the community

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.

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

sun’s picture

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.

webchick’s picture

Status: Reviewed & tested by the community » Needs work
  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?

webchick’s picture

Status: Needs work » Fixed

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!

  • Commit 2250c09 on 8.x by webchick:
    Issue #1939008 by sun, joelpittet, gnuget, Gokul N K, sphism,...
Mile23’s picture

Status: Fixed » Needs work

SRSLY needs a change record. :-)

joelpittet’s picture

Status: Needs work » Needs review

@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?
sun’s picture

Status: Needs review » Fixed

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.

joelpittet’s picture

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.

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.