(dev/core#635) CRM_Utils_Cache::nack() - Fix format

This is a follow-up to #13500.

Before
------

* `CRM_Utils_Cache::nack()` returns an array with a value named `nack`.
* The value returned is somewhat unique -- a random value is generated once per page-view.
* There is no explicit/direct unit-test.

After
-----

* `CRM_Utils_Cache::nack()` returns a string.
* The value returned is more unique -- combining that random value (per page-view) and an offset (per invocation).
* There is an explicit/direct unit-test.

Comments
--------

* The code was originally written with the intent of returning a string.
  However, there was a slight copy-paste error which caused it to return an
  array (which contained that string).  Functionally, that worked (because
  it was serializable and met the same minimum uniqueness constraint),
  but it's weird to read/inspect, and we should change quickly before
  something else locks-in the odd structure.

* The more unique the nack-value is, the more correct the nack-checking
  pattern is.  Appending a static-counter is a simple, fast way to provide
  stronger uniqueness within a page-view.

* There may be some obscure edge-cases in which the previous pattern was not
  sufficiently unique -- e.g.  combining tiers-of-tiers or
  decorators-of-decorators.  I haven't verified that, but it seems
  unimportant given that the static-counter is so straightforward.

* In `NaiveHasTrait`, the extra `ht` suffix was an attempt to increase the
  uniquness.  However, the static-counter seems better.

diff --git a/CRM/Utils/Cache.php b/CRM/Utils/Cache.php
index d147eb1f00815697e38137279066b265f330bc88..a60ccfd84ca8ca376733c58a69c18a75467769ef 100644 (file)
--- a/CRM/Utils/Cache.php
+++ b/CRM/Utils/Cache.php
@@ -285,8 +285,7 @@ class CRM_Utils_Cache {
    *   $value = $cache->get('foo', $nack);
    *   echo ($value === $nack) ? "Cache has a value, and we got it" : "Cache has no value".
    *
-   * The value should be unique to avoid accidental matches. As a performance
-   * tweak, we may reuse the NACK a few times within the current page-view.
+   * The value should be unique to avoid accidental matches.
    *
    * @return string
    *   Unique nonce value indicating a "negative acknowledgement" (failed read).
@@ -294,10 +293,12 @@ class CRM_Utils_Cache {
    *   use `get($key,$nack)`.
    */
   public static function nack() {
-    if (!isset(Civi::$statics[__CLASS__]['nack'])) {
-      Civi::$statics[__CLASS__]['nack'] = 'NACK:' . md5(CRM_Utils_Request::id() . CIVICRM_SITE_KEY . CIVICRM_DSN . mt_rand(0, 10000));
+    $st =& Civi::$statics[__CLASS__];
+    if (!isset($st['nack-c'])) {
+      $st['nack-c'] = md5(CRM_Utils_Request::id() . CIVICRM_SITE_KEY . CIVICRM_DSN . mt_rand(0, 10000));
+      $st['nack-i'] = 0;
     }
-    return Civi::$statics[__CLASS__];
+    return 'NACK:' . $st['nack-c'] . $st['nack-i']++;
   }
 
 }

diff --git a/CRM/Utils/Cache/NaiveHasTrait.php b/CRM/Utils/Cache/NaiveHasTrait.php
index 3a8260e7aff95aff4b1d6b35467c2ab20b5a697f..786e8c0d21a9e1be0ed2f6e603ebb3cc59d2f8fb 100644 (file)
--- a/CRM/Utils/Cache/NaiveHasTrait.php
+++ b/CRM/Utils/Cache/NaiveHasTrait.php
@@ -38,7 +38,7 @@
 trait CRM_Utils_Cache_NaiveHasTrait {
 
   public function has($key) {
-    $nack = CRM_Utils_Cache::nack() . 'ht';
+    $nack = CRM_Utils_Cache::nack();
     $value = $this->get($key, $nack);
     return ($value !== $nack);
   }

diff --git a/tests/phpunit/CRM/Utils/CacheTest.php b/tests/phpunit/CRM/Utils/CacheTest.php
new file mode 100644 (file)
index 0000000..d44b9a5
--- /dev/null
+++ b/tests/phpunit/CRM/Utils/CacheTest.php
@@ -0,0 +1,24 @@
+<?php
+
+/**
+ * Class CRM_Utils_CacheTest
+ * @group headless
+ */
+class CRM_Utils_CacheTest extends CiviUnitTestCase {
+
+  public function testNack() {
+    $values = [];
+    for ($i = 0; $i < 5; $i++) {
+      $nack = CRM_Utils_Cache::nack();
+      $this->assertRegExp('/^NACK:[a-z0-9]+$/', $nack);
+      $values[] = $nack;
+    }
+    sort($values);
+    $this->assertEquals($values, array_unique($values));
+
+    // The random token should at the start should same -- because we don't
+    // the overhead of re-generating it frequently.
+    $this->assertEquals(substr($values[0], 0, 37), substr($values[1], 0, 37));
+  }
+
+}