Silverstripe-framework: Config definitions in Extension instances override YAML definitions

[dhensby]

Affected Version

^4

Description

Config settings defined in yaml and private statics is overriden by Extension instances, rendering it impossible for user code to override default config values from modules.

Yaml and private statics should have preference over those defined on Extension classes.

Steps to Reproduce

This can be reproduced by following the instructions given by the comments module as reported here: https://github.com/silverstripe/silverstripe-comments/issues/242

[tractorcow]

I've left feedback on the PR. Please let me know your thoughts. :)

[dhensby]

closing as intended behaviour

[robbieaverill]

I realise this has been closed as expected behaviour, but we've got another example of this. The cwp/cwp-pdfexport module has a Page extension which defines a config static:

class PageExtension extends DataExtension
{
    private static $enabled = false;
}

This extension is applied to Page (for this example's sake).

The documentation says something like "to enable this, set enabled to true for for your page type in YAML configuration." A developer then adds:

---
Name: mypdfexportconfig
---
Page:
  pdf_export: true

The dev flushes, but the PageExtension's value of false still takes priority.

I see that we can "work around" this by setting the config value to true on the extension rather than the class the extension is applied to, but that would enable this functionality globally for every class that PageExtension is applied to (not shown in this example, but feasible).

Another alternative is for us to stop defining default config property values in PHP statics for extensions and define them all in module level YAML instead, so that mysite code can say After: '#pdfexportconfig', but then we lose visibility of the config statics in the code itself.

[dhensby]

This definitely seems to be an (unexpected for developers) change for 3 -> 4 and I'm still not entirely sure why that is :/

[dhensby]

@robbieaverill I think the only solution to your problem is to have another extension that you apply and hope it gets applied after all the other extensions... though I'm not sure how reliably you could control that.

or shift all the config to yaml?

---

Side note, another reason I thought that Extensions should be lowest priority is because we want to discourage their use where yaml config can be used; mainly because Extensions can be resource intensive...

[tractorcow]

Three solutions.

Extend the extension via yml:

---
Name: mypdfexportconfig
---
PageExtension:
  enabled: true

Or don't declare a config from the extension at all, and rely on empty as the default (may need to reverse boolean).

function something() 
{
    $enabled = !$this->owner->disabled;
}

Or add programmatic extra config in your extension. Kind of gross and excessive, but prevents extension from overwriting other config.

function get_extra_config($class)
{
    $enabled = Config::inst()->get($class, 'enabled', Config::EXCLUDE_EXTRA_SOURCES);
    if (!isset($enabled)) {
        $enabled = true;
    }
    return [ 'enabled' => $enabled ];
}

[robbieaverill]

I think the "best" option is to stop declaring PHP static defaults on extensions. It'd be a shame to lose it from an API documentation point of view though.

[tractorcow]

I think the "best" option is to stop declaring PHP static defaults on extensions. It'd be a shame to lose it from an API documentation point of view though.

If we do this, it will ALSO help us with possibly shifting extensions to traits in the future. I would like to one-day re-enable the "extensions as traits", but it will require us to stop treating extensions as config sources also.

[micmania1]

Summary

Config set on extensions, whether that's via YAML or PHP will overwrite (take precedence over) config set on the actual class you're applying the extension to.

How to replicate

Create an extension with some config:
eg.

SomeExtension:
  some_prop: 'set on extension'

<?php
class SomeExtension extends DataExtension 
{
    private static $some_prop = 'set on extension'
}

You only need to do one of the above, but both have the same result.

<?php

class MyClass extends DataObject 
{
    private static $extensions = [
        'SomeExtension'
    ];
    private static $some_prop = 'set on class';
}

The value of Config::inst()->get('MyClass', 'some_prop'):

Expected:

set on class

Actual:

set on extension

Changes from SS3

In SS3 this used to work in the opposite way, causing breaking changes in some modules. So far this has been found in comments & pdfexport.

The value in the above example in SS3 would have been 'set on class'.

There was nothing I could find that documented this change in the changelogs.

It was introduced as part of the config change here: https://github.com/silverstripe/silverstripe-framework/commit/a6e9a7111bf7b6d5d79e90611d6443cbce2204f2

Problems

• This makes module config more important than project config
• It prevents modules from setting sensible defaults
• It requires any modules which may be affected by this to come up with a new, non-standard way to provide config
• The change is subtle but severe - module owners probably don't know their module is broken yet
• Counter-intuitive
  * inheritance in PHP traits makes MyClass higher priority than traits (ie. extensions) applied to it.
  * This is the complete opposite of what people have been used to in previous SS versions.

Risks of fixing this

• Core framework code may rely on it
• Some modules may rely on it

Risks of NOT fixing this

• Core framework code may rely on the old way and we haven't encountered those bugs yet
• Some modules do rely on the old way (we have encountered this)
• Some modules which are already broken need to be upgraded
• If we choose to fix this later, it will be harder as the risks of fixing will increase

My thoughts

The idea that fixing this may cause breaking changes should be validated before rejecting this change. Since we already have concrete examples of breakages, these should be given higher precedence than theory.

Semver is meant to help communicate expectations. I think expectations are already out of sync and fixing this will help to realign them.

Ideally we would just release this fix as SS5 with a note of "easy upgrade, should be no problem but there is a low risk breaking change." However I understand that the reality of support contracts and such may prevent that or at least make them complicated for SS users.

[chillu]

This makes module config more important than project config

To clarify: You're assuming that SomeExtension is placed in a module, and MyClass is placed in either core or another dependant module, correct? And if that's the case, there's no way to override MyClass.some_prop in project code.

I agree with Michael that it's counter-intuitive for developers, and that there's more harm in accepting a bug as an "accidental API change" than there is to revert to the 3.x behaviour. There will be more code that relies on 3.x behaviour than code that's been adjusted to the accidental change in 4.x, both in terms of project and module code.

Some comments on the proposed alternatives:

1. Stop declaring static defaults on extensions: Statics are our way of providing a "config schema" (via PHPDoc annotations). IMHO that's a shitty way of doing schemas, but it's superior to defining these only in YAML outside of class context. Looking at Robbie's pdfexport example, if you don't define pdf_export in the Extension, then you either have to assume which class(es) the extension applies to, or leave it purely in dev docs.
2. Extend the extension via YAML: Not going to work if you want to vary behaviour based on subclasses of the class you've applied the extension to.
3. Don't declare a config from the extension at all, rely on empty as the default: Same concerns as "stop declaring defaults on extensions".
4. Add programmatic extra config: As Damian said, kind of gross and excessive. It's also a level of complexity on top of our already extremely complex config implementation.
5. Add separate _defaults private static: Same as above.

Tally on "votes":

• Revert to 3.x behaviour: @chillu, @micmania1, @robbieaverill, @wilr, @flamerohr, @dhensby
• Stick to semver and come up with alternative: @tractorcow, @kinglozzer

[wilr]

Hmm I thought this was fixed way back as part of https://github.com/silverstripe/silverstripe-framework/pull/7390...

Otherwise I vote on reverting to 3 behaviour - % higher chance that older code will rely on the old behaviour considering this was only picked up.

[flamerohr]

I would have expected the 3.x behaviour, the current implementation seems limiting/frustrating to the developer experience as well.

My vote goes to reverting to the 3.x behaviour.

[kinglozzer]

This is quite risky to revert in 4.x, there’s a high chance of causing breakages for project code (plus any modules that may have already been patched). 4.0.0 was released 8 months ago, and in that time we’ve seen the following activity about this issue:

• https://github.com/silverstripe/silverstripe-comments/issues/242
• https://github.com/silverstripe/cwp-pdfexport/issues/5#issuecomment-379628631
• https://github.com/littlegiant/silverstripe-catalogmanager/pull/56 - workaround for current behaviour
• https://github.com/Rhym/silverstripe-cms-theme/pull/31 (adopts the same approach as above)

Apologies if there are other modules this has caused problems for that I’m not aware of. Given that we’ve had one bug report (on framework’s issue tracker) about this with no activity/links except from those people working on the above modules, I think we may be overestimating the impact of this change and the importance of reverting it for 4.x.

Since we already have concrete examples of breakages, these should be given higher precedence than theory.

The problem is that we have absolutely no way of knowing how many breakages reverting the change would cause until we tag a stable release containing it. It might be none, it might be 10x as many as the reports linked above.

There will be more code that relies on 3.x behaviour than code that's been adjusted to the accidental change in 4.x

I think that’s a dangerous assumption. Especially considering new projects built in 4.x, rather than upgraded from 3.x, may have been built to rely on this behaviour.

I’m happy to discuss changing this behaviour for 5.x (arguably it should trigger a wider discussion about our config conventions) but I’m voting against reverting it for 4.x.

[chillu]

My vote goes for revert not only because of code regressions, but also because I haven't heard a good way to deal with this accidental new behaviour going forward. If you voted against the revert, can you please also vote for one or more of the five proposed options I've collected?

[dhensby]

@chillu my vote is to revert to 3.x behaviour - I did author this issue and a subsequent PR to put it back ;)

I completely agree with @micmania1's summary and thank you for putting the effort into describing in such detail what's going on.

I really don't like this idea of "the bug has become a feature" argument against fixing bugs, it's really frustrating to block bug fixes and prevent the code base becoming more intuitive based on "what if people rely on this bug"

[chillu]

OK, updated votes tally:

• Revert to 3.x behaviour: @chillu, @micmania1, @robbieaverill, @wilr, @flamerohr, @dhensby
• Stick to semver and come up with alternative: @tractorcow, @kinglozzer

[dhensby]

I'm going to re-open this issue as it's having active discussions again as well as my fix

[sminnee]

I think that reverting the behaviour sounds good, but do we want to do it as a minor, rather than patch, change?

[robbieaverill]

If we do it as a patch change we'll get it into everyone's 4.x projects eventually, but if we do it as a minor release we'll have two 4.x releases which have different behaviour than the rest of the release line

[dhensby]

I agree with @robbieaverill - it'll also mean the modules that want to support 4.0 and 4.1 will have to have some special work around to fix the bug anyway, which kind of defeats the purpose

[sminnee]

OK, are you suggesting that we backport the fix to both 4.0 and 4.1? I guess that affirms that this has been wrong all along ;-)

Will this break any of the modules that Loz has mentioned?

[robbieaverill]

Will this break any of the modules that Loz has mentioned?

Some, yes - we'll need to unbreak them again

It'll also _fix_ some of them too

[sminnee]

I think we'd want to work directly with those module maintainers, and possibly give a pre-announcement on community channels.

[dhensby]

OK, are you suggesting that we backport the fix to both 4.0 and 4.1?

Yes

@robbieaverill do the work arounds we have actually cause the new fixes to break? aren't they elaborate work arounds that will still work?

[sminnee]

Do the work arounds we have actually cause the new fixes to break? aren't they elaborate work arounds that will still work?

Being able to achieve this would be much less risky, if we can.

[robbieaverill]

So in terms of comments and cwp-pdfexport I think the "fix" was to document how to modify it by modifying extension config instead of base object. In this case the change should be safe and we'd just revert the docs back to suggesting applying config to the base object

[tractorcow]

Reverting the behaviour isn't a fix, it's a change in preference, and it's a massive semver violation. Flipping the order would mean extensions that override private statics would no longer do it, since statics are now higher priority than extensions, which wasn't even the case in 3.x.

I've outlined a possible option over at https://github.com/silverstripe/silverstripe-framework/pull/7971#issuecomment-404033771 that lets extensions opt into assigning themselves a lower priority than other extensions / statics / yml.

[tractorcow]

If you really want to change it back without semver violations you should be targeting this change to 5.x. You could introduce the priority feature in 4.x, and switch the default in 5.x to 3.x behaviour (before before by default rather than merge after).

How does that sound as a compromise to respect both semver, but enforce a new desired direction going forward?

[tractorcow]

Sorry I did re-test 3.6 and it does seem that private statics do override extensions for scalar values after all, as per @micmania1 's example at https://github.com/silverstripe/silverstripe-framework/issues/7970#issuecomment-402567988, so I was wrong about that.

I'm mostly concerned about breaking 4.x line more than compatibility with 3.x.

[dhensby]

Fixing a bug is not a breaking change; this isn't working as expected, we're having to come up with elaborate workarounds on a per module basis to avoid this problem.

@micmania1 has outlined a really good set of reasons why this needs to be changed to the behaviour we expected in 3.x. So far there's been no proper justification to keep the current behaviour other than "it is how it is", but that's an argument that can be used for any buggy feature and doesn't stop the behaviour being wrong or unintended.

It's clear to me that this behaviour was not changed on purpose, this change was not documented, modules were not updated to take account of it, and there's no clean or consistent way to work around the regression.

We should fix behaviour that is unexpected, unintended and impractical; that's not a semver breakage, it's a bug fix. SemVer allows for bug-fixes!

[tractorcow]

Fixing a bug is not a breaking change; this isn't working as expected, we're having to come up with elaborate workarounds on a per module basis to avoid this problem.

It's not fixing a bug, it's reverting a major version change in a minor version. As I said over in https://github.com/silverstripe/silverstripe-framework/pull/7971#issuecomment-404332907 (sorry for the split conversation) when I rewrote the entire middleware / config layer I implemented this intentionally. A change in preference / disagreement is not a bug. This is the same conversation we had with @hafriedlander when fixing the can't override non-falsey values "bug", and we agreed then not to change it because it would break semver.

That being said, I admit I am outvoted here, so I won't pretend my opinion is more important.

we expected in 3.x.

4.x is a separate major version. "it's how it used to be" is just as bad an argument as "it is how it is".

So far there's been no proper justification to keep the current behaviour other than "it is how it is",

It's intentional, not "just how it is", and a semver breakage to change. Which we do anyway, and I don't think that should block this change, I just want people to be honest about it.

[tractorcow]

I've checked the docs at https://docs.silverstripe.org/en/4/developer_guides/configuration/configuration/

from what I can tell, these are impossible to respect in 4.x; The docs are out of date and need fixing for 4.x.

Any values set via a call to Config#merge / Config#set The configuration values taken from the YAML files in _config/ directories (internally sorted in before / after order, where the item that is latest is highest priority) Any static set on an "additional static source" class (such as an extension) named the same as the name of the property Any static set on the class named the same as the name of the property The composite configuration value of the parent class of this class

Statics on the class AND yml are now merged together in 4.x. We can only merge extensions over those (4.x status quo) or under (violates both 3.x and 4.x status quo). Merging https://github.com/silverstripe/silverstripe-framework/pull/7971 would NOT make us follow 3.x; We'd be introducing a brand new merge pattern that doesn't exist in either 3.x or 4.0/4.1, which prioritises private statics over extensions.

What 3.x and 4.x both do is merge extensions on top of private statics. The only change in 4.x is that yml config is now merged in lower.

The new config module collects statics + yml and merges them for the cache, which is persisted for performance reasons. Extension are applied on top of this combined config at runtime.

That's why I intentionally wrote the middleware to apply extensions on top.

[dhensby]

OK - so the docs (and behaviour) in 3.x is effectively yaml is highest priority, then extensions, then classes. In 4, because yaml and classes are built and cached, the extension either has to be higher priority than yaml or lower than classes but not sandwiched between yaml and classes.

I think moving extensions to be be the lowest priority is ok because we can override class config settings so easily in YML rather than in an extension and that's the way devs really should be doing it...

[kinglozzer]

Sorry for the radio silence on my part, I’ve caught up on the discussion and to summarise my position now:

If the decision were solely up to me, I would leave the behaviour as-is in 4.x. I agree that the 3.x behaviour is more useful (e.g. using extensions to provide ”defaults” like we see in the comments module), but I still worry that changing this now may break things for people in subtle ways, and I’ve not seen enough complaints about the new behaviour to convince me that the risk of reverting is worth it. However, I trust the judgement of the rest of the team and if the majority disagree with me then I’m willing to listen and accept that I may be being too cautious on this occasion.

[kinglozzer]

Closed by https://github.com/silverstripe/silverstripe-framework/pull/7971

Can whoever merges up please make sure that the description at the top of the changelog is copied to the 4.1.x and 4.2.0 changelogs?

[dhensby]

done