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.
Comment | File | Size | Author |
---|---|---|---|
#9 | add_filtering_to-2326397-testandfix.patch | 1.15 KB | davidhernandez |
#9 | add_filtering_to-2326397-testonly.patch | 608 bytes | davidhernandez |
#6 | add_filtering_to-2326397-6.patch | 2.24 KB | davidhernandez |
adding-arrayfilter.patch | 568 bytes | davidhernandez | |
Comments
Comment #1
davidhernandezComment #2
davidhernandezComment #3
joelpittetMarking 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.
Comment #4
davidhernandezShould that be part of TwigFilterTest or AttributeTest?
Comment #5
joelpittetAttributeTest 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:)
Comment #6
davidhernandezI 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.
Comment #7
star-szr@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.
Comment #8
joelpittetRe #6 Couple of notes, though that should pass and do the trick.
This comment needs to be wrapped after 80 characters.
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?
Comment #9
davidhernandezOk, 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.
Comment #10
dawehnerWe do have tests.
Comment #12
dawehnerIts green!
Comment #13
joelpittet@davidhernandez I do approve:D thanks for keeping up the mountain pattern:)
Comment #14
webchickI 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!