Convert some more legacy caches to standardise cache container #14487

seamuslee001 · 2019-06-09T05:49:08Z

Overview

This converts some more legacy caches to Civi::cache containers

Before

The current status. Please provide screenshots or gifs (LICEcap, SilentCast) where appropriate.

After

What has been changed. Please provide screenshots or gifs (LICEcap, SilentCast) where appropriate.

ping @totten @eileenmcnaughton

civibot · 2019-06-09T05:49:10Z

(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).

seamuslee001 · 2019-06-09T16:38:55Z

Jenkins re test this please

seamuslee001 · 2019-06-09T23:21:11Z

Jenkins re test this please

monishdeb · 2019-06-13T03:50:14Z

CRM/ACL/BAO/ACL.php

@@ -688,7 +688,8 @@ public static function retrieve(&$params, &$defaults) {
   */
  public static function setIsActive($id, $is_active) {
    // note this also resets any ACL cache
-    CRM_Core_BAO_Cache::deleteGroup('contact fields');
+    $cache = Civi::cache('fields');


or simply

- CRM_Core_BAO_Cache::deleteGroup('contact fields'); + Civi::cache('fields')->flush();

monishdeb · 2019-06-13T03:50:26Z

CRM/ACL/Form/ACL.php

@@ -254,7 +254,8 @@ public static function formRule($params) {
   */
  public function postProcess() {
    // note this also resets any ACL cache
-    CRM_Core_BAO_Cache::deleteGroup('contact fields');
+    $cache = Civi::cache('fields');
+    $cache->flush();


same as above

monishdeb · 2019-06-13T03:56:13Z

CRM/Admin/Form/Setting/Localization.php


    //CRM-8559, cache navigation do not respect locale if it is changed, so reseting cache.
-    CRM_Core_BAO_Cache::deleteGroup('navigation');
+    $navigationCache = Civi::cache('navigation');
+    $navigationCache->flush();


monishdeb · 2019-06-13T03:57:43Z

CRM/Contact/BAO/Contact.php

@@ -1321,9 +1321,10 @@ public static function importableFields(

    $fields = CRM_Utils_Array::value($cacheKeyString, self::$_importableFields);


or

$fields = CRM_Utils_Array::value($cacheKeyString, self::$_importableFields) ?: Civi::cache('fields')->get($cacheKeyString);

monishdeb · 2019-06-13T04:00:12Z

CRM/Core/BAO/CustomGroup.php

@@ -259,7 +259,8 @@ public static function retrieve(&$params, &$defaults) {
   */
  public static function setIsActive($id, $is_active) {
    // reset the cache
-    CRM_Core_BAO_Cache::deleteGroup('contact fields');
+    $cache = Civi::cache('fields');
+    $cache->flush();


same as above

seamuslee001 · 2019-06-13T04:14:41Z

thanks @monishdeb i have updated now to hopefully be more streamlined in the code

monishdeb · 2019-06-13T04:34:03Z

Nice, from code/style POV I am happy with the patch. Waiting for @totten @eileenmcnaughton for their feedback :)

eileenmcnaughton · 2019-06-14T01:35:33Z

CRM/Campaign/Selector/Search.php

@@ -296,7 +296,7 @@ public function buildPrevNextCache($sort) {

      if (Civi::service('prevnext') instanceof CRM_Core_PrevNextCache_Sql) {
        // SQL-backed prevnext cache uses an extra record for pruning the cache.
-        CRM_Core_BAO_Cache::setItem($cacheKey, 'CiviCRM Search PrevNextCache', $cacheKey);
+        Civi::cache('prevNextCache')->set($cacheKey, $cacheKey);


@totten agreed that the effect of

Civi::cache('prevNextCache')->set($cacheKey, $cacheKey);

is a clumsy way of saying

Civi::cache('prevNextCache')->set($cacheKey, TRUE);

Just to be clear - I support setting the value in the same way it was set before. But Eileen had a question about whether it was meaningful to do set($cacheKey,$cacheKey), and (conceptually) these could be the same (although I'd make no assumption that they're actually equivalent in practice).

Also, note that CRM_Core_BAO_PrevNextCache::cleanupCache() specifically relies on this cache being stored in SQL. I'd recommend skeptical eye about using a cache-backend that is dynamically-chosen (as in prevNextCache' => 'CiviCRM Search PrevNextCache' ... 'type' => ['*memory*', 'SqlGroup', 'ArrayCache'] in Container.php below) when there are hard-coded bits that assume this cache is SQL-backed...

@totten good point have just pushed up change to ensure prevNextCache is sql Backed

@totten should CRM_Core_BAO_PrevNextCache:::cleanupCache() be aware of the backed provider of prev next service?

eileenmcnaughton · 2019-06-14T10:40:55Z

@seamuslee001 needs a rebase

Streamline caches calls as per Monish's comments Split prevNextCache out on its own as it needs to be SQLbacked all the time for the moment whilst clearing is hard coded

seamuslee001 · 2019-06-14T22:17:11Z

@eileenmcnaughton rebased now

eileenmcnaughton · 2019-06-15T10:24:35Z

@totten still hoping you will review this

seamuslee001 · 2019-06-18T23:00:25Z

following a chat with @totten i have now split this PR up into separate ones for each cache group

#14580 PrevNextCache
#14581 Navigation
#14582 Custom Data
#14583 contact fields
#14584 contact groups

I have left some notes on what reviewers should be looking at when reviewing. Hopefully split up they might be easier to review

ping @monishdeb @mattwire

seamuslee001 · 2019-07-22T21:21:24Z

closing this as it has been addressed via other PRs

civibot bot added the master label Jun 9, 2019

monishdeb reviewed Jun 13, 2019

View reviewed changes

seamuslee001 force-pushed the convert_caches branch from 6272ddf to 0fb1630 Compare June 13, 2019 04:14

seamuslee001 force-pushed the convert_caches branch from 0fb1630 to b973d97 Compare June 13, 2019 04:16

eileenmcnaughton reviewed Jun 14, 2019

View reviewed changes

seamuslee001 force-pushed the convert_caches branch from b973d97 to 93a63a5 Compare June 14, 2019 02:49

seamuslee001 added 2 commits June 15, 2019 08:16

Convert some more legacy caches to stanardise cache container

8de0274

Convert custom data and CiviCRM Search PrevNextCache search groups

3ade468

Streamline caches calls as per Monish's comments Split prevNextCache out on its own as it needs to be SQLbacked all the time for the moment whilst clearing is hard coded

seamuslee001 force-pushed the convert_caches branch from 93a63a5 to 3ade468 Compare June 14, 2019 22:17

This was referenced Jun 22, 2019

[REF] Add in utility function for resetting ACL and System Level Caches #14600

Merged

Convert Contact Groups cache group to standard cache definition #14584

Merged

seamuslee001 closed this Jul 22, 2019

seamuslee001 deleted the convert_caches branch July 22, 2019 21:21

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Convert some more legacy caches to standardise cache container #14487

Convert some more legacy caches to standardise cache container #14487

seamuslee001 commented Jun 9, 2019

civibot bot commented Jun 9, 2019

seamuslee001 commented Jun 9, 2019

seamuslee001 commented Jun 9, 2019

monishdeb Jun 13, 2019

monishdeb Jun 13, 2019

monishdeb Jun 13, 2019

monishdeb Jun 13, 2019

monishdeb Jun 13, 2019

seamuslee001 commented Jun 13, 2019

monishdeb commented Jun 13, 2019

eileenmcnaughton Jun 14, 2019

totten Jun 14, 2019

seamuslee001 Jun 14, 2019

seamuslee001 Jun 14, 2019

eileenmcnaughton commented Jun 14, 2019

seamuslee001 commented Jun 14, 2019

eileenmcnaughton commented Jun 15, 2019

seamuslee001 commented Jun 18, 2019

seamuslee001 commented Jul 22, 2019

		@@ -1321,9 +1321,10 @@ public static function importableFields(

		$fields = CRM_Utils_Array::value($cacheKeyString, self::$_importableFields);

Convert some more legacy caches to standardise cache container #14487

Convert some more legacy caches to standardise cache container #14487

Conversation

seamuslee001 commented Jun 9, 2019

Overview

Before

After

civibot bot commented Jun 9, 2019

seamuslee001 commented Jun 9, 2019

seamuslee001 commented Jun 9, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

seamuslee001 commented Jun 13, 2019

monishdeb commented Jun 13, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

eileenmcnaughton commented Jun 14, 2019

seamuslee001 commented Jun 14, 2019

eileenmcnaughton commented Jun 15, 2019

seamuslee001 commented Jun 18, 2019

seamuslee001 commented Jul 22, 2019