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

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.