Use variadics in Cache::merge* functions - for example Cache::mergeTags()

Looks good, just a docs clarity problem:

+++ b/core/lib/Drupal/Core/Cache/Cache.php
@@ -20,16 +20,14 @@ class Cache {
    *   Cache contexts array to merge.

@@ -46,18 +44,15 @@ public static function mergeContexts(array $a = [], array $b = []) {
    *   Cache tags array to merge.

Should these say 'Cache contexts arrayS to merge.'? The other param that has become variadic is being changed from saying 'value' to 'values'.

+++ b/core/tests/Drupal/Tests/Core/Cache/CacheTest.php
@@ -73,18 +80,26 @@ public function testMergeTags(array $a, array $b, array $expected) {
+      [Cache::PERMANENT, Cache::PERMANENT, FALSE, Cache::PERMANENT],

@@ -93,8 +108,13 @@ public function mergeMaxAgesProvider() {
+    if ($c === FALSE) {

Maybe nitpicking but NULL feels like a better thing to use to represent a missing value, I had to read this twice before I figured out what was going on here but I think NULL would make it clearer from the data provider alone.

edit: or even, just pass $a, $b and optionally $c as a single array argument, and use the splat operator in testMergeMaxAges() too!

edit: or even, just pass $a, $b and optionally $c as a single array argument, and use the splat operator in testMergeMaxAges() too!

Adopted this approach.

We could also change testMergeTags() the same way rather than adding a separate test method?

Also slightly surprised that CacheTest covers tags and max ages but not contexts - I can't find a unit test for mergeContexts, is adding basic coverage for that out of scope?

• Did the same for testMergeTags()
• covered mergeContexts()

1. And BTW, \Drupal\Tests\Core\Cache\CacheTest::validateTagsProvider() seems not being used.

2. +++ b/core/lib/Drupal/Core/Cache/Cache.php
   @@ -20,16 +20,14 @@ class Cache {
   +   * @param string[] ...
   +   *   Cache contexts arrays to merge.
   
   @@ -46,18 +44,15 @@ public static function mergeContexts(array $a = [], array $b = []) {
   +   * @param string[] ...
   

   Should the variable be added right after ...?

+++ b/core/tests/Drupal/Tests/Core/Cache/CacheTest.php
@@ -62,8 +65,15 @@ public function mergeTagsProvider() {
+    $this->assertEquals(['bar', 'foo', 'llama'], Cache::mergeTags(['bar', 'foo'], ['foo', 'bar'], ['llama', 'foo']));

Now we can just roll this into mergeTagsProvider()

Addressed #14, file a new issue for #13.1 if necessary.

A wrong patch uploaded in #15

+++ b/core/lib/Drupal/Core/Cache/Cache.php
@@ -20,16 +20,14 @@ class Cache {
+   * @param string[] ...$cache_contexts

@@ -46,18 +44,15 @@ public static function mergeContexts(array $a = [], array $b = []) {
+   * @param string[] ...

Do we use the variable name or not in variadic @params? The coding standards docs do not say.

All looks good - new functionality plus improved test coverage.

Random fail in FieldLayoutTest

For API changes

Change Record added, setting back to RTBC

Committed/pushed to 9.1.x, thanks!

I'm a bit borderline on backporting this to 8.9.x, but leaving in 9.1.x for now since it's an obvious API addition even if there are no bc issues as such.

@catch, thank you for committing!

Should the CR associated be published? And as a reminder, introduced in branch and Introduced in version should be adjusted before publishing.

OMG so nice! 😍