Silverstripe-framework: BUG: defaults are not inherited in SS3

[sunnysideup]

Affected Version

3.7.1

Description

Basically we have this:

ParentClass extends DataObject
{
    private static $db = [
        'FieldA' => 'Boolean'
    ];
    private static $defaults = [
         'FieldA' = 1;
    ];

}

and

class OtherClass extends ParentClass
{

}

When we create an object of the OtherClass type then FieldA is set to FALSE. We would expect this to be TRUE because that is the default. We tried both "true" and "1" as default values.

From what I know about Silverstripe this is not correct AND worked differently before AND if correct, has a massive impact.

[sminnee]

Even if this works in SS4 it would be good to add a unit test for it.

[sunnysideup]

I tried using

private static $db = [
     'FieldA' => Boolean(1)
];

in the parent class, but even this method did not seem to solve it.

Having said that, maybe I am wrong. I think the first thing is to get a confirmation of this bug.

[sminnee]

I've tested this and things seem to work. Is it because you have extends ModelAdmin instead of extends DataObject?

[sminnee]

SS3 test here: https://gist.github.com/7d3ff3845e6938a53388c4c9accc35e4

SS4 test here: https://gist.github.com/9962a05b11cec90896c42182e63da02d

Both pass

[sminnee]

Closing issue for now - feel free to reopen if you can amend that unit test to make it fail?

[sunnysideup]

ahhh no, it is extends DataObject for sure. I will do some more tests.

[kinglozzer]

This has been driving a client of ours mad! I’ve managed to track it down, private static $defaults are inherited if new MyDataObject() is used, but _not_ if created by MyDataObject::create(). I guess this is a regression from https://github.com/silverstripe/silverstripe-framework/pull/7850

[kinglozzer]

Question: given we no longer “officially” consider non-security issues on SS3, do we want to fix this? I’m happy to look at writing up tests + a patch if so

[robbieaverill]

I think if you want to fix something then go for it (personally)

[kinglozzer]

PR for review: https://github.com/silverstripe/silverstripe-framework/pull/8747

[maxime-rainville]

My view is we should avoid doing non-critical fixes against the 3.x branches. We'll still be doing releases for security reason and we don't want to introduce more changes that might create regressions. We don't want to be stuck doing more regression testing than what we have to.

Also, it's sending a mixed message to the community.

[robbieaverill]

Perhaps this is a clarification for the @silverstripe/core-team to discuss. My impression was that we wouldn't be putting core team's time into non-critical SS3 issues, but (as with all modules) if people want to submit PRs against old branches they'd be welcome to do so.

[sminnee]

I don't think it's a simple "yes" or "no" case – I think it comes down to a tradeoff of how critical the bug is vs the risk of introducing it. The fact that the PR comes from a long-standing core committer carries weight. Suggest the discussion happens on the PR.

Also – is this broken or fixed in SS4?

[kinglozzer]

Also – is this broken or fixed in SS4?

SS4 isn’t affected, Injector always explicitly requests a singleton or prototype in SS4 - it never leaves the affected argument blank

[ScopeyNZ]

but (as with all modules) if people want to submit PRs against old branches they'd be welcome to do so.

I think this is the most welcoming approach. Obviously some moderately sized (but not critical) bug we can say we're not accepting it because taking the time to code review and test it will be substantial. But simple fixes that take ~15 minutes to review/test I think we should still be receptive.

If the author abandons it though we can just close it.

Sorry about the slight side-track in this - perhaps this discussion can happen elsewhere

[robbieaverill]

Fixed with #8747 (merged up to 3)