Commit cdf3884e authored by totten's avatar totten

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.
parent 39c2c8dd
......@@ -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:
......
<?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});
}
}
Markdown is supported
0% or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment