RFC: Config API options

[tractorcow]

References

• Existing github issue: Switch to micmania1's config implementation #6477
• Google Groups discussion: https://groups.google.com/forum/#!searchin/silverstripe-dev/config%7Csort:relevance/silverstripe-dev/q5khashNiuY/0Q7zkKwwAwAJ

Note: I'm going to lock this temporarily until I finish preparing the questions for voting. :)

Introduction

We are looking to move the config system away from the current backend and replace it with one built by @micmania1 https://github.com/micmania1/silverstripe-config

The purpose of this RFC is not to discuss so much the details regarding the implementation of this backend, but the necessary API available to the user.

The goal of this RFC will be to build an idea of which features users rely on, and which features can be either trimmed or completely removed.

Following this RFC I will leave a series of comments that users can vote on individually.

Proposal

Config::get() - Inheritance

Vote link: #6611 (comment)

By default, getting a config value will automatically merge down from child classes to parent classes. Non-array values will overwrite.

This can be controlled via one of the options:

• Config::INHERITED - Merge all config in the hierarchy (default).
• Config::FIRST_SET - Instead of merging, return the first config in the hierarchy that isn't empty.
• Config::UNINHERITED - Return only the config on the class directly.
• Config::EXCLUDE_EXTRA_SOURCES - Include extension values

Please vote using:

• 👍 I rely on these options and we should keep them.
• 👎 Remove one (or all) options. Please comment on which options should be removed.

Config::update() - Caching

Vote link: #6611 (comment)

This simply merges any array values with the current config, or replaces non-array values.

However, these values are not cached, and are simply merged into the current config.

With the new caching system, we could allow this method to take a callable, which could be invoked by the cache backend. E.g.

Config::inst()->update(SiteTree::class, 'db', function() {
	return $this->buildFields();
});

Please vote using:

• 👍 Cachable user-defined config would be useful for me.
• 👎 This feature wouldn't be useful to me.

Config::update() - Merge behaviour

Vote link: #6611 (comment)

Currently Config::update() will default to merging array configs together, rather than replacing. Potentially, it would be better to either add an options to update() to control this. E.g. Config::MERGE (merge array values only) or Config::REPLACE.

Please vote using:

• 👍 We should be able to replace config with Config::update() without merging it. Please comment on which option should be the default, and whether it should be a second argument, or if we should create a new method(s) (e.g. replace() / merge()).
• 👎 Config::update() should only merge values (status quo)

Extensions - Extra Config

Vote link: #6611 (comment)

Extension classes can implement an get_extra_config() method, which allows config to be dynamically generated for extended classes.

Without this feature, config can only be applied to extended classes via actual private statics on those extensions, or via yml.

This is used in a few places in core, so substituting this will require some further discussion.

Please vote using:

• 👍 I rely on config defined dynamically via Extension::get_extra_config()
• 👎 This feature can be removed and it won't affect me. Please leave a comment if something else should be put in place that replicates this functionality, and what form you would like this to be.

Custom user-defined config source - Cacheable

Vote link: #6611 (comment)

As an alternative to the above, we could define a ConfigSource interface, which could be implemented by any class, and this config could be cacheable.

E.g.

class MyConfigSource implements ConfigSource
{
	/**
	 * Return a set of config to merge into the main config during manifest build.
	 * These values will be cached between requests, and re-requested during flush.
	 *
	 * @return array
	 */
	public function getConfig()
	{
		// Note: This will be invoked after static and yml manifest is built, so Config::inst()
		// can be used here
		return [
			'SiteTree' => [
				'db' => $this->buildfields(),
			],
		];
	}
}

Please vote using:

• 👍 Something like this would be useful and/or necessary to me (if one of the other config sources is removed)
• 👎 This would not be useful to me at all.

Falsey values

Vote link: #6611 (comment)

There has been a long-standing convention in config that falsey values cannot be overridden easily in config. e.g.

---
Name: base
---
Class:
  option:
    Key1: null
    Key2: 'test'
---
Name: extra
---
Class
  option:
    Key1: 'new key1'
    Key2: null

Config::inst()->get('Class', 'option') would be give

[
	'Key1' => 'new key1',
	'Key2' => 'test',
]

We propose fixing this so that later values, even if empty, replace prior values. I.e.

[
	'Key1' => 'new key1',
	'Key2' => null,
]

Related issue: #3272

Please vote using:

• 👍 Allow falsey values to override non-falsey values (new behaviour)
• 👎 Falsey values should always be overriden by non-falsey values (status quo).

[tractorcow]

Vote: Config::get() - Inheritance

By default, getting a config value will automatically merge down from child classes to parent classes. Non-array values will overwrite.

This can be controlled via one of the options:

Config::INHERITED - Merge all config in the hierarchy (default).
Config::FIRST_SET - Instead of merging, return the first config in the hierarchy that isn't empty.
Config::UNINHERITED - Return only the config on the class directly.
Config::EXCLUDE_EXTRA_SOURCES - Include extension values
Please vote using:

• 👍 I rely on these options and we should keep them.
• 👎 Remove one (or all) options. Please comment on which options should be removed.

[tractorcow]

Vote: Config::update() - Caching

This simply merges any array values with the current config, or replaces non-array values.

However, these values are not cached, and are simply merged into the current config.

With the new caching system, we could allow this method to take a callable, which could be invoked by the cache backend. E.g.

Config::inst()->update(SiteTree::class, 'db', function() {
	return $this->buildFields();
});

Please vote using:

• 👍 Cachable user-defined config would be useful for me.
• 👎 This feature wouldn't be useful to me.

[tractorcow]

Vote: Config::update() - Merge behaviour

Currently Config::update() will default to merging array configs together, rather than replacing. Potentially, it would be better to either add an options to update() to control this. E.g. Config::MERGE (merge array values only) or Config::REPLACE.

Please vote using:

• 👍 We should be able to replace config with Config::update() without merging it. Please comment on which option should be the default, and whether it should be a second argument, or if we should create a new method(s) (e.g. replace() / merge()).
• 👎 Config::update() should only merge values (status quo)

[tractorcow]

Vote: Extensions - Extra Config

Extension classes can implement an get_extra_config() method, which allows config to be dynamically generated for extended classes.

Without this feature, config can only be applied to extended classes via actual private statics on those extensions, or via yml.

This is used in a few places in core, so substituting this will require some further discussion.

Please vote using:

• 👍 I rely on config defined dynamically via Extension::get_extra_config()
• 👎 This feature can be removed and it won't affect me. Please leave a comment if something else should be put in place that replicates this functionality, and what form you would like this to be.

[tractorcow]

Vote: Custom user-defined config source - Cacheable

As an alternative to the above, we could define a ConfigSource interface, which could be implemented by any class, and this config could be cacheable.

E.g.

class MyConfigSource implements ConfigSource
{
	/**
	 * Return a set of config to merge into the main config during manifest build.
	 * These values will be cached between requests, and re-requested during flush.
	 *
	 * @return array
	 */
	public function getConfig()
	{
		// Note: This will be invoked after static and yml manifest is built, so Config::inst()
		// can be used here
		return [
			'SiteTree' => [
				'db' => $this->buildfields(),
			],
		];
	}
}

Please vote using:

• 👍 Something like this would be useful and/or necessary to me (if one of the other config sources is removed)
• 👎 This would not be useful to me at all.

[tractorcow]

Vote: Falsey values

There has been a long-standing convention in config that falsey values cannot be overridden easily in config. e.g.

---
Name: base
---
Class:
  option:
    Key1: null
    Key2: 'test'
---
Name: extra
---
Class
  option:
    Key1: 'new key1'
    Key2: null

Config::inst()->get('Class', 'option') would be give

[
	'Key1' => 'new key1',
	'Key2' => 'test',
]

We propose fixing this so that later values, even if empty, replace prior values. I.e.

[
	'Key1' => 'new key1',
	'Key2' => null,
]

Related issue: #3272

Please vote using:

• 👍 Allow falsey values to override non-falsey values (new behaviour)
• 👎 Falsey values should always be overriden by non-falsey values (status quo).

[tractorcow]

Unlocked for voting!

[micmania1]

I have a branch of cms and framework which i'll share when I can, but its outdated. It already covers a few things below - all I that's left to do is fix the tests (there's a lot failing).

Config::get() - Inheritance
I've already removed Config::FIRST_SET in a test implementation. In all current uses of it in framework and cms it can be replaced with Config::UNINHERITED. The use case for Config::FIRST_SET is very edge case.

Removing others might be too much of a change. I would support it but doubt it would be realistic to include in this change.

Config::update() - Caching
Config shouldn't change after runtime. If it does, then there's probably a better solution for what you're doing. Also, the reason for updating it at runtime would be based on some kind of logic which should already handle what it needs to such as db builds.

Further to this, if this was something we needed (updating on key changes), I think it should be implemented via an event system rather than callbacks which could become hard to maintain.

Config::update() - Merge behaviour
Update is quite ambiguous. We should probably deprecate it and replace it with replace/merge.

Extensions - Extra Config
-1 on dynamic generation. Again, this might take some time to implement so not sure if it would be realistic for now.

Custom user-defined config source - Cacheable
This adds an extra dependency on class manifest so i'd be against it. We'd need to be able to track all classes which implement an interface.

Also, having the logic in PHP makes it easy for people to call things before they're ready.

Falsey values
This should be possible.

[sminnee]

A few quick comments for now @tractorcow

• I agree with Michael that we should remove the special config options where we can - eg FIRST_SET

• I think that be config class should be a composite object but I don't think we necessarily need a config book for this: custom config objects could be something that you do with a custom index.php, which seems we don't need a special extension point.

• I think that config::update use-cases can probably use more of a review. For example, we might have different config backend for tests that it mutable but less efficient.

[tractorcow]

Hm, this puts me into a bit of a corner, because saying there are no allowed sources for config at all (other than yml / private statics) removes a lot of use cases that we currently rely on. I've attempted to suggest various alternatives to this, but it doesn't feel as though any of them are accepted.

Can we discuss what "the" solution should look like in case an application has a need for generated / extensible config? Do we need some kind of new "Extension, but for config" api?

[tractorcow]

Rather than an interface and requiring a class manifest, we could push additional config sources.

mysite/_config.php

<?php

use SilverStripe\Core\Config\ConfigLoader;

$configSource = new MyModuleSource();
ConfigLoader::instance()->getManifest()->registerSource($configSource);

mysite/code/MyModuleSource.php

<?php

use SilverStripe\Core\Config\ConfigSource;

class MyModuleSource implements ConfigSource {
	// Getter only?
	public function getConfig()
	{
		return [
			'SiteTree' => [
				'db' => $this->buildfields(),
			],
		];
	}
	
	// Decorate existing config?
	public function updateConfig(&$config)
	{
		$config['SiteTree']['db'] = array_merge($config['SiteTree']['db'], $this->buildFields());
	}
}

Additional config sources could be cached.

Additionally, we could leave Config::inst()->update() calls uncached, and have these affect only the current request (status quo).

[tractorcow]

Actually, we could load in additional sources via yml, given that the static and yml loaders are loaded first.

SilverStripe\Core\Config\ConfigLoader:
  config_sources:
    - MyModuleSource

[tractorcow]

Maybe it would help to use an example.

FulltextSearchable extension generates a dynamic indexes config for extended classes based on the fulltext columns specified.

public static function get_extra_config($class, $extensionClass, $args)
    {
        return array(
            'indexes' => array(
                'SearchFields' => array(
                    'type' => 'fulltext',
                    'name' => 'SearchFields',
                    'value' => $args[0]
                )
            )
        );
    }

How would this be approached with the new config system?

[tractorcow]

@micmania1 quick question for you over the weekend; Is ConfigCollection::get() supposed to return the value, or the value + metadata? It looks like ConfigCollection::set() doesn't include the metadata. Possible bug?

public function set($key, $value, $metadata = []) {
	...
	$this->config[$key] = $value;
}

[micmania1]

ConfigCollection::get() should not return the meta data. I think there's a ConfigCollection::getMetadata() method.

I've just pushed 3 new commits which includes some fixes: https://github.com/micmania1/silverstripe-config/commits/master

[tractorcow]

Thanks, I have a fork I'm playing around with that I'll rebase on. :D

[tractorcow]

I've had a chat with @sminnee about some of the concerns around caching / mutable configs. We've decided to split the current Config class into ImmutableConfig and MutableConfig, where the former is the default. Trying to mix inheritance, caching, and mutability wasn't ending up as maintainable or performant code.

Applications that wish to make local modifications to config (via update / remove) will need to use MutableConfig.

Additionally, we're going to move INHERITED out of the Config layer (and caching level) and require users to opt-into class inheritance to get a hierarchal list of configs. That and FIRST_SET being gone means a simpler config back-end. :) We'll probably add it in as optional-api to Config_ForClass.

[tractorcow]

Proof of concept of split at open-sausages@0a2b550

Includes re-implementation of the missing update() / remove() functions, although in this case I've split update() into set() and merge(), and deprecated the original.

[tractorcow]

closed with #6641