From 8b7abdb6cf4e15bc0b20c61435dc04f9db5d45b6 Mon Sep 17 00:00:00 2001
From: Tim Otten <totten@civicrm.org>
Date: Mon, 24 Aug 2020 02:47:22 -0700
Subject: [PATCH] CollectionAdderTrait - Use secondary ordering more akin to
 CRM_Core_Resources

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   | 12 ++++++
 CRM/Core/Resources/CollectionTrait.php        |  7 ++--
 .../Core/Resources/CollectionTestTrait.php    | 40 +++++++++++--------
 3 files changed, 39 insertions(+), 20 deletions(-)

diff --git a/CRM/Core/Resources/CollectionAdderTrait.php b/CRM/Core/Resources/CollectionAdderTrait.php
index 392cc4c55da9..469d1c426f88 100644
--- a/CRM/Core/Resources/CollectionAdderTrait.php
+++ b/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;
   }
diff --git a/CRM/Core/Resources/CollectionTrait.php b/CRM/Core/Resources/CollectionTrait.php
index f750f01db077..4d97aeeced74 100644
--- a/CRM/Core/Resources/CollectionTrait.php
+++ b/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;
       }
diff --git a/tests/phpunit/CRM/Core/Resources/CollectionTestTrait.php b/tests/phpunit/CRM/Core/Resources/CollectionTestTrait.php
index 14e8942350cb..61787dfd4845 100644
--- a/tests/phpunit/CRM/Core/Resources/CollectionTestTrait.php
+++ b/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]);
+      }
+    }
   }
 
 }