Improve the merge() method on Attribute objects

Please join us over in #2623708: Whitelist instances instead of specific classes in Twig sandbox policy, that would be lovely :)

Actually re-opening:) We have a proposal to fix the other in 8.0.x and this can be the 8.1 issue.

Posting latest patch from the other issue

In our project, we're trying to merge an attributes object with dynamic, case-specific attributes into a default attributes object, for which we require the merge() method, too.

We have already implemented the method in the corresponding JavaScript adapter for TwigJS some time ago and it has proven to be a big help: https://github.com/netzstrategen/twig-drupal/pull/15

However, we came to realize that it is not always possible to determine whether an Attributes object is defined or empty or populated, especially with regard to the internal classes property. Therefore I would recommend to make the argument nullable:

+++ b/core/lib/Drupal/Core/Template/Attribute.php
-  public function merge(Attribute $attribute) {
+  public function merge(?Attribute $attribute) {
+    if (!isset($attribute)) {
+      return $this;
+    }

That said, in the JavaScript port we also accept plain objects (associative arrays) as data to be merged. It would make sense to also support that here; at least I can't see a reason for not doing so.

Potentially related issue.

As Twig is automatically type-casting many values, we not only removed the type-hint, but are additionally checking very explicitly for the type of the passed variable to be merged (see patch below).

Will the core components separated into standalone packages soon?

diff --git a/vendor/drupal/core/Attribute.php b/vendor/drupal/core/Attribute.php
index 5449cde..e752340 100644
--- a/vendor/drupal/core/Attribute.php
+++ b/vendor/drupal/core/Attribute.php
@@ -280,6 +280,32 @@ public function hasClass($class) {
   }
 
   /**
+   * Merges the values of another Attributes object with this one.
+   *
+   * @param \Drupal\Core\Template\Attribute|array
+   *   The other Attribute object.
+   *
+   * @return $this
+   */
+  public function merge($attribute) {
+    if (!isset($attribute)) {
+      return $this;
+    }
+    if (!is_array($attribute) && !$attribute instanceof Attribute) {
+      ob_start();
+      var_dump($attribute);
+      error_log('Attribute: Unable to merge non-array argument into attributes: ' . trim(\ob_get_clean()));
+      return $this;
+    }
+    $merged_array = array_merge_recursive($this->toArray(), $attribute instanceof Attribute ? $attribute->toArray() : $attribute);
+    foreach ($merged_array as $attribute => $value) {
+      $this->setAttribute($attribute, $value);
+    }
+
+    return $this;
+  }
+
+  /**
    * Implements the magic __toString() method.
    */
   public function __toString() {

The merge method changes the Attributes object instead of cloning/forking into a new one, which causes unexpected behaviors and problems in loops in Twig templates, for example, when generating the output for a hierarchical navigation menu. We adjusted the method to instantiate a new class instead of modifying the existing object.

This makes the behavior of Attributes.merge consistent with the Twig |merge filter.

This only happens when data is merged, but not in the error/early-return conditions, which is an inconsistent behavior. However, I think this is a larger problem with the current Attributes objects; the same problem also exists for addClass(), setAttribute(), and other methods inside of loops. A copy() or clone() method would be helpful and perhaps more appropriate, so the user can control when a new object is necessary.

diff --git a/vendor/drupal/core/Attribute.php b/vendor/drupal/core/Attribute.php
index 5449cde..e752340 100644
--- a/vendor/drupal/core/Attribute.php
+++ b/vendor/drupal/core/Attribute.php
@@ -280,6 +280,28 @@ public function hasClass($class) {
   }
 
   /**
+   * Merges the values of another Attributes object with this one.
+   *
+   * @param \Drupal\Core\Template\Attribute|array
+   *   The other Attribute object.
+   *
+   * @return $this
+   */
+  public function merge($attribute) {
+    if (!isset($attribute)) {
+      return $this;
+    }
+    if (!is_array($attribute) && !$attribute instanceof Attribute) {
+      ob_start();
+      var_dump($attribute);
+      error_log('Attribute: Unable to merge non-array argument into attributes: ' . trim(\ob_get_clean()));
+      return $this;
+    }
+    $merged_array = array_merge_recursive($this->toArray(), $attribute instanceof Attribute ? $attribute->toArray() : $attribute);
+    return new static($merged_array);
+  }
+
+  /**
    * Implements the magic __toString() method.
    */
   public function __toString() {