Use Attribute::addClass() instead of CSS class concatenation in template_preprocess_views_view_table()

I saw the patch provided on the #3 comment at it seems that all the "['class']" instances has been removed, but when I tried to apply the patch, it doesn't work, I think that the reason could be for the differences between the core version (when the patch was sent it and the current).

Can you update the patch? I review it again.

I am having a look at this.

The tests failed because the order of the classes changed and xpath checks for precise string. Changed the test to use cssSelect method.

Quick reroll of #14 for 8.5.x, only short array syntax stuff changed.

This is surprisingly difficult to review for a 5 K patch.

These hunks are the easy ones to understand:

1. +++ b/core/modules/views/views.theme.inc
   @@ -487,19 +484,21 @@ function template_preprocess_views_view_table(&$variables) {
   -      $variables['header'][$field]['attributes'] = [];
   ...
   +      $variables['header'][$field]['attributes'] = new Attribute();
   
   @@ -514,13 +513,6 @@ function template_preprocess_views_view_table(&$variables) {
   -      $variables['header'][$field]['attributes'] = new Attribute($variables['header'][$field]['attributes']);
   

   This switches to defining an Attribute directly rather than an array that will be overwritten later.

2. +++ b/core/modules/views/views.theme.inc
   @@ -487,19 +484,21 @@ function template_preprocess_views_view_table(&$variables) {
   -      $class = $fields[$field]->elementLabelClasses(0);
   -      if ($class) {
   ...
   +      if ($class = $fields[$field]->elementLabelClasses(0)) {
   

   This just inlines the variable declaration in the if. This change probably isn't a necessary part of the issue scope.

3. +++ b/core/modules/views/views.theme.inc
   @@ -487,19 +484,21 @@ function template_preprocess_views_view_table(&$variables) {
   -        $variables['header'][$field]['attributes']['class'][] = $class;
   ...
   +        $variables['header'][$field]['attributes']->addClass($class);
   ...
   -        $variables['header'][$field]['attributes']['class'][] = $options['info'][$field]['responsive'];
   +        $variables['header'][$field]['attributes']->addClass($options['info'][$field]['responsive']);
   ...
   -        $variables['header'][$field]['attributes']['class'][] = Html::cleanCssIdentifier($options['info'][$field]['align']);
   +        $variables['header'][$field]['attributes']->addClass(Html::cleanCssIdentifier($options['info'][$field]['align']));
   

   These all switch to using the addClass() method instead of adding them to the array, now that it's an Attribute and not an array.

4. +++ b/core/modules/views/views.theme.inc
   @@ -543,16 +535,25 @@ function template_preprocess_views_view_table(&$variables) {
   -        $column_reference['attributes'] = [];
   +        $column_reference['attributes'] = new Attribute();
   
   @@ -583,7 +584,6 @@ function template_preprocess_views_view_table(&$variables) {
   -      $column_reference['attributes'] = new Attribute($column_reference['attributes']);
   

   As above, defines an attribute directly instead of overwriting an array.

5. +++ b/core/modules/views/views.theme.inc
   @@ -543,16 +535,25 @@ function template_preprocess_views_view_table(&$variables) {
   -        $column_reference['attributes']['class'][] = $classes;
   +        $column_reference['attributes']->addClass($classes);
   ...
   -        $column_reference['attributes']['class'][] = $options['info'][$field]['responsive'];
   +        $column_reference['attributes']->addClass($options['info'][$field]['responsive']);
   

   As above, switches to using addClass() now that the attribute is defined directly.

6. +++ b/core/modules/views_ui/src/Tests/PreviewTest.php
   @@ -281,7 +281,7 @@ public function testPreviewSortLink() {
   -    $elements = $this->xpath('//th[contains(@class, :class)]/a', [':class' => 'views-field views-field-name']);
   +    $elements = $this->cssSelect('th.views-field.views-field-name a');
   
   @@ -291,7 +291,7 @@ public function testPreviewSortLink() {
   -    $elements = $this->xpath('//th[contains(@class, :class)]/a', [':class' => 'views-field views-field-name is-active']);
   +    $elements = $this->cssSelect('th.is-active.views-field.views-field-name a');
   

   These changes are necessary because the test in HEAD hardcoded the classes in a certain order. cssSelect() is more of a best practice anyway.

These are the hunks that confuse me:

+++ b/core/modules/views/views.theme.inc
@@ -449,9 +449,6 @@ function template_preprocess_views_view_table(&$variables) {
-    if ($active == $field) {
-      $variables['fields'][$field] .= ' is-active';
-    }

@@ -487,19 +484,21 @@ function template_preprocess_views_view_table(&$variables) {
+      if ($active == $field) {
+        $variables['header'][$field]['attributes']->addClass('is-active');
       }

@@ -514,13 +513,6 @@ function template_preprocess_views_view_table(&$variables) {
-    // Add a CSS align class to each field if one was set.
-    if (!empty($options['info'][$field]['align'])) {
-      $variables['fields'][$field] .= ' ' . Html::cleanCssIdentifier($options['info'][$field]['align']);
     }

@@ -543,16 +535,25 @@ function template_preprocess_views_view_table(&$variables) {
+      if ($active == $field) {
+        $column_reference['attributes']->addClass('is-active');
+      }
...
+      // Add a CSS align class to each field if one was set.
+      if (!empty($options['info'][$field]['align'])) {
+        $column_reference['attributes']->addClass(Html::cleanCssIdentifier($options['info'][$field]['align']));
       }

So, previously, the active and alignment classes were being concatenated directly onto $variables['fields'][$field]. Now, we're more cleanly adding them with Attribute::addClass()... except that it is now being added to $variables['header'][$field]['attributes'] (in the case of the active class) and $column_reference['attributes'] (in both cases) instead of $variables['fields'][$field]['attributes']. Presumably this is because it gets split into those somewhere but I cannot for the life of me see where after half an hour of staring at the HEAD code. What am I missing?

Also, note the cheeky comment:

+++ b/core/modules/views/views.theme.inc
@@ -449,9 +449,6 @@ function template_preprocess_views_view_table(&$variables) {
     // Create a second variable so we can easily find what fields we have and
     // what the CSS classes should be.

"Easily". Ha!

This issue is being reviewed by the kind folks in Slack, #needs-review-queue-initiative. We are working to keep the size of Needs Review queue [2700+ issues] to around 400 (1 month or less), following Review a patch or merge request as a guide.

Points in #24 still need to be addressed.

Hoping this will help, here's how I can reproduce this error:

1. Create a View with Table display and add some fields
2. Exclude some fields from display or set some fields to the column of another field, save
3. Change the Responsive (High/Medium/Low) of some these columns, save
4. Get Error Notice: Indirect modification of overloaded element of Drupal\Core\Template\Attribute has no effect in template_preprocess_views_view_table() (line 569 of /var/www/html/web/core/modules/views/views.theme.inc)

Note that:

• You cannot remove the error until you remember or by chance set the previous columns' responsive class
• A workaround to stop the error is to change the view display and set it back to Table to reset those settings (fields' settings will stay)

Is there anything left here that's not covered by #2894449: Indirect modification of overloaded element with Views responsive table ?

Patch for 11.x.
Pointer in #24 still needs to be addressed.

There has been no answer to catch's query 11 months asking if there is more to do here. I am setting the status to Postponed (maintainer needs more info). If we don't receive confirmation that there is work to do here the issue will be closed in another month. .

Thanks!

After about 9 months there has been no response so state that this is still needed. Therefor, closing.

If there is work to do here, then either re-open the issue or open a new issue and reference this one. If the choice is to use this issue then add a comment change make sure to change the issue status to 'Active'.

Saving credits according to our core issue credit guidelines. Thanks!