Config System & Traits

[Zauberfisch]

The config system does seem recognise traits.

trait Foobar {
    private static $hello = 'World';
}
class Foo extends Object { use Foobar; }
class Bar extends Object { use Foobar; }

so, the way I see it, both Foo and Bar now the following statement should be true:

Config::inst()->get('hello') === 'World'

but currently is not.

and since statics configs are now also managed by the config system, the same counts for $db, $has_one, ....

[Zauberfisch]

also, let me note: of course I am perfectly aware of the existence of Extension, yml configs and Config->update(), and you can accomplish the desired results of the above example with any of the three, I still would like traits to be picked up by config

[stevie-mayhew]

There is a great trait-loader here: https://git.simon.geek.nz/simon_w/trait-loader

Its available through packagist, and is also listed on addons.silverstripe.org site. It would be great if it could be merged into framework by default. There is an issue opened here about it: https://git.simon.geek.nz/simon_w/trait-loader/issues/3

[dhensby]

Unfortunately traits are a 5.4 feature and SS minimum PHP version is 5.3, therefore we can't include trait support out of the box (at least not the trait-loader module as is). It'd need to be able to handle PHP versions that don't support traits and skip over them.

[stevie-mayhew]

So we can add something as long as we check for PHP versions before doing anything? Sounds alright.

I know its a topic for elsewhere, but PHP 5.3 has been unsupported for 3 months now - we really need a plan to move to 5.4+.

[Zauberfisch]

yeah, if you ask me 5.3 should have been dropped a while ago.

Our servers are on 5.5 right now, and even clients that have their own hosting have moved away from 5.3 a couple of month ago.

[hafriedlander]

We can adjust the required PHP version in 4.0, but we can't change requirements in the middle of a minor release - especially now we're looking to adopt semver. In the meantime it's good to have a module that can add support. @stevie-mayhew does that module support static integration like @Zauberfisch is talking about? It doesn't look like it does from a quick scan.

[dhensby]

So we can add something as long as we check for PHP versions before doing anything? Sounds alright.

I wouldn't check for versions exactly, checking to see if T_TRAIT was defined would be a start considering the trait-loader module.

I know its a topic for elsewhere, but PHP 5.3 has been unsupported for 3 months now - we really need a plan to move to 5.4+.

You're right, there does need to be a plan to move to 5.4, but PHP 5.3 is still the version that ships with RHEL 6 and if we want SS to be successful in a corporate environment, then I think we need to support that version of SS, at lease with a LTS version of SS... (thought that's just my personal view).

[dhensby]

Seeing things like 5.5 or 5.6 in production is pretty unrealistic in my experience.

Even clients that don't run RHEL tend to have servers they've had for a while and don't have newer versions of PHP.

To get away with something like PHP 5.5 in production you have to be provisioning new servers and whilst that's quite feasible for smaller companies, big corporates with IT teams aren't as easy to move.

[hafriedlander]

@dhensby I generally agree. We should be testing to make sure SilverStripe works on the newest PHP versions, and ideally supporting new language features when possible, but making one a requirement isn't really practical - one of the advantages of PHP is it's ubiquity, and requiring a still-uncommon version restricts that. Having said that, a reasonable metric for a new major SilverStripe release is "the version available in the latest Debian stable / RHEL", but I think constraining ourselves to the version available in a previous stable release is probably going too far (RHEL 7 is out, and contains PHP 5.4)

[Zauberfisch]

well, in truth we have php configured per vhost. but all of them run 5.5 at this time.

I can appreciate the point that we have to consider slower moving IT infrastructures, but I am not sure I approve holding back on features to support a version of php that has officially reached its end of life.

but if there is a practical way to satisfy both ends (eg in this case checking if traits are supported before trying to handle them), I am obviously happy to accept that and keep the old version supported

[stevie-mayhew]

@hafriedlander there is no static support with the trait loader as is, its something which we would have to look at as part of an upgrade.

Sorry for opening a can of worms with my comments about version support - this is a topic that should be handled on the mailing list, not here.

I think that at the moment the trait-loader from @simonwelsh is a stop-gap measure, but we should look to implement it fully very soon in the future, definitely before a release which supports PHP 5.4.

[dhensby]

@hafriedlander I generally agree with you too.

As RHEL 7 is now out and 5.4 is the release with that, I think dropping 5.3 support is very realistic for the near to mid term. But I don't think it should be considered as immediately as perhaps is being suggested here.

[stevie-mayhew]

I'm not suggesting immediately moving to 5.4 - its something which needs to be taken into consideration and a plan for it to happen. Currently with the action happening around semver we're obviously going to be in a better position to discuss this in 1-3 months time after 3.2 has landed. Thanks for the notes everyone, I'll have a look at implementing trait loading with the suggestions noted by @hafriedlander

[kinglozzer]

Just had a quick look, master now seems to support loading traits and private statics from them (perhaps the PR for replacing the config static parser with reflection enabled that?). The only problem - though not a bug - with using traits to augment config values is that you can’t overwrite/merge things, as they obviously don’t work like extensions do:

trait MyTestTrait {
    private static $db = array(
        'TestField' => 'Varchar(255)'
    );
}

class Page extends SiteTree {
    use MyTestTrait;

    private static $db = array(
        'AnotherField' => 'Varchar'
    );
}

Fatal error: Page and MyTestTrait define the same property ($db) in the composition of Page. However, the definition differs and is considered incompatible.

I know there’s a uservoice ticket for replacing extensions with traits entirely, this is something we’d need to consider if we’re still looking at that option. At the very least I think there’d need to be a change in how we think about extensions and config.

[micmania1]

Classes shouldn't inherit config from traits unless the config system is re-thought imo. Its already slow enough trying to merge YAML, class hierarchy, DataExtensions and all the variants (environment type, defined constants, config::update() etc), without adding traits.

[tractorcow]

Are we happy with the way config is currently working with 4.0? Traits seem to work as-expected (copy of code into parent class) and I agree with @micmania1 that we shouldn't have a magic "merge" from the trait into the parent of overlapping configs, as that's not how traits are meant to work (if I have understood your argument correctly).

[kinglozzer]

I’m happy. I think you’re right - what I was originally suggesting would go against how traits work so forget that