ENH Use FieldValidators for FormField validation

[emteknetnz]

Issue #11422

[emteknetnz]

Not using strong typing because you cannot strongly type a callable property in PHP, even though you can assign it to an untyped property

[emteknetnz]

This setValue() / assertion did nothing, the ArrayData is ignored in setValue(), so all it's testing is that setting a value of false is valid

[emteknetnz]

?? '' to prevent deprecation warning if setting a value of null

[emteknetnz]

Changed to public as required by FieldValidatorConverterInterface

[emteknetnz]

Duplicate as is already defined on FieldValidationTrait

[emteknetnz]

This argument should have been passed through to parent before this PR

[emteknetnz]

Moved location in the file to be closer to the other *Value() methods

[GuySartorelli]

Didn't do a full review, just looked at things that are relevant or adjacent to relevancy for the CMS 5 PRs.

[GuySartorelli]

Why are we getting rid of this? Did you look at the PR that added it and the reasoning for it? #10569 (comment)

[emteknetnz]

Yes I did look at it. That's no longer compatible with the new way of doing this

The old FormField::validate() method had a signature of validate(Validator $validator): bool, whereas this has now been updated to match DataObject and DBField with validate(): ValidationResult

The replicate the extension functionality, there's a new extension hook updateValidate on FormField::validate() and a ValidationResult param to use. I've used this in combination with $this->beforeExtending('updateValidate') and calling parent::validate() on any overridden validate method so that the extension hook is always called. I've added a unit test on FormFieldTest::testUpdateValidateHookCalled() to ensure this is happening

[GuySartorelli]

Sweet. I think so long as that test is still there that's the main thing.

I do like that this now means the updateValidate extension hook is the same method signature for both DataObject and FormField - we should call that out in the changelog probably.

[emteknetnz]

Have mentioned in 6.0.0 changelog - https://github.com/silverstripe/developer-docs/pull/621/files#diff-027bda823f29903fb90f1ca3e86dc918c3b6a814c50096c66a3074b1da24eb62R799

[GuySartorelli]

We now have four different variations on getValue().... that's confusing as hell. Why do we need 4 different ways to get the value?

[emteknetnz]

We need them all?

getValue() = Get the interval value of the field (this->value), which should be as "unmodified" as possible. Required to meet FieldValidationInterface
getValueForValidation() = Used for FieldValidators. Defaults to $this->getValue(). Something need modify this for some FieldValidators, such as CompositeFieldValidator() which validates child fields rather than the composite field itself. Required to the meet FieldValidationInterface.
dataValue() = Get a value the for insertion in the database. Will do some casting. If were were to get rid of one of the methods, I'd say it should be this one. But definitely not in this PR.
Value() = Get a value for use in templates. May modify the value quite a bit

Looking at this again, getValueForValidation() is possibly badly named as it's only intended to be used by FieldValidators, and not custom validate() methods which should use getValue() instead. There's a few options
a) Leave as is
b) Rename the method to getValueForFieldValidation()
c) Better doc block description
d) Remove from FormField + DBField and put it on FieldValidationTrait since they're identical

Perhaps b), c) and d)?

[GuySartorelli]

Yeah, c/d sounds good.
In my mind the name you've suggested there is functionally identical so I don't care if it gets renamed to that or not. This is a field, so all of its validation is "field validation" even if it's not all using a FieldValidator.

That still doesn't solve the problem that we're going from two methods to four in this PR. Most developers will be able to rule out getValueForValidation() for their use case, but this PR still makes the decision go from "do I use value() or dataValue() to now do I use value()orgetValue()ordataValue()`?

getValue() and Value() are so identically named that I would expect them to always return the same value, with one being a legacy alias for the other.

Value() = Get a value for use in templates. May modify the value quite a bit

I assume this is used via getDefaultAttributes() and getSchemaStateDefaults(), not directly via $Value in templates? If that's the case, it would be sensible to either rename it to something like getValueForHtmlAttributes() or else transformValueForDataAttribute($value) or something like that.
That makes it immediately clear that if I'm trying to just get the value from the field in my PHP code, I don't want to call that one.

That still leaves us with getValue() vs dataValue(), but at least that confusion already existing before this PR so I'd be okay with leaving it at that point.

[emteknetnz]

I assume this is used via getDefaultAttributes() and getSchemaStateDefaults(), not directly via $Value in templates?

I'm not sure where or how it's exactly used, my best understanding is that it prepares the value for display to a human. Here's a few examples of how it's implemented in subclasses

class DateField extends TextField
// ..
    public function Value()
    {
        return $this->internalToFrontend($this->value);
    }

class SiteTreeURLSegmentField extends TextField
// ...
    public function Value()
    {
        return rawurldecode($this->value ?? '');
    }

class ReadonlyField
// ...
    public function Value()
    {
        // Get raw value
        $value = $this->dataValue();
        if ($value) {
            return $value;
        }

        // "none" text
        $label = _t('SilverStripe\\Forms\\FormField.NONE', 'none');
        return "<i>('{$label}')</i>";
    }

We can't use getValue() as an alias for Value(), as Value() isn't simply returning $this->value

Feels like the best we can do here is to have better PHP doc for both Value() and getValue()

[GuySartorelli]

We can't use getValue() as an alias for Value()

I'm not suggesting we do that at all - I'm saying that developers will expect that because they're basically the same named method. The name of the methods is what's problematic here.

I'm suggesting we rename Value() to something more specific so that developers don't have any confusion about what the difference between getValue() and Value() is. PHPDoc will help. Well named methods is better.

I'm not sure where or how it's exactly used

Please find out where and how it's used, and then give it a name that is appropriate for that usage.
That will make it easier both to know when to call it, and to know what to implement inside that method when implementing new form fields.

[emteknetnz]

In PHP It's primarily called within FormFields as $this->Value()
There's a handful of other places, seems to be mainly GridField related such as GridFieldNestedForm, GridFieldAddNewInlineButton that will call something like $formField->Value(). You could make an argument that alot of these should be switched to use getValue() instead, but that's outside the scope of this PR

It's used a lot in FormField templates as $Value

I think we should simply keep things as they are

[GuySartorelli]

Keeping things as they are is not an option IMO. We can't add a getValue() and not rename Value() because the two methods are named too-similarly and it will cause unnecessary confusion. I'm already confused by them and this hasn't even been merged.

[emteknetnz]

I've split off as new card, the scope of work is too large (~50 files to change) and will just muddy up this PR. Also a high risk of breaking stuff

[GuySartorelli]

This PR adds the confusion, so this PR should clear it up, unless you can promise me the split off card is the very next thing you work on.

[GuySartorelli]

While I am glad to see we're not storing two sets of the value now... please explain what this was used for and why we don't need it anymore?

[emteknetnz]

On DateField/DatetimeField/TimeField it was was an entirely unmodified value that came in from setValue() / setSubmittedValue(). Then the internal value $this->value was changed due to all the localisation fun that happens. rawValue was then checked to see if the field was empty all along and to not validate it if was.

tl;dr it was pointless

[GuySartorelli]

Sweet, sounds like it was a non-standard workaround for the way the value property was being (ab)used. I'm okay with it being axed.