Config - override all vs merging

[Marcus Nyeholt]

Assume the following configuration, which sets up an AppLogger object of type Zend_Log

app-logging.yml

---

Name: applogging

---

Injector:

  AppLogWriter:

    class: Zend_Log_Writer_Stream

    constructor:

      - /var/log/silverstripe/mylog.log

  AppLogger:

    class: Zend_Log

    constructor:

      - %$AppLogWriter

  Page_Controller:

    properties:

      logger: %$AppLogger

Then, in my code I can just call $this->logger->info("Message to log");

Now, if I want to change this in a local system's configuration, what I want to be able to do is

local-env.yml

---

Name: locallogging

After: #applogging

---

Injector:

  AppLogWriter:

    class: Zend_Log_Writer_Stream

    constructor:

      - /tmp/mysystem.log

which should replace the AppLogWriter definition with the new configured item. Which it does; except it merges the "constructor" config setting (because it's an array) which then means that the AppLogWriter is created using TWO __construct parameters; /var/log/silverstripe/mylog.log and /tmp/mysystem.log. Which causes problems because Zend_Log_Writer_Stream will expect its second __construct param to be the filemode

Is there a way in a config block to explicity state that arrays should 'overwrite' instead of merge? 

Cheers,

Marcus

[Hamish Friedlander]

Short answer: No, not at the moment.

Longer answer: There are several things you might want to do to the current value of an array value in a config fragment -  replace, merge, insert new values to the start, end, or somewhere in the middle, remove some items, etc. I just decided to make merging the only option - we had several patterns where we need to merge (like routes), and I hadn't seen any pattern where replacing was required (except in the case of parent class / child class configuration, which is handled by the options passed to Config#get). 

I couldn't come up with a syntax in YAML that I liked that generically exposed all the options, especially taking into account nesting, etc. In your specific example "Injector" needs to merge, but the sub property "AppLogWriter" needs to replace. The solution might be some sort of setter system, so that a class can say "call this function with the various Injector fragments, I'll handle the merge / replace logic".

Hamish Friedlander

--
You received this message because you are subscribed to the Google Groups "SilverStripe Core Development" group.
To post to this group, send email to silverst...@googlegroups.com.
To unsubscribe from this group, send email to silverstripe-d...@googlegroups.com.
For more options, visit this group at http://groups.google.com/group/silverstripe-dev?hl=en.

[Marcus Nyeholt]

Yeah, in many (most?) cases merge is actually the preferred way of doing things for properties of injected objects. 

What about something along the lines of

Injector:

  AppLogWriter:

    class: Zend_Log_Writer_Stream

    constructor|replace:

      - /var/log/silverstripe/mylog.log

Cheers,

Marcus

[Uncle Cheese]

Hi, Marcus,

Off-topic, but I'm wondering if you can explain what you're doing in that snippet. I've read all about the Config layer, but I'm not always sure when to use it and what it can do. Can you talk about what this snippet does and how it's different than (and better than) SS2? I'd much appreciate the knowledge share. Thanks.

[Marcus Nyeholt]

Hi Aaron,

Not sure if you're asking specifically about the Injector fragment, or just the config in general, but I'll answer both :)

This config is specifically based around specifying config in the dependency injector, that'll be used for creating an instance of the object when referred to as a dependency of another object. 

So to break it down a bit

> Injector:

>   AppLogWriter:

>     class: Zend_Log_Writer_Stream

>     constructor:

>       - /var/log/silverstripe/mylog.log

      

This config is saying that within the injector, if something references 'AppLogWriter' as a dependency, the Injector knows to create an object of class Zend_Log_Writer_Stream, passing /var/log/silverstripe/mylog.log as a constructor var. If the 'AppLogWriter' object has been created already, it passes that instance instead (unless you specify its type as a 'prototype' object, in which case it gets created new every time)

      

>   AppLogger:

>     class: Zend_Log

>     constructor:

>       - %$AppLogWriter

When AppLogger is referenced, create an object of class Zend_Log, and pass through the AppLogWriter object as a param

>   Page_Controller:

>     properties:

>       logger: %$AppLogger

      

This says that when Page_Controller is created, pass in AppLogger as a property called logger. So then code in Page_Controller can call $this->logger->info etc

So, the alternative in SS 2.4 is to specify

Injector::$AppLogger = array(

'class' => 'Zend_Log',

'constructor' => array(

'%$AppLogWriter'

)

);

There's not a huge difference, but there are a few key points

* Config specification is available earlier in the request cycle than module specific _config.php declarations

* Configuration from various sources gets merged together in a mostly smart way (except as the topic of this thread, when you explicitly want to override things!). This is particularly useful in cases when you want to allow modules to provide additional items in lists - as an example, in the advancedworkflow stuff I'm playing around with letting people specify a workflow template as an array of config options. (see https://github.com/nyeholt/advancedworkflow/blob/workflow_templates/_config/workflows.yml). You'll notice that I define WorkflowService as having a property called 'templates' which is a list with a single value referencing SimpleReviewApprove; if someone else defines WorkflowService with 'templates' as a list with some other values, the two definitions get merged together so that when I reference $this->templates in WorkflowService, it contains all the specified items. 

* It's just neater :D

I'm sure Hamish can suggest other benefits, but those are why I'm using it particularly with the dependency injector.

To view this discussion on the web visit https://groups.google.com/d/msg/silverstripe-dev/-/j2T9Ju_dzM0J.

[Marcus Nyeholt]

I've got a sample implementation up at https://github.com/nyeholt/sapphire/tree/config_replace_option that allows for specifying yaml config blocks with a custom merge handler class. Testcase example for things are at https://github.com/nyeholt/sapphire/blob/config_replace_option/tests/core/manifest/ConfigManifestTest.php#L91 

A snip from the doco update

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
---
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 containingarray('Page', 'Sixth', 'DataObject', 'Monster');

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

Throwing it open to discussion before cleaning up the commit history. Would something like this be okay for a future 3.0 release given it doesn't change any existing API (and arguably fixes a limitation in existing code), or just for 3.1 development? 

Cheers,

Marcus

On Thu, Nov 8, 2012 at 11:05 AM, Hamish Friedlander <ham...@silverstripe.com> wrote:

[Hamish Friedlander]

Hi Marcus,

Thanks for looking at this. I'm still not keen on the nesting being different on the replacement version - when scanning down, it's hard to realise that one fragment is special and needs to be read differently.

I'd rather the replacement bit looked like:

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

That way the fragment doesn't have to be dedicated to just the replacement - for instance you could do:

---
MergeStrategy: 
  Injector/Something: replace
---
Injector:
  Something:
    constructor: 
      - DataObject
      - Monster
  SomeOtherThing:
    constructor:
      - Ghost

And "Something" would be replaced, but "SomeOtherThing" would be merged. What do other people think?

Adding a new API is a bit different to changing an API, but this still feels like something that should go in 3.1 rather than 3.0 - although I guess you could see it as a bug fix. Sam, Ingo?

Hamish

[Marcus Nyeholt]

> when scanning down, it's hard to realise that one fragment is special and needs to be read differently

I was thinking about it a bit more and found the converse; if it was defined in the same way as other fragments, I didn't realise there was something special about it

But your second point is something that I was mulling over in my head this morning, so will put something together around this too. I'm not sure it should handle anything not explicitly named in a merge strategy for a couple of reasons 

- how do you know what level is an unnamed piece? 

- having custom defined merge strategies feels like something a developer should be being explicit about to prevent hidden side effects

Will push something else up a bit later - time to run for some meetings!

---
MergeStrategy: 
  Injector/Something: replace
  Injector/SomeOtherThing: insertfirst
---
Injector:
  Something:
    constructor: 
      - DataObject
      - Monster
  SomeOtherThing:
    constructor:
      - Ghost

[Hamish Friedlander]

- how do you know what level is an unnamed piece? 

That's a problem when you don't keep nesting consistent, but not when you do: there's never a confusion about level, because we always start from the top level. Keys in the MergeStrategy are always from the top level (always Class/Property/....) and same thing with values in the configuration block.

- having custom defined merge strategies feels like something a developer should be being explicit about to prevent hidden side effects

We already have the equivalent of merge strategy happening on every configuration yaml file, but it depends only on the type of the value being set.

To expand: when you're setting a configuration parameter on a class, if there's no existing value for that parameter, no problem.

If there is an existing value we start a deep recursing, merging until there isn't, or we hit a non-hash, in which case if it's a string we replace the old value with the new value, and if it's an array we append the new values to the old value

That is, we're using these merge strategies based on the type of value:

Associative array = "merge"

Sequential array = "append"

Scalar = "replace"

What I'm suggesting is that each config fragment could override that default for some class / property / hash key - so at each level the logic would be:

Is there a declared merge strategy for this level? If so, use that. If not, determine by type

For the example you gave, the MergeStrategy would actually be:

---
MergeStrategy: 
  Injector/Something: replace
  Injector/SomeOtherThing/constructor: insertfirst
---
Injector:
  Something:
    constructor: 
      - DataObject
      - Monster
  SomeOtherThing:
    constructor:
      - Ghost

Because we're saying "when you get to Injector/SomeOtherThings/constructor, and you notice there's already a value there, don't do the default for a scalar array of "append", do "prepend" instead.

You can also do

---
MergeStrategy: 
  Injector/Something/constructor: replace
  Injector/SomeOtherThing/constructor: insertfirst
---
Injector:
  Something:
    constructor: 
      - DataObject
      - Monster
    someOtherProperty:
      Foo: 1
      Bar: 2
  SomeOtherThing:
    constructor:
      - Ghost

In which case "someOtherProperty" will use the default merge strategy for an associative array of "merge"

The one downside that I see is that by allowing large configuration blocks, the MergeStrategy declaration might end up a long way away from the key it's actually defining the merge strategy for. I'd rather enforce this by code convention than by artificially limiting the size a configuration block is allowed to be though.

Hamish Friedlander 

[Marcus Nyeholt]

> That's a problem when you don't keep nesting consistent, but not when you do: there's never a confusion about level, because we always start from the top level ... If there is an existing value we start a deep recursing, merging until there isn't

Which is what I'd initially been trying to avoid so that custom strategy 'paths' didn't have to be passed through the Config::merge_high_into_low chain and avoid additional complexity in that method (also - avoid me having to try and understand its inner demonic working :D). The current implementation handles blocks with a defined MergeStrategy separately to config fragments that don't, making for much less complexity. 

I'll take a look at what horror passing merge strategy information through to merge_high_into_low would cause. 

To summarise the different approaches

- provide an arbitrary config fragment, telling ConfigManifest to handle it using a non-default handler to insert it into the overall configuration  (what I've just implemented)

or 

-  for a block of configuration, provide a set of possible strategies of how to merge in each key/value pair encountered, which is recursively applied through the tree of merges, the default being a 'merge' strategy

[Marcus Nyeholt]

(woulda been a skype message but you're hopefully off having a beer by this time)

Can you clarify my understanding of some code in Config::merge_array_high_into_low() ? My understanding of what you're doing here is swapping the arrays being merged so that variable assignment later on uses the 'correct' array on the right hand side, thus making the internal logic equivalent to assigning the currently built up config over the top of the things you're wanting to add in. 

Just makes the handling of a non-merge style operation fiddly :) 

[Marcus Nyeholt]

Take a peek at https://github.com/nyeholt/sapphire/tree/config_merge_strategies

It now takes fragments in the form 

---

Name: second_bit

MergeStrategy: 

  Injector/Something: replace

---

Injector:

  Something:

    constructor: 

      - DataObject

      - Monster

  OtherThing:

    constructor:

      - one

which keeps the structure consistent, and allows for multiple merge strategies to be defined for different configuration blocks, so  that in the above example, the 'OtherThing' block is handled as usual meaning 'replace' config fragments can be defined inline with normal merge fragments

I haven't completely rewritten the config :: merge stuff - just the merge_array_high_into_low function to avoid the flipping of arrays. Test cases all pass (and have added a couple of other checks that existing tests didn't pick up). 

Cheers,

Marcus

[Hamish Friedlander]

Hi Marcus,

Cool, I think we're pretty close

Yeah, merging high into low is (or was, anyway) the inverse of merging low into high, so just re-calling low_into_high with the arguments reversed gave the correct result & kept the code duplication down (and I hadn't figured out that array_splice trick).

The problem is at the moment merge strategies don't know how to do a low-into-high merge (since that's never used when combining YAML fragments, only in Config::get), only high-into-low. So at the moment low_into_high isn't the inverse of high_into_low.

I think that's OK for now, the only change that needs to be made is to change merge_array_low_into_high to throw an error when it finds a strategy rather than (incorrectly) just perform the high-into-low merge.

Longer term it'd be good to make all merge strategies be able to perform either direction merges.

Hamish Friedlander

[Marcus Nyeholt]

Ah yes good point - will make that change too. 

[Marcus Nyeholt]

Have changed that around to only use the merge strategies for high -> low

If you're happy with that, I'll raise a merge request - but should it also go against 3.0? 

[Hamish Friedlander]

It took me an hour or so of head bending, but I've figured out what worried me about the new merge_array_high_to_low. When there's a key clash, it doesn't preserve the order correctly. I've written a couple of tests that pass with the original but fail with the new version here: https://github.com/silverstripe-rebelalliance/sapphire/commit/d0af596aa92be5aef2f682bfc3529b9ece4b2f78

I've got a naive solution, but there's some problems with it, specifically a merge strategy can't control the location of any value it adds to the merged configuration. I'll keep working on it.

Hamish Friedlander

[Marcus Nyeholt]

When you say "preserve" the order, are you meaning preserving the order of the existing config, or the new items being merged in?

I wasn't sure what was desired here, so I took it to be that existing keys stayed in their current place, and new keys were inserted after them, but it probably should be that the 'src' keys should be preserved in that order. 

[Hamish Friedlander]

Preserve is possibly the wrong word. The order is important, and it needs to follow the rules as per the docblock at the top of config. Basically: when merging a high block into a low block, and looking at the result's keys the high block's keys should always come first, and their order should be the same.

Probably the place you'd see the effect of wrong ordering most strongly is when overriding routes - you want the overriding route to be in a place that indicates it's priority, not the priority of the route you overrode. It wasn't well tested before, and the effects might be subtile.

What do you think of https://github.com/silverstripe-rebelalliance/sapphire/commits/config_merge_strategies ?

Hamish Friedlander

On 7 January 2013 16:11, Marcus Nyeholt <nye...@gmail.com> wrote:

When you say "preserve" the order, are you meaning preserving the order of the existing config, or the new items being merged in?

I wasn't sure what was desired here, so I took it to be that existing keys stayed in their current place, and new keys were inserted after them, but it probably should be that the 'src' keys should be preserved in that order. 

--

[Marcus Nyeholt]

> Probably the place you'd see the effect of wrong ordering most strongly is when overriding routes - you want the overriding route to be in a place that indicates it's priority, not the priority of the route you overrode. It wasn't well tested before, and the effects might be subtile.

Exactly what I'd hit on last night, but the result I'd come up with didn't account for subsequent elements in a different order. 

https://github.com/silverstripe-rebelalliance/sapphire/commits/config_merge_strategies  looks good to me - I was thinking along those lines of building up a 'new' config array to hold the correctly ordered elements, and it avoids some of the overhead of splicing things around. 

Cheers,

Marcus

[Marcus Nyeholt]

So yeah - I'm happy with that branch, shall I leave it with you to PR back in to core? 

[Hamish Friedlander]

Yeah, I'll raise a pull request later today.

Hamish Friedlander

[Damian Mooyman]

Sorry to necro an old thread, but was this ever revisited since this discussion? I've been trying to look for this functionality for a while, so I'm wondering how far progressed this has come. :)

For the time being I have been using Config calls directly in PHP to manipulate the configuration, but it would be great to have this available via YAML.

[Marcus Nyeholt]

I'm going to be the necromancer this time :) 

In a few of our projects I'm starting to build up quite a lot of things like

$service = Config::inst()->get('Injector', 'SomeRestfulService');

$service['constructor'] = array('http://stage.webservices.url', -1);

Config::inst()->update('Injector', 'SomeRestfulService', $trsService);

in local environment config to do overrides which should really be done via config.yml (not least because of the caching clear that calling update() needs to manage). Any chance of this being re-visited?

--
You received this message because you are subscribed to the Google Groups "SilverStripe Core Development" group.

To unsubscribe from this group and stop receiving emails from it, send an email to silverstripe-d...@googlegroups.com.

To post to this group, send email to silverst...@googlegroups.com.

Visit this group at http://groups.google.com/group/silverstripe-dev.
For more options, visit https://groups.google.com/groups/opt_out.
 
 

[nicolaas]

Hi Everyone

I found this thread when looking for how to update arrays in config settings.  What we wanted to do is to restrict the allowed actions in the Security class.  The default is:

private static $allowed_actions = array(

'index',

'login',

'logout',

'basicauthlogin',

'lostpassword',

'passwordsent',

'changepassword',

'ping',

'LoginForm',

'ChangePasswordForm',

'LostPasswordForm',

);

We wanted to remove a bunch of these options - as a quick (and hack-ish) way to disallow password reminders. 

This is how we did it:

$my_security_allowed_actions = array(

'index',

'login',

'logout',

'basicauthlogin',

'ping',

'LoginForm',

);

Config::inst()->remove('Security', 'allowed_actions');

Config::inst()->update('Security', 'allowed_actions', $my_security_allowed_actions);

You can also use this:

$my_security_allowed_actions = array( ...);

Config::inst()->update('Security', 'allowed_actions', null);

Config::inst()->update('Security', 'allowed_actions', $my_security_allowed_actions);

The following options do not get the desired result (they dont throw errors, but I think they merge the array rather than replacing it): 

$my_security_allowed_actions = array( ...);

$service = Config::inst()->get('Security', 'allowed_actions');

$service['constructor'] = $my_security_allowed_actions;

Config::inst()->update('Security', 'allowed_actions', $my_security_allowed_actions);

(not sure if I understood this correctly); and neither does:

$my_security_allowed_actions = array( ...);

Config::inst()->update('Security', 'allowed_actions', $my_security_allowed_actions);

OR 

$my_security_allowed_actions = array( ...);

Security::config()->allowed_actions =  array(  $my_security_allowed_actions );

I hope you dont mind me pasting this here as a reminder for myself and others who are looking at replacing rather than merging an array value for a config setting. Of course, I could be totally mistaken so feel free to point out any stupidity or irrelevance or new code that makes this discussion  obsolete  ;-)

THANK YOU

Nicolaas

[Conrad Dobbs]

This seems to be one of those topics which keeps dying.

My thoughts:

Merging seems to be the exception to the rule, it makes more sense that setting a config setting would overwrite other something set previously. With the main exceptions being the DataObject related statics ($db, $has_one etc)

In the long run it would be better to have config settings in YAML only, not as statics on classes.

In this case, would it make more sense to separate out the handling of DataObject configuration, and general configuration? The DataObjects work as is, merging the arrays, while general configuration in the YAML files overwrites.