(dev/core#635) Deprecate CRM_Core_BAO_Cache for I/O. Optionally redirect I/O to Redis or Memcache.

[totten]

Overview

CRM_Core_BAO_Cache provides an interface for reading and writing cache data (setItem($data,$group,$path), getItem($group,$path), getItems($group), deleteGroup($group,$path)). There are two problems with this:

• CRM_Core_BAO_Cache is hard-coded to an implementation with a specific data-storage policy (combining a static::$cache with a table SQL civicrm_cache). It's a fine default, but it's problematic for dev/core#635 -- in rodb configuration, writing to civicrm_cache forces the readers to switch to the master DB and generates distributed writes.
• There's a newer standard, PSR-16, which is more flexible and complete. PSR-16 supports TTLs, default-values, multi-key operations, etc. PSR-16 drivers can be extended/decorated/replaced. There are third-party implementations of PSR-16. Once you transition to PSR-16 interfaces, it becomes easier to swap implementations (e.g. swapping among Redis, Memcache, MySQL, APC).

On the other hand, CRM_Core_BAO_Cache has been around forever. We've migrated a few of the weirdest use-cases, but I still count ten distinct cache-groups which rely on CRM_Core_BAO_Cache (5 from civicrm-core and 5 from universe). Removing it outright would cause breakage (or at least front-load us with a game of whack-a-mole).

This patch is the next step toward phasing-out CRM_Core_BAO_Cache:

• Deprecate the I/O functions in CRM_Core_BAO_Cache. (Note: The I/O functions are specifically setItem(), getItem(), getItems(), deleteGroup(). Other functions -- like cleanKey(), encode(), decode() -- are qualitatively different.)
• Optionally, map the deprecated functions to equivalent PSR-16 functions.

Before

• Requests for CRM_Core_BAO_Cache ( setItem($data,$group,$path), getItem($group,$path), getItems($group), deleteGroup($group, $item) ) are always served by two tiers: (1) an in-memory array (static::$cache) and (2) an SQL table.

After

• There is a config option define('CIVICRM_BAO_CACHE_ADAPTER', 'CRM_Core_BAO_Cache_Psr16');.

  • When disabled (default), CRM_Core_BAO_Cache continues using the old code.
  • When enabled, CRM_Core_BAO_Cache changes behavior. Each $group is mapped to a PSR-16 object.

• The class/implementation for each $group depends on the configuration:

  • In a typical (non-Redis/non-Memcache) deployment, the implementation is CRM_Utils_Cache_SqlGroup, which has the same 2-tier structure (in-memory+SQL).
  • In Redis/Memcache deployment, the implementation combines FastArrayDecorator with CRM_Utils_Cache_Redis or CRM_Utils_Cache_Memcache. This gives a similar 2-tier structure (e.g. in-memory+Redis).

• In RODB configuration, I'm seeing far fewer occasions where it unexpectedly/unnecessarily directs the user the master DB.

Comments

This depends-on and incorporates #13500 and #13496. The commit list looks long, but it is actually only two items (at time of writing):

• CRM_Core_BAO_Cache - Deprecate getItems(), getItem(), setItem(), deleteGroup()
• Allow rerouting CRM_Core_BAO_Cache::{set,get}Item(s) to PSR-16 drivers

There are some small ways in which the PSR-16 adapter is different, e.g.

• getItems() will throw an exception. This is actually pretty difficult to implement in pure PSR-16. Fortunately, I can't find anything (in civicrm-core or universe) which calls it.
• All SQL-based caches are cleared when one does a system-flush (i.e. TRUNCATE TABLE civicrm_cache). To provide a similar behavior on non-SQL systems, we walk through the list of legacy cache-groups and clear each.
• It's conceivable that some unseen/unrecognized code intermingles calls to CRM_Core_BAO_Cache with direct SQL IO (civicrm_cache). The mingling would be poor form, but it's conceivable. Such code would be broken on non-SQL deployments.

I think these risks are pretty narrow, but they exist. At this step of the phase-out, the PSR-16 adapter requires an opt-in, so the initial risk is borne by folks who need rodb. I'd vote for dialing up the pressure gradually (e.g. after 2-3 months, change to an opt-out; and after another 2-3, remove the old code).

[eileenmcnaughton]

From an IDE POV this construct

if (($adapter = CRM_Utils_Constant::value('CIVICRM_BAO_CACHE_ADAPTER')) !== FALSE) {

doesn't give warnings. I believe the case for it is also that it disambiguates between '= means =' & '= is a bug because I forgot the second one'

[totten]

@eileenmcnaughton I've pushed up a revision to use that notation. Notes:

• FWIW, my IDE doesn't complain about either the old or new notation. (PHPStorm 2016.2.2.)
• CRM_Utils_Constant::value() returns a default of NULL, so I used that instead of FALSE.

[eileenmcnaughton]

I don't think we should be adding new functions with the & before them. Last time I dug into it that was to support Php 4

[totten]

I tend to agree. I'll change it if you want, but let me explain a little thinking.

Short version: we're essentially overriding CRM_Core_BAO_Cache::&getItem(), and that does return by reference.

Longer version: Your comment is very on-point in this use-case (and even understates it). The & is demonstrably pointless. getItem() delegates to PSR-16 get() which does not preserve references. In fact, the PSR-16 compliance tests go out of their way to ensure that cache-values do not work like references (even with objects which are innately more reference-ish). Thus, with the PSR-16 adapter, it's never going to behave like a true reference. I wager that's OK.

But check the way CIVICRM_BAO_CACHE_ADAPTER is framed -- it basically allow us to swap out the implementation of CRM_Core_BAO_Cache::&getItem() with $someClass::&getItem(). This is a sort of contingency... in the highly unlikely scenario that we discover that the implementation in CRM_Core_BAO_Cache_Psr16 is fundamentally problematic, one can deploy a different adapter. I'm not saying that it should happen or will happen. But spec'ing the adapter to precisely follow the original contract gives us more wiggle room.

[eileenmcnaughton]

I'm still with change.... it's cruft and if we need to change again we can migrate to something else but let's not introduce a new bad pattern

[totten]

@eileenmcnaughton I've pushed up b5e7d57

This required moving a little reference-trickery from CRM_Core_BAO_Cache_Psr16 to CRM_Core_BAO_Cache, but it's still the same trick.

[eileenmcnaughton]

You could remove this entitysetting one - I consider that replaced by Custom Data on any

[eileenmcnaughton]

I tested this & without the new define there is no impact.

With the new define it provides an alternative path that works & takes us forwards in our refactoring. This is safe IMHO - merging