fix(forms): handle numeric values properly in the validator #36157

sonukapoor · 2020-03-20T11:55:37Z

Previously, the behaviour of the minLength and maxLength validator
caused confusion, and making it appear as they work with numeric values.
This commit fixes this, by always passing the validation when a numeric value
is used.

BREAKING CHANGES:

The minLength and maxLength validators now verify that a value has
numeric length property and invoke validation only if that's the case.
Previously, falsey values without the length property (such as 0 or
false values) were triggering validation errors. If your code relies on
the old behavior, you can include other validators such as min or
requiredTrue to the list of validators for a particular field.

Closes #35591

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: #35591

What is the new behavior?

Does this PR introduce a breaking change?

Yes
No

Other information

petebacondarwin

This change is an improvement, generally, since it is always safer (and more performant) to compare against undefined and I am happy for it to land.

But we should be aware, and encourage people (perhaps via documentation) not to use minlength with number fields, precisely for some of the reasons laid out in the comments for the original issue. Use cases where the string length of the number are important seem to me that they should really be considered strings that are restricted to certain patterns, which can be dealt with by the pattern validator. For example a telephone number is not really a "number" in the HTML forms sense of the word. It is really a string of digits, which could be validated with something like Validators.pattern(/\d{9,11}/) for example.

packages/forms/src/validators.ts

sonukapoor · 2020-03-25T12:10:47Z

@AndrewKushnir / @petebacondarwin Something is wrong with CI. It's failing with a 403 error.

petebacondarwin · 2020-03-25T12:29:16Z

Not your fault @sonukapoor - the setup task makes requests to the Github API in order to check some stuff related to the current master commits, etc. Since this is done via the unauthenticated API, we occasionally overrun the number of allowed requests. I'll re-run the job and see if it gets through.

sonukapoor · 2020-04-09T18:55:12Z

@AndrewKushnir Thanks, your version sounds good to me. I will update both, the commit and the PR description.

sonukapoor · 2020-04-09T20:46:55Z

@AndrewKushnir Updated.

AndrewKushnir

Thanks for updating breaking changes section Sonu! 👍

The changes LGTM, I just left a final comment on the tests, could you please have a look when you get a chance?

packages/forms/test/validators_spec.ts

kara

Change LGTM, but can we update the first paragraph of the commit message to read:

Previously, the behavior of the `minLength` and `maxLength` validators
caused confusion, as they appeared to work with numeric values but 
did not in fact produce consistent results. This commit fixes the issue 
by skipping validation altogether when a numeric value is used.

AndrewKushnir · 2020-05-05T00:24:21Z

@sonukapoor I'll start Global Presubmit tonight and if it goes well, we'll be able to add this PR to the merge queue. I noticed that it was rebased 25 days ago, so it'd be great to do one more rebase, so this PR is in the up-to-date state and we have a fresh CI run. Thank you.

AndrewKushnir · 2020-05-05T02:04:00Z

Previously, the behavior of the `minLength` and `maxLength` validators caused confusion, as they appeared to work with numeric values but did not in fact produce consistent results. This commit fixes the issue by skipping validation altogether when a numeric value is used. BREAKING CHANGES: * The `minLength` and `maxLength` validators now verify that a value has numeric `length` property and invoke validation only if that's the case. Previously, falsey values without the length property (such as `0` or `false` values) were triggering validation errors. If your code relies on the old behavior, you can include other validators such as [min][1] or [requiredTrue][2] to the list of validators for a particular field. [1]: https://angular.io/api/forms/Validators#min [2]: https://angular.io/api/forms/Validators#requiredTrue Closes angular#35591

sonukapoor · 2020-05-05T02:37:24Z

@kara I have changed the message.

@AndrewKushnir All done and rebased.

AndrewKushnir · 2020-05-05T16:39:30Z

Thanks for the updates @sonukapoor 👍

FYI, presubmits went well, so I'm marking this PR as ready for merge.

angular-automatic-lock-bot · 2020-06-05T16:10:38Z

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

…36157) Previously, the behavior of the `minLength` and `maxLength` validators caused confusion, as they appeared to work with numeric values but did not in fact produce consistent results. This commit fixes the issue by skipping validation altogether when a numeric value is used. BREAKING CHANGES: * The `minLength` and `maxLength` validators now verify that a value has numeric `length` property and invoke validation only if that's the case. Previously, falsey values without the length property (such as `0` or `false` values) were triggering validation errors. If your code relies on the old behavior, you can include other validators such as [min][1] or [requiredTrue][2] to the list of validators for a particular field. [1]: https://angular.io/api/forms/Validators#min [2]: https://angular.io/api/forms/Validators#requiredTrue Closes angular#35591 PR Close angular#36157

googlebot added the cla: yes label Mar 20, 2020

pullapprove bot requested a review from AndrewKushnir March 20, 2020 11:55

petebacondarwin added area: forms target: patch This PR is targeted for the next patch release type: bug/fix labels Mar 20, 2020

ngbot bot modified the milestone: needsTriage Mar 20, 2020

petebacondarwin approved these changes Mar 20, 2020

View reviewed changes

AndrewKushnir added the action: review The PR is still awaiting reviews from at least one requested reviewer label Mar 20, 2020

AndrewKushnir requested changes Mar 20, 2020

View reviewed changes

packages/forms/src/validators.ts Outdated Show resolved Hide resolved

sonukapoor requested a review from AndrewKushnir March 20, 2020 19:14

AndrewKushnir requested changes Mar 20, 2020

View reviewed changes

packages/forms/src/validators.ts Outdated Show resolved Hide resolved

sonukapoor force-pushed the forms-min-validator-fix branch from ebed1ab to 83e15d8 Compare March 25, 2020 11:12

sonukapoor changed the title ~~fix(forms): verify 0 properly in the validator~~ fix(forms): handle numeric values properly in the validator Mar 25, 2020

sonukapoor requested a review from AndrewKushnir March 25, 2020 11:13

sonukapoor force-pushed the forms-min-validator-fix branch from 83e15d8 to 5631a5c Compare March 25, 2020 11:40

sonukapoor mentioned this pull request Mar 27, 2020

fix(forms): clarify that minLength and maxLength should be used w… #36242

Closed

14 tasks

sonukapoor force-pushed the forms-min-validator-fix branch from 23e9d3e to 48aa5eb Compare March 27, 2020 02:04

sonukapoor requested a review from petebacondarwin March 27, 2020 02:10

sonukapoor force-pushed the forms-min-validator-fix branch 3 times, most recently from 8d1ed82 to 1cc99f2 Compare March 31, 2020 09:45

sonukapoor force-pushed the forms-min-validator-fix branch from d497cd8 to 3e12381 Compare April 5, 2020 16:45

AndrewKushnir added breaking changes target: major This PR is targeted for the next major release and removed target: patch This PR is targeted for the next patch release labels Apr 6, 2020

sonukapoor force-pushed the forms-min-validator-fix branch 2 times, most recently from e377758 to c165918 Compare April 8, 2020 01:41

sonukapoor force-pushed the forms-min-validator-fix branch from 55eb59e to 0b6c43c Compare April 9, 2020 20:45

AndrewKushnir approved these changes Apr 9, 2020

View reviewed changes

packages/forms/test/validators_spec.ts Outdated Show resolved Hide resolved

AndrewKushnir requested a review from kara April 9, 2020 22:11

AndrewKushnir approved these changes Apr 9, 2020

View reviewed changes

petebacondarwin approved these changes Apr 15, 2020

View reviewed changes

AndrewKushnir mentioned this pull request Apr 22, 2020

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

Closed

14 tasks

kara approved these changes May 4, 2020

View reviewed changes

kara assigned sonukapoor May 4, 2020

sonukapoor added 3 commits May 4, 2020 22:18

fixup! fix(forms): handle numeric values properly in the validator

b60e251

fixup! fix(forms): handle numeric values properly in the validator

28c5190

sonukapoor force-pushed the forms-min-validator-fix branch from c0a9220 to 28c5190 Compare May 5, 2020 02:18

AndrewKushnir removed action: cleanup The PR is in need of cleanup, either due to needing a rebase or in response to comments from reviews action: presubmit The PR is in need of a google3 presubmit labels May 5, 2020

AndrewKushnir added action: merge The PR is ready for merge by the caretaker risk: low labels May 5, 2020

alxhub closed this in 88a235d May 5, 2020

IgorMinar mentioned this pull request May 16, 2020

fix(dev-infra): misc fixes for the compare master and patch script #37150

Closed

3 tasks

angular-automatic-lock-bot bot locked and limited conversation to collaborators Jun 5, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(forms): handle numeric values properly in the validator #36157

fix(forms): handle numeric values properly in the validator #36157

sonukapoor commented Mar 20, 2020 •

edited

petebacondarwin left a comment

sonukapoor commented Mar 25, 2020

petebacondarwin commented Mar 25, 2020

sonukapoor commented Apr 9, 2020

sonukapoor commented Apr 9, 2020

AndrewKushnir left a comment

kara left a comment

AndrewKushnir commented May 5, 2020

AndrewKushnir commented May 5, 2020

sonukapoor commented May 5, 2020

AndrewKushnir commented May 5, 2020

angular-automatic-lock-bot bot commented Jun 5, 2020

fix(forms): handle numeric values properly in the validator #36157

fix(forms): handle numeric values properly in the validator #36157

Conversation

sonukapoor commented Mar 20, 2020 • edited

PR Checklist

PR Type

What is the current behavior?

What is the new behavior?

Does this PR introduce a breaking change?

Other information

petebacondarwin left a comment

Choose a reason for hiding this comment

sonukapoor commented Mar 25, 2020

petebacondarwin commented Mar 25, 2020

sonukapoor commented Apr 9, 2020

sonukapoor commented Apr 9, 2020

AndrewKushnir left a comment

Choose a reason for hiding this comment

kara left a comment

Choose a reason for hiding this comment

AndrewKushnir commented May 5, 2020

AndrewKushnir commented May 5, 2020

sonukapoor commented May 5, 2020

AndrewKushnir commented May 5, 2020

angular-automatic-lock-bot bot commented Jun 5, 2020

sonukapoor commented Mar 20, 2020 •

edited