ENH Use FieldValidators for FormField validation

[emteknetnz]

Issue #11422

[GuySartorelli]

Found while testing:

• ENH Use FieldValidators for FormField validation #11449 (comment)
• 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.
• 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"
• 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)
• 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.
• 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.
• 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).

Some of the errors above are likely covered under #11453 - if that's the case please just list which those are and we can deal with them in that other card.

Disabled and readonly fields

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

[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.

[GuySartorelli]

This feels wrong. What is the specific intention here? Casting to int and then to string seems pointless? Is this try/catch a non-standard conditional that could be replaced with a is_numeric() check?

[emteknetnz]

$value could be anything, e.g. float, int, numeric string. There was existing logic to chop of decimal places if $this->getScale() is 0, hence the int cast.

I've updated the docblock description to say that the point of cast() is to get a numeric string and repect getScale()

Have changed try/catch to is_numeric()

[GuySartorelli]

This feels wrong. What is the specific intention here? Casting to int and then to string seems pointless?

This part still applies. I still don't understand what the purpose of this is. Perhaps it just needs an inline code comment explaining it? But you might also need to explain it to me in person so I really get it. It's definitely weird and it's not clear what the intention is.

[emteknetnz]

I've retained the logic though refactored the class and added a bunch of inline comments explaining all the things

[GuySartorelli]

1. When would the value be explicitly false?
2. Why do we return 0 here, but cast returns false?

[emteknetnz]

false $value happens when the localisation formatter fails to parse the in setSubmittedValue()

dataValue() is passed to the DBField, we do not want to be settings the DBField value to false. 0 is appropriate.

[GuySartorelli]

We shouldn't be passing the wrong value into DBInt. If someone sets an invalid value to the form field, we shouldn't be replacing whatever value used to be stored in the database with 0

Let it return false from here, and the validation logic in DBInt will pick it up.

[emteknetnz]

That's true. Won't necessarily be DBInt as it could be any DBField e.g. DBVarchar. However NumericField::validate() will prevent code from even reaching this point so it doesn't really matter. I've expanded on the inline comment to explain this. In hindsight the only reason I was returning zero here was to get an existing unit test to pass, I've now updated the unit test to expect false instead of 0

[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)