CRM_Core_Config_MagicMerge - Fix thread-local updates for aliased properties
authorTim Otten <totten@civicrm.org>
Fri, 12 Jul 2019 22:44:01 +0000 (15:44 -0700)
committerTim Otten <totten@civicrm.org>
Fri, 12 Jul 2019 23:08:33 +0000 (16:08 -0700)
commitcdf3884eae49daca8cedfb9f962b3caf05db86fb
treeeaa2c1fb81bc77b50627af0e8eacb8e7ad2707c6
parent39c2c8dd21afb89f77179a7ab2648cba4abbee0f
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.
CRM/Core/Config/MagicMerge.php
tests/phpunit/CRM/Core/Config/MagicMergeTest.php [new file with mode: 0644]