Issue #2006282 by joelpittet: Refactor Attribute classes - Cleanup, Security, and Readability and minor performance.

core/lib/Drupal/Core/Template/Attribute.php

@@ -18,7 +18,7 @@ use Drupal\Component\Utility\String;
  *  $attributes = new Attribute(array('id' => 'socks'));
  *  $attributes['class'] = array('black-cat', 'white-cat');
  *  $attributes['class'][] = 'black-white-cat';
- *  echo '<cat ' . $attributes . '>';
+ *  echo '<cat' . $attributes . '>';
  *  // Produces <cat id="socks" class="black-cat white-cat black-white-cat">
  * @endcode
  *
@@ -27,7 +27,7 @@ use Drupal\Component\Utility\String;
  *  $attributes = new Attribute(array('id' => 'socks'));
  *  $attributes['class'] = array('black-cat', 'white-cat');
  *  $attributes['class'][] = 'black-white-cat';
- *  echo '<cat class="cat ' . $attributes['class'] . '" ' . $attributes . '>';
+ *  echo '<cat class="cat ' . $attributes['class'] . '"' . $attributes . '>';
  *  // Produces <cat class="cat black-cat white-cat black-white-cat" id="socks">
  * @endcode
  */
@@ -65,6 +65,21 @@ class Attribute implements \ArrayAccess, \IteratorAggregate {
    * Implements ArrayAccess::offsetSet().
    */
   public function offsetSet($name, $value) {
+    $this->storage[$name] = $this->createAttributeValue($name, $value);
+  }
+
+  /**
+   * Creates the different types of attribute values.
+   *
+   * @param string $name
+   *   The attribute name.
+   * @param mixed $value
+   *   The attribute value.
+   *
+   * @return \Drupal\Core\Template\AttributeValueBase
+   *   An AttributeValueBase representation of the attribute's value.
+   */
+  protected function createAttributeValue($name, $value) {
     if (is_array($value)) {
       $value = new AttributeArray($name, $value);
     }
@@ -74,13 +89,7 @@ class Attribute implements \ArrayAccess, \IteratorAggregate {
     elseif (!is_object($value)) {
       $value = new AttributeString($name, $value);
     }
-    // The $name could be NULL.
-    if (isset($name)) {
-      $this->storage[$name] = $value;
-    }
-    else {
-      $this->storage[] = $value;
-    }
+    return $value;
   }
 
   /**
@@ -104,9 +113,9 @@ class Attribute implements \ArrayAccess, \IteratorAggregate {
     $return = '';
     foreach ($this->storage as $name => $value) {
       if (!$value->printed()) {
-        $rendered = is_object($value) ? $value->render() : (String::checkPlain($name) . ' = "' . String::checkPlain($value) . '"');
+        $rendered = $value->render();
         if ($rendered) {
-          $return .= " $rendered";
+          $return .= ' ' . $rendered;
         }
       }
     }
@@ -118,9 +127,7 @@ class Attribute implements \ArrayAccess, \IteratorAggregate {
    */
   public function  __clone() {
     foreach ($this->storage as $name => $value) {
-      if (is_object($value)) {
-        $this->storage[$name] = clone $value;
-      }
+      $this->storage[$name] = clone $value;
     }
   }
 

core/lib/Drupal/Core/Template/AttributeArray.php

@@ -15,13 +15,13 @@ use Drupal\Component\Utility\String;
  * To use with Attribute, the array must be specified.
  * Correct:
  * @code
- *  $attributes = new Attribute(array());
+ *  $attributes = new Attribute();
  *  $attributes['class'] = array();
  *  $attributes['class'][] = 'cat';
  * @endcode
  * Incorrect:
  * @code
- *  $attributes = new Attribute(array());
+ *  $attributes = new Attribute();
  *  $attributes['class'][] = 'cat';
  * @endcode
  *
@@ -67,7 +67,7 @@ class AttributeArray extends AttributeValueBase implements \ArrayAccess, \Iterat
    */
   public function __toString() {
     $this->printed = TRUE;
-    return implode(' ', array_map(array('Drupal\Component\Utility\String', 'checkPlain'), $this->value));
+    return String::checkPlain(implode(' ', $this->value));
   }
 
   /**

core/lib/Drupal/Core/Template/AttributeBoolean.php

@@ -17,12 +17,12 @@ use Drupal\Component\Utility\String;
  *
  * To set a boolean attribute on the Attribute class, set it to TRUE.
  * @code
- *  $attributes = new Attribute(array());
+ *  $attributes = new Attribute();
  *  $attributes['disabled'] = TRUE;
- *  echo '<select ' . $attributes . '/>';
+ *  echo '<select' . $attributes . '/>';
  *  // produces <select disabled>;
  *  $attributes['disabled'] = FALSE;
- *  echo '<select ' . $attributes . '/>';
+ *  echo '<select' . $attributes . '/>';
  *  // produces <select>;
  * @endcode
  *

core/lib/Drupal/Core/Template/AttributeValueBase.php

@@ -7,6 +7,8 @@
 
 namespace Drupal\Core\Template;
 
+use Drupal\Component\Utility\String;
+
 /**
  * Defines the base class for an attribute type.
  *
@@ -53,7 +55,7 @@ abstract class AttributeValueBase {
    *   The string representation of the attribute.
    */
   public function render() {
-    return $this->name . '="' . $this . '"';
+    return String::checkPlain($this->name) . '="' . $this . '"';
   }
 
   /**

core/tests/Drupal/Tests/Core/Common/AttributesTest.php

@@ -31,9 +31,13 @@ class AttributesTest extends UnitTestCase {
   public function providerTestAttributeData() {
     return array(
       // Verify that special characters are HTML encoded.
+      array(array('&"\'<>' => 'value'), ' &amp;&quot;&#039;&lt;&gt;="value"', 'HTML encode attribute names.'),
       array(array('title' => '&"\'<>'), ' title="&amp;&quot;&#039;&lt;&gt;"', 'HTML encode attribute values.'),
       // Verify multi-value attributes are concatenated with spaces.
       array(array('class' => array('first', 'last')), ' class="first last"', 'Concatenate multi-value attributes.'),
+      // Verify boolean attribute values are rendered correctly.
+      array(array('disabled' => TRUE), ' disabled', 'Boolean attribute is rendered.'),
+      array(array('disabled' => FALSE), '', 'Boolean attribute is not rendered.'),
       // Verify empty attribute values are rendered.
       array(array('alt' => ''), ' alt=""', 'Empty attribute value #1.'),
       array(array('alt' => NULL), ' alt=""', 'Empty attribute value #2.'),