From 898614742256b2047d0064219a3958c80864b375 Mon Sep 17 00:00:00 2001
From: Tim Otten <totten@civicrm.org>
Date: Tue, 29 Jan 2019 21:08:23 -0800
Subject: [PATCH] (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.
---
 CRM/Utils/Cache.php                   | 11 ++++++-----
 CRM/Utils/Cache/NaiveHasTrait.php     |  2 +-
 tests/phpunit/CRM/Utils/CacheTest.php | 24 ++++++++++++++++++++++++
 3 files changed, 31 insertions(+), 6 deletions(-)
 create mode 100644 tests/phpunit/CRM/Utils/CacheTest.php

diff --git a/CRM/Utils/Cache.php b/CRM/Utils/Cache.php
index d147eb1f00..a60ccfd84c 100644
--- 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 3a8260e7af..786e8c0d21 100644
--- 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
index 0000000000..d44b9a54b5
--- /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));
+  }
+
+}
-- 
2.25.1