self::$runCMSFieldsExtensions needs to be changed to $this->config()->get('run_cms_fields_extensions')

[sunnysideup]

self::$runCMSFieldsExtensions needs to be changed to $this->config()->get('run_cms_fields_extensions') ???

[sunnysideup]

Thanks to people helping me:

[robbieaverill]

The reason this isn't a configuration property is that it's designed to be run in a way that temporarily modifies the value of the property (hence the getters and setters for it) rather than a global configuration setting. If you change it to a configuration property then the config manifest would need to be mutated which we usually try to avoid.

It's possible that we could replace it with something like a SiteTreeState::singleton()->withState(function ($newState) { $newState->setRunCMSFieldExtensions(false); $this->doStuff(); }); style callback like we do with subsites and fluent, or just deprecate the property entirely and remove it for SS5. I'm not sure how often it gets used.

Either way - changing it to a configuration property isn't the right way forward for this particular instance I'm afraid.

[dnsl48]

t's possible that we could replace it with something like a SiteTreeState::singleton()->withState(function ($newState) { $newState->setRunCMSFieldExtensions(false); $this->doStuff(); });

Don't see a lot of value in refactoring this particular bit of code. Maybe we could do that some time when we refactor the SiteTree class. If that bit of code confuses people we could potentially add some comments around with explanation, but functionally it works, so I'm closing the issue for now.

If you'd like to make a PR with commenting that code, please feel free to reopen and attach the PR to this issue.

[sunnysideup]

I think it would be better to have a bit more explanation on how / when / etc... to use this variable and why it is different from the usual (i.e. private static config Or non-private protected var).

[dnsl48]

There is a bit of documentation about it - https://github.com/silverstripe/silverstripe-cms/blob/4/code/Model/SiteTree.php#L2922-L2923

It seems to be solving rather an edge case when a deriving class wants to override getCMSFields method, call parent::__getCMSFields, then perform some of its own logic, and then perform $this->extend('updateCMSFields', $fields);

To me it looks like the variable is for rather internal use by the class and has public API provided to control it. Although, I'm puzzled to see it as private static and not protected... Might need a getter method then to check its value in children. Or better refactor it as @robbieaverill suggested above. I guess now I see some actual value in that as it's not refactoring on its own, but rather providing children with a way to have more than 1 descendants.

[sunnysideup]

We run into issues with the sequence on how CMS Fields are put together a lot (almost daily) so I dont see that as an edge case. You see that a lot of modules now do really weird (in my opinion) constructs with callbacks to create CMS Fields.

Here is an example:

https://github.com/silverstripe/silverstripe-userforms/blob/5.3/code/UserForm.php#L172-L176

I say weird because it is hard to understand for me how this works and why a call back is used rather thn just normal code.

As an underlying assumption, I think it is important that Silverstripe code is really easy to understand for people with limited coding experience because it makes it a much better CMS. Therefore I see this runCMSFieldsExtensions as a band-aid just like the example in userforms - where, if we had a better system, the actual function: getCMSFields would add the CMS Fields.

Maybe each "getCMSFields" / "updateCMSFields" call would add fiields with a particular signature where you could remove OR add in a different order any of the CMS Fields instructions that make up the final concoction of CMS Fields for a particular DataObject.

[dnsl48]

I say weird because it is hard to understand for me how this works and why a call back is used rather then just normal code

I think that's the fallout of the SilverStripe extension system coming from the pre-Trait era. A callback here is the only sensible option to extend the parent (without manually using $this->disableCMSFieldsExtensions). And as I mentioned above, you cannot use disableCMSFieldsExtensions method consistently as you don't know its current state and cannot guess if you need to enable it after or if there's another child in the inheritance tree that wants it to be left disabled.

Simply speaking, a callback here is the only consistent way to add something in between the inheritance chain and extensions.

[sunnysideup]

Simply speaking, a callback here is the only consistent way to add something in between the inheritance chain and extensions.

Yeah agree. It is the only way ...and so I reckon we should probably look at a bigger overhaul to make that more manageable so that we do not have to resort to such weird constructions.

[GuySartorelli]

This property should have @internal applied so it gets ignored by the config manifest.