Refactor Attribute rendering so that class="" is not printed

This looks good to me. Tests seem to be set up correctly. Pretty straightforward. Let's get this in, so we can move forward on other preprocess conversions. We can file another issue for remove classes if necessary.

+++ b/core/lib/Drupal/Core/Template/Attribute.php
@@ -143,7 +143,12 @@ public function addClass() {
+        $classes = array_filter(array_merge($classes, (array) $arg));
+      }
+      // If there are no classes to add, return early.

Let's just do one array_filter()... we can move $classes = array_filter($classes); outside the loop.

Why is AttributeArray not protecting us from this?

  /**
   * Implements the magic __toString() method.
   */
  public function __toString() {
    // Filter out any empty values before printing.
    $this->value = array_filter($this->value);
    return String::checkPlain(implode(' ', $this->value));
  }

Oops - I already did that :(

How about something like the patch attached - this means we only do the filtering and uniqueness once per ArrayAttribute and on render - plus all array attributes get this benefit.

Note that I didn't push these change to AttributeValueBase::render() is because it breaks expectations around code like this:

$a1 = new Attribute(array('alt' => ''));
print $a1;
$a2 = new Attribute(array('alt' => NULL));
print $a2

The first print will output alt="" the second print will output nothing. This is tested in AttributesTest (btw AttributesTest vs AttributeTest :) that's nuts).

Personally I feel the the ability to output alt="" and prevent class="" are kind of contrary expectations.

No interdiff because it is a new approach.

Refined the approach so that ArrayAttribute does not have a custom render.

+++ b/core/lib/Drupal/Core/Template/AttributeValueBase.php
@@ -17,6 +17,11 @@
   /**
+   * Renders '$name=""' if $value is to an empty string.
+   */
+  const PRINT_IF_EMPTY_STRING = TRUE;

@Cottser noticed an extra word

lol - thanks @Cottser

Awesome, this was exactly the way I was going on Friday, but I got stuck in trying to debug the UnitTest failures... then gave up :(

1. +++ b/core/lib/Drupal/Core/Template/AttributeArray.php
   @@ -30,6 +30,14 @@
   +  const PRINT_IF_EMPTY_STRING = FALSE;
   

   Didn't think to abstract out that exception for alt tags, nice!

2. +++ b/core/tests/Drupal/Tests/Core/Template/AttributeTest.php
   @@ -69,6 +69,25 @@ public function testAddClasses() {
   +      $this->assertEmpty((string) $attribute);
   

   This is great, actually testing the output and not the structure.

3. +++ b/core/lib/Drupal/Core/Template/Attribute.php
   @@ -185,7 +181,7 @@ public function removeClass() {
   -      $classes = array_diff($this->storage['class']->value(), $classes);
   +      $classes = array_values(array_diff($this->storage['class']->value(), $classes));
   

   Not sure why this array_values() is needed? Secret sauce?

Re #15.3 because it reindexes the array. So if the values array is 'bb', 'aa' if you remove 'bb' then the values array is 'aa' and if you var_dump() it it will be array(0 => 'aa') not array(1 => 'aa').

Patch adds a comments to explain and show where test coverage is. Leaving at RTBC since this just adds clarification.

Ah thanks for the clarification, yes still RTBC:) And now we have clean indexes too!

Cottser and I were discussing this change on IRC and were curious about what affect on performance it might have. I did some profiling. I tested many times with a custom page that displays teasers for 100 nodes. I also tested a lot on just one node page. We have a few of the preprocess changes already in, including node (Thanks, Alex!) and that template alone has three calls to addClass in it. The numbers I'm seeing fluctuate between 0.5% slower and 0.5% faster, though the majority of runs were in the positive direction. I assume most of it is the margin of error. I'd, unscientifically, call the mode to be around 0.1% faster.

Improved constant name after discussions with @catch. This class is fully tested by AttributeTest and AttributesTest we should merge them into a single AttributeTest since Attribute is the class under test. Leaving at rtbc since this is a very minor change.

re #19 nice work - this is a patch that will keep on giving though - as we ramp up the use of addClass its affect will be bigger.

Committed/pushed to 8.0.x, thanks!