CiviCRM Community Forums (archive)

Complaints

None of these are new. Several have seen substantial progress; some haven't. Each has lingered for a few release cycles:

• Settings should be stored consistently in `civicrm_setting` instead of `config_backend`.
• Static caches don't have a good home, which makes them hard to manage in testing.
• The contents of CRM_Core_Config are unclear, hard to trace, and not extensible. We see bugs where, e.g., data from one request gets cached in CRM_Core_Config/config_backend and erroneously reused on another request.
• The logic for migrating a database to a different server is ... not simple.

Generally, CRM_Core_Config contains a mix of things which have different mechanics, e.g.

• Administrative settings (e.g. $dateformatFull or $mapAPIKey)
• Runtime server parameters (e.g. $dsn or $resourceBase)
• System services (e.g. getLog(), getMailer(), $userSystem)
• Temporary / cached / static data (e.g. $inCiviCRM and $enableComponentIDs])

Breaking these apart could seem a bit overwrought, but there are important distinctions.

• Administrative settings and runtime server parameters may seem similar (since admins influence both). However, administrative settings are portable; you can reasonably copy them among production/staging/development. Runtime parameters are not portable. Moreover, in many cases, we can figure them out automatically (by getting details from the CMS). Treating them the same has led to some really weird migration processes.
• Temporary / cached /static data would seem to be unnecessary if you use a service container and abide OOP conventions. But that's a long haul, and random statics cause problems today. One can mitigate and normalize them even without undertaking a full OOP-styled rewrite.

Proposals

Hollow out CRM_Core_Config. Move the parts elsewhere (respectively):

• civicrm_settings
• Parameters in Civi\Core\Container
• Services in Civi\Core\Container
• A consolidated $statics variable

And leave CRM_Core_Config as a stub for backward compatibility. Loosely:

<?php
class CRM_Core_Config {
  private $settings; // List of legacy settings.
  private $parameters; // List of legacy parameters.
  private $services; // List of legacy services.
  private $aliases; // List of field aliases.
  private $statics; // List of legacy statics

  public function __construct() {
    $this->settings = array('dateformatFull' => 1, 'mapAPIKey' => 1, ...);
    $this->parameters = array('dsn' => 1, 'resourceBase' => 1, ...);
    $this->services = array('userSystem' => 1, ...);
    $this->statics = array('inCiviCRM' => 1, 'doNotResetCache' => 1, ...);
    $this->aliases = array('debug' => 'debug_enabled', 'enableComponents' => 'enable_components', ...);
  }

  public function __get($k) {
    $k = isset($this->aliases[$k]) ? $this->aliases[$k] : $k;

    if (isset($this->settings[$k]) {
      return \Civi::settings()->get($k);
    }
    elseif (isset($this->parameters[$k]) {
      return \Civi::container()->getParameter($k);
    }
    elseif (isset($this->services[$k])) {
      return \Civi::service($k);
    }
    elseif (isset($this->statics[$k])) {
      return \Civi::$statics[$k];
    }
    // ^^^^ Micro-optimizations: Search in order of most-likely
    // buckets, and use isset() instead of empty() or array_key_exists().

    throw new \CRM_Core_Exception("Unrecognized property CRM_Core_Config::{$k}.");
  }

  public function __set($k, $v) {
    throw new \CRM_Core_Exception("Cannot modify properties of CRM_Core_Config.");
  }
}

But this leaves a different question - what replaces CRM_Core_Config, and how should you work with them?

I'd like to borrow a pattern that you can see in D8, where the \Drupal class (https://github.com/drupal/drupal/blob/8.0.x/core/lib/Drupal.php ) provides an entry point for accessing the most important subsystems (such as the service container, the configuration engine, and the caches). A few things to note about this pattern:

• There is no real data model in \Drupal. There are maybe 1-3 static variables.
• The interface is dominated by functions. These functions could be rewritten without changing the public interface.
• The functions are very thin. In practice, it's easy to understand how the functions relate to the container.
• You can clear or reset the entire system in one call.
• If you were writing an entirely new object-oriented system from the ground up, you probably wouldn't use this pattern. However, both Drupal and Civi have strong legacies of procedural programming, and there will inevitably be cases where the procedural code needs to summon some OOP code.

So using the same pattern, the recommended procedural interface would be:

<?php
class Civi {
  /**
   * @var ContainerInterface
   */
  private static $container;

  /**
    * @var array
    * A place to stash temporary data during this request.
    */
  public static $statics = array();

  /**
    * @param string $id
    * @return mixed
    */
  public static function service($id) {
    return self::$container->get($id);
  }

  /**
   * @return ParameterBag
   */
  public static function settings() {
    return self::$container->get('settings');
  }
  ...
}

In Depth: Civi\Core\Container, v4.5 and v4.6

We've been using Civi\Core\Container since v4.5. There are a few issues unaddressed in past releases:

• Extensions had no way to define services or event listeners in the container. The scheduled-reminder refactor includes a resolution to this, hook_civicrm_container. https://github.com/civicrm/civicrm-core/pull/6297
• The notation to access the container has been cumbersome: Civi\Core\Container::singleton()->get('foo').
• The service definitions are written in straight PHP, and Civi\Core\Container is becoming quite long. It's much nicer to split this information up, creating smaller YAML files or annotations. Symfony provides components/patterns to address this... we're not using them yet

My proposal would be to address the first two in the next release, and leave the third for a subsequent release.

In Depth: Settings en masse

I'd like to simplify the notation/interface as well as the implementation for working with settings.

// Current
CRM_Core_BAO_Setting::getItem('Localization Preferences', 'dateformatPartial');

// New
\Civi::settings()->get('dateformatPartial');

There are several bits of CRM_Core_Config and and CRM_Core_BAO_Setting which would be simplified if we didn't have to deal with groups and config_backend. Basically, I'm suggesting that we simply run "SELECT name, value FROM civicrm_setting WHERE domain_id=123" during Civi bootstrap.

However, there's a counter-point: the current arrangement seems optimized to reduce SQL queries. After all, when loading config_backend, a single query+row+cell provides dozens of settings. And for civicrm_setting, you only fetch the groups you need.

How would simplification affect performance? Well, I drafted some crude patches to add extra queries to every page request ( https://gist.github.com/totten/c1da0f59b0567cd2ad43) and ran Apache "ab" (eg ab -n20 -c4 'http://dmaster.l/civicrm/event/info?id=1'"). Generally, either query seems to add 5-10ms (~1% of 800-1000ms), but it's hard to tell.... even without changes, the benchmarks often varied by 10-100ms.

Notably, I'm suggesting this simplification at the same time that we remove the existing queries. (It's not simplification if you leave the old junk in place.) So, e.g., we'd add one query (selecting everything from civicrm_setting) and remove others (selecting config_backend and selecting a medley of settings). From a performance perspective, it seems unlikely to change.

Now... it wouldn't be so terrible to say \Civi::settings('localization')->get('dateformatPartial'); So if folks think that it's better to retain setting-groups... just in case... then so be it. But I thought I'd put out some data.

In Depth: Backports

Let's say you're writing an extension that targets an older version of Civi -- something which doesn't have the \Civi facade. Can you still use it?

I expect that about half of the interface could be trivially backported -- e.g. \Civi::service() and \Civi::$statics would be easy, but \Civi::settings() would not.

However, even if it weren't formally backported, I think it would be acceptable for extensions to embed their own copy of the class with suitable guards, e.g.

<?php
if (!defined('CIVICRM_CIVI_CLASS')) {
  define('CIVICRM_CIVI_CLASS', 1);
  class Civi {
    public static $statics = array();
  }
}


Yeah, $config is used basically everywhere (in PHP and Smarty; in core and contrib). I think we could deprecate it, but we should maintain backwards compatibility for a few major release cycles.

Looks good in general. On the trivial level some of the comment blocks wouldn't pass my phpcs checks which are set a bit closer to the drupal standard that what is enforced by Jenkins. (It requires a first line if the description that fits on one line & has a full stop, followed by a space & then any further comments :-)

Tim, I feel like I'm being supportive by adding tangental comments since I don't have anything to add on your main efforts :-)

Anyway there is currently an intermittent bug where setting metadata is added to the cache table without extension metadata being included (leading to it extension metadata not working). I don't know when that happens - perhaps when caches are cleared via drush or something. Anyway - you might notice something that might cause it at some point.

This is a bit tangential, but I think that we should encourage standard directories for extensions. One global (/vendor/civicrm) directory, and one site-specific (for each CMS: sites/example.org/extensions, wp-files, etc).

Having to configure an URL and Path in order to use extensions is a superfluous, error-prone step. Although I guess that not supporting custom directories/paths could annoy a lot of people during upgrades (CiviCRM doesn't like it when we move extensions around without disabling them).

Also reminds me of... https://issues.civicrm.org/jira/browse/CRM-16266 (/vendor + i18n path prefix in Drupal)

I'm starting to understand some of what goes on with the path and URL construction (which has been the hurdle to setting up a default extension folder), and I've almost gotten to a point where we can remove retrieveDirectoryAndURLPreferences() and storeDirectoryAndURLPreferences().

To that end, I've hit an issue in updating the admin form for URLs/paths. If it completely skips the config object and just works with the URLs as plain text settings, then the admin form no longer displays absolute URLs for typical values -- because they're internally represented as relative values. But the values can't be resaved because validation fails. (They're not well-formed absolute URLs.)

Options:

 1. Make up a new aspect to the settings metadata for filtering input/output. Wire it up to the existing admin forms. Or,similarly, figure out how to make a new data type in the current admin forms.
 2. Relax or remove the URL validation rule.

#1 seems kind of stupid - project is already big enough and ultimately aims to deprecate those forms. I don't see much downside to #2.

A few random updates:

• Relaxed the rules on URLs and directories so that you can view/save relative paths.
• Added a PSR-3-compliant log helper -- e.g. Civi::log()->warn("Oh noes!")). In retrospect, it's weird -- PEAR Log is actually pretty decent, and we've had it there, but some reason we wrapped it around and made it worse.
• Removed retrieveDirectoryAndURLPreferences, storeDirectoryOrURLPreferences, and fixAndStoreDirAndURL. You can use Civi::settings()->getPath(...) or ->getUrl(...) if you want the setting to be adapted to some particular format.
• However, there's a couple settings which have very dynamic behavior -- behaviors which aren't expressed in the setting metadata (e.g. adapting the defaults depending on CMS; creating the folders; restricting access). The interim solution (eg https://github.com/totten/civicrm-core/blob/d529069483142d3e83330a433b94d5a6a2992320/CRM/Core/Config/Defaults.php#L112 ) isn't beautiful, but it should be easier to trace / reorganize / imitiate.
• I didn't fix the locale_custom_strings/multi-lingual issues, but I disintermediated CRM_Core_Config and its entourage (_Variables, _Defaults, _ConfigSetting), so you should only have two classes to look at.

https://github.com/civicrm/civicrm-core/blob/4.6/CRM/Core/BAO/Setting.php#L677

I'm looking at CRM_Core_BAO_Setting::getSettingSpecification() and CRM_Utils_Hook::alterSettingsMetaData from the perspective of early stage scripts (e.g. xml/Gencode.php or install/index.php) which nominally use 1 or 2 settings without a full installation environment. (For example, if a code-generator calls ts() to translate sample data, then it accesses localization settings... even though there's no domain or DB yet.)

I think it's OK that the hook fires during early stage scripts (it's a null-op), but I don't understand some of the data being passed through:

• $domainID -- The settings metadata wouldn't actually change for different domains, would it? What would domainID be? And if settings did change per-domain... it reads like domainID=NULL is acceptable, but wouldn't that lead to cache-coherency problems when you have multiple domains? (eg domainID=NULL usually means "current domain", but "current domain" is subjective)
• $profile -- No idea what this does. As far as I can tell, it's passed through for the Setting.getfields API (if you supply it), but not in any other circumstance.

On second thought... I could understand changing the default values in the metadata based on runtime factors (e.g. domain ID, site URL, locale, user role). But I think it would be fairly hard to compute a proper cache-key. If the idea is to support these kinds of variations, then maybe we shouldn't cache the output of hook_alterSettingsMetaData()? (Although we should, of course, cache the initial folder-scan.)