Ensure that mixing array and Attribute objects in theme rendering is managed properly

Instead of only supporting 'real'(non-object) arrays and instances of Attribute, shouldn't we additionally allow all instances of ArrayAcces?

1. +++ b/core/lib/Drupal/Core/Template/Attribute.php
   @@ -356,4 +356,78 @@ public function jsonSerialize() {
   +  /**
   +   * Returns an attribute object or array always as an array.
   +   *
   +   * @param \Drupal\Core\Template\Attribute|array $attribute
   +   *   The Attribute object or an array of attributes.
   +   *
   +   * @return array
   +   *   The attributes as an array.
   +   */
   +  public static function getAsArray($attribute) {
   

   This is an odd method.

2. +++ b/core/lib/Drupal/Core/Template/Attribute.php
   @@ -356,4 +356,78 @@ public function jsonSerialize() {
   +   * @param array ...
   +   *   Attributes to merge.
   

   We support varidics now... see https://www.php.net/manual/en/migration56.new-features.php

3. +++ b/core/lib/Drupal/Core/Template/Attribute.php
   @@ -356,4 +356,78 @@ public function jsonSerialize() {
   +   * @return \Drupal\Core\Template\Attribute|array
   +   *   The merged attributes.
   +   */
   +  public static function mergeDeep() {
   

   How can this return an Attribute object?

4. +++ b/core/lib/Drupal/Core/Template/Attribute.php
   @@ -356,4 +356,78 @@ public function jsonSerialize() {
   +   * @return array
   +   *   The merged attributes, as an array.
   +   */
   +  public static function mergeDeepAttributes(array $attributes, $preserve_integer_keys = FALSE) {
   

   This returning an array feels quite odd when its on the Attributes object.

Instead of the two new methods on the Attribute class, I think we should do a simple ternary assignment like $attributes = $variables['attributes'] instanceof Attribute ? $variables['attributes']->toArray() : $variables['attributes']; in those three places.

Looks ready to go, could use follow-up to deprecate array argument types and to find a way to clean-up usage

Tested the patch #10. It works for me. The only concern is about coding standards.

+++ b/core/lib/Drupal/Core/Template/Attribute.php
@@ -356,4 +356,70 @@ public function jsonSerialize() {
+  public static function getnames($attribute) {

According to it, methods should use lowerCamel naming.

+++ b/core/lib/Drupal/Core/Template/Attribute.php
@@ -356,4 +356,70 @@ public function jsonSerialize() {
+   * Returns the keys of an Attribute object or array.

This is perhaps bikeshedding but why not getKeys instead of getNames?

+++ b/core/includes/theme.inc
@@ -819,7 +819,7 @@ function template_preprocess_image(&$variables) {
+      if (in_array($key, Attribute::getNames($variables['attributes']))) {

Or, this could become a keyExists() helper?

1. +++ b/core/lib/Drupal/Core/Template/Attribute.php
   @@ -356,4 +356,70 @@ public function jsonSerialize() {
   +  public static function getnames($attribute) {
   

   +1 on renaming this. Or even changing the functionality to be hasKey or keyExists().

2. +++ b/core/lib/Drupal/Core/Template/Attribute.php
   @@ -356,4 +356,70 @@ public function jsonSerialize() {
   +        // Otherwise, use the latter value, overriding any previous value.
   +        else {
   +          $result[$key] = $value;
   +        }
   

   It'd be good to have something other then arrays of classes in the tests to test this a bit more. For href or id or name - actually the name attribute is another reason why getnames() is badly named.

3. +++ b/core/tests/Drupal/Tests/Core/Template/AttributeTest.php
   @@ -452,4 +452,31 @@ public function testStorage() {
   +    $this->expectException(\InvalidArgumentException::class);
   +    $this->expectExceptionMessage('Invalid attribute argument');
   +    Attribute::merge('not an array', new Attribute(['class' => ['class1']]));
   +    $this->expectException(\InvalidArgumentException::class);
   +    $this->expectExceptionMessage('Invalid attribute argument');
   +    Attribute::merge(new Attribute(['class' => ['example-class']]), 'not an array');
   

   This tests should be broken into separate tests. I'm not sure the second test is even running. Checked locally and I can change the second exception message to whatever I want and the test still passes.

I've updated the patch from #10 according to the last comments.

Fixed coding standards issues in last patch.

1. +++ b/core/lib/Drupal/Core/Template/Attribute.php
   @@ -356,4 +356,73 @@ public function jsonSerialize() {
   +    if ($attributes instanceof static) {
   +      return in_array($key, array_keys($attributes->toArray()));
   +    }
   +    elseif (is_array($attributes)) {
   +      return in_array($key, array_keys($attributes));
   +    }
   +    throw new \InvalidArgumentException('Invalid attribute argument');
   

   in_array() is prone to strange results plus we're doing unnecessary work here. We can do this instead:

       if (!($attributes instanceof static || is_array($attributes))) {
         throw new \InvalidArgumentException('Invalid attribute argument');
       }
       return array_key_exists($key, $attributes instanceof static ? $attributes->storage : $attributes);

2. +++ b/core/lib/Drupal/Core/Template/Attribute.php
   @@ -356,4 +356,73 @@ public function jsonSerialize() {
   +   * @param \Drupal\Core\Template\Attribute|array $attribute_a
   ...
   +   * @param \Drupal\Core\Template\Attribute|array $attribute_b
   

   Let's call these $a and $b - the prefix makes this harder to read because so much of the variable name is the same. Also we have precedence for this in core - see \Drupal\jsonapi\JsonApiResource\Link::merge() and \Drupal\Core\Render\BubbleableMetadata::mergeAttachments()

3. +++ b/core/lib/Drupal/Core/Template/Attribute.php
   @@ -356,4 +356,73 @@ public function jsonSerialize() {
   +  public static function merge($attribute_a, $attribute_b) {
   +    if (!($attribute_a instanceof static || is_array($attribute_a)) || !($attribute_b instanceof static || is_array($attribute_b))) {
   +      throw new \InvalidArgumentException('Invalid attribute argument');
   +    }
   +
   +    $merge_a = $attribute_a instanceof static ? $attribute_a->toArray() : $attribute_a;
   +    $merge_b = $attribute_b instanceof static ? $attribute_b->toArray() : $attribute_b;
   +    $result = [];
   +    foreach ([$merge_a, $merge_b] as $attribute) {
   +      foreach ($attribute as $key => $value) {
   +        // Renumber integer keys as array_merge_recursive() does.
   +        if (is_int($key)) {
   +          $result[] = $value;
   +        }
   +        // Recurse when both values are arrays.
   +        elseif (isset($result[$key]) && is_array($result[$key]) && is_array($value)) {
   +          $result[$key] = self::merge($result[$key], $value);
   +        }
   +        // Otherwise, use the latter value, overriding any previous value.
   +        else {
   +          $result[$key] = $value;
   +        }
   +      }
   +    }
   +
   +    return $attribute_a instanceof static ? new static($result) : $result;
   +  }
   

   I don't think we should be duplicating logic from NestedArray here. We could do

     public static function merge($a, $b) {
       if (!($a instanceof static || is_array($a)) || !($b instanceof static || is_array($b))) {
         throw new \InvalidArgumentException('Invalid attribute argument');
       }
       $result = NestedArray::mergeDeep(
         $a instanceof static ? $a->toArray() : $a,
         $b instanceof static ? $b->toArray() : $b,
       );
   
       return $a instanceof static ? new static($result) : $result;
     }
   

   Because everything we're passing is an array we don;t run into the problem @chx mentions in the issue summary.

4. +++ b/core/tests/Drupal/Tests/Core/Template/AttributeTest.php
   @@ -452,4 +452,47 @@ public function testStorage() {
   +    $this->expectException(\InvalidArgumentException::class);
   +    $this->expectExceptionMessage('Invalid attribute argument');
   +    Attribute::keyExists('class', 'not an array');
   

   For similar reasons let's move this to a separate method for exception testing.

5. +++ b/core/tests/Drupal/Tests/Core/Template/AttributeTest.php
   @@ -452,4 +452,47 @@ public function testStorage() {
   +    $this->assertEquals(['class' => ['example-class', 'class1']], Attribute::merge(['class' => ['example-class']], ['class' => ['class1']]));
   +    $this->assertEquals(['class' => ['example-class', 'class1']], Attribute::merge(['class' => ['example-class']], new Attribute(['class' => ['class1']])));
   +    $this->assertEquals(new Attribute(['class' => ['example-class', 'class1']]), Attribute::merge(new Attribute(['class' => ['example-class']]), ['class' => ['class1']]));
   +    $this->assertEquals(new Attribute(['class' => ['example-class', 'class1']]), Attribute::merge(new Attribute(['class' => ['example-class']]), new Attribute(['class' => ['class1']])));
   

   It would be good to not only test class => array here... something like name or id.

@mondrake I'm not sure that #19 will work. The problem is these methods should not change the type of the variable during template preprocessing.

Re getNames vs getKeys - your point is a very good one. If the documentation of the gertNames method made it clear that it was this "name" it was getting then it'd be better. Also - very very good point on Attribute vs Attribute collection. I''d love to see that re-name as part of #2664570: Move Attribute classes under Drupal\Component

+++ b/core/includes/theme.inc
@@ -1578,7 +1581,9 @@ function template_preprocess_field(&$variables, $hook) {
+    $attributes->merge((array) $element['#items'][0]->_attributes);

Why do we need to cast to an array?

Looks good to go for me.

I'm sorry I was wrong. By removing the typecasting we're introducing a very subtle API change. Let's not do that. Especially when this issue is all about not making subtle API changes by converting between arrays and attribute objects.

+++ b/core/includes/theme.inc
@@ -1243,7 +1244,9 @@ function template_preprocess(&$variables, $hook, $info) {
+      $attributes = $variables['attributes'] instanceof Attribute ? $variables['attributes'] : new Attribute($variables['attributes']);
+      $attributes->merge($variables[$key]['#attributes']);
+      $variables['attributes'] = $variables['attributes'] instanceof Attribute ? $attributes : $attributes->toArray();

So I was really surprised by the change from #17 to #22. I preferred the static method helpers that dealt with type for the developer. Here's why:
1. Looking at the above code that's a lot a theme developer to know about in order to use this method.
2. As we've seen preprocess functions can change an array to an attributes object and vice versa so less thinking anyone has to do the better.

Can we go back to #17 + the fixes asked for in #18?

@mondrake but then we’re going to have even more naming fun

@mondrake it's not pretty :) but then again having to deal with either an array or an attribute object is not pretty either so I think the compromise of adding an AttributeHelper is a good one. I think we need a change record to tell developers that this helper exists and why it is good to use it.

1. +++ b/core/includes/theme.inc
   @@ -821,7 +821,7 @@ function template_preprocess_image(&$variables) {
   -      if (array_key_exists($key, $variables['attributes'])) {
   +      if (AttributeHelper::attributeExists($key, $variables['attributes'])) {
   
   @@ -1243,7 +1243,7 @@ function template_preprocess(&$variables, $hook, $info) {
   -      $variables['attributes'] = NestedArray::mergeDeep($variables['attributes'], $variables[$key]['#attributes']);
   +      $variables['attributes'] = AttributeHelper::mergeCollections($variables['attributes'], $variables[$key]['#attributes']);
   
   @@ -1578,7 +1578,7 @@ function template_preprocess_field(&$variables, $hook) {
   -    $variables['attributes'] = NestedArray::mergeDeep($variables['attributes'], (array) $element['#items'][0]->_attributes);
   +    $variables['attributes'] = AttributeHelper::mergeCollections($variables['attributes'], (array) $element['#items'][0]->_attributes);
   

   This looks great from a developer experience POV.

2. +++ b/core/lib/Drupal/Core/Template/Attribute.php
   @@ -208,6 +209,19 @@ public function setAttribute($attribute, $value) {
   +  public function hasAttribute($name) {
   
   @@ -356,4 +370,20 @@ public function jsonSerialize() {
   +  public function merge(Attribute $collection) {
   

   Having these methods here will be great if we ever get to the place where all #attributes properties in themes are Attribute objects. Nice.

@mondrake the CR looks good thanks.

I've updated the issue summary with the solution and other required information.

This fix should target 8.8.x as we'd like to support PHP 7.4 on 8.8.x

Committed f90ca9b and pushed to 9.0.x. Thanks!

Cherry-picked to 8.9.x. But leaving at 'to be ported' against 8.8.x for after packaging is fixed. I think adding the new method and class to a patch release is OK given this is a critical bug fix although would be nice to commit it just after rather than just before a patch release.

8.8.2 is out now. Cherry-picked to 8.8.x