From 0626851edc4885058da8a0c9de7ca51c302a03e5 Mon Sep 17 00:00:00 2001 From: eileen Date: Wed, 6 Sep 2017 14:29:15 +1200 Subject: [PATCH] CRM-21109 only clear caches once on cli script, consolidate code --- CRM/ACL/BAO/Cache.php | 3 +++ CRM/Contact/BAO/Contact.php | 8 +------ CRM/Contact/BAO/Contact/Utils.php | 21 ++++++++++------- CRM/Contact/BAO/GroupContact.php | 26 ++------------------- CRM/Contact/BAO/GroupContactCache.php | 19 ++++++++++----- CRM/Contact/Form/Merge.php | 2 +- CRM/Contact/Import/Form/Preview.php | 2 +- CRM/Contact/Import/Parser/Contact.php | 7 +++--- CRM/Core/Config.php | 20 ++++++++++++++++ CRM/Dedupe/Merger.php | 4 +--- bin/cli.class.php | 3 +++ tests/phpunit/CRM/Contact/BAO/QueryTest.php | 2 +- tests/phpunit/api/v3/ContactTest.php | 8 ++++++- tests/phpunit/api/v3/ReportTemplateTest.php | 8 +++---- 14 files changed, 72 insertions(+), 61 deletions(-) diff --git a/CRM/ACL/BAO/Cache.php b/CRM/ACL/BAO/Cache.php index 392d15f48a..1bb9bf5ca0 100644 --- a/CRM/ACL/BAO/Cache.php +++ b/CRM/ACL/BAO/Cache.php @@ -142,6 +142,9 @@ WHERE contact_id = %1 * Deletes all the cache entries. */ public static function resetCache() { + if (!CRM_Core_Config::isPermitCacheFlushMode()) { + return; + } // reset any static caching self::$_cache = NULL; diff --git a/CRM/Contact/BAO/Contact.php b/CRM/Contact/BAO/Contact.php index 42637eb920..2599d975f4 100644 --- a/CRM/Contact/BAO/Contact.php +++ b/CRM/Contact/BAO/Contact.php @@ -425,13 +425,7 @@ class CRM_Contact_BAO_Contact extends CRM_Contact_DAO_Contact { 'name' ); - if (!$config->doNotResetCache) { - // Note: doNotResetCache flag is currently set by import contact process and merging, - // since resetting and - // rebuilding cache could be expensive (for many contacts). We might come out with better - // approach in future. - CRM_Contact_BAO_Contact_Utils::clearContactCaches($contact->id); - } + CRM_Contact_BAO_Contact_Utils::clearContactCaches(); if ($invokeHooks) { if ($isEdit) { diff --git a/CRM/Contact/BAO/Contact/Utils.php b/CRM/Contact/BAO/Contact/Utils.php index 024ef85042..89d8394b62 100644 --- a/CRM/Contact/BAO/Contact/Utils.php +++ b/CRM/Contact/BAO/Contact/Utils.php @@ -897,18 +897,21 @@ INNER JOIN civicrm_contact contact_target ON ( contact_target.id = act.contact_i * caches, but are backing off from this with every release. Compromise between ease of coding versus * performance versus being accurate at that very instant * - * @param $contactID - * The contactID that was edited / deleted. + * @param bool $isEmptyPrevNextTable + * Should the civicrm_prev_next table be cleared of any contact entries. + * This is currently done from import but not other places and would + * likely affect user experience in unexpected ways. Existing behaviour retained + * ... reluctantly. */ - public static function clearContactCaches($contactID = NULL) { - // clear acl cache if any. - CRM_ACL_BAO_Cache::resetCache(); - - if (empty($contactID)) { - // also clear prev/next dedupe cache - if no contactID passed in + public static function clearContactCaches($isEmptyPrevNextTable = FALSE) { + if (!CRM_Core_Config::isPermitCacheFlushMode()) { + return; + } + if ($isEmptyPrevNextTable) { CRM_Core_BAO_PrevNextCache::deleteItem(); } - + // clear acl cache if any. + CRM_ACL_BAO_Cache::resetCache(); CRM_Contact_BAO_GroupContactCache::opportunisticCacheFlush(); } diff --git a/CRM/Contact/BAO/GroupContact.php b/CRM/Contact/BAO/GroupContact.php index 8833d1958d..73bf59b76b 100644 --- a/CRM/Contact/BAO/GroupContact.php +++ b/CRM/Contact/BAO/GroupContact.php @@ -143,15 +143,7 @@ class CRM_Contact_BAO_GroupContact extends CRM_Contact_DAO_GroupContact { list($numContactsAdded, $numContactsNotAdded) = self::bulkAddContactsToGroup($contactIds, $groupId, $method, $status, $tracking); - // also reset the acl cache - $config = CRM_Core_Config::singleton(); - if (!$config->doNotResetCache) { - CRM_ACL_BAO_Cache::resetCache(); - } - - // reset the group contact cache for all group(s) - // if this group is being used as a smart group - CRM_Contact_BAO_GroupContactCache::opportunisticCacheFlush(); + CRM_Contact_BAO_Contact_Utils::clearContactCaches(); CRM_Utils_Hook::post('create', 'GroupContact', $groupId, $contactIds); @@ -245,21 +237,7 @@ class CRM_Contact_BAO_GroupContact extends CRM_Contact_DAO_GroupContact { } } - // also reset the acl cache - $config = CRM_Core_Config::singleton(); - if (!$config->doNotResetCache) { - CRM_ACL_BAO_Cache::resetCache(); - } - - // reset the group contact cache for all group(s) - // if this group is being used as a smart group - // @todo consider what to do here - it feels like we should either - // 1) just invalidate the specific group's cache(& perhaps any parents) & let cron do it's thing or - // possibly clear this specific groups cache, or just call opportunisticCacheFlush() - which would have the - // same effect as the remove call. The reservation about that is that it is no more aggressive for the group that - // we know is altered than for all the others, or perhaps, more the point with it's parents & groups that use it in - // their criteria. - CRM_Contact_BAO_GroupContactCache::remove(); + CRM_Contact_BAO_Contact_Utils::clearContactCaches(); CRM_Utils_Hook::post($op, 'GroupContact', $groupId, $contactIds); diff --git a/CRM/Contact/BAO/GroupContactCache.php b/CRM/Contact/BAO/GroupContactCache.php index 3840a0f284..7ba7d6a27a 100644 --- a/CRM/Contact/BAO/GroupContactCache.php +++ b/CRM/Contact/BAO/GroupContactCache.php @@ -297,8 +297,8 @@ WHERE id IN ( $groupIDs ) * In fact it turned out there is little overlap between the code when group is passed in * and group is not so it makes more sense as separate functions. * - * @todo remove last call to this function from outside the class then make function protected, - * enforce groupID as an array & remove non group handling. + * @todo enforce groupID as an array & remove non group handling. + * Use flushCaches when no group id provided. * * @param int $groupIDs * the groupID to delete cache entries, NULL for all groups. @@ -306,7 +306,14 @@ WHERE id IN ( $groupIDs ) * run the function exactly once for all groups. */ public static function remove($groupIDs = NULL, $onceOnly = TRUE) { - static $invoked = FALSE; + if (!is_array($groupIDs)) { + Civi::log() + ->warning('Deprecated code. This function should not be called without groupIDs. Extensions can use the api job.group_cache_flush', array('civi.tag' => 'deprecated')); + return; + } + if (!isset(Civi::$statics[__CLASS__]['remove_invoked'])) { + Civi::$statics[__CLASS__] = array('remove_invoked' => FALSE); + } // typically this needs to happy only once per instance // this is especially TRUE in import, where we don't need @@ -315,14 +322,14 @@ WHERE id IN ( $groupIDs ) // i.e. cache is reset for all groups if ( $onceOnly && - $invoked && + Civi::$statics[__CLASS__]['remove_invoked'] && $groupIDs == NULL ) { return; } if ($groupIDs == NULL) { - $invoked = TRUE; + Civi::$statics[__CLASS__]['remove_invoked'] = TRUE; } elseif (is_array($groupIDs)) { foreach ($groupIDs as $gid) { @@ -479,7 +486,7 @@ AND refresh_date IS NULL * @throws \CRM_Core_Exception */ protected static function getLockForRefresh() { - if (!isset(Civi::$statics[__CLASS__])) { + if (!isset(Civi::$statics[__CLASS__]['is_refresh_init'])) { Civi::$statics[__CLASS__] = array('is_refresh_init' => FALSE); } diff --git a/CRM/Contact/Form/Merge.php b/CRM/Contact/Form/Merge.php index 6381852a57..5a8f5a77c5 100644 --- a/CRM/Contact/Form/Merge.php +++ b/CRM/Contact/Form/Merge.php @@ -108,7 +108,7 @@ class CRM_Contact_Form_Merge extends CRM_Core_Form { // get user info of main contact. $config = CRM_Core_Config::singleton(); - $config->doNotResetCache = 1; + CRM_Core_Config::setPermitCacheFlushMode(FALSE); $mainUfId = CRM_Core_BAO_UFMatch::getUFId($this->_cid); $mainUser = NULL; diff --git a/CRM/Contact/Import/Form/Preview.php b/CRM/Contact/Import/Form/Preview.php index 26c8e14641..ca8aff5818 100644 --- a/CRM/Contact/Import/Form/Preview.php +++ b/CRM/Contact/Import/Form/Preview.php @@ -306,7 +306,7 @@ class CRM_Contact_Import_Form_Preview extends CRM_Import_Form_Preview { // Clear all caches, forcing any searches to recheck the ACLs or group membership as the import // may have changed it. - CRM_Contact_BAO_Contact_Utils::clearContactCaches(); + CRM_Contact_BAO_Contact_Utils::clearContactCaches(TRUE); // add all the necessary variables to the form $importJob->setFormVariables($this); diff --git a/CRM/Contact/Import/Parser/Contact.php b/CRM/Contact/Import/Parser/Contact.php index 3fc0d0172e..fab423281d 100644 --- a/CRM/Contact/Import/Parser/Contact.php +++ b/CRM/Contact/Import/Parser/Contact.php @@ -1716,11 +1716,10 @@ class CRM_Contact_Import_Parser_Contact extends CRM_Contact_Import_Parser { $this->formatParams($formatted, $onDuplicate, (int) $contactId); } - // pass doNotResetCache flag since resetting and rebuilding cache could be expensive. - $config = CRM_Core_Config::singleton(); - $config->doNotResetCache = 1; + // Resetting and rebuilding cache could be expensive. + CRM_Core_Config::setPermitCacheFlushMode(FALSE); $cid = CRM_Contact_BAO_Contact::createProfileContact($formatted, $contactFields, $contactId, NULL, NULL, $formatted['contact_type']); - $config->doNotResetCache = 0; + CRM_Core_Config::setPermitCacheFlushMode(TRUE); $contact = array( 'contact_id' => $cid, diff --git a/CRM/Core/Config.php b/CRM/Core/Config.php index bc567631b8..eefeb3b3ab 100644 --- a/CRM/Core/Config.php +++ b/CRM/Core/Config.php @@ -568,4 +568,24 @@ class CRM_Core_Config extends CRM_Core_Config_MagicMerge { Civi::settings()->set('installed', 1); } + /** + * Is the system permitted to flush caches at the moment. + */ + static public function isPermitCacheFlushMode() { + return !CRM_Core_Config::singleton()->doNotResetCache; + } + + /** + * Set cache clearing to enabled or disabled. + * + * This might be enabled at the start of a long running process + * such as an import in order to delay clearing caches until the end. + * + * @param bool $enabled + * If true then caches can be cleared at this time. + */ + static public function setPermitCacheFlushMode($enabled) { + CRM_Core_Config::singleton()->doNotResetCache = $enabled ? 0 : 1; + } + } diff --git a/CRM/Dedupe/Merger.php b/CRM/Dedupe/Merger.php index d52d84340b..c2ebae7119 100644 --- a/CRM/Dedupe/Merger.php +++ b/CRM/Dedupe/Merger.php @@ -783,9 +783,7 @@ INNER JOIN civicrm_membership membership2 ON membership1.membership_type_id = m $resultStats = array('merged' => array(), 'skipped' => array()); // we don't want dupe caching to get reset after every-merge, and therefore set the - // doNotResetCache flag - $config = CRM_Core_Config::singleton(); - $config->doNotResetCache = 1; + CRM_Core_Config::setPermitCacheFlushMode(FALSE); $deletedContacts = array(); while (!empty($dupePairs)) { diff --git a/bin/cli.class.php b/bin/cli.class.php index 1957925540..7f432deb93 100644 --- a/bin/cli.class.php +++ b/bin/cli.class.php @@ -99,6 +99,7 @@ class civicrm_cli { public function callApi() { require_once 'api/api.php'; + CRM_Core_Config::setPermitCacheFlushMode(FALSE); // CRM-9822 -'execute' action always goes thru Job api and always writes to log if ($this->_action != 'execute' && $this->_joblog) { require_once 'CRM/Core/JobManager.php'; @@ -111,6 +112,8 @@ class civicrm_cli { $this->_params['auth'] = FALSE; $result = civicrm_api($this->_entity, $this->_action, $this->_params); } + CRM_Core_Config::setPermitCacheFlushMode(TRUE); + CRM_Contact_BAO_Contact_Utils::clearContactCaches(); if (!empty($result['is_error'])) { $this->_log($result['error_message']); diff --git a/tests/phpunit/CRM/Contact/BAO/QueryTest.php b/tests/phpunit/CRM/Contact/BAO/QueryTest.php index 7ee1e1a3aa..6c8061d240 100644 --- a/tests/phpunit/CRM/Contact/BAO/QueryTest.php +++ b/tests/phpunit/CRM/Contact/BAO/QueryTest.php @@ -385,7 +385,7 @@ class CRM_Contact_BAO_QueryTest extends CiviUnitTestCase { $this->callAPISuccess('GroupContact', 'create', array('group_id' => $groupID, 'contact_id' => $householdID, 'status' => 'Added')); // Refresh the cache for test purposes. It would be better to alter to alter the GroupContact add function to add contacts to the cache. - CRM_Contact_BAO_GroupContactCache::remove($groupID, FALSE); + CRM_Contact_BAO_GroupContactCache::clearGroupContactCache($groupID); $sql = CRM_Contact_BAO_Query::getQuery( array(array('group', 'IN', array($groupID), 0, 0)), diff --git a/tests/phpunit/api/v3/ContactTest.php b/tests/phpunit/api/v3/ContactTest.php index 1c0d905a72..fb7e01a237 100644 --- a/tests/phpunit/api/v3/ContactTest.php +++ b/tests/phpunit/api/v3/ContactTest.php @@ -83,6 +83,10 @@ class api_v3_ContactTest extends CiviUnitTestCase { 'civicrm_acl_contact_cache', 'civicrm_activity_contact', 'civicrm_activity', + 'civicrm_group', + 'civicrm_group_contact', + 'civicrm_saved_search', + 'civicrm_group_contact_cache', ); $this->quickCleanup($tablesToTruncate, TRUE); @@ -132,8 +136,10 @@ class api_v3_ContactTest extends CiviUnitTestCase { // Rinse & repeat, but with the option. $this->putGroupContactCacheInClearableState($groupID, $contact); - $this->callAPISuccess('contact', 'create', array('id' => $contact['id'], 'options' => array('do_not_reset_cache' => 1))); + CRM_Core_Config::setPermitCacheFlushMode(FALSE); + $this->callAPISuccess('contact', 'create', array('id' => $contact['id'])); $this->assertEquals(1, CRM_Core_DAO::singleValueQuery("SELECT count(*) FROM civicrm_group_contact_cache")); + CRM_Core_Config::setPermitCacheFlushMode(TRUE); } /** diff --git a/tests/phpunit/api/v3/ReportTemplateTest.php b/tests/phpunit/api/v3/ReportTemplateTest.php index 41022c7680..6b606a0306 100644 --- a/tests/phpunit/api/v3/ReportTemplateTest.php +++ b/tests/phpunit/api/v3/ReportTemplateTest.php @@ -42,7 +42,7 @@ class api_v3_ReportTemplateTest extends CiviUnitTestCase { */ public function tearDown() { $this->quickCleanUpFinancialEntities(); - $this->quickCleanup(array('civicrm_group', 'civicrm_saved_search', 'civicrm_group_contact')); + $this->quickCleanup(array('civicrm_group', 'civicrm_saved_search', 'civicrm_group_contact', 'civicrm_group_contact_cache', 'civicrm_group')); parent::tearDown(); } @@ -123,7 +123,7 @@ class api_v3_ReportTemplateTest extends CiviUnitTestCase { $result = $this->callAPIAndDocument('report_template', 'getrows', array( 'report_id' => 'contact/summary', 'options' => array('metadata' => array('labels', 'title')), - ), __FUNCTION__, __FILE__, $description, 'Getrows', 'getrows'); + ), __FUNCTION__, __FILE__, $description, 'Getrows'); $this->assertEquals('Contact Name', $result['metadata']['labels']['civicrm_contact_sort_name']); //the second part of this test has been commented out because it relied on the db being reset to @@ -470,7 +470,7 @@ class api_v3_ReportTemplateTest extends CiviUnitTestCase { } // Refresh the cache for test purposes. It would be better to alter to alter the GroupContact add function to add contacts to the cache. - CRM_Contact_BAO_GroupContactCache::remove($groupID, FALSE); + CRM_Contact_BAO_GroupContactCache::clearGroupContactCache($groupID); return $groupID; } @@ -510,7 +510,7 @@ class api_v3_ReportTemplateTest extends CiviUnitTestCase { } // Refresh the cache for test purposes. It would be better to alter to alter the GroupContact add function to add contacts to the cache. - CRM_Contact_BAO_GroupContactCache::remove($groupID, FALSE); + CRM_Contact_BAO_GroupContactCache::clearGroupContactCache($groupID); if ($returnAddedContact) { return array($groupID, $individualID); -- 2.25.1