Replace SilverStripe Config

[Michael Strong]

Hi Everyone,

This is quite a lengthy post so you may want to make yourself comfortable.

I've spent quite a few hack days at SilverStripe looking at ways to improve the config system, mostly in relation to performance. I've found this difficult and frustrating to work with and there are very few gains that can be made without re-factoring the whole thing. Furthermore, I don't think very many people really understand how the config system works at its core (eg. SS_Dag???) and I've been thinking for quite a while about how the entire thing could be simplified so that how it works is more visible and easy to understand by everybody.

I've spent a few days working on a stand-alone package which is effectively the SilverStripe config system, but without a dependency on SilverStripe. So far I have only worked on the YAML part and have implemented all features of the YAML config (to my knowledge). I do intend to carry on working on this to add private statics and with it, inheritance.

You can view the work done so far here: https://github.com/micmania1/silverstripe-config

Why re-invent the wheel?

'Make the framework faster' is listed as the mostly highly sought after improvement in SilverStripe on uservoice. The config system is the slowest component and with the way its architected, there's very few performance gains that can be made. We can cache more, but this creates more issues as I've found in the past (https://github.com/silverstripe/silverstripe-framework/pull/4748).

Some parts of the config system are fundamentally flawed. Take for example:

---

Only:

 environment: 'dev'

---

Director:

 environment_type: 'prod'

send_money_to: 'dev'

---

Only:

 environment: 'prod'

---

Director:

 environment_type: 'dev'

send_money_to: 'prod'

To sum up the above, the config system depends on itself and I don't even know what the outcome would be. In a real use case, modules could clash and some important settings could be set on the wrong environment type. The solution to this is simple: environment type should not be configurable through the config system.

Whilst writing this I decided to test this and see what the outcome was and found the following bug which confirms my theory: https://github.com/silverstripe/silverstripe-framework/issues/5908

Another small flaw is the wildcard '*'. If you have at least two before statements or two after statements which use the '*' wildcard then they can't possibly both work as expected. They can't both be before each other and they can't both be after each other. Somehow this 'just works' at the moment and adds randomness to which yaml document takes priority (ie. one value could overwrite another).

Solving the problems

A big performance issue is that all merging of yaml documents and hierarchal structure (class inheritance) is done at run time. In the case of the yaml documents, this is done once per variant. A new variant is created every time the environment type changes to something new or a constant/environment variable is added or changed. 

This is roughly what a call to Config->get('class', 'key') looks like:

1. Is the value cached in memory? (if yes, then return the value and skip all other steps)
   
2. Starts looking at YAML for key. At this point it builds the variant key based on the current application state.
   
3. If the variant key does not already have merged (yaml docs) cache, then it will merge yaml docs together which pass only/except statements.
   
4. Loads the key value from the merged yaml config
   
5. Looks up the PHP private static on the given class, and then merges with the YAML and all parent classes (assuming we're doing inherited lookup)
   
6. Caches the final value for use in step 1.
   

It does this for every single config key/value that you lookup using Config::inst()->get() or any alias of it. Whenever the variant key changes (ie. updating environment type) it clears all cache in memory (step 1).

In setups where you're using a shared cache (APCu) this would clear the cache for all requests each time the page loads rendering APCu useless and most likely slow the site down even more (maybe related: http://api.silverstripe.org/3.3/source-class-ManifestCache.html#69).

In my implementation I've been very opinionated about how this should work. Whatever the state is when 'flush=1' is ran, is the state of the config until you run 'flush=1' again. Any Config->update() calls will be applied at runtime for that single request only and may slow that request down. These calls would be discouraged, but I think this is a sensible default. Personally, I'd like to discourage any environment dependent settings being put into the application config, but I feel that would be too big of a jump for now.

With this approach we're now able to cache the config before runtime leaving us with a key lookup (or as close as we'll get). It means running APCu and other shared caches should speed up a site and we can use these tools safe in the knowledge that the config is the same for every user/request (no data leaking). It also cleans up the dependency chain by removing the config's dependency on the application (now one way).

Removing the complexity

First off, we'd remove environment type from the config system and replace it with a normal static. This would prevent any circular dependency.

We'd also be able to remove SS_Dag which only Hamish has really contributed to and I'd question whether anybody else understands it. The job of SS_Dag is to resolve and sort yaml document dependencies based on the before/after statements. Instead of SS_Dag, I've used an external dependency which is well maintained and fully covered by tests: https://github.com/marcj/topsort.php

By also removing the runtime variations, as shown above this would reduce the workload of a single Config->get() call considerably. It should also allow developers to use APCu to improve performance. It should not be underestimated how much complexity we'd be removing here and the code would become infinitely more maintainable.

Also, because we've stripped everything back to a key/value lookup, we'd be able to remove Config_LRU (already deprecated) and Config_MemCache.

New Features

Whilst the primary goal was to improve performance and increase simplicity, there have also been some new features added and the stability has been improved.

Define your own rules

This would be used by frameworks (ie. SilverStripe core) to define rules for only/except statements.

$yamlTransformer->addRule('constantdefined', function($const) {

return defined($const);

});

---

Only:

 constantdefined: 'test'

---

test: 'test'

Create more complex rules by passing values

$yamlTransformer->addRule('constantequals', function($const, $value) {

return defined($const) && constant($const) == $value;

});

---

Only:

 constantequals:

   'myconst': 'testvalue'

---

MyConst: 'is defined'

Ignore built-in (or custom) rules

$yamlTransformer->ignoreRule('environment');

Multiple comparison checks of the same kind

---

Only:

 constantdefined:

   - 'myconst'

   - 'myotherconst'

---

MyConst: 'is test value and myotherconst is othertestvalue'

Improved YAML document parsing

With improved yaml document parsing, you can now have empty yaml files or empty yaml documents without SilverStripe throwing errors during flush.

Unlimited config formats

This package doesn't just work with SilverStripe. Its stand-alone meaning you can implement it with whatever framework you like.

Monitor your config

Because everything is merged before runtime, you can see all config keys and values that your app has available to it. This is untrue when you use Config::inst()->update().

What isn't discussed here?

Caching

Although caching is an important piece to making this work for SilverStripe, it isn't strictly the responsibility of this package. The caching mechanism is up to the framework implementation.

"I don't like YAML"

I know some people out there don't like the yaml format, but I don't want to get into a discussion about that here. It remains that the entire SilverStripe ecosystem relies on it and there's already a vast amount of changes coming in SS4. If you don't like yaml, this package might be the first step to replace it but that is not its intention.

What Next?

I'd appreciate some feedback from core committers and the wider community. Is this too much of a stretch for SS4? Have I missed something obvious? If you have any concerns or questions i'd be very interested to hear about them.

Cheers,

Michael

--

Michael Strong | Developer
SilverStripe
http://silverstripe.com/

Phone: +64 4 978 7330

[Ingo Schommer]

That's brilliant, thanks for persisting on this Michael! In order to get other people excited enough to get into this deep topic, do you have any idea what performance improvements we could be looking at? Or anecdotal evidence what percentage of wall time is spent in the existing Config system on a moderate sized SS codebase?

I've read through your codebase, and it's quite readable - as much as it can be given the complex topic :D It's going to be hard to determine if there's any regressions or missing/changed functionality. How much coverage do you think we can get with the existing ConfigTest and ConfigManifestTest in core when plugging in your code?

> Because everything is merged before runtime, you can see all config keys and values that your app has available to it. This is untrue when you use Config::inst()->update().

I find the before/after/only/except rules very hard to reason about by looking at individual YAML files. So you're saying this would be easier in your new approach? 

> A big performance issue is that all merging of yaml documents and hierarchal structure (class inheritance) is done at run time

We'll still need ConfigStaticManifest, right? Isn't that where most of the slowdown would be due to reflection (see existing Config implementation)?

> A new variant is created every time the environment type changes to something new or a constant/environment variable is added or changed.

My assumption is that happens rarely (since it needs a code deployment or env file change) - so wouldn't be a big problem in practice?

I assume this would be a dependency of ConfigManifest, replacing about half of the class implementation there? ConfigManifest still contains other responsibilities which won't get replaced with your module, right? For example, module registration.

I can't see any tests about merging of false-ish values - doesn't look like that's implemented? The underlying array_merge is always overwriting with the values of later keys in its arguments.

I've always been intrigued by the Symfony Config component and its ability to define config schemas through a fluent API, including custom merge strategies for specific nodes. Have you given any thought how this could work in your approach? Scope creep of course, but do you have an idea if it would be easier or harder to add this functionality later?

On a related note: The combined complexity of the Config and Injector APIs in SilverStripe has an effect on how much dependency injection is used throughout core and in modules. Private statics for config are really just a legacy hack in my opinion, and should be replaced by components which are initialised with constructor args and setters through dependency injection parameters configured in YAML. So making the Config API more understandable by the average SilverStriper will have wider positive effects than "just" performance :)

Ingo

[Michael Strong]

I haven't done any performance testing yet but its on my list of things to do. I'm not sure how helpful it will be at the moment as it might be difficult to create a fair test - the problem being that this config system merges the yaml documents whereas the current system doesn't (until runtime).

My aim is to make it fast enough that you could develop without flushing. Sadly, other things would still require flush, and you'd need to do a dev/build but I hope to make a step forward at least. Whether its possible or not will become more apparent when inheritance is introduced.

In theory ConfigTest shouldn't change (but probably will). The Config API will likely stay the same for the most part and remain in the core of SilverStripe, with this package just providing the merged config to it. It could also just turn into a front for the cache, but keeping the same API still.

I find the before/after/only/except rules very hard to reason about by looking at individual YAML files. So you're saying this would be easier in your new approach? 

With the new approach you'd end up with something like 1 big file containing an array of the config. Its not easy to see how the config was built (before/after etc.) but you'd see its final state very easily. Maybe I need to add something in to cover this. 

ps. 1 big file is likely something like the final solution but probably different. For example, we'd need at least two files, one for inherited and one for uninherited config flags. It also has memory implications so don't focus too much on that at the moment as it hasn't really been solved yet.

We'll still need ConfigStaticManifest, right? Isn't that where most of the slowdown would be due to reflection (see existing Config implementation)?

Nope, we shouldn't need it. I haven't implemented the static config transformer yet, but the plan is that the framework implementation provides it with the list of classes to check. With SilverStripe this is easy as we can just use the ClassManifest and then use reflection as is already done. The difference is, we'd be doing it before runtime (when we flush) so we can pre-merge it.

My assumption is that happens rarely (since it needs a code deployment or env file change) - so wouldn't be a big problem in practice?

Yeah, in reality this happens very few times and when it does, it caches the first time so every time after is quicker. You can trigger it as admin with isDev=1 though. This would change your config on the production environment (for that user) to the development config and could potentially leak config to other users if you're using a shared cache.

I can't see any tests about merging of false-ish values - doesn't look like that's implemented? The underlying array_merge is always overwriting with the values of later keys in its arguments.

I'm not sure if this really matters. The Symfony YAML package has good type support so people should be using the correct types. eg 0 !== '0'. false !== 'false'. At the moment, my code doesn't do any lookups so won't include this yet. It probably won't include at all because this only supplies the merged config - its not responsible for lookups. 

I haven't looked too much at the Symfony config component much, but I have considered different merge strategies. One thought I had was that you could define the merge strategy in the YAML document header. (eg. "strategy: negate") and it would remove everything that you define if it already exists which would solve the "I can't remove an array key" problem without hacking something in to the yaml document content.

There's also a few merge strategies that i've defined and use to do merges in various different places already: https://github.com/micmania1/silverstripe-config/tree/master/src/micmania1/config/MergeStrategy

--
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-dev+unsubscribe@googlegroups.com.
To post to this group, send email to silverstripe-dev@googlegroups.com.
Visit this group at https://groups.google.com/group/silverstripe-dev.
For more options, visit https://groups.google.com/d/optout.

[Conrad Dobbs]

Is it worth talking about the pros and cons of the current config system? I've always felt the current system is trying to do too much and has made compromises to do so.

Don't want to hijack the thread if the goal is just really performance based.

[Michael Strong]

HI Conrad,

I agree with you that the config system currently does too much. My intention was to scale back/turn off some features by default, especially if they degraded performance. However, because the heavy lifting is being moved to the flush process this is less important. The features that still exist in my implementation are all commonly used features such as only/except/before/after.

Of course, this doesn't talk about private statics but as Ingo mentioned this is more of a legacy feature that's still quite core to development.

On 25 August 2016 at 08:06, Conrad Dobbs <con...@webtorque.co.nz> wrote:

Is it worth talking about the pros and cons of the current config system? I've always felt the current system is trying to do too much and has made compromises to do so.

Don't want to hijack the thread if the goal is just really performance based.

--

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-dev+unsubscribe@googlegroups.com.
To post to this group, send email to silverstripe-dev@googlegroups.com.
Visit this group at https://groups.google.com/group/silverstripe-dev.
For more options, visit https://groups.google.com/d/optout.

[Sam Minnée]

Hey Michael, this looks really promising and I know that you've battled the Config system for some time, so good to see some real fruit coming.

A few thoughts:

 - 1000 times yes to topsort.php. This is exactly the kind of complex but not SilverStripe-specific service that we should push out to the packagist ecosystem.

 - Shifting Config into its own package, as you have done, will likely make it easier to clean & simplify the config implementation. However, it doesn't in and of it self simplify things. For this to be of value I think that we need to clarify what services the config package is going to provide, and confirm that SS4 will be able to operate without those. Ideally, the config API presented to SilverStripe will be relatively straightforward, and that most of the complexity will occur only within the domain of the config object.

 - I'm wary about saying that the framework should be responsible for caching, not the config library. It feels like we're shifting a potentially complex piece of the system away from where it should be. It means that you can't assess the performance of the config package without also looking at how it's integrated into a framework.

The Config API as presented to SilverStripe

Off the top of my head it will be something like:

If you'd want to have a way of accessing a whole-config object:

 - A way of saying where config files are (and their format?)

 - A way of saying which classes have their private statics read

 - A way of getting a whole-config object for a given set of special parameters (modules installed, environment type, etc)

Then you'd want an API that the whole-config object exposes.

    - A way of asking that whole-config object for config settings of a given class

        - A way of asking that class-config object for a named config setting

           - With the option to for a given config setting to be inherited from parent classes, aggregated with parent class values, or uninherited.

I think the whole-config object should conform to a nicely documented interface. This would pave the way for developers to build adapters to other config systems.

The Config API as presented within the config module

This is where you would cover items such as before/after handling of yaml files. SilverStripe doesn't need to know how these work although SilverStripe developers would. Notably, because of this you could change these systems in the config package without needing to change the SilverStripe framework code that calls it.

On some other topics...

Dealing with caching

If you have a whole-config API object, you could create a CachedConfig class implementing the same PAI that takes 2 arguments:

 - An underling config implementation (which would be another whole-config object)

 - A cache handler (a PSR-6 cache object?)

It's not clear whether the construction of this would be the responsibility of the framework or the config package, but I think that CachedConfig's code should reside in the config package.

Dealing with laying private statics and yaml config

If you created another whole-config object, CascadedConfig, which takes 1 or more whole-config objects as parameters, then you could construct something like this:

  new CachedConfig(

    new CascadedConfig([

      new YamlConfig(...),

      new PrivateStatticConfig(...),

    ]),

    new PSR6CacheHandler(...)

  );

  

The advantage of breaking out the config cascade into something that takes any old config implementation as its parameters is that you could replace YamlConfig with LaravelConfigBinding or SymfonyConfigBinding, and leave the PrivateStaticConfig component in-tact.

Private Statics are deprecated?

"Private statics for config are really just a legacy hack in my opinion, and should be replaced by components which are initialised with constructor args and setters through dependency injection parameters configured in YAML"

Given that $db, $has_one, etc are handled be the config system, I disagree with Ingo's view quite strongly here, at least for SS4. The level of refactoring needed in the ORM and Controller systems would be massive. I'm not convinced that the end result would be better for developers, and it would probably be less work to remove our own ORM and replace it with another implementation.

Long term, the idea of SilverStripe being a CMS UI that can manage content held with the databases of a variety of different MVC stacks is an appealing goal, but these are massive changes and not even close to being justified by a desire to clean up a piece of the config system.

On Thu, 25 Aug 2016 at 12:00 Michael Strong <mst...@silverstripe.com> wrote:

HI Conrad,

I agree with you that the config system currently does too much. My intention was to scale back/turn off some features by default, especially if they degraded performance. However, because the heavy lifting is being moved to the flush process this is less important. The features that still exist in my implementation are all commonly used features such as only/except/before/after.

Of course, this doesn't talk about private statics but as Ingo mentioned this is more of a legacy feature that's still quite core to development.

On 25 August 2016 at 08:06, Conrad Dobbs <con...@webtorque.co.nz> wrote:

Is it worth talking about the pros and cons of the current config system? I've always felt the current system is trying to do too much and has made compromises to do so.

Don't want to hijack the thread if the goal is just really performance based.

--
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 https://groups.google.com/group/silverstripe-dev.
For more options, visit https://groups.google.com/d/optout.

--

Michael Strong | Developer
SilverStripe
http://silverstripe.com/

Phone: +64 4 978 7330

--

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 https://groups.google.com/group/silverstripe-dev.
For more options, visit https://groups.google.com/d/optout.

--

Sam Minnée

CEO

SilverStripe Limited

[Patrick Nelson]

This has gotten fairly in-depth, but I'll chime in on a few points that resonate specifically with me in order of importance (again, subjectively to me):

1. I prefer optimizing for developer speed and comprehension, not for CPU just for the sake of speed (unless it's obviously super-slow). Speed optimizations can be persued under the hood once we make things simple, compehensible and powerful for the developer.
   
   
2. Private statics may be a necessary evil (or "hack") which I've come to terms with :) But ONLY specifically with DataObjects. Personally I see no need for them elsewhere (see #3 below). If we have to define fields, they need to be located intuitively with the DataObject that represents them. While using private statics may feel dirty, it satisfies our needs: 
• Define fields at the data object level and aren't inherited at the native level (thus no accidental override or intermingling where it's not intended)
• Works well on the flip side for "merging" fields together at the magical SilverStripe/framework level so that child DataObject's have their own abilities (and then some from parents) as defined by SS itself.
  
  
3. I hate configuring things in 2 or 3 different ways in just as many places for one framework. To me it seems arcane, but once you begin to understand how config values are evaluated, their ordering and etc... you still can hit some bugs/roadblocks For example, you end up in 3 different configuration realms/formats when you: 
   
1. Start in YAML by setting a configuration parameter as 'false' but it doesn't work because...
2. You realize in a private static it was non-falsey already so you have to then...
3. Go into procedural code somewhere (plain old PHP) to override it manually. I discovered it when simply attempting to set LeftAndMain.session_keepalive_ping to false and ended up down that rabbit hole. It confused me since I thought a bug was fixed only to find it needed to be reverted (for good reason I'm sure). 

The simpler we can keep things, the better. We can optimize for speed as well, but I think developer accessibility is more important (since that cannot change as often as the internal implementation which itself is how we address speed, usually). Laravel: What about their approach? First load by environment alias (dev, prod, etc), then merge with a global config, then load plugins. They even provide a nice dot notation for querying it, dead simple. It's been a few years now since I've dealt with it on a daily basis, but now I think it's standard/common to then utilize dotenv to load environment specific (or sensitive) information. Also... PHP is fast. Especially if you use opcache. But if you want to optimize for speed, you can then work under the hood to optimize speed. I actually like that I never had to go through a real build phase just to load my updated configuration which was, of course, stored as intuitive and easy to read, modify and highly expressive: PHP arrays.

Is there a weakness with their configuration approach as it pertains to "The SilverStripe Way?" I'm not seeing it but, I could be missing something.

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

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

--

Michael Strong | Developer
SilverStripe
http://silverstripe.com/

Phone: +64 4 978 7330

--
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-dev+unsubscribe@googlegroups.com.
To post to this group, send email to silverstripe-dev@googlegroups.com.

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

--

Sam Minnée

CEO

SilverStripe Limited

--

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-dev+unsubscribe@googlegroups.com.
To post to this group, send email to silverstripe-dev@googlegroups.com.

[Patrick Nelson]

p.s. even though I've already train wrecked the topic a bit, I'll persist a bit more in response to Conrad Dobbs:

Is it worth talking about the pros and cons of the current config system? I've always felt the current system is trying to do too much and has made compromises to do so.

Yes I agree strongly, this seems like too much. That's largely what I get out of this and, in fact, the effort to simplify should be what we pursue. I think there's some amount that the configuration can and should do (e.g. store structures of data which can vary slightly from environment to environment) and things that I think should be dictated by convention (e.g. storing/retrieving class-level configuration). I'm pretty purist on these things (maybe I'm stupid, I like things simple) but features like these IMHO don't belong in the configuration system itself:

• "Option to for a given config setting to be inherited from parent classes, aggregated with parent class values, or uninherited"
  
• I should just set/get configuration at a specific query/path ("ParentClass.Property", whatever the path to that setting may be called). If it also exists at "ChildClass.Property" then it better be there because something else put it there, not because it was imputed by some arcane logic that is not transparent to me, the developer trying to wrap his mind around a bug (or feature).
  
  
• "Rules" ... "more complex rules" ... "defining custom rules"...
• As far as I'm concerned, this should be outside of the scope of a configuration system. In web development, most of the time the only rules/variation I need are:
1. Environment variability (non-sensitive)
2. Environment variability (sensitive)
   
   
• Also on rules: "Its not easy to see how the config was built (before/after etc.) but you'd see its final state very easily" -- well, that's progress  -- but the problem of rules persists. This should maybe be outside of the scope of the configuration. That is, maybe there's a more fundamental issue here if a merging strategy is even needed in the first place (at the framework level, at least). Another "separation of concerns" point -- basically I as the developer should be unifying separate components, not letting them tamper each each other causing a race / config war. Or am I way off topic on that? 
  
  
• Minor: "Dealing with caching"
• A legitimate issue. As long as that doesn't affect the root configuration approach itself; i.e. PHP arrays vs YAML vs. [???]. I look at this as an abstraction and should be a separation of concerns (i.e. configuration can be cached, here's a caching API -- cache however you see fit).
  
  
• "... Symfony Config component and its ability to define config schemas through a fluent API, including custom merge strategies for specific nodes..."
• One thing I've not taken into account is probably defining custom merging strategies. That aside, there's a reason why I think JSON works so well since it's so much simpler. XML (which is effectively what that fluent PHP syntax looked like) just doesn't seem to match up well with most data structures that we would typically work with. I'm on the PHP array side of the camp, since it integrates pretty well with ... PHP... and you can define your types and custom rule sets outside of the configuration, if necessary (could be rare and one off, or at least it was for me). Either way -- that hack was up to me and didn't need to be a feature of the config subsystem and also didn't require learning yet another syntax. 
• This functionality vs. simplicity I think is well outlined here in this slightly biased comparison of XML vs. JSON: http://www.json.org/xml.html

[Christopher Pitt]

Ya'll make some interesting points!

About Symfony config: I detest the fluent interface for this. A complex configuration is almost as verbose as XML and not nearly clear enough.

About PHP array-based config files: I love these. Need config values to lazy load? make a callback. Want to include an FQDN? Have a use statement and ::class constant. I can't speak highly enough of the simplicity and cohesion array-based config files bring to an already complex domain.

[Sam Minnée]

We had PHP configuration in SilverStripe 2 and ironically we shifted to a non-executable approach as part of an attempt to speed things up. That is, you only had _config.php available.

The problem was that the _config.php files needed to be executed on every call and it ended up being easy to put slow code in here.

There are a few background reasons for this:

 - Configuration was mainly applied by calling static methods on classes. So you had to load a bunch of classes into memory, regardless of whether you were going to call them.

 - There weren't any anonymous functions and so lazy callback evaluation was a lot harder.

So, you could probably make a high-performance PHP-based configuration option available. In my view, deprecating all use of Yaml config would be a step too far, but if a PHP-based configuration plugin were available for the Mike's new config system, that would be very cool.

But:

>  Want to include an FQDN? Have a use statement and ::class constant.

I think this is a bad idea because it loads in unnecessary code on every pageview. It's possible you could make it performant if used in concert with lazy evaluation. Regardless, with a PHP array people can do what they want if their circumstances make it a non-issue.

--

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 https://groups.google.com/group/silverstripe-dev.
For more options, visit https://groups.google.com/d/optout.

[Sam Minnée]

A few comments on Patrick's notes:

 - Yaml should always override private statics. The bugs that prevent this from being the case should be fixed in master. The reversion of the PR that Patrick identified was only due to semver - we broke public APIs with the change :-/

 - Introducing .env as an alternative to _ss_environment.php is not a bad idea. One approach to this would be to deprecate our use of defines in favour of $_ENV variables (probably through some kind of wrapper rather than direct access to the super-global). An alternative would be to make an amended version of dotenv that set defines instead of $_ENV values. This latter approach would be able to work with SS3 as well. However, I wouldn't couple this to Michael's work on config.

 - Rhetoric about optimising for developer experience first and speed second is nice, but the fact is that config performance is mission critical. The scalability of any dynamic silverstripe app will be directly proportional to the speed of the config system. So, we need to optimise it for execution speed too.

 - Environment type shouldn't be able to be set in the config, totally agree wit this. However, it begs the question "what other circular loops do we risk tripping over?"

A notable dependency chain is this one:

  Environment => Config => Injector => Database

Implications:

 - We can't use the Injector to set up configuration

 - We can't use the database to define configuration or injector set-up

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 https://groups.google.com/group/silverstripe-dev.
For more options, visit https://groups.google.com/d/optout.

--

Michael Strong | Developer
SilverStripe
http://silverstripe.com/

Phone: +64 4 978 7330

--
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 https://groups.google.com/group/silverstripe-dev.
For more options, visit https://groups.google.com/d/optout.

--

Sam Minnée

CEO

SilverStripe Limited

--
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 https://groups.google.com/group/silverstripe-dev.
For more options, visit https://groups.google.com/d/optout.

--

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 https://groups.google.com/group/silverstripe-dev.
For more options, visit https://groups.google.com/d/optout.

[Christopher Pitt]

>  think this is a bad idea because it loads in unnecessary code on every pageview.

No it doesn't. You can include this config file without the class even existing, and it will still execute as expected. ::class is a magic constant, which doesn't require the class to be in scope.

[Michael Strong]

Then you'd want an API that the whole-config object exposes.

    - A way of asking that whole-config object for config settings of a given class

        - A way of asking that class-config object for a named config setting

           - With the option to for a given config setting to be inherited from parent classes, aggregated with parent class values, or uninherited.

I think the whole-config object should conform to a nicely documented interface. This would pave the way for developers to build adapters to other config systems.

To SilverStripe the the config is a plain PHP array so it would be hard to separate it into objects with these properties. If something like this were to exist, it would exist as part of this package and output some kind of meta data that could be consumed.

I prefer optimizing for developer speed and comprehension, not for CPU just for the sake of speed (unless it's obviously super-slow). Speed optimizations can be persued under the hood once we make things simple, compehensible and powerful for the developer.

I disagree with this. This is the current state of the config system where developers and features were put ahead of performance. When you try to do that you end up adding magic and complexity for good developer UX and it becomes unmaintainable.

This package takes both into account and I think balances correctly. 

Documentation

I've added some documentation to the git repo which visualises and talks about the architecture a bit. I'll add more detailing each transformer soon.

• Manifesto
• Architecture
• Usage
• Testing
• Glossary

On 25 August 2016 at 17:54, Christopher Pitt <cgp...@gmail.com> wrote:

>  think this is a bad idea because it loads in unnecessary code on every pageview.

No it doesn't. You can include this config file without the class even existing, and it will still execute as expected. ::class is a magic constant, which doesn't require the class to be in scope.

--

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-dev+unsubscribe@googlegroups.com.
To post to this group, send email to silverstripe-dev@googlegroups.com.

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

--

[Marcus Dalgren]

Just chiming in as a sometimes but not all that regular SilverStripe developer.
If the goal of the current config system was developer UX then boy did it fail massively.
There's few things in SilverStripe I've had a harder time with what with all the merging rules and before/after etc.
Patricks earlier example brought back some painful memories from spending hours on something that should have been SIMPLE.

I'm with Pitt when it comes to configuration through simple arrays that don't make strange calls all over the place.
I might be oversimplifying things a tad but the basic needs are that modules need to be able to override core and the application needs to be able to override core and modules.
Laravel (man that gets thrown around alot) achieves this seamlessly and I've never had any of these kinds of issues that crop up when using the SilverStripe config.
It's not as all powerful but I've never missed the SilverStripes config system when working with other systems and it feels like it creates as many issues as it solves.
I understand that considering where SilverStripe is at right now just ripping out the current system and inserting a system using simple array config is probably not feasible but ease of use and understandability should really be kept in mind.

On Thursday, August 25, 2016 at 11:13:58 AM UTC+2, Michael Strong wrote:

Then you'd want an API that the whole-config object exposes.

    - A way of asking that whole-config object for config settings of a given class

        - A way of asking that class-config object for a named config setting

           - With the option to for a given config setting to be inherited from parent classes, aggregated with parent class values, or uninherited.

I think the whole-config object should conform to a nicely documented interface. This would pave the way for developers to build adapters to other config systems.

To SilverStripe the the config is a plain PHP array so it would be hard to separate it into objects with these properties. If something like this were to exist, it would exist as part of this package and output some kind of meta data that could be consumed.

I prefer optimizing for developer speed and comprehension, not for CPU just for the sake of speed (unless it's obviously super-slow). Speed optimizations can be persued under the hood once we make things simple, compehensible and powerful for the developer.

I disagree with this. This is the current state of the config system where developers and features were put ahead of performance. When you try to do that you end up adding magic and complexity for good developer UX and it becomes unmaintainable.

This package takes both into account and I think balances correctly. 

Documentation

I've added some documentation to the git repo which visualises and talks about the architecture a bit. I'll add more detailing each transformer soon.

• Manifesto
• Architecture
• Usage
• Testing
• Glossary

On 25 August 2016 at 17:54, Christopher Pitt <cgp...@gmail.com> wrote:

>  think this is a bad idea because it loads in unnecessary code on every pageview.

No it doesn't. You can include this config file without the class even existing, and it will still execute as expected. ::class is a magic constant, which doesn't require the class to be in scope.

--
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 https://groups.google.com/group/silverstripe-dev.
For more options, visit https://groups.google.com/d/optout.

[Patrick Nelson]

Re: Michael Strong:

I prefer optimizing for developer speed and comprehension, not for CPU just for the sake of speed (unless it's obviously super-slow). Speed optimizations can be persued under the hood once we make things simple, compehensible and powerful for the developer.

I disagree with this. This is the current state of the config system where developers and features were put ahead of performance. When you try to do that you end up adding magic and complexity for good developer UX and it becomes unmaintainable.

True -- but core to my point of simplicity, you automatically get speed for free. If it's not face, it's simple to optimize for speed because: simple.

 

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

[Patrick Nelson]

Sorry, not to over simplify. I've reviewed your manifesto and I'm 100% on board with that. Ultimately you have to actually document an actual process (maybe have a legitimate RFC for folks to hash out the details) and then build out an implementation so, I respect your effort on that for sure. I just think we're all (or speaking for myself at least) a bit jaded working with configs here in SS. Whatever solution we work on should be a singular and unified solution. After reviewing the architecture I think core nitpick points might be:

• Initial entry point "transformers" (or really just base configuration arrays, IMHO) could be in different formats. YAML, PHP, JSON, etc. 
• Variable/modifiable merge strategies There's yet another transform stage. 

I think maybe it'd help to have a simple merge strategy based on the simple concept that whoever's last, wins. That means that you then need to ensure that each [thing] (whatever that is) which wants to set a particular configuration parameter at a particular location needs to make damned sure it's minding it's business. Do we have any good examples, at least, of any complex situation where these merge strategies (beyond "whoever is last wins") are really needed? If so, I wonder if that concept of "mind your own business" would apply better there. I'm totally up to be proven wrong here and, if so, maybe then the config system DOES need all this complexity. That's why I think that you should really just have two ways to define your core configuration array/data structure:

• PHP arrays (like Laravel)
• Procedural (like Laravel and like we already do, sort of)

ENV  (like dotenv) is different and just runs once and before the main configuration array is generated so it can import necessary data from $_ENV. But, thankfully dotenv "minds its own business" and just does what it's good at. Our interface is thus quite simple and easy for the developer to grok and thus we have a convention that we recommend (for good reason) but it's up to you to follow or break it.

[Sam Minnée]

Everyone agrees that the config system is more complex than they would like. But why did it turn out that way?

The key reasons are that the config system sought:

 - replace a diverse number of config-like structures inside SilverStripe.

 - retain a reasonable level of compatibility with those previous structures

It's insufficient to say "let's have simplicity": we need to decide how we'll address that challenge differently. Most likely, it will involve a bigger compatibility break in order to allow for a simpler config engine.

So what features does the config system have? Can we get rid of any of these?

Initial values provided in private statics

I touched on this previously, but $db, $has_one, $allowed_actions, $url_handlers, and others make up a key part of the ORM and Controller system. Deprecating private statics as the main way of defining such values would mean that your models and controllers are confusingly scattered between different files, and in my view would be a step backwards. Previous discussions about how to improve on this have drifted towards annotations and the like, which wouldn't be of any help in the march toward a simpler config system.

On the other hand, do we need to use the config system to manage these variables? Could we just use private statics and ignore the config engine? That's what we had in SS2 and it is probably a more feasible, but the ability to amend db in a config yaml provides a much more lightweight alternative to something that would otherwise require a DataExtension, and under the hood the DataExtension / DataObject resolution will require something very much like the config system, so I think we'd end up maintaining two config engines, which isn't of much help when trying to simplify.

So, in my view, the default config values being provided by private statics is a feature that we need to retain. That said, it should be easy to override config options, in particular with falsey values.

After/Before

The after/before system is more important than in other config systems because it's important that modules can provide changes to config, that that subsequent modules and/or project code can make calls as to whether their configuration supercedes or is superceded by other that of other modules. Any system that assumes a clear delineation between "the app", which is the arbiter of all configuration and "modules" that simply provide services to be configured, wont' have this problem, and I wonder if this is where some of the simpler example are coming from.

Order indexes are an alternative to after/before, and were considered in the original design. Myself, I find after/before to be a more natural way of specifying the ordering of a number of loosely coupled items. I guess I could be in the minority here, but I wonder if something else is at play.

One issue may be that the configuration is a little ambiguous sometimes about whether 'after' means supercedes or 'before' means supercedes. After means "added to the configuration after" and so would overwrite values. But the configuration is being aggregated into an array, and the first element of the array is being used, then configuration entries added *before* would end up taking precedence. Director::rules is probably the most common example of this.

It's not obvious what the best solution to this is. One option, for example would be to prepend new director rules to the start of the rule list, meaning that rules added later in the configuration assembly would be evaluated first. Another option is to keep the multiple definitions of configuration rules such as Director::rules separate, and evaluate each module's set of rules, last-provided to first-provided.

However, it's worth noting that using order indexes instead of after/before would be of no help.

The other thing that seems to have caused confusion with after/before is "after: *" or "before: *". Perhaps some kind of "is-first" and "is-last" marker would be a better way of expressing these. Or, at least, ensure that they are internally treated this way.

Different merge rules

The configuration system doesn't just let you define scalar values. It also lets you define maps (some of which are order-dependent, some of which are not) and lists.

 - Director rules

 - DB fields

 - Allowed actions

The config system has two support methods: merge_low_into_high and merge_high_into_low. I think that this can be simplified with a pre-cached merge that happens in 1 direction.

However, the merging of order-dependent maps will need to be clarified. I assume that Director::rules is a pain point here, and by extension RequestHandler::url_handlers. Does anyone know of other instances where this causes problems?

Inheritance merging

Configurations also get merged from parent classes, in a number of different ways:

Config::INHERITED - the default option

Config::UNINHERITED - use by db/has_one/has_many/many_many to prevent fields from being redefined on child classes

Config::FIRST_SET - inherit to subclasses but don't allow redefinition. Use in LeftAndMain config options, ViewableData::default_cast,  It's not clear why just using inherited wouldn't be acceptable here.

Config::EXCLUDE_EXTRA_SOURCES - used by the 'allowed_actions' and 'extensions' properties to ensure that values from Extensions are incorporated with appropriately.

The most radical approach here would be to do away with configuration inheritance altogether. I haven't thought this through entirely but perhaps that would actually work? The main places where inheritance is a critical part of configuration (db/has_one, url_rules, allowed_actions, etc) already have specialised code to walk the ancestor list in specialised ways. Perhaps a simple helper function that can get inherited values can be used iff it's needed.

Extensions

The last item above touches on a point: Extensions have the ability to amend the configuration of classes to which they are applied. The most common use of this is for a DataExtension to alter the database schema by amending the db/has_one/etc configurations.

This functionality is required but it could be handled by having DataObject say "give me the db property of the current class and that of any extensions applied to it" rather than relying on the config system to do this magically. In addition to db properties, allowed_actions is the other place where this gets used. So perhaps we could do away with this, which would mean we could drop the EXCLUDE_EXTRA_SOURCES option too.

This would also resolve the circular dependency of configuration defining new extensions to apply that in turn add configuration entries.

-----

TL;DR

I think that any new config system will have to support:

 - config default values coming from private statics

 - after/before and some kind of is-first/is-last

 - some way of merging arrays & maps so that the intuitive order is provided

But we can likely drop:

 - Config amendment by extensions

 - Config inheritance from parent classes

Michael: how does this fit with your plans?

[Michael Strong]

I was originally planning on dropping features which added performance issues, but so far I've implemented most of the features I expected to drop in a way that only affects flush.

The two items you mention dropping haven't been done yet, however I doubt we'd be able to drop inheritance. Most config calls use/expect inheritance so dropping it would mean working everything out on the fly (at runtime) which is what i'm trying to avoid and what it currently does anyway.

Also, I think removing static config from extensions is the same as removing it from standard PHP classes. The ease for the developer is taken away, however having thought about how to solve this, its likely to be on the features that introduce a performance issue.

I want to carry on implementing the config system as-is because its been quite valuable in learning which parts are responsible for its complexities and why. I can carry on with development and report back like i've done so far, but I'll keep in mind that some features might be dropped and build accordingly.

The next thing I think is to try and get some fair performance tests for the work done so far.

--

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-dev+unsubscribe@googlegroups.com.
To post to this group, send email to silverstripe-dev@googlegroups.com.
Visit this group at https://groups.google.com/group/silverstripe-dev.
For more options, visit https://groups.google.com/d/optout.

[Ingo Schommer]

Great discussion! I can understand the frustrations with understanding configuration rules, been there myself. 

We might not necessarily have to change core APIs to fix that though, a lot of this comes down to transparency during development:

- A debug view showing the "final" config tree (as cached by the system). We'll have to add a disclaimer that it excludes any execution path specific Config::inst()->update() calls though.

- Tracing from which YAML file or PHP class a particular value in the config tree is set

- Logging the lookup chain on a specific Config::inst()->get() call (e.g. making it clear that a class static fallback is used)

- A debug view of the dependency tree for different config blocks (might be impossible to make this "readable" without lots of effort?)

- Provide hints on how conditionals like moduleexists are resolved.

Michael, are the above realistic in the new system?

Another factor which hasn't been mentioned here: Most project configuration overrides would be fine with a simple core->modules->mysite merging structure. But at the moment we don't have a unified system to order configuration merges based on "folder priority", outside of the before/after notation. At the same time, different parts of SilverStripe still rely on folder structure: ThemeManifest does this alphabetically, with a special case for project() (usually "mysite"). Hence the infamous "zzz_templateoverride" modules you see in SilverStripe projects. Is there still a case for a general "module priority" system? From what I can tell, the new cascadable themes feature in SS4 doesn't change the alphabetised fallback, but gives devs an option to be explicit about ordering in YAML. There's no docs on the pull request and a lot of evolving discussion so a bit hard to tell.

So while the before/after system is needed for some modules, the average SilverStripe dev shouldn't need it as often. It looks like the "folder priority" is already a fallback in Michael's solution: If two config blocks don't have before/after statements, they'd be merged based on the order in which the folders have been traversed (alphabetically?). Maybe if we made that fallback more predictable (choose project() last) devs would need to worry less about before/after?

Ingo Schommer | Solutions Architect
SilverStripe (http://silverstripe.com)
DDI: 04978 7330 ext 4422

Mobile: 0221601782
Skype: chillu23

[Chris Joe]

The discussion has grown quite a bit, lots of ideas thrown in.

I have run into instances where extension execution order turned into a strong pain point... I think it was with the advanced-workflow tweaking the behaviour of framework and then clashing with some custom behaviour that a client wanted at the same time.

It was pretty convoluted with how to resolve it, and not even obvious that the root cause of the problem was by config order.

I agree that the two "likely to be removed" items listed are good candidates. They aren't very clear to begin with, but they could be sort of solved with the point below.

It would definitely ease developer time with a trace of where the config came from and a "final view" of the config settings.

The moving private statics away from Classes is a logical step but the feel of scattering configs into even more files is not very appealing. I also like that private statics are very clearly being overridden by config files, it's not so clear for config files being overridden by another config file.

[Sam Minnée]

So while the before/after system is needed for some modules, the average SilverStripe dev shouldn't need it as often. It looks like the "folder priority" is already a fallback in Michael's solution: If two config blocks don't have before/after statements, they'd be merged based on the order in which the folders have been traversed (alphabetically?). Maybe if we made that fallback more predictable (choose project() last) devs would need to worry less about before/after?

 

Or you could just put "after: *" in all our sample/default config files. Or "order: last" as a synonym if that's more palatable. I'd prefer that over special magic for the project() value.

[Sam Minnée]

The two items you mention dropping haven't been done yet, however I doubt we'd be able to drop inheritance. Most config calls use/expect inheritance

Do they really? I had a look through and couldn't really see where this was important. I'm wondering if this is just one of those areas where we've all gone "they're classes, of course inheritance is needed!" and haven't really thought through whether it's used.

For areas such as allowed_actions / db / has_one, it's not using inheritance, it's using something more abstract and complex.

In the case where you have build a subclass that replaces a parent class, and you want to grab all it's config. This kind of monkey-patching is a bit messy at the best of times, and perhaps you want to copy 

What are some examples of config settings that would be very confusing if inheritance was completely removed?

Alternatively, if the config inheritance was only used in a few places, would it be better to enable it with an explicit marker? Something like:

Page: 

  inherit-config: SiteTree

If it was broken out like this you could also use it in more flexible ways, such as allowing multiple inheritance. "copy-config-from" would be another way of re-labelling the setting here.

so dropping it would mean working everything out on the fly (at runtime) which is what i'm trying to avoid and what it currently does anyway.

I was suggesting that it was removed entirely.

[Sam Minnée]

Also, I think removing static config from extensions is the same as removing it from standard PHP classes. The ease for the developer is taken away, however having thought about how to solve this, its likely to be on the features that introduce a performance issue.

My suggestion was more that the application of extension configuration to base classes was done outside of the config system, and potentially that it was only done on the core ORM / controller properties.

In any case, I think it definitely has to be done after the config is compiled, since extensions can be applied by the config system. Otherwise you get circular references.

It doesn't necessarily need to be done at run time: you could potentially cache the result config + extensions, but it would get more complex.

[Michael Strong]

What are some examples of config settings that would be very confusing if inheritance was completely removed?

I was just under that impression because Config->get() defaults to Config::INHERITED. Any use out in the wild in modules and project code assumes inheritance (eg. $this->config()->my_var).

I hadn't actually considered how the database and allowed actions config worked in that respect. I did a quick grep to try and get some numbers but i'm not sure how helpful they are:

In framework there's 43 uses of Config::UNINHERITED and 28 uses of Config::FIRST_SET which I believe uses inheritance. 

However, there's 479 uses of private static excluding db/has_one/allowed_actions etc. (820 including)

grep 'private static' framework/ -ri | grep -Ev '\$(db|has_one|has_many|many_many|allowed_actions)' | wc -l

There was only 5 instances of EXCLUDE_EXTRA_SOURCES and all of the above includes docs too... because i'm too lazy to redo them.

YAML Inheritance

In regards to the multiple inheritance comments there is some way for YAML already to re-use yaml values in the spec. I'm not sure whether symfony/yaml supports it and I don't think we'd be able to use it across files or with complex structures anyway. There's some details here: http://yaml.org/spec/1.2/spec.html (see 2.2 Structures)

Alternatively we could just add an 'extends' header but this adds quite a lot of complexity. It does keep the meta data in the header and out of the content though.

Performance

I setup a little performance test comparing the current SS4 'flush' vs my package. I had to disable caching in SS4 to make it as fair a test as possible (writing to disk would be slow).

It should also be noted that my package does merging of YAML documents whereas the current SS4 doesn't.

I ran both tests looped 20 times and the results were recorded with xhprof:

I don't really want to read into these at the moment as the performance gain i'm trying to achieve is at runtime, not flush and I'm not yet in a position to test that.

--

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-dev+unsubscribe@googlegroups.com.
To post to this group, send email to silverstripe-dev@googlegroups.com.
Visit this group at https://groups.google.com/group/silverstripe-dev.
For more options, visit https://groups.google.com/d/optout.

[Loz Calver]

Lots of great ideas in here, I’ve been lurking and trying to keep track of everything - as other’s have said we have a lot to discuss here, so perhaps we’ll need to break up the discussion a bit at some point to keep it manageable!

Firstly, just to get it off my chest (sorry):

"I don't like YAML"

I know some people out there don't like the yaml format, but I don't want to get into a discussion about that here. It remains that the entire SilverStripe ecosystem relies on it and there's already a vast amount of changes coming in SS4. If you don't like yaml, this package might be the first step to replace it but that is not its intention.

This was totally aimed at me :D. I’ve never really been very keen on it - I don’t like the awkward space rules, or the lack of support for non-scalar/list values (%$ServiceName anyone?). We have to hack in support for “blocks” because no parsing libraries support it, and I think it introduces unnecessary complexity when it comes to parsing it out. I’d much prefer a PHP array-based system (Config::update() obviously doesn’t cut it currently) - even if it were just a plugin of some description for Michael’s library, as Sam suggested. There would still be problems to solve (how to integrate ordering/priority for example), but I don’t think any of them would be insurmountable.

If we were building the config system entirely from scratch now, ignoring backwards compatibility, would we still choose YAML? Of course we can’t just drop YAML immediately, but if the answer to that question is “no” then I think it deserves attention as a long-term goal.

---

Back on topic:

Inheritance / PHP statics

"Private statics for config are really just a legacy hack in my opinion, and should be replaced by components which are initialised with constructor args and setters through dependency injection parameters configured in YAML"

 

Given that $db, $has_one, etc are handled be the config system, I disagree with Ingo's view quite strongly here, at least for SS4. The level of refactoring needed in the ORM and Controller systems would be massive. I'm not convinced that the end result would be better for developers, and it would probably be less work to remove our own ORM and replace it with another implementation.

The relationship between config/PHP statics is something I remember being confused about when I first started using SilverStripe. Config values don’t necessarily have a matching PHP equivalent class/property, and sometimes private statics aren’t used anywhere in config - it’s not immediately obvious to developers what the connection is between the two, and whether config behaves like PHP would natively or if it makes PHP behave strangely. The Config class makes a number of references to PHP classes/inheritance, so it looks like it was originally intended for config values to map directly to a private static.

I partly agree with Ingo’s point about private statics being a “hack”. It sometimes feels like we’ve added private statics because:

• Developers don’t have simple enough access to an object to set something at an instance level, or;
• We feel we have to - as above, config is sorta tied to statics, so setting something in YAML without setting a PHP equivalent “feels” wrong

I definitely think that moving $db, $has_one etc out of the DataObject classes and into YAML would be wrong, so if we moved them (or other values) out of config and just treated them as instance properties, what problems would we encounter? The only (read: huge) problem I can think of off the top of my head is how we’d allow modules/user code to influence that data easily.  I can’t think of a solution that doesn’t involve an extra PHP class as an “extension” of some sort, so perhaps this is a problem we can’t solve without compromising developer experience (and therefore shouldn’t) :(

(I’ve just read one of Sam’s other posts after typing this, and it seems he came to the same conclusion!)

After/Before

I agree that we’ll need to keep this, and by extension some kind of “block naming”. We can’t rely on a module’s directory name as one module may have multiple “blocks” that need to be merged in different places/with different priorities. No modules spring to mind that do this, but I’ve definitely written “mysite” code that does. Going back to a PHP-only implementation mentioned above, it would need to support this somehow.

I think that the current before/after naming convention does make sense for setting priorities. The only place that it doesn’t make sense IMO is extension hooks - extensions merged into config “last” will actually have their extension hooks called first - https://gist.github.com/kinglozzer/b81ca4eb04ea42b56e78. We could array_reverse() or something to sort that, but that would obviously break backward-compatibility.

Anyway, I’m getting distracted. tldr; I agree with keeping this, and can’t see any way around it without changing how SilverStripe and the module ecosystem work at a fundamental level.

Inheritance merging

I wrote a huge wall of text filled with examples on this, but a lot of it had already been covered so here’s the short version:

I’m in favour of removing Config::INHERITED, or at least changing the default to something else. I think that change would make the config system more closely match how PHP private statics would natively work, and would likely improve performance (though I don’t know by how much). As mentioned, $db, $allowed_actions and co already have code in place to handle inheritance themselves, and it wouldn’t be hard to implement it in other places. Alternatively, we could keep the code but make Config::UNINHERITED the default?

Extensions

This functionality is required but it could be handled by having DataObject say "give me the db property of the current class and that of any extensions applied to it" rather than relying on the config system to do this magically. In addition to db properties, allowed_actions is the other place where this gets used. So perhaps we could do away with this, which would mean we could drop the EXCLUDE_EXTRA_SOURCES option too.

The only other thing I can think of besides those examples is possibly $casting, but that shouldn’t be too hard to fix up if needed. +1 on removing that.

Other notes

YAML Inheritance
In regards to the multiple inheritance comments there is some way for YAML already to re-use yaml values in the spec. I'm not sure whether symfony/yaml supports it and I don't think we'd be able to use it across files or with complex structures anyway. There's some details here: http://yaml.org/spec/1.2/spec.html (see 2.2 Structures)
Alternatively we could just add an 'extends' header but this adds quite a lot of complexity. It does keep the meta data in the header and out of the content though.

Perhaps others disagree, but I think this would add too much complexity. The extends header would be much nicer than the weird syntax for repeated nodes, but would the code burden be worth it?

 

- A debug view showing the "final" config tree (as cached by the system). We'll have to add a disclaimer that it excludes any execution path specific Config::inst()->update() calls though.
- Tracing from which YAML file or PHP class a particular value in the config tree is set

These two would be hugely helpful. Given that a value can be set and overridden in multiple places, perhaps you could click on a value and it would list each of the sources in priority order. I’m dreaming here...

Loz

[Nicolaas Thiemen Francken - Sunny Side Up]

Hi,

TL;DR: current system is pretty good, but it could be a bit faster / simpler with better management tools.

Just to add a not-so-technical, not-so-knowledgeable voice:

Strengths:

a. yml is easy to read

b. one system for all modules

c. clear separation between config and code

d. easy to add / change config

e. ability to set different configs for separate environment (e.g. test / live / but also other environments)

f. yml forces you to keep it simple (if we had php then there is a risk you make it too complex)

Weaknesses:

a. speed is everything. I dont care about a ?flush=all load, but after that it should be fast as a tesla. I gather the current config system is a little sluggish

b. lack of central tool: it would be great if there was a tool that would allow you to explore / discover the total config set and options, as well as update them (right now it is really scattered and you can spend a lot of time finding where things are)

c. the headers with stuff like before / after are too hard to understand and too difficult to implement properly, so most of the time I try to do this different ways.

d. arrays are tricky: it is annoying if you want to add an item somewhere in the middle of a non-associative array (e.g.  in grouped-menu-items, if you want to put a menu item between two existing sub-nodes, you have to jump through crazy hoops) or remove an item from an array

e. I dont care about advanced features about $this->Config()->foo_bar and other "magical" shortcuts and would be happy to write Config::inst()->get('Class', 'Var') every time. I'd rather have one method as this simplifies things in terms of coding and makes it easier to upgrade if we choose a new config system. 

f. debugging can be a difficult at times. 

Cheers

Nicolaas

​

[Florian Thoma]

I have been trying to follow the discussion and there are certainly parts I don't really understand. But I would like to throw in my thoughts anyway.

If/when the config system gets an overhaul I would like to see a possibility to have a environment config for the new system. If the system stays on yml, this would be a file `_ss_environment.yml` that lies in or above the web root like the `_ss_environment.php` does at the moment.

The reason for this is that I would like to be able to keep e.g. API keys and passwords for payment gateways etc out of the code base.

At the moment, to have environment specific values, you have stuff like
Only: environment: 'live'
in e.g. mysite/_config/config.yml. This means the API keys and even passwords for certain things are in the project code base.

At the moment I use a modified version of https://github.com/silverstripe/silverstripe-framework/blob/master/core/Constants.php#L37 in mysite/_config.php to load a custom PHP config file where I use Config::inst()->update() to add/modify the settings for the environment. Not the nicest solution, but it works.

Any thoughts on that?

[Damian Mooyman]

I think that's an excellent idea Florian. It would fit a lot better into a "single source of truth" pattern, although I doubt we'll ever see the last of _ss_environment.php.

~ Damian

[floria...@innoweb.com.au]

Hi Damian,

 

I agree. With both. ;)

 

Cheers, Flo

--
You received this message because you are subscribed to a topic in the Google Groups "SilverStripe Core Development" group.
To unsubscribe from this topic, visit https://groups.google.com/d/topic/silverstripe-dev/q5khashNiuY/unsubscribe.
To unsubscribe from this group and all its topics, send an email to silverstripe-d...@googlegroups.com.
To post to this group, send email to silverst...@googlegroups.com.

[Michael Strong]

Hi All,

Our monthly hack day at SilverStripe in September saw my first attempt at integrating the new config package with SilverStripe 4. This was not an in-depth integration - just generating the config, printing it out and then exiting.

When doing so I came across a few strange issues but didn't really have a way of debugging. That led me to my next piece of work which I worked on as part of October's hack day on Friday. This was adding the ability to audit the config to give an insight as to how the config was being generated and where config keys came from.

This resulted in the first refactor to make auditing easier, but also keeping in mind the need for caching later on. I've left the thinking behind the refactor in a pull request so that there's a record of why the change was made. I've left my reasoning in a PR for posterity: https://github.com/micmania1/silverstripe-config/pull/1

To unsubscribe from this group and all its topics, send an email to silverstripe-dev+unsubscribe@googlegroups.com.
To post to this group, send email to silverstripe-dev@googlegroups.com.

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

--

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-dev+unsubscribe@googlegroups.com.

To post to this group, send email to silverstripe-dev@googlegroups.com.

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

[Florian Thoma]

Hi Michael,

What is the status of this? Is this going to go into SS4?

Cheers, Flo

[Michael Strong]

Hey Flo,

I've been integrating it with SilverStripe and I have it pretty much working. Still got some work to do to performance test it and there's a few things not quite there yet but making good progress.

Whether it will be in SS4 or not? I'm not sure but that's i'm hoping.

I'm using PSR-6 cache in my implementation so this is probably blocked by the PSR-6 RFC anyway.

--

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-dev+unsubscribe@googlegroups.com.
To post to this group, send email to silverstripe-dev@googlegroups.com.
Visit this group at https://groups.google.com/group/silverstripe-dev.
For more options, visit https://groups.google.com/d/optout.

[Florian Thoma]

That sounds great. Let's hope it goes thorugh to 4!

Have you also had a look at my suggestion https://groups.google.com/d/msg/silverstripe-dev/q5khashNiuY/QlEYSCU1BgAJ? That's something I would highly welcome...

Cheers, Flo

On Wednesday, 16 November 2016 17:39:11 UTC+11, Michael Strong wrote:

Hey Flo,

I've been integrating it with SilverStripe and I have it pretty much working. Still got some work to do to performance test it and there's a few things not quite there yet but making good progress.

Whether it will be in SS4 or not? I'm not sure but that's i'm hoping.

I'm using PSR-6 cache in my implementation so this is probably blocked by the PSR-6 RFC anyway.

On 15 November 2016 at 15:51, Florian Thoma <floria...@innoweb.com.au> wrote:

Hi Michael,

What is the status of this? Is this going to go into SS4?

Cheers, Flo

--
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 https://groups.google.com/group/silverstripe-dev.
For more options, visit https://groups.google.com/d/optout.

[Michael Strong]

Hey Flo,

Sorry for the late reply and I hope what I'm about to say isn't gibberish!

I think SilverStripe is already over-dependent on the config system. Its already so entrenched to the point that SilverStripe core components can't function without it which is a bad position to be in in an ever-changing ecosystem.

The addition of _ss_env.yml would create a very low-level dependency on the config system and I can't see how it offers any more value than the existing _ss_environment.php.

I have often seen API keys and such stored in YAML but this poses a major security risk. The obvious one being if somebody were to access your codebase. However, if you're using a shared host its not uncommon for permissions to be set incorrectly on the /tmp directory where SilverStripe config gets cached, potentially giving read access to any random website that happens to be hosted on the same server. 

You should always store these keys securely and load them through your environment config - NEVER store them in yaml or in private statics.

I've uploaded an example showing how i've dealt with API keys in past. The example notifies our slack channel upon failed deployment: https://gist.github.com/micmania1/96b0b98ad350a8c8fc2dbee172d54c44

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

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

[floria...@innoweb.com.au]

Hi Michael,

 

I agree with everything you say.

 

We are now dependant on that config system. There will always be some sort of a config, whether that’s private statics, constants, yml or XYZ doesn’t really matter.

 

Your example works fine if you develop all modules yourself, but we need a way to handle API keys for other modules too, like payment etc.

 

We know, that a lot of modules use the yml config for the API keys.

We also know that the API keys can’t be in the code base of the project.

And we now also know that just adding a env.yml thing is not secure either.

 

As I mentioned before, I currently use a modified version of https://github.com/silverstripe/silverstripe-framework/blob/3.4/core/Constants.php#L36-L77 in mysite/_config.php to load a custom PHP config file where I use Config::inst()->update() to add/modify the settings for the environment. Not the most performant solution, but it works.

 

What would be your suggestion to solve this?

 

Cheers, Flo

 

[Michael Strong]

My advice would be to submit a PR to the offending modules - they should also rely on constants or provide setter methods (they're still a thing!).

For now, update seems like a the best option. If its just replacing strings then it shouldn't introduce any performance issues.

--

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-dev+unsubscribe@googlegroups.com.
To post to this group, send email to silverstripe-dev@googlegroups.com.
Visit this group at https://groups.google.com/group/silverstripe-dev.
For more options, visit https://groups.google.com/d/optout.

[floria...@innoweb.com.au]

Thanks, Michael! ;)

[Damian Mooyman]

Hello, I have written up an RFC regarding gathering user-feedback on the proposed user-visible config API.

Please see https://github.com/silverstripe/silverstripe-framework/issues/6611 and leave your vote and/or comments.

Note that this RFC only covers the API surface, and does not include in-depth architectural discussion. (e.g. regarding how caching works). We only want to ensure that the new config API remains useful for developers in 4.0.