This issue can be closed when #1939008: Convert theme_table() to Twig is committed.
Problem/Motivation
Passing an Attribute object to theme_table()
without a class array throws exceptions when sticky headers or responsive headers are used.
This is an example of a render array that would throw exceptions:
$attributes = new Attribute(array('id' => 'blocks'));
drupal_add_tabledrag('blocks', 'order', 'sibling', 'block-weight');
$variables['table'] = array(
'#theme' => 'table',
'#header' => $header,
'#rows' => $rows,
'#attributes' => $attributes,
);
Relevant snippet from the Attribute change notice:
Classes are an example to use array for. When using array, you must initialize the array first before adding items.
$variables['item_attribute'] = new Attribute(); $variables['item_attribute']['class'] = array(); $variables['item_attribute']['class'][] = 'class-1';
Relevant code from theme_table()
:
// Add sticky headers, if applicable.
if (count($header) && $sticky) {
drupal_add_library('system', 'drupal.tableheader');
// Add 'sticky-enabled' class to the table to identify it for JS.
// This is needed to target tables constructed by this function.
$attributes['class'][] = 'sticky-enabled';
}
// 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.
if (count($header) && $responsive) {
drupal_add_library('system', 'drupal.tableresponsive');
// Add 'responsive-enabled' class to the table to identify it for JS.
// This is needed to target tables constructed by this function.
$attributes['class'][] = 'responsive-enabled';
}
Proposed resolution
In theme_table()
, initialize $attributes['class']
to an empty array if it is not already set.
Remaining tasks
TBD
User interface changes
n/a
API changes
n/a
Related Issues
#1898034: block.module - Convert PHPTemplate templates to Twig
#1898416: filter.module - Convert theme_ functions to Twig
Comment | File | Size | Author |
---|---|---|---|
#8 | 1868212-8.patch | 613 bytes | star-szr |
#6 | 1868212-6.patch | 1.32 KB | star-szr |
#6 | interdiff.txt | 1.42 KB | star-szr |
#3 | 1868212-3.patch | 1.36 KB | star-szr |
Comments
Comment #1
steveoliver CreditAttribution: steveoliver commentedCommitted this fix in d49269e:
Comment #3
star-szrMoving to core queue with a patch and bumping to major, this is blocking at least these two conversions:
#1898034: block.module - Convert PHPTemplate templates to Twig
#1898416: filter.module - Convert theme_ functions to Twig
Comment #4
star-szrOkay, this might be handled in the theme.inc conversion, stay tuned and sorry for the noise. Downgrading priority for now.
Comment #6
star-szrOops, picked the wrong patch to extract this change from. This one should pass testing.
Comment #7
star-szrSee #1939008: Convert theme_table() to Twig for the theme.inc conversion issue. This patch could maybe be incorporated there.
Comment #8
star-szrSimpler patch after having some time to review the Attribute object change notice: http://drupal.org/node/1727592
Comment #9
star-szrMaybe the comment could make reference to the Attribute object and even have a link to the change notice as well. Not sure if that's overkill.
Comment #10
star-szrAfter looking into this further, this
patchissue may not be blocking Twig conversion after all. The conversions that are throwing exceptions should be fine after #1938904: Convert filter theme tables to table #type and #1938898: Convert block theme tables to table #type land. Still might not be a bad idea to add this, though, because it can cause problems with render arrays that use'#theme' => 'table'
.This code would throw exceptions:
This doesn't throw exceptions, but IMO the empty class array shouldn't be needed:
Comment #11
star-szrRe-titling.
Comment #11.0
star-szrAdding blocking issues to summary
Comment #11.1
star-szrUpdating issue summary to use template and reflect current issue status.
Comment #12
steveoliver CreditAttribution: steveoliver commentedI'd keep this titled as the error message people will see, and then search for once they hit this issue.
...and close this, since the fix is simple: initialize a variable/element before you use it.
Comment #13
star-szrSorry for the confusion. This patch would really only make sense if Twig conversion wasn't removing theme_table(). But because theme_table() conversion is already under way, this fix can be rolled in there.
So for now, postponing on #1939008: Convert theme_table() to Twig. I'll update the issue summary here and revise the patch at #1939008: Convert theme_table() to Twig to include this logic.
Comment #13.0
star-szrAdd example of a render array that would throw exceptions.
Comment #14
star-szrClosing this out now…
Comment #14.0
star-szrAdd note about why this issue is postponed.