Skip to content

Commit

Permalink
API Allow for different merge strategies in config
Browse files Browse the repository at this point in the history
Added ability to provide custom classes for handling the 'merge' of yaml config
fragments other than the default 'merge' strategy

Reimplemented merge_array_high_into_low so that it doesn't perform the swapping
of src and dest arrays when merging values, in such a way that it obeys
correct ordering of element arrays as required.

Added assertions for ConfigManifestTest

Updated configuration docs

Fixed issue where merges of high_into_low wouldn't maintain the 'high' array's
key order when merged into the lower array - the keys would be swapped in order
so an associative array_splice was used to continually splice the associative
data into place.

Keep merge strategy management in the ConfigManifest
as they're only applicable to yaml configuration, and removed the rules
from merge_low_into_high as they're not applicable for this either.
  • Loading branch information
nyeholt authored and Hamish Friedlander committed Jan 7, 2013
1 parent 04176dc commit b871872
Show file tree
Hide file tree
Showing 5 changed files with 277 additions and 13 deletions.
116 changes: 107 additions & 9 deletions core/Config.php
Expand Up @@ -220,6 +220,21 @@ public function forClass($class) {
}
}


/**
* Retrieve a strategy for merging in configuration information
*/
protected static function merge_strategy($strategy = 'default') {
if (!is_string($strategy)) return;

$class = 'Config_MergeStrategy_'.ucfirst($strategy);
if (class_exists($class)) {
$strategy = Injector::inst()->get($class);
return $strategy;
}
}


/**
* Merge a lower priority associative array into an existing higher priority associative array, as per the class
* docblock rules
Expand All @@ -244,8 +259,12 @@ public static function merge_array_low_into_high(&$dest, $src) {
// Throw error if types don't match
if ($currentType !== $newType) self::type_mismatch();

if ($currentType == self::IS_ARRAY) self::merge_array_low_into_high($dest[$k], $v);
else continue;
if ($currentType == self::IS_ARRAY) {
self::merge_array_low_into_high($dest[$k], $v);
}
else {
continue;
}
}
else {
$dest[$k] = $v;
Expand All @@ -264,13 +283,60 @@ public static function merge_array_low_into_high(&$dest, $src) {
* @param $dest array - The existing low priority associative array
* @param $src array - The high priority array to merge in
*/
public static function merge_array_high_into_low(&$dest, $src) {
$res = $src;
self::merge_array_low_into_high($res, $dest);
$dest = $res;
public static function merge_array_high_into_low(&$dest, $src, $rule = array()) {

// We often want to add things to the beginning of dest, but this not possible to do in-place (with non-int keys).
// Instead we append items to $next and remove from $dest. At the end of the function everything left in $dest
// is appended to $next and that becomes the result
$next = array();

foreach ($src as $k => $v) {
$strategy = null;
if (isset($rule[$k]) && is_string($rule[$k])) {
$strategy = self::merge_strategy($rule[$k]);
}
if ($strategy) {
$strategy->mergeHighIntoLow($next, $dest, $k, $v, $rule[$k]);
} else {
// default handling
if (!$v) {
continue;
}
else if (is_int($k)) {
$next[] = $v;
}
else if (isset($dest[$k])) {
$newType = self::get_value_type($v);
$currentType = self::get_value_type($dest[$k]);

// Throw error if types don't match
if ($currentType !== $newType) self::type_mismatch();

if ($currentType == self::IS_ARRAY) {
$next[$k] = $dest[$k];

$subrule = isset($rule[$k]) ? $rule[$k] : array();
self::merge_array_high_into_low($next[$k], $v, $subrule);
}
else {
$next[$k] = $v;
}

unset($dest[$k]);
}
else {
$next[$k] = $v;
}
}
}

if ($next) {
$merged = array_merge($next, $dest);
$dest = $merged;
}
}

public static function merge_high_into_low(&$result, $value) {
public static function merge_high_into_low(&$result, $value, $rule = array()) {
if (!$value) return;
$newType = self::get_value_type($value);

Expand All @@ -281,8 +347,11 @@ public static function merge_high_into_low(&$result, $value) {
$currentType = self::get_value_type($result);
if ($currentType !== $newType) self::type_mismatch();

if ($currentType == self::ISNT_ARRAY) $result = $value;
else self::merge_array_high_into_low($result, $value);
if ($currentType == self::ISNT_ARRAY) {
$result = $value;
} else {
self::merge_array_high_into_low($result, $value, $rule);
}
}
}

Expand Down Expand Up @@ -535,3 +604,32 @@ public function forClass($class) {
return Config::inst()->forClass($class);
}
}

interface Config_MergeStrategy {
public function mergeHighIntoLow(&$next, &$current, $key, $value, $rule = array());
}

class Config_MergeStrategy_Remove implements Config_MergeStrategy {
public function mergeHighIntoLow(&$next, &$current, $key, $value, $rule = array()) {
unset($current[$key]);
}
}

class Config_MergeStrategy_Replace implements Config_MergeStrategy {
public function mergeHighIntoLow(&$next, &$current, $key, $value, $rule = array()) {
unset($current[$key]);
$next[$key] = $value;
}
}

class Config_MergeStrategy_Prepend implements Config_MergeStrategy {
public function mergeHighIntoLow(&$next, &$current, $key, $value, $rule = array()) {
$next[] = $value;
}
}

class Config_MergeStrategy_Append implements Config_MergeStrategy {
public function mergeHighIntoLow(&$next, &$current, $key, $value, $rule = array()) {
$current[] = $value;
}
}
26 changes: 23 additions & 3 deletions core/manifest/ConfigManifest.php
Expand Up @@ -489,7 +489,9 @@ public function buildYamlConfigVariant($cache = true) {
$failsonly = isset($fragment['only']) && !$this->matchesVariantRules($fragment['only']);
$matchesexcept = isset($fragment['except']) && $this->matchesVariantRules($fragment['except']);

if (!$failsonly && !$matchesexcept) $this->mergeInYamlFragment($this->yamlConfig, $fragment['fragment']);
if (!$failsonly && !$matchesexcept) {
$this->mergeInYamlFragment($this->yamlConfig, $fragment['fragment'], isset($fragment['mergestrategy']) ? $fragment['mergestrategy'] : array());
}
}

if ($cache) {
Expand Down Expand Up @@ -547,9 +549,27 @@ public function matchesVariantRules($rules) {
* @param $fragment
* @return void
*/
public function mergeInYamlFragment(&$into, $fragment) {
public function mergeInYamlFragment(&$into, $fragment, $rules=array()) {
$ruleset = array();

foreach ($rules as $path => $strategy) {
$bits = explode('/', $path);
$enterInto = &$ruleset;
$entry = null;
// this builds the array structure
foreach ($bits as $entry) {
$enterInto[$entry] = array();
$enterInto = &$enterInto[$entry];
}
// and this sets the actual value
if ($entry) {
$enterInto = $strategy;
}
}

foreach ($fragment as $k => $v) {
Config::merge_high_into_low($into[$k], $v);
$rule = isset($ruleset[$k]) ? $ruleset[$k] : array();
Config::merge_high_into_low($into[$k], $v, $rule);
}
}

Expand Down
43 changes: 43 additions & 0 deletions docs/en/topics/configuration.md
Expand Up @@ -89,6 +89,49 @@ They are much simpler. They consist of a list of key / value pairs. When applied

- If the composite value is not an array, if that value matches any value in the mask it is removed

### Customising the merge

The behaviour of merging can be altered by providing a custom MergeStrategy directive with a configuration block.

For example, the following configuration makes use of the ReplaceYamlMergeStrategy class to completely replace a
config block instead of merging subsequent values

Assume the following defined in core_code/_config/core.yml

---
Name: original_config
---
Injector:
Something:
constructor:
- Page
- Sixth

And the following in my_custom_module/_config/override.yml

---
Name: second_bit
MergeStrategy:
Injector/Something: replace
---
Injector:
Something:
constructor:
- DataObject
- Monster


The MergeStrategy entry of the metadata block defines a configuration_path:strategy pair;

- **configuration_path** defines the array hierarchy that should be manipulated
- **strategy** defines the YamlMergeStrategy implementation that should be used to handle the merge.

In the above example, a reference to `$config->get('Injector', 'Something');` would return an array containing
`'constructor' => array('DataObject', 'Monster');`, as opposed to the default behaviour which would return an array
containing `array('Page', 'Sixth', 'DataObject', 'Monster');`

To handle the merge process in a custom way, simply define a class that implements YamlMergeStrategy

## Reading and updating configuration via the Config class

The Config class is both the primary manner of getting configuration values and one of the locations you can set
Expand Down
58 changes: 57 additions & 1 deletion tests/core/ConfigTest.php
Expand Up @@ -144,8 +144,64 @@ public function testMerges() {
Config::merge_array_high_into_low($result, array('C' => array('Bar' => 3, 'Baz' => 4)));
$this->assertEquals($result,
array('A' => 1, 'B' => 2, 'C' => array('Foo' => 1, 'Bar' => 3, 'Baz' => 4), 'D' => 3));

// test specific ordering of output values, not just that they exist
$result = array('dev' => 'sc');
Config::merge_array_high_into_low($result, array('first' => 'First', 'second' => 'Second', 'last' => 'Last'));
$this->assertEquals($result, array(
'first' => 'First',
'second' => 'Second',
'last' => 'Last',
'dev' => 'sc',
));

$keys = array_keys($result);
$this->assertEquals($keys[0], 'first');
$this->assertEquals($keys[3], 'dev');

// If a higher priority item has a non-integer key which is the same as a lower priority item, the value of
// those items is merged using these same rules, and the result of the merge is located in the same location the
// higher priority item would be if there was no key clash.

// First, check non-array subelements keep order of highest priority items

$result = array('Foo' => 1, 'Bar' => 2);
Config::merge_array_high_into_low($result, array('Bar' => 3, 'Foo' => 4));

$this->assertEquals(array_keys($result), array('Bar', 'Foo'));

// Then check array subelements do too

$result = array(
'Foo' => array('F1' => 1),
'Bar' => array('B1' => 1)
);
Config::merge_array_high_into_low($result, array(
'Bar' => array('B2' => 2),
'Foo' => array('F2' => 2),
));

$this->assertEquals(array_keys($result), array('Bar', 'Foo'));

// What about when there's a mix of associative & sequential keys?
$result = array(
'Foo' => 'FooL',
'Zap',
'Bar' => 'BarL',
'Baz' => 'BazL'
);
Config::merge_array_high_into_low($result, array(
'Bar' => 'BarH',
'Zop',
'Zup',
'Foo' => 'FooH'
));

$this->assertEquals(array_keys($result), array('Bar', 0, 1, 'Foo', 2, 'Baz'));
$this->assertEquals(array_values($result), array('BarH', 'Zop', 'Zup', 'FooH', 'Zap', 'BazL'));
}

public function testStaticLookup() {
$this->assertEquals(Object::static_lookup('ConfigTest_DefinesFoo', 'foo'), 1);
$this->assertEquals(Object::static_lookup('ConfigTest_DefinesFoo', 'bar'), null);
Expand Down
47 changes: 47 additions & 0 deletions tests/core/manifest/ConfigManifestTest.php
Expand Up @@ -88,4 +88,51 @@ public function testRelativeOrder() {
), 'after');
}

public function testMergeStrategy() {
$manifest = new SS_ConfigManifest(BASE_PATH, true, false);

$into = array(
'Top' => array(
'Second' => array(
'Leaf1' => 'scalar',
'Leaf2' => array('another array')
)
)
);

$normal = array(
'Top' => array(
'Second' => array(
'Leaf3' => 'scalar2',
'Leaf4' => array('more array')
)
)
);

$replace = array(
'Second' => array(
'Leaf3' => 'scalar2',
'Leaf4' => 'all',
)
);

$manifest->mergeInYamlFragment($into, $normal);

$match = array(
'Leaf1' => 'scalar',
'Leaf2' => array('another array'),
'Leaf3' => 'scalar2',
'Leaf4' => array('more array')
);
$this->assertEquals($into['Top']['Second'], $match);

$manifest->mergeInYamlFragment($into, $normal, array('Top/Second' => 'replace'));

$match = array(
'Leaf3' => 'scalar2',
'Leaf4' => array('more array')
);
$this->assertEquals($into['Top']['Second'], $match);

}
}

7 comments on commit b871872

@tractorcow
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Any chance of this making it into a pull request?

@hafriedlander
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Word from Sam was "don't do anything that makes the config system slower". This could be re-worked and pulled against master, but it's probably too late now for 3.1. It'll likely need rewriting though after I've finished the change needed to make Environment rule support work.

@tractorcow
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the quick response Hamish,

I think that's ok... it's really just a nice to have for now, but I'd like to see something along these lines make it into master in the long term.

Is there a temporary workaround for developers who want to have an overridable default value that's an array type? (that can be replaced via yaml instead of merged)

@nyeholt
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Bump for status update? Now that 3.1 is forcing people into using the config system more and more (finally :)) it's becoming more and more necessary to be able to override config settings

@nyeholt
Copy link
Author

@nyeholt nyeholt commented on b871872 Dec 5, 2013

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Bump again - very keen to have this in core...

@SebastianMucke
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Bump, too. I think the very cool way, configuring SilverStripe via yaml gets unnecessarily unattractive when the word is "but replacing arrays you have to do in your old _config.php, Dude". So give it a chance. 👍

@hafriedlander
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unfortunately there's moderate a performance impact with it just as it is. We're hopefully going to include some config system performance improvements in 3.2 which amongst other things will eliminate that performance cost, so we can include it in that work.

Please sign in to comment.