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

Splitting this off from the theme_table to twig conversion.

CommentFileSizeAuthor
#20 interdiff.txt3.26 KBjoelpittet
#20 2216407-remove-_theme_table_cell-20.patch8.55 KBjoelpittet
#17 diff.txt392 bytespakmanlh
#17 2216407-remove-_theme_table_cell-17.patch8.75 KBpakmanlh
#15 interdiff.txt1.15 KBjoelpittet
#15 2216407-remove-_theme_table_cell-15.patch8.57 KBjoelpittet
#8 interdiff.txt520 bytesjjcarrion
#6 screenshot_tables.png32.08 KBjjcarrion
#6 2216407-remove-_theme_table_cell-6.patch8.58 KBjjcarrion
#1 2216407-remove-_theme_table_cell-1.patch8.56 KBjoelpittet
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

joelpittet’s picture

joelpittet
Primary language English
Location Vancouver
CreditAttribution: joelpittet commented
Status: Active » Needs review
FileSize
2216407-remove-_theme_table_cell-1.patch8.56 KB

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

joelpittet
Primary language English
Location Vancouver
CreditAttribution: joelpittet commented
Status: Needs work » Needs review

1: 2216407-remove-_theme_table_cell-1.patch queued for re-testing.

barraponto’s picture

barraponto CreditAttribution: barraponto commented 
+++ 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

joelpittet
Primary language English
Location Vancouver
CreditAttribution: joelpittet commented 
+++ 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

jjcarrion
Primary language Spanish
Location Spain
CreditAttribution: jjcarrion commented
FileSize
2216407-remove-_theme_table_cell-6.patch8.58 KB
screenshot_tables.png32.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

jjcarrion
Primary language Spanish
Location Spain
CreditAttribution: jjcarrion commented
Status: Needs work » Needs review
FileSize
interdiff.txt520 bytes

Here is the interdiff.

jjcarrion’s picture

jjcarrion
Primary language Spanish
Location Spain
CreditAttribution: jjcarrion commented

6: 2216407-remove-_theme_table_cell-6.patch queued for re-testing.

Status: Needs review » Needs work

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

jjcarrion’s picture

jjcarrion
Primary language Spanish
Location Spain
CreditAttribution: jjcarrion commented
Assigned: joelpittet » jjcarrion
Status: Needs work » Needs review
jjcarrion’s picture

jjcarrion
Primary language Spanish
Location Spain
CreditAttribution: jjcarrion commented
Status: Needs review » Needs work
jjcarrion’s picture

jjcarrion
Primary language Spanish
Location Spain
CreditAttribution: jjcarrion commented
Assigned: jjcarrion » Unassigned
jjcarrion’s picture

jjcarrion
Primary language Spanish
Location Spain
CreditAttribution: jjcarrion commented

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

joelpittet’s picture

joelpittet
Primary language English
Location Vancouver
CreditAttribution: joelpittet commented
Status: Needs work » Needs review
FileSize
2216407-remove-_theme_table_cell-15.patch8.57 KB
interdiff.txt1.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

pakmanlh
he/him
Primary language Catalan
Location Barcelona
CreditAttribution: pakmanlh commented
Status: Needs work » Needs review
FileSize
2216407-remove-_theme_table_cell-17.patch8.75 KB
diff.txt392 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

pakmanlh
he/him
Primary language Catalan
Location Barcelona
CreditAttribution: pakmanlh commented

17: 2216407-remove-_theme_table_cell-17.patch queued for re-testing.

joelpittet’s picture

joelpittet
Primary language English
Location Vancouver
CreditAttribution: joelpittet commented
Status: Needs work » Needs review
FileSize
2216407-remove-_theme_table_cell-20.patch8.55 KB
interdiff.txt3.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

joelpittet
Primary language English
Location Vancouver
CreditAttribution: joelpittet commented

20: 2216407-remove-_theme_table_cell-20.patch queued for re-testing.

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

sun’s picture

sun
Primary language German
Location Karlsruhe
CreditAttribution: sun commented

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

sun
Primary language German
Location Karlsruhe
CreditAttribution: sun commented
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.