Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Merged
merged 1 commit into from
Jan 30, 2019

Conversation

totten
Copy link
Member

@totten totten commented Jan 30, 2019

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
    the array met the same minimum uniqueness constraint and support comparison checks),
    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.

This is a follow-up to civicrm#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.
@civibot
Copy link

civibot bot commented Jan 30, 2019

(Standard links)

@civibot civibot bot added the master label Jan 30, 2019
@eileenmcnaughton eileenmcnaughton merged commit da667a5 into civicrm:master Jan 30, 2019
@totten totten deleted the master-nack-rev branch January 31, 2019 06:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants