Follow-up from #1939008: Convert theme_table() to Twig.

Splitting this off from the theme_table to twig conversion.

Files: 
CommentFileSizeAuthor
#20 interdiff.txt3.26 KBjoelpittet
#20 2216407-remove-_theme_table_cell-20.patch8.55 KBjoelpittet
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 67,096 pass(es), 0 fail(s), and 41,266 exception(s). View
#17 diff.txt392 bytespakmanlh
#17 2216407-remove-_theme_table_cell-17.patch8.75 KBpakmanlh
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 66,502 pass(es), 0 fail(s), and 41,087 exception(s). View
#15 interdiff.txt1.15 KBjoelpittet
#15 2216407-remove-_theme_table_cell-15.patch8.57 KBjoelpittet
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 66,500 pass(es), 0 fail(s), and 41,087 exception(s). View
#8 interdiff.txt520 bytesjjcarrion
#6 screenshot_tables.png32.08 KBjjcarrion
#6 2216407-remove-_theme_table_cell-6.patch8.58 KBjjcarrion
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 66,386 pass(es), 1 fail(s), and 40,987 exception(s). View
#1 2216407-remove-_theme_table_cell-1.patch8.56 KBjoelpittet
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 65,299 pass(es). View

Comments

joelpittet’s picture

Status: Active » Needs review
FileSize
8.56 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 65,299 pass(es). View

Here's the patch

Status: Needs review » Needs work

The last submitted patch, 1: 2216407-remove-_theme_table_cell-1.patch, failed testing.

joelpittet’s picture

Status: Needs work » Needs review
barraponto’s picture

+++ b/core/includes/theme.inc
@@ -1602,9 +1602,19 @@ function theme_table($variables) {
     $output .= (count($rows) ? ' <thead><tr>' : ' <tr>');
...
+      $is_header = TRUE;
+      $is_header |= isset($cell['header']);

I think I never saw |= in Drupal before. What does it mean? Is it needed?

joelpittet’s picture

+++ b/core/includes/theme.inc
@@ -1857,47 +1900,6 @@ function template_preprocess_container(&$variables) {
-    $header |= isset($cell['header']);

It's here, though it's a shortcut that is rarely used. I could go with the long version for clarity:

// If the header flag is set, use it's value otherwise default to TRUE.
$is_header = isset($cell['header']) ? $cell['header'] : TRUE;

Thanks for the review @barraponto!

jjcarrion’s picture

FileSize
8.58 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 66,386 pass(es), 1 fail(s), and 40,987 exception(s). View
32.08 KB

I've maked the patch with the little modification. It works good on my tables as a show in a screenshot.

Status: Needs review » Needs work

The last submitted patch, 6: 2216407-remove-_theme_table_cell-6.patch, failed testing.

jjcarrion’s picture

Status: Needs work » Needs review
FileSize
520 bytes

Here is the interdiff.

jjcarrion’s picture

Status: Needs review » Needs work

The last submitted patch, 6: 2216407-remove-_theme_table_cell-6.patch, failed testing.

jjcarrion’s picture

Assigned: joelpittet » jjcarrion
Status: Needs work » Needs review
jjcarrion’s picture

Status: Needs review » Needs work
jjcarrion’s picture

Assigned: jjcarrion » Unassigned
jjcarrion’s picture

The patch #6 breaks the testing page /admin/config/development/testing. I don't know how to fix it.

joelpittet’s picture

Status: Needs work » Needs review
FileSize
8.57 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 66,500 pass(es), 0 fail(s), and 41,087 exception(s). View
1.15 KB

Thanks @jjcarrion for the fix. I think that is a random testbot fail... but not sure... I touched up what you did there.

Wondering if maybe you could compare this patch against a duplicate issue and see if there is anything in it that needs to be merged into this one?

#1842326: Merge _theme_table_cell() into theme_table()

Status: Needs review » Needs work

The last submitted patch, 15: 2216407-remove-_theme_table_cell-15.patch, failed testing.

pakmanlh’s picture

Status: Needs work » Needs review
FileSize
8.75 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 66,502 pass(es), 0 fail(s), and 41,087 exception(s). View
392 bytes

After a revision I think that all the functionality of the #1842326: Merge _theme_table_cell() into theme_table() issue are in. Only I modified a comment that made references at the removed function _theme_table_cell.

Status: Needs review » Needs work

The last submitted patch, 17: 2216407-remove-_theme_table_cell-17.patch, failed testing.

pakmanlh’s picture

joelpittet’s picture

Status: Needs work » Needs review
FileSize
8.55 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 67,096 pass(es), 0 fail(s), and 41,266 exception(s). View
3.26 KB

I cleaned up a few mistakes and docs but they aren't the exception fix I'm looking for... will tackle this more tomorrow.

The last submitted patch, 17: 2216407-remove-_theme_table_cell-17.patch, failed testing.

Status: Needs review » Needs work

The last submitted patch, 20: 2216407-remove-_theme_table_cell-20.patch, failed testing.

joelpittet’s picture

The last submitted patch, 20: 2216407-remove-_theme_table_cell-20.patch, failed testing.

sun’s picture

Actually, the patch in #1842326: Merge _theme_table_cell() into theme_table() looks more sane to me... (unless that patch misses something, which is possible since seemingly outdated...)

sun’s picture

Status: Needs work » Closed (duplicate)

I worked on this, and ultimately cherry-picked code from both issues/patches...

...since this issue is newer, marking as duplicate of the older one.