Issue #2475397 by Wim Leers: Tiny follow-up for #2474121 + unit test coverage for (Bubbleable|Cacheable)Metadata::merge()

core/lib/Drupal/Core/Render/BubbleableMetadata.php

@@ -39,16 +39,13 @@ class BubbleableMetadata extends CacheableMetadata {
    *
    * @return static
    *   A new bubbleable metadata object, with the merged data.
-   *
-   * @todo Add unit test for this in
-   *       \Drupal\Tests\Core\Render\BubbleableMetadataTest when
-   *       drupal_merge_attached() no longer is a procedural function and remove
-   *       the '@codeCoverageIgnore' annotation.
    */
   public function merge(CacheableMetadata $other) {
     $result = parent::merge($other);
-    $result->attached = \Drupal::service('renderer')->mergeAttachments($this->attached, $other->attached);
-    $result->postRenderCache = NestedArray::mergeDeep($this->postRenderCache, $other->postRenderCache);
+    if ($other instanceof BubbleableMetadata) {
+      $result->attached = \Drupal::service('renderer')->mergeAttachments($this->attached, $other->attached);
+      $result->postRenderCache = NestedArray::mergeDeep($this->postRenderCache, $other->postRenderCache);
+    }
     return $result;
   }
 

core/tests/Drupal/Tests/Core/Cache/CacheableMetadataTest.php

@@ -12,6 +12,7 @@ use Drupal\Core\Cache\CacheableMetadata;
 use Drupal\Tests\Core\Render\TestCacheableDependency;
 use Drupal\Tests\UnitTestCase;
 use Drupal\Core\Render\Element;
+use Symfony\Component\DependencyInjection\ContainerBuilder;
 
 /**
  * @coversDefaultClass \Drupal\Core\Cache\CacheableMetadata
@@ -19,6 +20,46 @@ use Drupal\Core\Render\Element;
  */
 class CacheableMetadataTest extends UnitTestCase {
 
+  /**
+   * @covers ::merge
+   * @dataProvider providerTestMerge
+   *
+   * This only tests at a high level, because it reuses existing logic. Detailed
+   * tests exist for the existing logic:
+   *
+   * @see \Drupal\Tests\Core\Cache\CacheTest::testMergeTags()
+   * @see \Drupal\Tests\Core\Cache\CacheTest::testMergeMaxAges()
+   * @see \Drupal\Tests\Core\Cache\CacheContextsTest
+   */
+  public function testMerge(CacheableMetadata $a, CacheableMetadata $b, CacheableMetadata $expected) {
+    $cache_contexts_manager = $this->getMockBuilder('Drupal\Core\Cache\CacheContextsManager')
+      ->disableOriginalConstructor()
+      ->getMock();
+    $container = new ContainerBuilder();
+    $container->set('cache_contexts_manager', $cache_contexts_manager);
+    \Drupal::setContainer($container);
+
+    $this->assertEquals($expected, $a->merge($b));
+  }
+
+  /**
+   * Provides test data for testMerge().
+   *
+   * @return array
+   */
+  public function providerTestMerge() {
+    return [
+      // All empty.
+      [(new CacheableMetadata()), (new CacheableMetadata()), (new CacheableMetadata())],
+      // Cache contexts.
+      [(new CacheableMetadata())->setCacheContexts(['foo']), (new CacheableMetadata())->setCacheContexts(['bar']), (new CacheableMetadata())->setCacheContexts(['bar', 'foo'])],
+      // Cache tags.
+      [(new CacheableMetadata())->setCacheTags(['foo']), (new CacheableMetadata())->setCacheTags(['bar']), (new CacheableMetadata())->setCacheTags(['bar', 'foo'])],
+      // Cache max-ages.
+      [(new CacheableMetadata())->setCacheMaxAge(60), (new CacheableMetadata())->setCacheMaxAge(Cache::PERMANENT), (new CacheableMetadata())->setCacheMaxAge(60)],
+    ];
+  }
+
   /**
    * This delegates to Cache::mergeTags(), so just a basic test.
    *

core/tests/Drupal/Tests/Core/Render/BubbleableMetadataTest.php

@@ -9,9 +9,11 @@ namespace Drupal\Tests\Core\Render;
 
 use Drupal\Core\Cache\Cache;
 use Drupal\Core\Cache\CacheableDependencyInterface;
+use Drupal\Core\Cache\CacheableMetadata;
 use Drupal\Core\Render\BubbleableMetadata;
 use Drupal\Tests\UnitTestCase;
 use Drupal\Core\Render\Element;
+use Symfony\Component\DependencyInjection\ContainerBuilder;
 
 /**
  * @coversDefaultClass \Drupal\Core\Render\BubbleableMetadata
@@ -19,6 +21,82 @@ use Drupal\Core\Render\Element;
  */
 class BubbleableMetadataTest extends UnitTestCase {
 
+  /**
+   * @covers ::merge
+   * @dataProvider providerTestMerge
+   *
+   * This only tests at a high level, because it reuses existing logic. Detailed
+   * tests exist for the existing logic:
+   *
+   * @see \Drupal\Tests\Core\Cache\CacheTest::testMergeTags()
+   * @see \Drupal\Tests\Core\Cache\CacheTest::testMergeMaxAges()
+   * @see \Drupal\Tests\Core\Cache\CacheContextsTest
+   * @see \Drupal\system\Tests\Common\MergeAttachmentsTest
+   * @see \Drupal\Tests\Core\Render\RendererPostRenderCacheTest
+   */
+  public function testMerge(BubbleableMetadata $a, CacheableMetadata $b, BubbleableMetadata $expected) {
+    // Verify that if the second operand is a CacheableMetadata object, not a
+    // BubbleableMetadata object, that BubbleableMetadata::merge() doesn't
+    // attempt to merge assets.
+    if (!$b instanceof BubbleableMetadata) {
+      $renderer = $this->getMockBuilder('Drupal\Core\Render\Renderer')
+        ->disableOriginalConstructor()
+        ->getMock();
+      $renderer->expects($this->never())
+        ->method('mergeAttachments');
+    }
+    // Otherwise, let the original ::mergeAttachments() method be executed.
+    else {
+      $renderer = $this->getMockBuilder('Drupal\Core\Render\Renderer')
+        ->disableOriginalConstructor()
+        ->setMethods(NULL)
+        ->getMock();
+    }
+
+    $cache_contexts_manager = $this->getMockBuilder('Drupal\Core\Cache\CacheContextsManager')
+      ->disableOriginalConstructor()
+      ->getMock();
+    $container = new ContainerBuilder();
+    $container->set('cache_contexts_manager', $cache_contexts_manager);
+    $container->set('renderer', $renderer);
+    \Drupal::setContainer($container);
+
+    $this->assertEquals($expected, $a->merge($b));
+  }
+
+  /**
+   * Provides test data for testMerge().
+   *
+   * @return array
+   */
+  public function providerTestMerge() {
+    return [
+      // Second operand is a BubbleableMetadata object.
+      // All empty.
+      [(new BubbleableMetadata()), (new BubbleableMetadata()), (new BubbleableMetadata())],
+      // Cache contexts.
+      [(new BubbleableMetadata())->setCacheContexts(['foo']), (new BubbleableMetadata())->setCacheContexts(['bar']), (new BubbleableMetadata())->setCacheContexts(['bar', 'foo'])],
+      // Cache tags.
+      [(new BubbleableMetadata())->setCacheTags(['foo']), (new BubbleableMetadata())->setCacheTags(['bar']), (new BubbleableMetadata())->setCacheTags(['bar', 'foo'])],
+      // Cache max-ages.
+      [(new BubbleableMetadata())->setCacheMaxAge(60), (new BubbleableMetadata())->setCacheMaxAge(Cache::PERMANENT), (new BubbleableMetadata())->setCacheMaxAge(60)],
+      // Assets.
+      [(new BubbleableMetadata())->setAssets(['library' => ['core/foo']]), (new BubbleableMetadata())->setAssets(['library' => ['core/bar']]), (new BubbleableMetadata())->setAssets(['library' => ['core/foo', 'core/bar']])],
+      // #post_render_cache callbacks.
+      [(new BubbleableMetadata())->setPostRenderCacheCallbacks(['callback' => [['token' => 'A']]]), (new BubbleableMetadata())->setPostRenderCacheCallbacks(['callback' => [['token' => 'B']]]), (new BubbleableMetadata())->setPostRenderCacheCallbacks(['callback' => [['token' => 'A'], ['token' => 'B']]])],
+
+      // Second operand is a CacheableMetadata object.
+      // All empty.
+      [(new BubbleableMetadata()), (new CacheableMetadata()), (new BubbleableMetadata())],
+      // Cache contexts.
+      [(new BubbleableMetadata())->setCacheContexts(['foo']), (new CacheableMetadata())->setCacheContexts(['bar']), (new BubbleableMetadata())->setCacheContexts(['bar', 'foo'])],
+      // Cache tags.
+      [(new BubbleableMetadata())->setCacheTags(['foo']), (new CacheableMetadata())->setCacheTags(['bar']), (new BubbleableMetadata())->setCacheTags(['bar', 'foo'])],
+      // Cache max-ages.
+      [(new BubbleableMetadata())->setCacheMaxAge(60), (new CacheableMetadata())->setCacheMaxAge(Cache::PERMANENT), (new BubbleableMetadata())->setCacheMaxAge(60)],
+    ];
+  }
+
   /**
    * @covers ::applyTo
    * @dataProvider providerTestApplyTo