From: Tim Otten <totten@civicrm.org>
Date: Fri, 12 Jul 2019 22:44:01 +0000 (-0700)
Subject: CRM_Core_Config_MagicMerge - Fix thread-local updates for aliased properties
X-Git-Url: https://vcs.fsf.org/?a=commitdiff_plain;h=cdf3884eae49daca8cedfb9f962b3caf05db86fb;p=civicrm-core.git

CRM_Core_Config_MagicMerge - Fix thread-local updates for aliased properties

Overview
--------

`CRM_Core_Config_MagicMerge` allows properties to be temporarily modified
(for the duration the object's lifespan). However, this behavior does
not work correctly if the property-name uses aliasing.

The PR addresses a test-failure that became visible in #14718, and it adds
test-coverage for some typical and atypical examples.

Before
------

* (Vast majority of properties) If a property has a simple name (e.g.
  `maxFileSize`), then `$cache`-reads and `$cache`-writes are consistent.
* (Handful of properties) If a property has a split name (e.g.
  `maxAttachments`, `max_attachments`), then `$cache`-reads and
  `$cache`-writes are *not* consistent.  Writes basically don't work.

After
-----

* Aliased properties work the same as normal/non-aliased.

Technical Details
-----------------

To understand the change, you should skim `MagicMerge` and observe
a few things:

* In `getPropertyMap()`,  observe that:
    * (a) Most properties have a singular name -- for example, `maxFileSize`
      has one name, which will be the same in the high-level facade (`$config->maxFileSize`)
      and in the underlying data-store (`settings->get('maxFileSize')`).
    * (b) Some properties are aliased. Ex: `$config->maxAttachments`
      corresponds to underlying item `settings->get('max_attachments')`.
* In `__get()` and `__set()`, note that properties are loaded initally from
  the underlying data-store once; thereafter, any reads or writes go to
  `$this->cache`. That's a thread-local place that stores temporary revisions.

To see the cache consistency problem, specifically note that:

* `__get()` consistently references `$this->cache[$k]`
* `__set()` was confused; at the start, it used `$this->cache[$k]`,
  and later it used `$this->cache[$name]`.

This PR adds a unit-test which ensures that the thread-local/cached value
works consistently.
---

diff --git a/CRM/Core/Config/MagicMerge.php b/CRM/Core/Config/MagicMerge.php
index ffb7e64347..801f803f3b 100644
--- a/CRM/Core/Config/MagicMerge.php
+++ b/CRM/Core/Config/MagicMerge.php
@@ -318,10 +318,6 @@ class CRM_Core_Config_MagicMerge {
     unset($this->cache[$k]);
     $type = $this->map[$k][0];
 
-    // If foreign name is set, use that name (except with callback types because
-    // their second parameter is the object, not the foreign name).
-    $name = isset($this->map[$k][1]) && $type != 'callback' ? $this->map[$k][1] : $k;
-
     switch ($type) {
       case 'setting':
       case 'setting-path':
@@ -331,12 +327,12 @@ class CRM_Core_Config_MagicMerge {
       case 'callback':
       case 'boot-svc':
         // In the past, changes to $config were not persisted automatically.
-        $this->cache[$name] = $v;
+        $this->cache[$k] = $v;
         return;
 
       case 'local':
         $this->initLocals();
-        $this->locals[$name] = $v;
+        $this->locals[$k] = $v;
         return;
 
       default:
diff --git a/tests/phpunit/CRM/Core/Config/MagicMergeTest.php b/tests/phpunit/CRM/Core/Config/MagicMergeTest.php
new file mode 100644
index 0000000000..250075df5b
--- /dev/null
+++ b/tests/phpunit/CRM/Core/Config/MagicMergeTest.php
@@ -0,0 +1,74 @@
+<?php
+/*
+ +--------------------------------------------------------------------+
+ | CiviCRM version 5                                                  |
+ +--------------------------------------------------------------------+
+ | Copyright CiviCRM LLC (c) 2004-2019                                |
+ +--------------------------------------------------------------------+
+ | This file is a part of CiviCRM.                                    |
+ |                                                                    |
+ | CiviCRM is free software; you can copy, modify, and distribute it  |
+ | under the terms of the GNU Affero General Public License           |
+ | Version 3, 19 November 2007 and the CiviCRM Licensing Exception.   |
+ |                                                                    |
+ | CiviCRM is distributed in the hope that it will be useful, but     |
+ | WITHOUT ANY WARRANTY; without even the implied warranty of         |
+ | MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.               |
+ | See the GNU Affero General Public License for more details.        |
+ |                                                                    |
+ | You should have received a copy of the GNU Affero General Public   |
+ | License and the CiviCRM Licensing Exception along                  |
+ | with this program; if not, contact CiviCRM LLC                     |
+ | at info[AT]civicrm[DOT]org. If you have questions about the        |
+ | GNU Affero General Public License or the licensing of CiviCRM,     |
+ | see the CiviCRM license FAQ at http://civicrm.org/licensing        |
+ +--------------------------------------------------------------------+
+ */
+
+/**
+ *
+ * @package CiviCRM
+ * @copyright CiviCRM LLC (c) 2004-2019
+ * $Id: $
+ *
+ */
+
+/**
+ * Class CRM_Core_Config_MagicMergeTest
+ * @group headless
+ */
+class CRM_Core_Config_MagicMergeTest extends CiviUnitTestCase {
+
+  public function getOverrideExamples() {
+    return [
+      // Check examples of a few different types
+      ['configAndLogDir', '/tmp/zoo'],
+      ['maxAttachments', '112358'],
+      ['initialized', 'for sure'],
+      ['userFrameworkBaseURL', 'http://example.com/use/the/framework/luke'],
+      ['inCiviCRM', 'all the data'],
+      ['cleanURL', 'as clean as a url can be'],
+      ['defaultCurrencySymbol', ':)'],
+    ];
+  }
+
+  /**
+   * @param string $field
+   * @param mixed $tempValue
+   * @dataProvider getOverrideExamples
+   */
+  public function testTempOverride($field, $tempValue) {
+    $config = CRM_Core_Config::singleton();
+    $origValue = $config->{$field};
+
+    $config->{$field} = $tempValue;
+    $this->assertEquals($tempValue, $config->{$field});
+
+    $config = CRM_Core_Config::singleton();
+    $this->assertEquals($tempValue, $config->{$field});
+
+    $config = CRM_Core_Config::singleton(TRUE, TRUE);
+    $this->assertEquals($origValue, $config->{$field});
+  }
+
+}