From 1209c8a71c8f980a0bb8d773f3ef9411848f78c1 Mon Sep 17 00:00:00 2001
From: Coleman Watts <coleman@civicrm.org>
Date: Wed, 15 May 2019 23:35:14 -0400
Subject: [PATCH] Remove wasteful double-caching of settings metadata

Settings metadata was being cached once per domain and one extra time for good measure.
hook_civicrm_alterSettingsMetaData() was invoked even when fetching cached data,
even though the hook-altered data was stored in the cache.

Now that the hook is invoked only once and the results cached, tests have been tweaked
to cache after setting the hook instead of before.
---
 Civi/Core/SettingsMetadata.php              | 25 +++++----------------
 tests/phpunit/CiviTest/CiviUnitTestCase.php |  4 ++--
 tests/phpunit/api/v3/SettingTest.php        |  1 -
 3 files changed, 8 insertions(+), 22 deletions(-)

diff --git a/Civi/Core/SettingsMetadata.php b/Civi/Core/SettingsMetadata.php
index 00d04d3421..6aeaa3c490 100644
--- a/Civi/Core/SettingsMetadata.php
+++ b/Civi/Core/SettingsMetadata.php
@@ -33,8 +33,6 @@ namespace Civi\Core;
  */
 class SettingsMetadata {
 
-  const ALL = 'all';
-
   /**
    * WARNING: This interface may change.
    *
@@ -72,25 +70,14 @@ class SettingsMetadata {
 
     $cache = \Civi::cache('settings');
     $cacheString = 'settingsMetadata_' . $domainID . '_';
-    // the caching into 'All' seems to be a duplicate of caching to
-    // settingsMetadata__ - I think the reason was to cache all settings as defined & then those altered by a hook
     $settingsMetadata = $cache->get($cacheString);
-    $cached = is_array($settingsMetadata);
-
-    if (!$cached) {
-      $settingsMetadata = $cache->get(self::ALL);
-      if (empty($settingsMetadata)) {
-        global $civicrm_root;
-        $metaDataFolders = [$civicrm_root . '/settings'];
-        \CRM_Utils_Hook::alterSettingsFolders($metaDataFolders);
-        $settingsMetadata = self::loadSettingsMetaDataFolders($metaDataFolders);
-        $cache->set(self::ALL, $settingsMetadata);
-      }
-    }
-
-    \CRM_Utils_Hook::alterSettingsMetaData($settingsMetadata, $domainID, NULL);
 
-    if (!$cached) {
+    if (!is_array($settingsMetadata)) {
+      global $civicrm_root;
+      $metaDataFolders = [$civicrm_root . '/settings'];
+      \CRM_Utils_Hook::alterSettingsFolders($metaDataFolders);
+      $settingsMetadata = self::loadSettingsMetaDataFolders($metaDataFolders);
+      \CRM_Utils_Hook::alterSettingsMetaData($settingsMetadata, $domainID, NULL);
       $cache->set($cacheString, $settingsMetadata);
     }
 
diff --git a/tests/phpunit/CiviTest/CiviUnitTestCase.php b/tests/phpunit/CiviTest/CiviUnitTestCase.php
index 39b9811ea8..c6c473e430 100644
--- a/tests/phpunit/CiviTest/CiviUnitTestCase.php
+++ b/tests/phpunit/CiviTest/CiviUnitTestCase.php
@@ -1942,13 +1942,13 @@ AND    ( TABLE_NAME LIKE 'civicrm_value_%' )
    * @return void
    */
   public function setMockSettingsMetaData($extras) {
-    Civi::service('settings_manager')->flush();
-
     CRM_Utils_Hook::singleton()
       ->setHook('civicrm_alterSettingsMetaData', function (&$metadata, $domainId, $profile) use ($extras) {
         $metadata = array_merge($metadata, $extras);
       });
 
+    Civi::service('settings_manager')->flush();
+
     $fields = $this->callAPISuccess('setting', 'getfields', array());
     foreach ($extras as $key => $spec) {
       $this->assertNotEmpty($spec['title']);
diff --git a/tests/phpunit/api/v3/SettingTest.php b/tests/phpunit/api/v3/SettingTest.php
index aa75cf7115..f8d6ed9620 100644
--- a/tests/phpunit/api/v3/SettingTest.php
+++ b/tests/phpunit/api/v3/SettingTest.php
@@ -110,7 +110,6 @@ class api_v3_SettingTest extends CiviUnitTestCase {
   public function testGetFieldsCaching() {
     $settingsMetadata = array();
     Civi::cache('settings')->set('settingsMetadata_' . \CRM_Core_Config::domainID() . '_', $settingsMetadata);
-    Civi::cache('settings')->set(\Civi\Core\SettingsMetadata::ALL, $settingsMetadata);
     $result = $this->callAPISuccess('setting', 'getfields', array());
     $this->assertArrayNotHasKey('customCSSURL', $result['values']);
     $this->quickCleanup(array('civicrm_cache'));
-- 
2.25.1