fix(forms): MinLength Validator treats 0 as zero length

[ntsokos]

remove truthy statement for assessing value existence because:

• it is already checked previously
• evaluating truthy value of 0 resolves to false which is not the desired behavior.

all that we need is to check the length property of the value

Closes #23749

PR Checklist

Please check if your PR fulfills the following requirements:

• The commit message follows our guidelines: https://github.com/angular/angular/blob/master/CONTRIBUTING.md#commit
• Tests for the changes have been added (for bug fixes / features)
• Docs have been added / updated (for bug fixes / features)

PR Type

What kind of change does this PR introduce?

• Bugfix
• Feature
• Code style update (formatting, local variables)
• Refactoring (no functional changes, no api changes)
• Build related changes
• CI related changes
• Documentation content changes
• angular.io application / infrastructure changes
• Other... Please describe:

What is the current behavior?

Issue Number: #23749

What is the new behavior?

MinLength Validator no longer treats 0 as zero length

Does this PR introduce a breaking change?

• Yes
• No

Other information

[gkalpak]

If I understand correctly, #23749 is about number inputs. AFAICT, the minlength validator is not intended to be used on number inputs (and the native minlength validator does indeed seem to be ignored on number inputs - on Chrome at least).

This change basically makes the minlength validator a no-op on number inputs.
Not sure if that is the intended behavior or not (cc @kara), but in any case there should be tests verifying the new behavior.

[ntsokos]

Hi George.

You are indeed right. I assembled a little example on stackblitz and i can see that minlength does not work with numbers.
There is still something weird going on though as you can see in case 2> as i would not anticipate the input to be marked as ng-invalid.

I tried to dig a little deeper in order to find what is causing this but my research did not yield any results. Any idea on where this truthy/falsy check is made?

[gkalpak]

Just to be clear: I meant that the native (built into browsers) minlength does not work on number inputs. This doesn't mean that Angular's minlength validator is not applied. (But it seems that it is not designed to work with number inputs.)

Not sure what the intention is here - ping @kara.

Given that the validator currently does the following:

const length: number = control.value ? control.value.length : 0;
return length < minLength ?
    {'minlength': {'requiredLength': minLength, 'actualLength': length}} :
    null;

...here is what happens for number inputs:

• If control.value is the number 0 (which is truthy), length is set to 0. Since 0 < minlength, the validator marks the input as invalid.
• If control.value is any other number (which is truthy), length is set to control.value.length, i.e. undefined. Since undefined < minlength always evaluates to false (for any minlength), the validator marks the input as valid.

So, basically, Angular's minlength validator does not work correctly on number inputs (and it is not even clear what "correctly" would mean in this case).

We first need to decided what the intention is and then implement it. Some options off the top of my head (probably in personal order of preference):

1. Document that minlength is not supposed to be used on number inputs (and assume control.value is always a string for this validator).
2. Same as above, but also throw an error if trying to use minlength on an unsupported input type (e.g. type="number").
3. Change minlength to do something reasonable on number inputs (and document it). E.g. it could count the number of digits or something.
   (For reference, this is what AngularJS does ✌️)

[ntsokos]

Thank you for the detailed response

If we have a look at w3c HTML recommendations it is pretty clear that minlength and maxlength do not apply to numbers.

Based on your insights and recommendations i would suggest the following

• Introduce a guard against inappropriate value types to be used by both min and max length. (exit validation in case of a number)
• communicate inappropriate usage by a console.error instead of throwing error which seems a little bit too big of a breaking change.

That said, i would like to also mention that it is my first attempt to contribute to angular’s codebase and that i would be happy to proceed with any approach deemed suitable.

[gkalpak]

Your suggestion sounds reasonable (but I don't know much about @angular/forms). Let's wait for @kara's input.

[AndrewKushnir]

Hi @ntsokos, thanks for creating this PR.

It turned out that there is also a PR #36157 (created recently) that contains a fix to the same problem. The changes in PR #36157 are similar to what you proposed in #29625 (comment), specifically: the validation logic doesn't get triggered for unsupported types (such as numbers that don't contain length property). I will close this PR as a duplicate of #36157, feel free to review it and add comments.

Thank you.

[angular-automatic-lock-bot]

This issue has been automatically locked due to inactivity.
Please file a new issue if you are encountering a similar or related problem.

Read more about our automatic conversation locking policy.

_{This action has been performed automatically by a bot.}