From: Tim Otten <totten@civicrm.org>
Date: Mon, 24 Aug 2020 09:47:22 +0000 (-0700)
Subject: CollectionAdderTrait - Use secondary ordering more akin to CRM_Core_Resources
X-Git-Url: https://vcs.fsf.org/?a=commitdiff_plain;h=8b7abdb6cf4e15bc0b20c61435dc04f9db5d45b6;p=civicrm-core.git

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`.
---

diff --git a/CRM/Core/Resources/CollectionAdderTrait.php b/CRM/Core/Resources/CollectionAdderTrait.php
index 392cc4c55d..469d1c426f 100644
--- a/CRM/Core/Resources/CollectionAdderTrait.php
+++ b/CRM/Core/Resources/CollectionAdderTrait.php
@@ -99,6 +99,9 @@ trait CRM_Core_Resources_CollectionAdderTrait {
   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 @@ trait CRM_Core_Resources_CollectionAdderTrait {
   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 @@ trait CRM_Core_Resources_CollectionAdderTrait {
   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 @@ trait CRM_Core_Resources_CollectionAdderTrait {
   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 f750f01db0..4d97aeeced 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 @@ trait CRM_Core_Resources_CollectionTrait {
       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 14e8942350..61787dfd48 100644
--- a/tests/phpunit/CRM/Core/Resources/CollectionTestTrait.php
+++ b/tests/phpunit/CRM/Core/Resources/CollectionTestTrait.php
@@ -42,7 +42,7 @@ trait CRM_Core_Resources_CollectionTestTrait {
     };
 
     $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 @@ trait CRM_Core_Resources_CollectionTestTrait {
         '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 @@ trait CRM_Core_Resources_CollectionTestTrait {
         'name' => 'http://example.com/foo.js',
         'disabled' => FALSE,
         'weight' => 100,
-        'sortId' => 1,
         'type' => 'scriptUrl',
         'scriptUrl' => 'http://example.com/foo.js',
       ]
@@ -84,7 +93,6 @@ trait CRM_Core_Resources_CollectionTestTrait {
         'name' => 'http://example.com/foo.css',
         'disabled' => FALSE,
         'weight' => 1,
-        'sortId' => 1,
         'type' => 'styleUrl',
         'styleUrl' => 'http://example.com/foo.css',
       ]
@@ -99,7 +107,6 @@ trait CRM_Core_Resources_CollectionTestTrait {
         'name' => 'civicrm:css/civicrm.css',
         'disabled' => FALSE,
         'weight' => 1,
-        'sortId' => 1,
         'type' => 'styleFile',
         'styleFile' => ['civicrm', 'css/civicrm.css'],
         'styleFileUrls' => [
@@ -111,7 +118,6 @@ trait CRM_Core_Resources_CollectionTestTrait {
     $basicFooJs = [
       'name' => 'civicrm:js/foo.js',
       'disabled' => FALSE,
-      'sortId' => 1,
       'type' => 'scriptFile',
       'scriptFile' => ['civicrm', 'js/foo.js'],
       'scriptFileUrls' => [
@@ -153,10 +159,10 @@ trait CRM_Core_Resources_CollectionTestTrait {
         '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 @@ trait CRM_Core_Resources_CollectionTestTrait {
       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]);
+      }
+    }
   }
 
 }