CollectionAdderTrait - Use secondary ordering more akin to CRM_Core_R…

…esources

This is a very subtle behavioral change.  To understand it, we must consider
the traditional ordering in `CRM_Core_Resources::addScriptUrl()` and
`CRM_Core_Region::add()`.  Compare these two examples:

```
Civi::resources()->addScriptUrl('http://example.com/foo.js', 100, 'page-footer');
Civi::resources()->addScriptUrl('http://example.com/bar.js', 100, 'page-footer');

CRM_Core_Region::instance('page-footer')->add([
  'scriptUrl' => 'http://example.com/foo.js',
  'weight' => 100,
]);
CRM_Core_Region::instance('page-footer')->add([
  'scriptUrl' => 'http://example.com/bar.js',
  'weight' => 100,
]);
```

You might expect this to output `<SCRIPT>`s in the same order.  After all,
the examples use the same URLs, the same weights, and the same
procedural/natural order.  Moreover, `Civi::resources()` is just a wrapper
for `CRM_Core_Region`.  Surely they were the same!

They were not. The output order would differ.

The reason is that the ordering had two keys (`weight,name`), and the
secondary-key (`name`) was hidden from us:

* In `CRM_Core_Resources::addScriptUrl()`, the default `name` was the URL.
  This was handy for de-duping resources.
* In `CRM_Core_Region::add()`, the default `name` was a numeric position.
  This made it behave like procedural/natural-ordering.

Of course, the goal in this branch is to make various collections support
the same methods and the same behaviors.  But they didn't agree before, so
something has to give.  Either:

1. Standardize on the behavior of `Resources::addScriptUrl()` and change `Region::add(scriptUrl)`.
2. Standardize on the behavior of `Region::add(scriptUrl)` and change `Resources::addScriptUrl()`.
3. Embrace a split. All `CRM_Core_Resources::add*()` behave one way, and all `CRM_Core_Region::add*()` behave another.
4. Embrace a split. All rich adders (`*::addScriptUrl()`) behave one way, and all generic adders (`*::add()`) behave another.

This developmental branch has been using `#3`. The patch changes it to `#4`.

Both splits achieve backward compatibility wrt `Resources::addScriptUrl()`
and `Region::add(scriptUrl)`.  The difference is that `#4` allows us to have
the same behavior in all collections (regardless of subtype), which means
that collections can be more interoperable and share more code.  From my
POV, it's still quirkier than `#1` or `#2`, but it's not as quirky as `#3`.

CRM/Core/Resources/CollectionAdderTrait.php

@@ -99,6 +99,9 @@ public function addScript(string $code, ...$options) {
   public function addScriptFile(string $ext, string $file, ...$options) {
     $this->add(self::mergeStandardOptions($options, [
       'scriptFile' => [$ext, $file],
+      'name' => "$ext:$file",
+      // Setting the name above may appear superfluous, but it preserves a historical quirk
+      // where Region::add() and Resources::addScriptFile() produce slightly different orderings.
     ]));
     return $this;
   }
@@ -119,6 +122,9 @@ public function addScriptFile(string $ext, string $file, ...$options) {
   public function addScriptUrl(string $url, ...$options) {
     $this->add(self::mergeStandardOptions($options, [
       'scriptUrl' => $url,
+      'name' => $url,
+      // Setting the name above may appear superfluous, but it preserves a historical quirk
+      // where Region::add() and Resources::addScriptUrl() produce slightly different orderings.
     ]));
     return $this;
   }
@@ -215,6 +221,9 @@ public function addStyle(string $code, ...$options) {
   public function addStyleFile(string $ext, string $file, ...$options) {
     $this->add(self::mergeStandardOptions($options, [
       'styleFile' => [$ext, $file],
+      'name' => "$ext:$file",
+      // Setting the name above may appear superfluous, but it preserves a historical quirk
+      // where Region::add() and Resources::addScriptUrl() produce slightly different orderings.
     ]));
     return $this;
   }
@@ -235,6 +244,9 @@ public function addStyleFile(string $ext, string $file, ...$options) {
   public function addStyleUrl(string $url, ...$options) {
     $this->add(self::mergeStandardOptions($options, [
       'styleUrl' => $url,
+      'name' => $url,
+      // Setting the name above may appear superfluous, but it preserves a historical quirk
+      // where Region::add() and Resources::addScriptUrl() produce slightly different orderings.
     ]));
     return $this;
   }

CRM/Core/Resources/CollectionTrait.php

@@ -64,6 +64,7 @@ trait CRM_Core_Resources_CollectionTrait {
    */
   public function add($snippet) {
     $snippet = array_merge($this->defaults, $snippet);
+    $snippet['id'] = $this->nextId();
     if (!isset($snippet['type'])) {
       foreach ($this->types as $type) {
         // auto-detect
@@ -85,18 +86,18 @@ public function add($snippet) {
       switch ($snippet['type']) {
         case 'scriptUrl':
         case 'styleUrl':
-          $snippet['sortId'] = $this->nextId();
+          $snippet['sortId'] = $snippet['id'];
           $snippet['name'] = $snippet[$snippet['type']];
           break;
 
         case 'scriptFile':
         case 'styleFile':
-          $snippet['sortId'] = $this->nextId();
+          $snippet['sortId'] = $snippet['id'];
           $snippet['name'] = implode(':', $snippet[$snippet['type']]);
           break;
 
         default:
-          $snippet['sortId'] = $this->nextId();
+          $snippet['sortId'] = $snippet['id'];
           $snippet['name'] = $snippet['sortId'];
           break;
       }

tests/phpunit/CRM/Core/Resources/CollectionTestTrait.php

@@ -42,7 +42,7 @@ public function getSnippetExamples() {
     };
 
     $addCases(
-    // List of equivalent method calls
+      // List of equivalent method calls
       [
         'add(scriptUrl): dfl' => ['add', ['scriptUrl' => 'http://example.com/foo.js']],
         'addScriptUrl(): dfl' => ['addScriptUrl', 'http://example.com/foo.js'],
@@ -53,12 +53,22 @@ public function getSnippetExamples() {
         'name' => 'http://example.com/foo.js',
         'disabled' => FALSE,
         'weight' => 1,
-        'sortId' => 1,
         'type' => 'scriptUrl',
         'scriptUrl' => 'http://example.com/foo.js',
       ]
     );
 
+    // For historical reasons, the `add(scriptUrl)` and `addScriptUrl()` calls
+    // differ very slightly in how data is ordered.
+    $addCases(
+      ['add(scriptUrl): dfl: sortId' => ['add', ['scriptUrl' => 'http://example.com/foo.js']]],
+      ['sortId' => ($this instanceof CRM_Core_RegionTest ? 2 : 1)]
+    );
+    $addCases(
+      ['addScriptUrl(): dfl: sortId' => ['addScriptUrl', 'http://example.com/foo.js']],
+      ['sortId' => 'http://example.com/foo.js']
+    );
+
     $addCases(
       [
         'add(scriptUrl): wgt' => ['add', ['scriptUrl' => 'http://example.com/foo.js', 'weight' => 100]],
@@ -69,7 +79,6 @@ public function getSnippetExamples() {
         'name' => 'http://example.com/foo.js',
         'disabled' => FALSE,
         'weight' => 100,
-        'sortId' => 1,
         'type' => 'scriptUrl',
         'scriptUrl' => 'http://example.com/foo.js',
       ]
@@ -84,7 +93,6 @@ public function getSnippetExamples() {
         'name' => 'http://example.com/foo.css',
         'disabled' => FALSE,
         'weight' => 1,
-        'sortId' => 1,
         'type' => 'styleUrl',
         'styleUrl' => 'http://example.com/foo.css',
       ]
@@ -99,7 +107,6 @@ public function getSnippetExamples() {
         'name' => 'civicrm:css/civicrm.css',
         'disabled' => FALSE,
         'weight' => 1,
-        'sortId' => 1,
         'type' => 'styleFile',
         'styleFile' => ['civicrm', 'css/civicrm.css'],
         'styleFileUrls' => [
@@ -111,7 +118,6 @@ public function getSnippetExamples() {
     $basicFooJs = [
       'name' => 'civicrm:js/foo.js',
       'disabled' => FALSE,
-      'sortId' => 1,
       'type' => 'scriptFile',
       'scriptFile' => ['civicrm', 'js/foo.js'],
       'scriptFileUrls' => [
@@ -153,10 +159,10 @@ public function getSnippetExamples() {
         'addScript()' => ['addScript', 'window.alert("Boo!");'],
       ],
       [
-        'name' => 1,
+        // Regions always have a 'default' with ID#1, so our test always becomes #2.
+        'name' => ($this instanceof CRM_Core_RegionTest ? 2 : 1),
         'disabled' => FALSE,
         'weight' => 1,
-        'sortId' => 1,
         'type' => 'script',
         'script' => 'window.alert("Boo!");',
       ]
@@ -321,17 +327,17 @@ public function assertSameSnippet($expect, $actual, $message = '') {
       return $snippet;
     };
 
-    // If there isn't an explicit expectation for 'region', then we won't check it.
-    if (!isset($expect['region']) || '*' === $expect['region']) {
-      if (isset($actual['region'])) {
-        unset($expect['region']);
-        unset($actual['region']);
-      }
-    }
-
     $expect = $normalizeSnippet($expect);
     $actual = $normalizeSnippet($actual);
-    $this->assertEquals($expect, $actual, $message);
+
+    foreach ($expect as $expectKey => $expectValue) {
+      if ($expectValue === '*') {
+        $this->assertTrue(!empty($actual[$expectKey]));
+      }
+      else {
+        $this->assertEquals($expectValue, $actual[$expectKey]);
+      }
+    }
   }
 
 }