(REF) CRM_Core_Config - Simplify dependencies between MagicMerge and Runtime
authorTim Otten <totten@civicrm.org>
Wed, 3 May 2023 04:54:24 +0000 (21:54 -0700)
committerTim Otten <totten@civicrm.org>
Wed, 3 May 2023 19:23:33 +0000 (12:23 -0700)
Overview
--------

Simplify the dependencies between classes `CRM_Core_Config_MagicMerge` and `CRM_Core_Config_Runtime`.

Before
------

There is a sort of dependency loop:

```php
class CRM_Core_Config extends CRM_Core_Config_MagicMerge { ... }
class CRM_Core_Config_MagicMerge { ... \Civi\Core\Container::getBootService('runtime')->get() ... }
class CRM_Core_Config_Runtime extends CRM_Core_Config_MagicMerge { ... }
```

Thus, we have `$config` (an instance of `MagicMerge`) which uses `$runtime` (another
instance of `MagicMerge`).  While it works, it twists the brain to understand properties
that actually through each of these layers (e.g.  `$config->initialized`).

After
-----

There is no loop.

```php
class CRM_Core_Config extends CRM_Core_Config_MagicMerge { ... }
class CRM_Core_Config_MagicMerge { ... \Civi\Core\Container::getBootService('runtime')->get() ... }
class CRM_Core_Config_Runtime { ... }
```

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

Removing the `extends MagicMerge` bit means that:

1. In `Runtime.php`, it shouldn't call `$this->getSettings()`.
    * I believe that this loop was added by 1b81ed503682ef2d88f65673b0dce9f112078000 in order to make use of `getSettings()`.
      However, `Civi::settings()` should be equally valid here. (After all, `getSettings()` calls out to `civi::settings()`...)
2. In `Runtime.php`, the property `$initialized` cannot be stored through inherited-magic.
    * By declaring the property, we make it similar to all the other properties in `Runtime.php` (which are also declared).

Unit-testing this change would be quite tricky.  For testing, I basically wanted to ensure
that `$config->initialized` behaved the same way before+after.  To do this, I hacked some log
statements (https://gist.github.com/totten/4038882da7a5a4014b73c0596057715d) and ran

```
cv ev 'return CRM_Core_Config::singleton()->initialized;'
```

This showed that `$initialized` starts out as `null` and becomes `1`.  The behavior is the
same before+after the patch.  (Actually, it's a little better - since it resolves some
warnings emitted by the first `var_export()` call.)

CRM/Core/Config/MagicMerge.php
CRM/Core/Config/Runtime.php

index 1c7be169eae9aaa24d2a0823973288d540db9bf8..3e5f3b197dcbd4cee36f388d563b33a6c21d2380 100644 (file)
@@ -402,7 +402,6 @@ class CRM_Core_Config_MagicMerge {
         'inCiviCRM' => FALSE,
         'doNotResetCache' => 0,
         'keyDisable' => FALSE,
-        'initialized' => FALSE,
         'userFrameworkFrontend' => FALSE,
         'userPermissionTemp' => NULL,
       ];
index 1fd76d270dc1bf760d9c8a1f4af51569348649ab..c642d699db6ec9e50224c0bb8508fc1ab60ab779 100644 (file)
@@ -16,7 +16,7 @@
  * the DSN, CMS type, CMS URL, etc. Generally, runtime properties must be
  * determined externally (before loading CiviCRM).
  */
-class CRM_Core_Config_Runtime extends CRM_Core_Config_MagicMerge {
+class CRM_Core_Config_Runtime {
 
   public $dsn;
 
@@ -63,6 +63,11 @@ class CRM_Core_Config_Runtime extends CRM_Core_Config_MagicMerge {
    */
   public $templateDir;
 
+  /**
+   * @var bool
+   */
+  public $initialized;
+
   /**
    * @param bool $loadFromDB
    */
@@ -117,7 +122,7 @@ class CRM_Core_Config_Runtime extends CRM_Core_Config_MagicMerge {
   public function includeCustomPath() {
     $customProprtyName = ['customPHPPathDir', 'customTemplateDir'];
     foreach ($customProprtyName as $property) {
-      $value = $this->getSettings()->get($property);
+      $value = Civi::settings()->get($property);
       if (!empty($value)) {
         $customPath = Civi::paths()->getPath($value);
         set_include_path($customPath . PATH_SEPARATOR . get_include_path());