ENH Use FieldValidators for FormField validation

[emteknetnz]

Issue #11422

[emteknetnz]

DropdownField with int based source + DBVarchar

Have updated SingleSelectField and MultiSelectField to do casting string to int in use getValueForValidation() instead of setSubmittedValue(). This means that the internal value of the field stays and a numeric string

If I have a TextField with a DBInt leaving it blank results in "Must be an integer" error even though it's not a required field. Setting numberic values works as expected.

Have update FormField::setSubmittedValue() to alter blank strings to null

Setting a value of 9999999999999999999999999999999999999999999999999999999999999 for a TextField with DBInt gives the error "Must be an integer"
Setting a value of 9999999999999999999999999999999999999999999999999999999999999 for a NumericField with DBInt gives the error "Must be numeric"

Note: in the PRs current state they now give "Must be an integer" validation messages

The validation message comes from IntFieldValidator, which TextField and NumericField do not have, though DBInt does. The issue here is that it's above the max size of a signed 64 bit integer which is 9223372036854775807. DBInt has logic in setValue() to cast numeric string to int, however in the case of huge numbers it cannot, so it retains the value a numeric string, hence why it fails validation. Instead of failing to casting if we throw an InvalidArgumentException we just end up with a server error toast which is bad, and, throwing ValidationException in DBInt::setValue() is just wrong. The alternative would be to "truncate" the value to the max PHP int size, but that just defeats the whole purpose of all this validation work. So I don't really know what we can really do here to improve UX. While the message isn't great, it is enforcing data integrity by preventing saving a value to the database which cannot be stored.

Setting maxLength on a NumericField limits the length in the UI, but submitting a value that is too long does not result in a validation error. (doing the same with TextField does result in a validation error)

Have added created NumericNonStringFieldValidator and moved the 'is_string() && is_numeric() to it, and gotten DBInt / DBDouble to extend that. NumericField uses the NumericStringFieldValidator so it does allow string values

Using TextField with a DBDate if I set the value 2024-01-24/124 I get a simple "error" toast (no validation error). In the network tab I see a 500 error with 'Uncaught InvalidArgumentException: Invalid date: '. Use y-MM-dd to prevent this error."
If I input 01-24-2025 I also get that exception but with a different message: "Invalid date: '01-24-2025'. Use y-MM-dd to prevent this error"
I get similar errors as above using TextField with DBDatetime and with DBTime and entering invalid values.

These are all out of scope as this is existing behaviour. The exceptions are coming from DBDate/DBTime::setValue()

With MoneyField if I enter too many characters the validation message appears at the top of the page, instead of under the relevant form field.
With MoneyField if I enter an invalid currency amount the validation message appears at the top of the page, instead of under the relevant form field.

Have updated FieldValidationTrait to remove the subfield portion of the name so that the validation message now shows on the composite field

With MoneyField if I call setAllowedCurrencies() and set only one value, and there was an old value in the currency field before hand, saving the page results in a "Currency abc is not in the list of allowed currencies" validation error (and there's no way to change the value from the form).

MoneyField is weird to begin with in that the currency field can either be a DropDownField (two or more allowed currencies) a HiddenField (one allowed currency) or a TextField (zero allowed currencies). The issue you had is that when there's only one allowed currency, it becomes a HiddenField. I've updated setValue() to swap out the currency field to a TextField if the currency being set is not in the allowed value list. This isn't ideal and I've rather do this in the MoneyField constructor, but when the form is rendering the value is set via setValue(), rather than as a contructor argument.

Users can't edit values in disabled or readonly fields, but those still get validated. Should we disable validation for readonly and disabled fields?

Out of scope as these are existing issues, e.g.

• $fields->replaceField('MyVarchar', UrlField::create('MyVarchar'));
• Fill in value as http://example.com and save form
• $fields->replaceField('MyVarchar', UrlField::create('MyVarchar')->setAllowedProtocols(['https'])->setReadonly(true));
• Save form - validation error you cannot alter. Same issue with setDisabled(true)

[GuySartorelli]

Still not done: #11449 (comment)

Numeric field problem

DBInt with its default scaffolded form (NumericField) and a value of "99999999999999999999999999999999999999999999999999999999999999999999" now says "Must be a string" as well as "Must be numeric"

It must not be a string, and it is numeric so these error messages are clearly wrong.

Regarding the "Must be numeric" part of this which I mentioned last review, you said

The issue here is that it's above the max size of a signed 64 bit integer which is 9223372036854775807. DBInt has logic in setValue() to cast numeric string to int, however in the case of huge numbers it cannot, so it retains the value a numeric string, hence why it fails validation.

One option to provide a more accurate validation message would be to check:

1. Is the original value a numeric string? (is_numeric('99999999999999999999999999999999999999999999999999999999999999999999') will return true even though it's larger than the int that can be represented by PHP)
2. Does casting the value to int result in a value different to the original? (e.g. (int)$val == $val will return true if the value can be represented as a PHP int, but false if it cannot)

Still not resolved from previous review

With MoneyField if I enter too many characters the validation message appears at the top of the page, instead of under the relevant form field.

With MoneyField if I enter an invalid currency amount the validation message appears at the top of the page, instead of under the relevant form field.

[emteknetnz]

Still not done: #11449 (comment)

It's following the standard pattern of $this->beforeExtending('updateValidate', function (ValidationResult $result) { followed by return parent::validate();, meaning that $this->extend('updateValidate') is called in FormField::validate(). I'm not sure what the issue is?

Update: have changed the return ValidateResult::create(); to return parent::validate(); so that the extend hook is called in the parent class

Numeric field problem

I've added "manual" validation to NumericField to check if $this->getValue() === false, which means the localisation formatter was unable to parse the number. In the '999999999999999999999999999999999999999999' scenario I think it's simply too large i.e. bigger than a 64 bit int, so the formatter cannot parse it.

It'll now simply give a validation message of "Invalid number", which is the same message if you enter "fish". $this->getFormatter()->getErrorMessage() gives non-useful message e.g. U_ZERO_ERROR so we cannot use that in the error message.

With MoneyField if I enter an invalid currency amount the validation message appears at the top of the page, instead of under the relevant form field.

As noted here, that was previously resolved, currently works as it should

With MoneyField if I enter too many characters the validation message appears at the top of the page, instead of under the relevant form field.

Even though the PR has added the validation, that's an existing issue. This is currently happened because of the newly added NumericField::validate() logic which adds a field error where $this->getName() is MyMoney[Amount].

In order for this to show in the correct location $this->getName() would need to be MyMoney. This issue has to do with ValidationException thrown by FormFields in CompositeField's in general. The NumericField, or any FormField in general is not aware that it's in a CompositeField. So I'd say this is out of scope.

[GuySartorelli]

I don't understand how having the Currency field from a MoneyField display validation errors in the correct place is in scope, but doing the same thing for the Amount field from a MoneyField is out of scope, but I'll leave it in the interest of getting this merged.

[GuySartorelli]

Sorry, didn't start a review before adding comments.
Unresolved conversations:

• ENH Use FieldValidators for FormField validation #11449 (comment)
• ENH Use FieldValidators for FormField validation #11449 (comment)

[GuySartorelli]

There's a merge conflict

[emteknetnz]

Updated

[GuySartorelli]

LGTM