Skip to content

Feature/config #1244

Merged
merged 10 commits into from Mar 12, 2013

5 participants

@hafriedlander
SilverStripe Ltd. member

Some changes to the Config system

  • Merged versions are cached in an intra-process in-mem LRU cache
  • Statics are extracted pre-execution via code parsing a-la the class manifest

This is a slight API change. Statics used as config value defaults are now considered immutable after declaration, and any assignment to them will be ignored.

@hafriedlander hafriedlander referenced this pull request in silverstripe/silverstripe-cms Feb 27, 2013
Merged

FIX CMSMainTest to access batch_action config property properly #295

@wilr
SilverStripe Ltd. member
wilr commented Feb 28, 2013

Slight API change enough to warrant going into 3.1 after 2 betas slight?

@sminnee
SilverStripe Ltd. member
sminnee commented Feb 28, 2013

The underlying issue is that the absence of this patch causes quite serious performance regressions when compared to 2.4.

@hafriedlander
SilverStripe Ltd. member

TLDR; It's pretty slight. I think it should be OK. We might want to do a Beta 3 - Sam?

The basic change is that before, some code might do this to update a config value:

Foo::$some_var = "new value";

That used to work or not work depending on when that code was run & what the config value was. For instance this:

Page::$db = array( .... );

would either work fine or cause errors (or maybe even database corruption) depending on when it's executed.

Now code like that will always be a no-op. You can do anything you like to any static and it won't matter because the static has been extracted before code execution happened.

In master I'll introduce a bigger API change which will deprecate having config values with statics that aren't marked private. That'll make attempts to change config statics throw an error instead of just work silently.

I'll also introduce a freeze method, so that the database layer can do Page::config()->freeze('db') and then any attempt to even do Page::config()->db = .... will cause an error

@chillu
SilverStripe Ltd. member
chillu commented Mar 11, 2013

Can't really say much about the implementation, it's over my head ;)
Hamish, given this should get some exposure before we're getting close to 3.1 final,
I'll recommend that you merge yourself, or get somebody with more insight into the config system to have a look.
But it does need a mention in the upgrading docs first.

@sminnee
SilverStripe Ltd. member

Is the loss of caching here okay due to other changes?

SilverStripe Ltd. member

Answered my own question; looks good.

@sminnee
SilverStripe Ltd. member

I can't see any tests for this? It's the only part of the code that really makes my eyes glaze over; it seems like a pretty key candidate for some unit tests.

@simonwelsh

There's also a test that's currently being skipped (I think it's a MySQL-specific one), since it relies the db config being immutable. Would be good to get that test re-enabled :)

@sminnee
SilverStripe Ltd. member
sminnee commented Mar 12, 2013

Alright I'm going to merge this and update the upgrade docs.

@sminnee sminnee merged commit 362ca9b into silverstripe:3.1 Mar 12, 2013

1 check failed

Details default The Travis build failed
@chillu
SilverStripe Ltd. member
chillu commented Mar 12, 2013
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Something went wrong with that request. Please try again.