(dev/core#174) Full PSR-16 compliance for SqlGroup

[totten]

Overview

#12342 provides nominal compliance with PSR-16; however, some drivers raise exceptions or warnings for new options.

(This is a cherry-pick from #12360.)

Before

• $cache->set(...$ttl) would throw an error with any custom TTL.
• $cache->get(...$default) would throw an error for any not-NULL default.
• Cache-keys are not validated.
• Some variants of $cache->set() / $cache->get() handle objects in ways that are unsafe.

After

• SqlGroup has been fixed to comply (per SimpleCacheTest), and a unit-test ensures compliance.

[civibot]

(Standard links)

• If this is your first pull-request for CiviCRM, please browse CONTRIBUTING.md for information about the development and testing processes.
• If you are reviewing this pull-request, you may wish to consult the test sites and the Review Standards (long template, short template).

[eileenmcnaughton]

Per comments on parent PR

"SqlGroup is substantially rewritten. It no longer wraps around CRM_Core_BAO_Cache - instead, it does SQL queries which are similar in nature and uses its own front-cache."

[eileenmcnaughton]

Are these largely ignored? Failure to aquire a lock should not be a fatal error

[totten]

FWIW, this behavior is reproducing the behavior of CRM_Core_BAO_Cache::setItem(). The only change (based on feedback from Seamus) was replacing CRM_Core_Error::fatal(...) with throw new Exception(...).

IIRC, what you're referring to about the hard-failure for CiviMail is addressed within the locking subsystem (ie within CRM_Core_Lock). That has some hacks which say, "If we're doing a CiviMail cronjob, then we won't try to acquire more locks, and we'll tell callers that our locks are working just fine." From the perspective of a specific lock user, they should still request/check locks as normal. The fudgery (as I understood it) was encapsulated within CRM_Core_Lock.

This code should behave just as well (or badly) as the previous code from CRM_Core_BAO_Cache.

[seamuslee001]

The only reason why i suggested Exception instead of fatal is that i believed that fatal calls were being removed in favor of exceptions.

[eileenmcnaughton]

This seems OK - I just want to check that the fail on failure to aquire lock is not a hard fail (given that we currently cannot grab a lock while civimail is running. )

[eileenmcnaughton]

On current master I'm currently getting this - with & without this patch - but with

1. apc enabled
2. no entry in civicrm_cache for 'checks' but entries for other caches (like js_strings) that ARE being found

[seamuslee001]

sounds like its due to https://github.com/civicrm/civicrm-core/pull/12336/files, so somehow the status checks aren't actually getting hit into the cache

[totten]

@eileenmcnaughton @seamuslee001 - Yeah, that error (ServiceNotFoundException; You have requested a non-existent service "foo") generally indicates either a typo or a stale copy of CachedCiviContainer.XXXX.php. Since #12366 added this service, what you probably had was something like this:

1. While running an older version/revision/commit, the list of services was written to a file ([civicrm.files]/templates_c/CachedCiviContainer.XXXX.php).
2. We've updated Civi\Core\Container to define a new service (foo or cache.checks).
3. The file CachedCiviContainer.XXXX.php is out-of-date.

There are a couple ways one might respond:

• If you're just concerned about your local dev site right now...
  • Then do a system flush.
  • Or delete the file manually.
  • Or do a civibuild reinstall.
• If you're just concerned about your local dev site in the future (whenever you juggle patches/revisions)....
  • Then edit civicrm.settings.php (or put a similar file in /etc/civicrm.settings.d) and set CIVICRM_CONTAINER_CACHE to auto or never. This should ensure that service-definitions are kept up-to-date, but it may add some #ms to each page-request.
• If you're concerned about upgraders who might get a similar error (during the transitional moments between extracting new tarball and running the DB upgrade)...
  • Then let's patch CRM_Core_Config_Runtime::getId(). This determines the XXXX in CachedCiviContainer.XXXX.php. In the list of fiddlybits constituting XXXX, add CRM_Utils_System::version(). When someone has a new version, they'll be guaranteed to reset the cache.

[eileenmcnaughton]

OK - let's monitor how much of a disruption this turns out to be & see.

I'm happy to merge this now