When addClass and removeClass were created the original patch had the use of array_filter, but it was latter removed. The reasoning being that it seemed overly defensive, only stopping cases where a themer/developer purposely added a blank array element. While working on adding classes in Twig templates, we discovered that there are definite cases where an empty array element can get in. The biggest being when using ternary operators to dynamically add a class.

{%
  set classes = [
    'field',
    'field-' ~ entity_type|clean_class ~ '--' ~ field_name|clean_class,
    'field-name-' ~ field_name|clean_class,
    'field-type-' ~ field_type|clean_class,
    'field-label-' ~ label_display,
    label_display == 'inline' ? 'clearfix',
  ]
%}

In the code above, when evaluating the line label_display == 'inline' ? 'clearfix', if the result is false, an unwanted array element is added. Below is an example dump from a field label, which had the same problem.

["class"]=>
    object(Drupal\Core\Template\AttributeArray)#1755 (2) {
      ["value":protected]=>
      array(3) {
        [0]=> string(11) "field-label"
        [1]=> string(6) "inline"
        [2]=> string(0) ""
      }
      ["name":protected]=>string(5) "class"
    }

This results in <h3 class="field-label inline ">. Note the extra space at the end. Using Twig whitespace modifiers has not helped, as the space is within the attribute, not around the whole set of printed attributes.

See #2285451-73: Create addClass() and removeClass() methods on Attribute object for merging css class names. for the prior discussion.

CommentFileSizeAuthor
#9 add_filtering_to-2326397-testandfix.patch1.15 KBdavidhernandez
#9 add_filtering_to-2326397-testonly.patch608 bytesdavidhernandez
#6 add_filtering_to-2326397-6.patch2.24 KBdavidhernandez
adding-arrayfilter.patch568 bytesdavidhernandez
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

davidhernandez’s picture

davidhernandez
Primary language English
Location USA
CreditAttribution: davidhernandez commented
Title: Addd filtering to Attribute » Addd filtering to AttributeArray
davidhernandez’s picture

davidhernandez
Primary language English
Location USA
CreditAttribution: davidhernandez commented
Title: Addd filtering to AttributeArray » Add filtering to AttributeArray
joelpittet’s picture

joelpittet
Primary language English
Location Vancouver
CreditAttribution: joelpittet commented
Category: Feature request » Bug report
Issue summary: View changes
Status: Needs review » Needs work
Issue tags: +Quick fix, +Needs tests

Marking needs tests and I think this is a bug report because many tests were failing due to it's lack of existence.

@see #2217731-135: Move field classes out of preprocess and into templates for failures.

davidhernandez’s picture

davidhernandez
Primary language English
Location USA
CreditAttribution: davidhernandez commented

Should that be part of TwigFilterTest or AttributeTest?

joelpittet’s picture

joelpittet
Primary language English
Location Vancouver
CreditAttribution: joelpittet commented

AttributeTest will be quicker and still test the problem. TwigFilterTest will show the problem as we see it in the template... but they are so slooooow! So AttributeTest please:)

davidhernandez’s picture

davidhernandez
Primary language English
Location USA
CreditAttribution: davidhernandez commented
Status: Needs work » Needs review
FileSize
add_filtering_to-2326397-6.patch2.24 KB

I added empty strings to the test data in providerTestAttributeClassHelpers. That seemed like the easiest, and most straightforward. I received the expected success and failures, with and without the array_filter.

star-szr’s picture

star-szr
he/him
Primary language English
CreditAttribution: star-szr commented

@davidhernandez - great, thank you! The best way to demonstrate that on the issue is to upload two patches, one with the tests (or test changes) only that should fail, and a complete test + fix patch. As seen on the bottom of https://www.drupal.org/contributor-tasks/write-tests. Red then green.

Then we can remove the Needs tests tag.

joelpittet’s picture

joelpittet
Primary language English
Location Vancouver
CreditAttribution: joelpittet commented
Status: Needs review » Needs work

Re #6 Couple of notes, though that should pass and do the trick.

  1. +++ b/core/tests/Drupal/Tests/Core/Template/AttributeTest.php
@@ -148,20 +148,21 @@ public function testTwigAddRemoveClasses($template, $expected, $seed_attributes
+    // The empty array elements check that blank class names are filtered out when printed.

    This comment needs to be wrapped after 80 characters. 

  2. +++ b/core/tests/Drupal/Tests/Core/Template/AttributeTest.php
@@ -148,20 +148,21 @@ public function testTwigAddRemoveClasses($template, $expected, $seed_attributes
-      array("{{ attributes.addClass(['k2', 'kangchenjunga']).class }}", 'k2 kangchenjunga'),
-      array("{{ attributes.addClass('lhotse', 'makalu', 'cho-oyu').class }}", 'lhotse makalu cho-oyu'),
+      array("{{ attributes.addClass(['k2', 'kangchenjunga', '']).class }}", 'k2 kangchenjunga'),
+      array("{{ attributes.addClass('lhotse', 'makalu', 'cho-oyu', '').class }}", 'lhotse makalu cho-oyu'),

    This should be fine, though I likely wrote those tests for a reason to test something specific. Can you just add them to the end with your comment?

davidhernandez’s picture

davidhernandez
Primary language English
Location USA
CreditAttribution: davidhernandez commented
Status: Needs work » Needs review
FileSize
add_filtering_to-2326397-testonly.patch608 bytes
add_filtering_to-2326397-testandfix.patch1.15 KB

Ok, I added it as one separate data set at the end. Hopefully, joel approves of the class name. :) I missed the 80 character wrap - oh the growing pains of switching to a new editor.

First should fail, second should pass.

dawehner’s picture

dawehner
Primary language German
CreditAttribution: dawehner commented
Issue tags: -Needs tests

We do have tests.

The last submitted patch, 9: add_filtering_to-2326397-testonly.patch, failed testing.

dawehner’s picture

dawehner
Primary language German
CreditAttribution: dawehner commented
Status: Needs review » Reviewed & tested by the community

Its green!

joelpittet’s picture

joelpittet
Primary language English
Location Vancouver
CreditAttribution: joelpittet commented

@davidhernandez I do approve:D thanks for keeping up the mountain pattern:)

webchick’s picture

webchick
she/they
Primary language English
Location Vancouver 🇨🇦
CreditAttribution: webchick commented
Status: Reviewed & tested by the community » Fixed

I was originally going to push back on this a bit due to "babysitting" but the issue summary does a good job of laying out why this is actually a nice "TX" (themer experience) improvement.

Committed and pushed to 8.x. Thanks!

  • webchick committed 6a168b0 on 
    Issue #2326397 by davidhernandez: Fixed Add filtering to AttributeArray.

Status: Fixed » Closed (fixed)

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