Problem/Motivation

We need a way to be able to add in classes from the template files to the core/contrib provided attribute object's class value.

Proposed resolution

@see patch.

Remaining tasks

User interface changes

API changes

CommentFileSizeAuthor
#85 2285451-attribute-addClass-removeClass-85.patch9.2 KBjoelpittet
#85 interdiff.txt965 bytesjoelpittet
#85 2285451-attribute-addClass-removeClass-85-tests-only-fail.patch9.03 KBjoelpittet
#74 2285451-attribute-addClass-removeClass-74.patch8.94 KBjoelpittet
#74 interdiff.txt1.49 KBjoelpittet
#71 interdiff.txt1.88 KBjoelpittet
#71 2285451-attribute-addClass-removeClass-71.patch9.08 KBjoelpittet
#71 interdiff.txt1.89 KBjoelpittet
#63 2285451-attribute-addClass-removeClass-63.patch9.06 KBjoelpittet
#63 interdiff.txt1.89 KBjoelpittet
#53 interdiff-2285451-50-53.txt958 bytesRainbowArray
#53 2285451-attribute-addClass-removeClass-53.patch9 KBRainbowArray
#50 2285451-attribute-addClass-removeClass-50.patch8.93 KBjoelpittet
#50 interdiff.txt4.45 KBjoelpittet
#48 2285451-attribute-addClass-removeClass-48.patch9.04 KBjoelpittet
#48 interdiff.txt1.78 KBjoelpittet
#47 2285451-attribute-addClass-removeClass-44.patch10.36 KBjoelpittet
#47 interdiff.txt4.23 KBjoelpittet
#38 interdiff.txt2.4 KBjoelpittet
#38 attribute_addclass-2285451-38.patch7 KBjoelpittet
#34 interdiff.txt941 bytesjoelpittet
#34 2285451-attribute-addClass-removeClass-34.patch7.17 KBjoelpittet
#32 2285451-attribute-addClass-removeClass-32.patch7.21 KBjoelpittet
#32 interdiff.txt8.2 KBjoelpittet
#29 interdiff-2285451-29.txt998 bytesdavidhernandez
#29 create_addclass_and-2285451-29.patch5.9 KBdavidhernandez
#27 create_addclass_and-2285451-27.patch5.86 KBdavidhernandez
#27 interdiff.txt618 bytesdavidhernandez
#24 2285451-24.patch5.86 KBcilefen
#24 interdiff-19-24.txt4.59 KBcilefen
#19 2285451-19.patch5.29 KBcilefen
#19 interdiff-1-19.txt5.29 KBcilefen
#5 interdiff.txt4.03 KBjoelpittet
#5 2285451-5-2254153-node-classes-in-template.patch8.82 KBjoelpittet
#4 2285451-attributes-class-methods-2.patch7 KBmortendk
#1 2285451-attributes-class-methods-1.patch4.99 KBjoelpittet
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

joelpittet’s picture

Title: add, remove & merge functions for css classnames in templates » Create add() and remove() methods on AttributeArray object for merging css class names.
Issue summary: View changes
Issue tags: +DX (Developer Experience), +FX (Front End Experience), +Needs tests
FileSize
4.99 KB

So here's my POC, it could use more tests in an actual twig template but I did do some tests and it works there just the same.

Here is my test data for the twig template if someone wants to roll that into a new test.

Any renderable array with an Attribute object passed (eg usually $variables['attributes'] array on every hook_theme will do that).

{% set node_type = 'article' %}
{% set view_mode = 'teaser' %}
{% set node_promoted = true %}
{% set node_sticky = true %}
{% set node_unpublished = true %}
{% set preview = false %}
{% set classes = [
  'node',
  'node--type-' ~ node_type,
  node_promoted ? 'node--promoted',
  node_sticky ? 'node--sticky',
  node_unpublished ? 'node--unpublished',
  preview ? 'node--preview',
  'node--view-mode-' ~ view_mode
]
%}
{# {{ dump(classes) }} #}
<h2>Add Classes</h2>
<p><strong>Classes:</strong>
  {{ attributes.class.add(classes) }}</p>
<p><strong>Other attributes:</strong>
  {{ attributes|without('class') }}</p>

<h2>Remove Classes</h2>
<p><strong>Classes:</strong>
  {{ attributes.class.remove(classes) }}</p>

<h2>Add Comma Separated Classes</h2>
<p><strong>Classes:</strong>
  {{ attributes.class.add('one', 'dos', 'trois') }}</p>

<h2>Chain Classes (removes 'one' and adds 'two', 'three')</h2>

{% set preview = true %}
<p><strong>Classes:</strong>
{{ attributes.class
  .remove('one', 'dos')
  .add(['cat', 'dog'])
  .add('fish', preview ? 'node--preview')
}}</p>
mortendk’s picture

RainbowArray’s picture

This looks good to me too.

mortendk’s picture

Status: Active » Needs review
FileSize
7 KB

This looks good heres a patch for the node.html.twig file as described in #2254153: Move node classes out of preprocess and into templates

<article class="{{ attributes.class.add(classes) }}" {{ attributes|without('class') }}>

it looks a bit strange at a first - but it make sense :)

joelpittet’s picture

May be a bit confusing that you posted on this issue;) I'm hiding your patch and mine here... but we should move this over to the node issue for discussion. And maybe get support for this over there @mortendk?

Anyways, here's my refactoring of #4
I added #2283301: Add Twig filters for drupal_html_class() and drupal_clean_id_identifier() so we don't need any classes in preprocess.

mortendk’s picture

yeah i was a bit confused to figure out where to put it ( and between to worldcup games *coughs*) but the most important think is that the test went through *woho* and it works (more woo) Im fine with moving it over to the node issue

oooh had no idea about

+++ b/core/modules/node/templates/node.html.twig
@@ -79,16 +79,15 @@
+  'node--type-' ~ node.bundle|class,
+  node.promoted ? 'node--promoted',
+  node.sticky ? 'node--sticky',
+  not node.published ? 'node--unpublished',
+  preview ? 'node--preview',
+  'node--view-mode-' ~ view_mode|class

thats pretty slick - but we need some documentation for it

Alright so im gonna move the patch to #2254153: Move node classes out of preprocess and into templates

joelpittet’s picture

Issue tags: +Needs documentation

@mortendk Where is a good place to document that? That trickery is part of twig. Any method with Object.getSomething can be printed in twig with {{ object.something }} and same goes for the Object.isSomething convention can be checked with {% if object.something %} twig looks for array keys, object public properties and methods, then it goes and looks for magic methods like __get() and __isset().

Since we have the node object being passed to the template, we've got all it's public methods at our fingertips.

mortendk’s picture

@joel I think we should put it in the comment header for the template, then its at the fingertips
lets carry this on over at #2254153: Move node classes out of preprocess and into templates

joelpittet’s picture

+++ b/core/modules/node/templates/node.html.twig
@@ -77,7 +77,17 @@
+{% set classes = [
+  'node',
+  'node--type-' ~ node.bundle|class,
+  node.promoted ? 'node--promoted',
+  node.sticky ? 'node--sticky',
+  not node.published ? 'node--unpublished',
+  preview ? 'node--preview',
+  'node--view-mode-' ~ view_mode|class
+]
+%}
+<article class="{{ attributes.class.add(classes) }}"{{ attributes|without('class') }}>

This may be our downfall, but it's what we are asking for... got some feedback from @webchick and maybe we should have this ability but not do this in core and leave this in preprocess?

dawehner’s picture

Just a drive-by review.

  1. +++ b/core/tests/Drupal/Tests/Core/Template/AttributeTest.php
    @@ -66,6 +66,56 @@ public function testRemove() {
    +  /**
    +   * Tests adding class attributes with the AttributeArray helper method.
    +   */
    +  public function testAddClasses() {
    ...
    +  /**
    +   * Tests removing class attributes with the AttributeArray helper method.
    +   */
    +  public function testRemoveClasses() {
    ...
    +  /**
    +   * Tests removing class attributes with the AttributeArray helper method.
    +   */
    +  public function testChainAddRemoveClasses() {
    

    We could/should add @covers here for the phpunit code coverage report.

  2. +++ b/core/tests/Drupal/Tests/Core/Template/AttributeTest.php
    @@ -66,6 +66,56 @@ public function testRemove() {
    +    $this->assertFalse(in_array('example-class', $attribute['class']->value()));
    ...
    +    $this->assertFalse(in_array(array('xx', 'yy'), $attribute['class']->value()));
    ...
    +    $this->assertFalse(in_array(array('red', 'green', 'blue'), $attribute['class']->value()));
    

    Are you sure there is no dedicated assert for that?

mortendk’s picture

no were not leaving anything in the preprocess and we should do this in core, its about time we break out of the drupal chains (chants with a fist in the air)

markhalliwell’s picture

Status: Needs review » Needs work

Personally I would much rather see:

{% attributes.class.add([
  'node',
  'node--type-' ~ node.bundle|class,
  node.promoted ? 'node--promoted',
  node.sticky ? 'node--sticky',
  not node.published ? 'node--unpublished',
  preview ? 'node--preview',
  'node--view-mode-' ~ view_mode|class
]).remove([
  'some-module-class'
]) %}
<article {{attributes}}>

vs.

{% set classes = [
  'node',
  'node--type-' ~ node.bundle|class,
  node.promoted ? 'node--promoted',
  node.sticky ? 'node--sticky',
  not node.published ? 'node--unpublished',
  preview ? 'node--preview',
  'node--view-mode-' ~ view_mode|class
]
%}
<article class="{{ attributes.class.add(classes).remove(['some-module-class']) }}"{{ attributes|without('class') }}>
joelpittet’s picture

@Mark Carver please add that to #2254153: Move node classes out of preprocess and into templates issue too. As it will serve more people over there.

Though because class is not a method but an object property I wonder if your example will work... we'll be sure to add that to a twig template test. It's that or we may need {% attributes.addClass(...) %}

markhalliwell’s picture

@joelpittet, I'm not sure what you're referring to. I am using the exact same thing as in the patch attributes.class.add(classes), except instead of setting up a separate variable classes, I am just passing the array directly to the function. Thus, eliminating needless clutter.

RainbowArray’s picture

@joelpittet: The syntax markcarver used is functionally the same as what's in your patch.

You have:

<article class="{{ attributes.class.add(classes) }}"{{ attributes|without('class') }}>

So using an add method with an array parameter on attributes.class. That's exactly what markcarver is doing with:

{% attributes.class.add([
  'node',
  'node--type-' ~ node.bundle|class,
  node.promoted ? 'node--promoted',
  node.sticky ? 'node--sticky',
  not node.published ? 'node--unpublished',
  preview ? 'node--preview',
  'node--view-mode-' ~ view_mode|class
]) %}

That too is an add method with an array parameter on attributes.class.

This just lets us keep the element a lot cleaner, with a simple {{ attributes }}, by moving the add method up to where the classes are being set.

joelpittet’s picture

@Mark Carver re #14 It will likely work, I just need to test because I'm not confident the references to the parent will work, so I'll just be sure to include that (sanity check) in the test.

jwilson3’s picture

This seems like a duplicate of an idea that was discussed first 2 years ago first here: #1808254: Standardize and simplify the attribute syntax in Twig template files, and then formalized and spun off to a separate issue here: #1835396: Attribute modifier functions for themers. Interesting back story, and maybe there are some gems there, for the people that are taking this on now.

joelpittet’s picture

@jwilson3 thanks for this, I followed and commented on that 2 years ago... didn't recall that issue. I don't think using filters are a good idea for attributes, but methods will work great because they are related to that object. Filters are usually working on simple types like string or array. But I do need to re-read that issue to see if I can't get some more ideas. attributes.addClass() I think is the way to go here and I'll rewrite this to that syntax and iterate from there.

cilefen’s picture

Status: Needs work » Needs review
FileSize
5.29 KB
5.29 KB

Because this patch is needed to proceed now on #2289511: [meta] Results of Drupalcon Austin's Consensus Banana, we need this in. I tweaked code standards in this patch.

joelpittet’s picture

Title: Create add() and remove() methods on AttributeArray object for merging css class names. » Create addClass() and removeClass() methods on Attribute object for merging css class names.
Assigned: Unassigned » joelpittet

@cilefen thanks sorry for the delay on this one. I'll tackle this in the evening tonight. Changing the title to reflect the direction this will be taking.

Same code just moved to the main Attribute class to make it look like jQuery as I've wanted to do and seems has historic precedence as @jwilson3 pointed out.

cilefen’s picture

@joelpittet I'll try that now.

cilefen’s picture

How can we add those methods to Attribute when it has already been cast to AttributeArray?

cilefen’s picture

Forget that, I'm dumb...

cilefen’s picture

FileSize
4.59 KB
5.86 KB
joelpittet’s picture

Ha, well that's one way to do it, nice work @cilefen! I never thought of doing that... I was going to scrap my work on the add()/remove() methods and just move their guts to the new methods on Attributes, which we still may have to do, but for now that looks like it's green!

Nice, probably needs a more tests still.

joelpittet’s picture

Assigned: joelpittet » Unassigned

Bah time is not on my side at the moment. #24 looks good, maybe we can get a few reviews before I change things further?

davidhernandez’s picture

Yeah so cilefen and I were hashing this out at CapitalCamp before adding the patch. Abstracting the functions made sense and was very clean. This also made sense having the add/remove methods themselves in AttributeArray as we are forcing 'class' to always be AttributeArray, though we want addClass and removeClass to be part of Attribute. I think this would maintain consistency if extrapolated in the future to add other methods

I'm uploading another patch, just fixing some comments. I'm also going to manually test it in some templates to make sure we are getting the results we need.

Status: Needs review » Needs work

The last submitted patch, 27: create_addclass_and-2285451-27.patch, failed testing.

davidhernandez’s picture

Sorry, missed one. That last test didn't fail; we had it cancelled.

davidhernandez’s picture

Status: Needs work » Needs review
joelpittet’s picture

@davidhernandez that is looking very nice so far, thanks for the standards cleanup and addClass/removeClass additions. Likely we will talk about this at 4PST in the #drupal-twig hangout you are welcome to drop in if you are around.

joelpittet’s picture

This is now attributes.addClass and attributes.removeClass
I've added those covers @dawehner mentioned #10's drive by but couldn't find a PHPUnit assert for in_array().

You're welcome to try it out with your banana themes.

Here is my inside twig test, still haven't got a twig tests for the attributes setup.

{% set node_type = 'article' %}
{% set view_mode = 'teaser' %}
{% set node_promoted = true %}
{% set node_sticky = true %}
{% set node_unpublished = true %}
{% set preview = false %}
{% set classes = [
  'node--type-' ~ node.bundle,
  node.promoted ? 'node--promoted',
  node.sticky ? 'node--sticky',
  not node.published ? 'node--unpublished',
  preview ? 'node--preview',
  'node--view-mode-' ~ view_mode,
  'extra'
]
%}
{# {{ dump(classes) }} #}

Classes before: {{ attributes.class }}

<h2>Add Classes</h2>
<pre><strong>Classes:</strong>
{{ attributes.addClass(classes).class.value()|join('\n') }}</pre>
<h2>Remove Classes</h2>
<pre><strong>Classes:</strong>
{{ attributes.removeClass(classes).class.value()|join('\n') }}</pre>
<h2>Add Comma Separated Classes</h2>
<pre><strong>Classes:</strong>
{{ attributes.addClass('one', 'dos', 'trois').class.value()|join('\n') }}</pre>
<h2>Chain Classes (removes 'one' and adds 'two', 'three')</h2>
{% set preview = true %}
<pre><strong>Classes:</strong>
{{ attributes.removeClass('one', 'dos')
  .addClass(['cat', 'dog'])
  .addClass('fish', preview ? 'node--preview').class.value()|join('\n')
}}</pre>

Status: Needs review » Needs work

The last submitted patch, 32: 2285451-attribute-addClass-removeClass-32.patch, failed testing.

joelpittet’s picture

Status: Needs work » Needs review
FileSize
7.17 KB
941 bytes

Boo that works in 5.5, oh well I should know better:P Likely don't need that check too, it's a micro optimization anyway.

cilefen’s picture

I know you are not finished the detailed twig test yet, but just watch out for these in the final:

  1. +++ b/core/lib/Drupal/Core/Template/Attribute.php
    @@ -107,6 +117,68 @@ public function offsetExists($name) {
    +    // var_dump($classes);
    

    commented code

  2. +++ b/core/lib/Drupal/Core/Template/Attribute.php
    @@ -107,6 +117,68 @@ public function offsetExists($name) {
    +    // var_dump($this->storage['class']);
    

    commented code

  3. +++ b/core/tests/Drupal/Tests/Core/Template/AttributeTest.php
    @@ -58,6 +58,83 @@ public function testRemove() {
    +    var_dump($attribute);
    

    var_dump

RainbowArray’s picture

I can't speak much to the way the code is written, but I like the way this works in the template.

We'll want to make sure in the change notice we have one or two good examples of how people would use this in a template. attributes.addClass(classes).class.value() feels clunky to me when we're chaining it like that. Would it be something like this?

{% set classes = [
  'node--type-' ~ node.bundle,
  node.promoted ? 'node--promoted',
  node.sticky ? 'node--sticky',
  not node.published ? 'node--unpublished',
  preview ? 'node--preview',
  'node--view-mode-' ~ view_mode,
  'extra'
]
%}
<div {{ attributes.addClass(classes) }}>
</div>
davidhernandez’s picture

@mdrummond, that is correct.

joelpittet’s picture

You don't need .value() That is because I wanted the array and I wanted to put new line characters instead of spaces for me to read it easier in my tests:)

@cilefen thanks for spotting those leftover debug statements. Here's a cleaner version.

@mdrummond Yeah real world examples will be like yours in #36

joelpittet’s picture

Minor nit pick with #36 don't put the space with <div[NO SPACE HERE]{{ attributes }}> BUT <div class="class[SPACE HERE NEEDED]{{ attributes.class }}"{{ attributes.without('class') }}>

Attributes put a space before every attribute printed and if there are non your tag doesn't have a weird empty space. Classes on the otherhand don't add a space in their value. I think it makes sense if you think about it?

davidhernandez’s picture

For adding twig test for this, what was envisioned? I full webtestbase to check the markup with classes?

davidhernandez’s picture

@joelpittet, is that exchangeArray() function necessary? I don't want to nitpick though, so I don't care if it is there. Just curious why add it there, if you felt having the add() and remove() functions in AttributeArray was confusing. Wouldn't that also be confusing?

I manually tested #38, with modifications to templates. The expected output was rendered while removing and adding classes.

I'm not seeing an easy way of adding an actual twig test at this point. Because the templates have not been modified yet to take advantage of this new method, I believe a test module implementing a fake theme function would be needed to use a test twig file. That seems like overkill for this, when it can be tested for later after the templates have been modified. Shouldn't the unit tests be enough for now?

joelpittet’s picture

For the twig test, just similar items as the PHP test but in a twig file, and if we can avoid a full bootstrap of a webtest I'll try to do that too.

exchange array is necessary because I don't have access to the value() direclty but there is a myriad of other ways I could have handled that I tried something that was in PHP already so it's familiar in use.

As long as you can access the Attribute object and the Twig service it should just work. I'll take a crack at it here in a bit.

joelpittet’s picture

The unit test should be good to know that it works as expected for PHP, the twig one is just to confirm it works as expected in the twig template.

davidhernandez’s picture

I agree the twig test is useful, I just couldn't figure out how to get it to work without too much effort. If you can do it, I'll review it as soon as I can. I just want to make sure we don't get stalled on that.

joelpittet’s picture

I think it is good enough to test it out with the banana stuff so you are welcome to run with that patch it's in a good spot at the moment.

davidhernandez’s picture

It would be good to get this in before digging into the preprocess functions, if we can. Otherwise, we'll have to link them all here which will create some confusion/complication.

joelpittet’s picture

Twig tests added to the same UnitTest so it should be much faster than a web test. Bunch of mountains.

Edit: Sorry wrong patch #.

joelpittet’s picture

Well there was garbage in that one anyway. Removed here.

dawehner’s picture

Just a quick review ...

  1. +++ b/core/lib/Drupal/Core/Template/Attribute.php
    @@ -107,6 +117,65 @@ public function offsetExists($name) {
    +  public function addClass() {
    +    $args = func_get_args();
    +    $classes = array();
    +    foreach ($args as $arg) {
    

    In case you wonder how to document it. There is a proposal how to do that in PSR5: https://github.com/phpDocumentor/fig-standards/issues/40 and an actual implementation in phpDocumentor https://github.com/phpDocumentor/phpDocumentor2/issues/629 This uses the php 5.5 syntax for variadic arguments

  2. +++ b/core/lib/Drupal/Core/Template/Attribute.php
    @@ -107,6 +117,65 @@ public function offsetExists($name) {
    +   * @return \Drupal\Core\Template\Attribute
    +   *   The attribute with the class arguments removed.
    

    We do use @return $this to make it clear what exactly is returned.

  3. +++ b/core/lib/Drupal/Core/Template/AttributeArray.php
    @@ -83,4 +83,21 @@ public function value() {
    +   * @param  array $input
    

    two spaces ...

  4. +++ b/core/tests/Drupal/Tests/Core/Template/AttributeTest.php
    @@ -58,6 +58,126 @@ public function testRemove() {
    +    $this->assertFalse(in_array('example-class', $attribute['class']->value()));
    ...
    +    $this->assertFalse(in_array(array('xx', 'yy'), $attribute['class']->value()));
    ...
    +    $this->assertFalse(in_array(array('red', 'green', 'blue'), $attribute['class']->value()));
    

    You could also use assertContains instead

joelpittet’s picture

@dawehner thank you very much I never know how to document those 'variadics' nor did I know they had a name.

Hopefully I did it correctly here? Also I used assertNotContains() because assertContains() would give some false positives but I did change the others to be more consistent to assertArrayEquals().

dawehner’s picture

Status: Needs review » Reviewed & tested by the community

Cool!

chx’s picture

Status: Reviewed & tested by the community » Needs work

I am sorry but

+   * Adds the argument values by merging them on to the value array.
+   *
+   * @param mixed ...
+   *   Classes arguments to add.

this is beyond not helpful: it doesn't make any sense and although English is not my first language, is "Classes arguments" even grammatically correct?? "Adds the arguments to the existing CSS classes". "CSS classes to add". Perhaps. Feel free to add better but there's not a word in the current doxygen indicating that addClasses is specific and hardwired to the 'class' key. Similarly for removeClass.

RainbowArray’s picture

Patch to address chx's concerns with the docblocks:

  /**
   * Adds argument values by merging them on to array of existing CSS classes.
   *
   * @param mixed ...
   *   CSS classes to add to the class attribute array.
  /**
   * Removes argument values from array of existing CSS classes.
   *
   * @param mixed ...
   *   CSS classes to remove from the class attribute array.
dawehner’s picture

Status: Needs review » Reviewed & tested by the community

I agree this is an improvement.

chx’s picture

Thanks for fixing this!

alexpott’s picture

+++ b/core/lib/Drupal/Core/Template/Attribute.php
@@ -107,6 +117,70 @@ public function offsetExists($name) {
+   * @param mixed ...
+   *   CSS classes to add to the class attribute array.
...
+   * @param mixed ...
+   *   CSS classes to remove from the class attribute array.

Is this the correct documentation and shouldn't they always be strings?

+++ b/core/lib/Drupal/Core/Template/Attribute.php
@@ -107,6 +117,70 @@ public function offsetExists($name) {
+      // Filter out any empty values.
+      $classes = array_filter($classes);

Should we be worrying about uniqueness here too?

alexpott’s picture

Status: Reviewed & tested by the community » Needs work
+++ b/core/lib/Drupal/Core/Template/Attribute.php
@@ -80,8 +80,18 @@ public function offsetSet($name, $value) {
+    if (is_array($value) || $name == 'class') {
+      // Cast the value to an array if the value was passed in as a string.
+      // @todo Decide to fix all the broken instances of class as a string
+      // in core or cast them.
+      $value = new AttributeArray($name, (array) $value);

I think this might be fixed now... #2304399: "class" attribute in drupal_render should be an array

davidhernandez’s picture

The methods can be used with a string class name or array of class names.

alexpott’s picture

So then maybe it should be

+   * @param string|array ...
+   *   CSS classes to add to the class attribute array.
davidhernandez’s picture

It can be used with a single argument:

<div{{ attributes.addClass('test') }}>

Multiple arguments:

<div{{ attributes.addClass('test1', 'test2') }}>

Or an array:

{% set classes = [
  'test1',
  'test2',
  'test3',
  'test4',
]
%}
<div {{ attributes.addClass(classes) }}></div>

Are they all covered by string|array ?

alexpott’s picture

The unknown arguments (a variadic function) is what the ... in the PHPDoc is about. But those arguments are expected to be either strings or arrays so we should tell people.

{% set classes = [
  'test1',
  'test2',
  'test3',
  'test4',
]
%}
{% set more_classes = [
  'test5',
  'test6',
]
%}

<div {{ attributes.addClass(classes, more_classes) }}></div>

Would work too :)

davidhernandez’s picture

Oh ok. So the 'type' part is strict to exactly what is expected?

joelpittet’s picture

I was under the impression more than one type was documented as mixed... but I like that you can use the pipe to denote the options. Thanks for the tip @alexpott! Also added unique as mentioned in #54 which is a good idea, was originally just trying to keep it light as possible but I think that is still reasonable.

#57 maybe fixed but I'd like to enforce the rule instead of hoping people do it right. Even in core there was/is some accidental attribute.class strings.

joelpittet’s picture

Status: Needs work » Needs review
joelpittet’s picture

Drafted a change notice here https://www.drupal.org/node/2315471

davidhernandez’s picture

Status: Needs review » Reviewed & tested by the community

I tested this manually in twig templates and still get the desired results.

alexpott’s picture

Status: Reviewed & tested by the community » Needs work
  1. +++ b/core/lib/Drupal/Core/Template/Attribute.php
    @@ -107,6 +117,70 @@ public function offsetExists($name) {
    +      $classes = array_unique(array_filter($classes));
    +      $this->storage['class']->exchangeArray($classes);
    

    Also I'm confused why are we using array_filter? Just in case someone passes in array('') or an empty string? That seems too defensive?

    Also we should add a test for the use of array_unique.

  2. +++ b/core/lib/Drupal/Core/Template/Attribute.php
    @@ -107,6 +117,70 @@ public function offsetExists($name) {
    +      // Filter out any empty values.
    +      $classes = array_unique(array_filter($classes));
    

    We should not need to run array_unique here. See above comment about array_filter too.

davidhernandez’s picture

Are you saying there is a better way to check for duplicates, or not to bother checking?

alexpott’s picture

I'm asking why we are using array_filter in both the addClass and removeClass functions?

Also the array_unique is definitely not necessary when calling removeClass.

For some reason the dreditor snippets are not very helpful because i was not commenting on the offsetExists function.

joelpittet’s picture

@alexpott you're right it is defensive. Maybe it would be better if moved the array_unique and array_filter calls to the AttributeArray when it's printed? Save a few CPU cycles?

joelpittet’s picture

Status: Needs work » Needs review
FileSize
1.89 KB
9.08 KB
1.88 KB

How's this? I think this should be defensive but not sure where, so I'll save that for another issue because it may affect performance. Merging will deal with most duplicates (that aren't there already).

joelpittet’s picture

Wrong interdiff, hiding.

alexpott’s picture

+++ b/core/lib/Drupal/Core/Template/AttributeArray.php
@@ -66,6 +66,8 @@ public function offsetExists($offset) {
+    // Filter out any empty values before printing.
+    $this->value = array_filter($this->value);

Adding empty values is poor code - adding duplicates well that is hard to defend against. So I'd prefer to drop the array_filter and just use array_unique. Also putting it here means we're now doing this for all AttributeArrays not just the class. Can we just add it to addClasses and be done.

joelpittet’s picture

... OK, I'd like to clean empty values out as well, but I can do that in a separate issue, I guess.

Status: Needs review » Needs work

The last submitted patch, 74: 2285451-attribute-addClass-removeClass-74.patch, failed testing.

Status: Needs work » Needs review
penyaskito’s picture

davidhernandez’s picture

I tested this manually in templates and get the expected results in all the different ways. Duplicates are removed. Empty space is not, as expected. Technically, extra spaces don't break anything, and they can only get there from a module or template, so it shouldn't be a deal breaker.

Shall we RTBC?

Also, any more suggestions for the change record? https://www.drupal.org/node/2315471

RainbowArray’s picture

Status: Needs review » Reviewed & tested by the community

Looks to me like all of the issues have been addressed. I did another pass through, and this all seems pretty sensible to me.

joelpittet’s picture

Perfect is the enemy of good

Thanks @mdrummond;)

star-szr’s picture

I would recommend the title of the change record be in the past tense.

davidhernandez’s picture

Past tense is indeed in keeping with the other change records. I updated it.

star-szr’s picture

Gave it a further tweak so that it makes a better "headline" :) thanks @davidhernandez!

alexpott’s picture

Status: Reviewed & tested by the community » Needs work

Can we add a test for the array_unique functionality.

joelpittet’s picture

davidhernandez’s picture

Status: Needs review » Reviewed & tested by the community

Nice, Joel. So the first patch should fail because it has the uniqueness test, but not the array_unique() filter. The second has the filter and the test, which passed.

alexpott’s picture

Status: Reviewed & tested by the community » Fixed
Issue tags: +TCDrupal 2014

Committed 65cb382 and pushed to 8.0.x. Thanks!

  • alexpott committed 65cb382 on 8.0.x
    Issue #2285451 by joelpittet, davidhernandez, cilefen, mdrummond,...
davidhernandez’s picture

Awesome. I published the change record.

Status: Fixed » Closed (fixed)

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