Follow-up to #2502089: Remove SafeMarkup::set() in template_preprocess_views_view_table()

Follow-up to #2280965: [meta] Remove every SafeMarkup::set() call

Problem/Motivation

@MauPalantir noticed there was a bunch of class concatenation done in template_preprocess_views_view_table()

Proposed resolution

Use Attribute() object to addClass() and/or move the class building to the template.

Remaining tasks

Manual testing steps

User interface changes

N/A

API changes

N/A

Members fund testing for the Drupal project. Drupal Association Learn more

Comments

joelpittet created an issue. See original summary.

Cottser’s picture

Title: Clean up CSS class concatination in template_preprocess_views_view_table() » Clean up CSS class concatenation in template_preprocess_views_view_table()
joelpittet’s picture

Status: Active » Needs review
Issue tags: +code cleanup, +Needs manual testing
FileSize
4.26 KB

This likely needs to be postponed till after 8.0.0 release. There should be no markup diff.

Status: Needs review » Needs work

The last submitted patch, 3: clean_up_css_class-2554957-3.patch, failed testing.

Version: 8.0.x-dev » 8.1.x-dev

Drupal 8.0.6 was released on April 6 and is the final bugfix release for the Drupal 8.0.x series. Drupal 8.0.x will not receive any further development aside from security fixes. Drupal 8.1.0-rc1 is now available and sites should prepare to update to 8.1.0.

Bug reports should be targeted against the 8.1.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.2.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.1.x-dev » 8.2.x-dev

Drupal 8.1.9 was released on September 7 and is the final bugfix release for the Drupal 8.1.x series. Drupal 8.1.x will not receive any further development aside from security fixes. Drupal 8.2.0-rc1 is now available and sites should prepare to upgrade to 8.2.0.

Bug reports should be targeted against the 8.2.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.3.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

joelpittet’s picture

Version: 8.2.x-dev » 8.3.x-dev
Status: Needs work » Needs review
Issue tags: +Novice
xito’s picture

Status: Needs review » Needs work
Issue tags: +Dublin2016

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.

joelpittet’s picture

Status: Needs work » Needs review
FileSize
4.25 KB

Seems to still apply but with fuzz. Would you like to review this @xito?

Status: Needs review » Needs work

The last submitted patch, 9: 2554957-9.patch, failed testing.

The last submitted patch, 9: 2554957-9.patch, failed testing.

mmrares’s picture

I am having a look at this.

Anansi_boy’s picture

The patch doesn't work for me too and I'm also trying to figure how to fix that today at DrupalCon Dublin.

mmrares’s picture

Status: Needs work » Needs review
FileSize
5.41 KB
1.15 KB

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

Anansi_boy’s picture

Damn, I was close to propose my patch first ! :)
Can I propose you to correct a attributes['id'] with a ->setAttribute('id',...) ?

I don't have enough time to review the patch now, so I just give you my correction to yours

Status: Needs review » Needs work

The last submitted patch, 15: 2554957-15.patch, failed testing.

joelpittet’s picture

Status: Needs work » Needs review

@Anansi_boy thanks for the suggestion but actually you can do both ways because the Attribute object implements ArrayAccess. I think your patch failed there because maybe it's missing the -p1 for the patch command but that's a guess.

#14 seems like the one that needs some review so I'll hide #15.

Version: 8.3.x-dev » 8.4.x-dev

Drupal 8.3.0-alpha1 will be released the week of January 30, 2017, which means new developments and disruptive changes should now be targeted against the 8.4.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.4.x-dev » 8.5.x-dev

Drupal 8.4.0-alpha1 will be released the week of July 31, 2017, which means new developments and disruptive changes should now be targeted against the 8.5.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Cottser’s picture

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

tacituseu’s picture

Component: theme system » views.module
joelpittet’s picture

Status: Needs review » Reviewed & tested by the community

Nice cleanup

xjm’s picture

Title: Clean up CSS class concatenation in template_preprocess_views_view_table() » Use Attribute::addClass() instead of CSS class concatenation in template_preprocess_views_view_table()
xjm’s picture

Status: Reviewed & tested by the community » Needs review

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!

Version: 8.5.x-dev » 8.6.x-dev

Drupal 8.5.0-alpha1 will be released the week of January 17, 2018, which means new developments and disruptive changes should now be targeted against the 8.6.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.