Form schema fixes

[chillu]

Further work on #4938, in preparation for a React-powered asset and campaign admin.

/cc @tractorcow @flashbackzoo

[chillu]

@silverstripe/core-team We're hoping to get this pull request over the line in the next 48h because we have an opportunity for staff to fix things during an internal hackday here at SilverStripe Ltd on NZ Friday, ideally without too much branching fragmentation. Can you please review in your respective areas of expertise (PHP and/or JS/React)? @flashbackzoo, @scott1702 and myself have largely peer-reviewed each others work here, but would be good to get the thumbs up from at least one other core committer since these pull requests lay the ground work for further React integration. You'll see some @todo tags, some of it is early stages, but we agree with @sminnee's guidance that we should try to integrate early and often in our product development cycle.

[kinglozzer]

Would there be any merit in storing these somewhere, perhaps as constants? I can foresee myself getting the case wrong

[sminnee]

+1 for class constants on the trait object, with the values set to these strings?

FormFieldSchemaTrait::TYPE_STRING = 'String';
FormFieldSchemaTrait::TYPE_HIDDEN = 'Hidden';

It'll also make changes that create a new one of these data types will have a commit back here with a new constant, which seems an appropriate level of announcement.

[sminnee]

Can we add a docblock note saying that the value for this should be set in definition of the FormField subclasses. I'm not sure if that blocks us from using class constants for the values, though.

[tractorcow]

+1 for constants for types. They should be reasonably fixed, unless the universe invents new primitive data types. :D

[kinglozzer]

Gah, it sucks that we have to lose the trait. Is it just the one $schemaDataType property that’s the problem? Can we work around it somehow?

public function getSchemaDataType() {
    if (!$this->schemaDataType) {
        $this->schemaDataType = 'Text';
    }
    return parent::getSchemeDataType();
}

Or possibly call $this->setSchemaDataType('Foo'); in the subclass constructors?

[chillu]

Gah, it sucks that we have to lose the trait.

@kinglozzer @tractorcow Where do you see the advantage in the trait? For me, it's on the same level as creating a "section" in a class definition through some kind of PHP comment markers. I can't see a reuse for this trait outside of the FormField class. So the only thing it does is confuse devs with less powerful IDEs on where a certain method is defined.

[sminnee]

OK, so this comment is about this entire commit but I have to say that this is quite surprising. I don't see the fact that the trait is used in 1 place as a reason not to break the code into smaller pieces.

Perhaps the trait should mark abstract function getSchemaDataType(), and the implementation of only that method is pulled into FormField?

[sminnee]

I'm with @kinglozzer on this one. I think breaking down code into smaller pieces is a pretty fundamental piece of good software design.

I'd shift only the implementation of getSchemaDataType() (and the protected property) into FormField, and declare it as abstract in the trait.

[chillu]

OK three votes for traits (@sminnee, @kinglozzer, @tractorcow , one against (@chillu - in this context). I'll add the trait back in, and move getSchemaDataType() and getSchemaComponent() to FormField.

[hafriedlander]

Jumping in for no good reason, while I agree that breaking code into smaller pieces is good design, using a trait to do it seems a bit smelly. You can't inject or replace traits, and they share the same access space as the class they're attached to. Why isn't this a seperate class with a decorator pattern? If the argument is "Form Fields should be able to override specific behaviour", the argument for moving that out into a different class is the same as the argument for moving the base trait behaviour into a seperate class.

[tractorcow]

I like traits but I don't care that strongly about it in this case. I'd have done it if it were up to me, but happy for it to be left out for this PR.

Why isn't this a separate class with a decorator pattern?

Also fine with me.

[tractorcow]

I'm going to merge, since "moving schema into a trait, extension, or other class" isn't something that we can't do at a later date. The tests pass so let's keep moving onwards.

[tractorcow]

Maybe I'll wait for @chillu to respond to the query on using constants; Since that seems to be the only un-addressed point here.

See #5239 (comment)

[sminnee]

OK well, if Ingo and Damian are both okay with the solution, I'm not going to block merge of this.

[sminnee]

However, it would be good to hear if @kinglozzer thinks we should hold fire and refactor this some more before merging...

[kinglozzer]

Yeah moving it back into a trait is my preference, but I don’t feel strongly enough about it to block merge, so +1. Same for the constants I guess - there’s no reason we can’t merge this and address them in a subsequent PR

[chillu]

OK, updated the PR according to feedback:

• Added a comment regarding definition of $schemaDataType in subclasses
• Using constants: FormField::SCHEMA_DATA_TYPE_*. A bit lengthy, but just TYPE might be too confusing.
• Haven't re-instated the trait, as per latest feedback.