Notice: Indirect modification of overloaded element of Drupal\Core\Template\Attribute has no effect (theme_table) [#1868212]

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

#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
#8
#6	1868212-6.patch	1.32 KB	star-szr
#6
#6	interdiff.txt	1.42 KB	star-szr
#3	1868212-3.patch	1.36 KB	star-szr
#3

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Comment #1

steveoliver CreditAttribution: steveoliver commented 17 December 2012 at 07:17

Status:

Active

» Fixed

Committed this fix in d49269e:

diff --git a/core/includes/theme.inc b/core/includes/theme.inc
index 5b7bc50..f6e8e88 100644
--- a/core/includes/theme.inc
+++ b/core/includes/theme.inc
@@ -2199,7 +2199,12 @@ function template_preprocess_table(&$variables) {
       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.
-      $table_attributes['class'][] = 'sticky-enabled';
+      if (isset($table_attributes['class'])) {
+        $table_attributes['class'][] = 'sticky-enabled';
+      }
+      else {
+        $table_attributes['class'] = array('sticky-enabled');
+      }
     }
 
     // If the table has headers and it should react responsively to columns hidden
@@ -2209,7 +2214,12 @@ function template_preprocess_table(&$variables) {
       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.
-      $table_attributes['class'][] = 'responsive-enabled';
+      if (isset($table_attributes['class'])) {
+        $table_attributes['class'][] = 'responsive-enabled';
+      }
+      else {
+        $table_attributes['class'] = array('responsive-enabled');
+      }
     }
   }

Comment #2

31 December 2012 at 07:20

Status:

Fixed

» Closed (fixed)

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

Comment #3

star-szr

he/him

English

CreditAttribution: star-szr commented 10 March 2013 at 21:22

Project:		» Drupal core
Version:		» 8.x-dev
Component:	Twig templates conversion (front-end branch)	» theme system
Assigned:	steveoliver	» star-szr
Priority:	Normal	» Major
Status:	Closed (fixed)	» Needs review
Issue tags:		+Twig

File	Size
1868212-3.patch	1.36 KB

Moving 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-szr

he/him

English

CreditAttribution: star-szr commented 10 March 2013 at 21:35

Priority:

Major

» Normal

Okay, this might be handled in the theme.inc conversion, stay tuned and sorry for the noise. Downgrading priority for now.

Comment #5

10 March 2013 at 23:59

Status:

Needs review

» Needs work

The last submitted patch, 1868212-3.patch, failed testing.

Comment #6

star-szr

he/him

English

CreditAttribution: star-szr commented 11 March 2013 at 00:06

Status:

Needs work

» Needs review

File	Size
interdiff.txt	1.42 KB
1868212-6.patch	1.32 KB

Oops, picked the wrong patch to extract this change from. This one should pass testing.

Comment #7

star-szr

he/him

English

CreditAttribution: star-szr commented 11 March 2013 at 04:25

See #1939008: Convert theme_table() to Twig for the theme.inc conversion issue. This patch could maybe be incorporated there.

Comment #8

star-szr

he/him

English

CreditAttribution: star-szr commented 29 March 2013 at 19:24

File	Size
1868212-8.patch	613 bytes

Simpler patch after having some time to review the Attribute object change notice: http://drupal.org/node/1727592

Comment #9

star-szr

he/him

English

CreditAttribution: star-szr commented 29 March 2013 at 19:30

Maybe 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-szr

he/him

English

CreditAttribution: star-szr commented 29 March 2013 at 20:01

After looking into this further, this ~~patch~~ issue 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:

$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,
);

This doesn't throw exceptions, but IMO the empty class array shouldn't be needed:

$attributes = new Attribute(array('id' => 'blocks', 'class' => array()));
drupal_add_tabledrag('blocks', 'order', 'sibling', 'block-weight');
$variables['table'] = array(
  '#theme' => 'table',
  '#header' => $header,
  '#rows' => $rows,
  '#attributes' => $attributes,
);

Comment #11

star-szr

he/him

English

CreditAttribution: star-szr commented 29 March 2013 at 20:02

Title:

Notice: Indirect modification of overloaded element of Drupal\Core\Template\Attribute has no effect in template_preprocess_table

» Passing an Attribute object to theme_table() without a class array throws exceptions

Re-titling.

Comment #11.0

star-szr

he/him

English

CreditAttribution: star-szr commented 29 March 2013 at 20:02

Issue summary:

View changes

Adding blocking issues to summary

Comment #11.1

star-szr

he/him

English

CreditAttribution: star-szr commented 29 March 2013 at 20:15

Issue summary:

View changes

Updating issue summary to use template and reflect current issue status.

Comment #12

steveoliver CreditAttribution: steveoliver commented 31 March 2013 at 01:12

Title:	Passing an Attribute object to theme_table() without a class array throws exceptions	» Notice: Indirect modification of overloaded element of Drupal\Core\Template\Attribute has no effect
Status:	Needs review	» Fixed

I'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-szr

he/him

English

CreditAttribution: star-szr commented 31 March 2013 at 01:45

Title:	Notice: Indirect modification of overloaded element of Drupal\Core\Template\Attribute has no effect	» Notice: Indirect modification of overloaded element of Drupal\Core\Template\Attribute has no effect (theme_table)
Status:	Fixed	» Postponed

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