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
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 75,198 pass(es). View
#9 add_filtering_to-2326397-testonly.patch608 bytesdavidhernandez
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 75,198 pass(es), 1 fail(s), and 0 exception(s). View
#6 add_filtering_to-2326397-6.patch2.24 KBdavidhernandez
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 75,198 pass(es). View
adding-arrayfilter.patch568 bytesdavidhernandez
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 75,119 pass(es). View
Members fund testing for the Drupal project. Drupal Association Learn more

Comments

davidhernandez’s picture

Title: Addd filtering to Attribute » Addd filtering to AttributeArray
davidhernandez’s picture

Title: Addd filtering to AttributeArray » Add filtering to AttributeArray
joelpittet’s picture

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

Should that be part of TwigFilterTest or AttributeTest?

joelpittet’s picture

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

Status: Needs work » Needs review
FileSize
2.24 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 75,198 pass(es). View

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.

Cottser’s picture

@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

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

Status: Needs work » Needs review
FileSize
608 bytes
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 75,198 pass(es), 1 fail(s), and 0 exception(s). View
1.15 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 75,198 pass(es). View

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

Issue tags: -Needs tests

We do have tests.

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

dawehner’s picture

Status: Needs review » Reviewed & tested by the community

Its green!

joelpittet’s picture

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

webchick’s picture

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.