feat(validator): allow to use custom validator

[emri99]

PR Type

New feature

BC Break

Yes, see CHANGELOG file

Description

Allow end user to define a custom validator

Sample

PhoneFormField(
  validator: (PhoneNumber? phoneNumber) {
    if (phoneNumber == null) {
      return null;
    }
    final PhoneParser parser = PhoneParser();
    if (!parser.validate(phoneNumber, PhoneNumberType.mobile)) {
      return 'not a mobile line';
    }
    if (!parser.validate(phoneNumber)) {
      return 'invalid format';
    }
  },
);

[cedvdb]

In your example you use the PhoneParser with the phoneNumberType.

I assume that's just an example you picked to be explicit. I'm not against this feature however I'm still wondering what's the use case where you'd not want the validation to happen by this lib. I can see one:

• The validation that happens in the library is faulty

In which case it should be fixed.

[emri99]

Not only, in my case I want 2 errors messages depending on invalid or not expected type. I picked up this from my real project :)

[cedvdb]

I see the need. I've one question: would it make sens to use a BasePhoneFormField as it would be mostly UI, and do the validation, formatting manually or would that be a worse api ?

[emri99]

I understand the goal of the BasePhoneFormField. Although, it may be useful for specifics uncommon use cases, missing features, or if end user want to use a third party parser, letting user do all the work manually seems like a redundant task, especially just to use a custom validator in this case.
I think the goal of this package is to provide a ready to use solution, and to provide a simple solution for common use cases.
So IMHO, I think end user should use BasePhoneFormField, and perform various task manually as a last resort solution.

One last thing out of focus: if BasePhoneFormField is expected to be used standalone by end user, it should be tested independently of the PhoneFormField; also version bump should consider this class to follow semver principles.

[emri99]

Another common use case is to set this field required as well as using a phone number validator.

You can even expose a validator class, so that it can used as below:

PhoneFormField(
  // message argument is optional
  validator: PhoneValidator.compose([
    PhoneValidator.required(context, errorText: "This field is required"),
    PhoneValidator.invalid(context, errorText: "Invalid phone number"),
    PhoneValidator.type(context, PhoneNumberType.mobile, errorText: "Not a mobile phone number"),
    PhoneValidator.country(context, ['FR', 'BE'], errorText: "Country not allowed"),
    // ...
  ]),
)

This way, end user may use multiple validators depending on the use case and define a specific message for each type if wanted. Inspired by flutter_form_builder validation

The phoneNumberType property could be a shortcut for using this kind of validator in the implementation (with assertion that phoneNumberType and validator are not supplied simultaneously)

[emri99]

I've done the changes PARTIALLY :

• PhoneValidator is created with defaults error messages, no localization texts added for now.
• I don't change the phoneNumberType usage for now.

This may be confusing to have phoneNumberType, errorText message, and validator, I can see 2 ways of handling this :

1. perform an assertion on PhoneFormField constructor to check that phoneNumberType and validator cannot be defined simultaneously, and then internally :
   • when neither phoneNumberType nor validator are supplied : create the invalid validator (using errorText if supplied)
   • when phoneNumberType is supplied : create the type validator (using errorText if supplied)
   • in all case : remove the default errorText value and let property be nullable as this is now handle by the validator itself
2. remove the phoneNumberType & errorText and let user defines it own validator if needed, otherwise add a default invalid validator

In both case above, I suppose it makes sense to always add an invalid validator when user does not explicitly set a validator.
In rare case where the user don't want this default validator, it can be disable by adding a fake validator : (PhoneNumber? p) => null

IMHO I think that solution 2 is more reliable : less conditions to test, less tests to write, less complexity, so less code and less bugs. (Keep in mind, that solution 2 is however a BC break change)

Any ideas, recommandations before I perform these latests changes ?

[cedvdb]

In your last message, are you using the word navigator incorrectly or is it on purpose ?

For example here

remove the phoneNumberType & errorText and let user defines it own navigator if needed, otherwise add a default invalid validator

I've not yet checked the code but I like the idea of a validator interface and the user can give a new one (deprecation of old fields), because it would allow for compose method like in your example, which is an easy to read API imo.

[emri99]

It's a mistake, should be validator, last message updated ;)

[cedvdb]

I'm not against breaking changes as this library is kind of young it's better to do those changes now, with deprecation when possible is the way to go.

So all in all I'd go with option 2 maybe with a default that depends on the provided phoneNumberType, errorTxt for deprecation. However making the invalid and type validation separates makes for redudant work (validating with the type is enough, PhoneValidator.type is stronger than PhoneValidator.invalid).

[emri99]

My initial use case was to have 2 distincts message for invalid & type not allowed.

But with your remark, I think you're right, and exposing an invalid validator, with an optional type may be sufficient, just a bit less explicit.

Moreover, my original use case is still available like this:

  validator: PhoneValidator.compose([
    PhoneValidator.invalid(context, errorText: "Invalid phone number"),
    PhoneValidator.invalid(context, type: PhoneNumberType.mobile, errorText: "Not a mobile phone number"),
    // ...
  ]),

Is this what you had in mind ?

---

So all in all I'd go with option 2 maybe with a default that depends on the provided phoneNumberType, errorTxt for deprecation.

My post wasn't clear about that, suggested solution 2 is to completely remove the phoneNumberType from PhoneFormField property, otherwise it's solution one. I'm not sure to understand which properties you want to deprecate, can you explicit your idea ?

[cedvdb]

I would like to deprecate the phoneNumberType property to completely remove it in a future release. Or alternatively, we could completely remove it now but that introduce a breaking change without warning.

For a library as young as this one I'm not sure it makes sens to deprecate (as there is not a massive amount of active user) but it's recommended nevertheless.

[emri99]

I will update this way. Good practices are good practices, whatever amount of users ;)

• What about the errorText property, do you think it makes sense to remove it in a future release too (and thus, deprecate it too) ? My opinion is yes, looking forward to hear yours ;)
• Do you want me to write a migration guide ?

But with your remark, I think you're right, and exposing an invalid validator, with an optional type may be sufficient, just a bit less explicit.

• May you tell me what you think of my proposition above when you have time ?

Sorry for multiple questions in one, as I will be offline in a moment for the next few hours and will try to finish this tonight, this might be helpful for me to have your opinion when I will get back so I can make the changes in the expected way ;)

[cedvdb]

What about the errorText property, do you think it makes sense to remove it in a future release too (and thus, deprecate it too) ?

Yes, it is now the responsibility of the validator.

Do you want me to write a migration guide ?

In my opinion if the validation is explained in the documentation and the changelog is explicit, then a migration guide is not necessary.

But with your remark, I think you're right, and exposing an invalid validator, with an optional type may be sufficient, just a bit less explicit.

Well I'm not sure and I was not sure when writing it either. I hoped the discussion would bring us toward a decision. If we consider the use cases people generally have I'd say they generally want any phone number or a mobile phone only.

In that case maybe PhoneValidator.invalidMobile & PhoneValidator.invalidFixedLine & PhoneValidator.invalid, would be more explicit. I don't think using two of those in a compose is a common use case even though it is your use case. I mean that if you require a mobile phone number and the phone number entered is not a mobile phone, the error message 'Invalid mobile phone number' still makes sens.

[emri99]

In that case maybe PhoneValidator.invalidMobile & PhoneValidator.invalidFixedLine & PhoneValidator.invalid, would be more explicit.

Totally agree. Naming is hard and so important. Will update this way

However, I don't totally agree with your last assumption about required & composed validators as I have exactly this specific use case:

• missing field should display error : You must enter a value
• invalid phoneNumber should display : Incorrect format
• fixed line field should display : This number is not a valid cellphone number

Moreover, I think that as this is a very simple feature to implement, it's worth providing an easy way to achieve such needs even if majority of them would probably use only one validator without using composed validator.

[cedvdb]

Wouldn't this work ?

Validators.compose([
  Validators.required(),
  Validators.invalid('Incorrect format'),
  Validators.invalidType(PhoneNumberType.mobile, 'This number is not a valid cellphone number'),
])

I just think validator.type is not explicit enough

[emri99]

I've refactor code according to your advices.

PhoneValidator.required(context, ...),
PhoneValidator.invalid(context, ...)
PhoneValidator.invalidType(context, expectedType, ...)

// convenience method for PhoneValidator.invalidType(context, PhoneNumberType.mobile, ...)
PhoneValidator.invalidMobile(context, ...)  

// convenience method for PhoneValidator.invalidType(context, PhoneNumberType.fixedLine, ...)
PhoneValidator.invalidFixedLine(context, ...)

Can you please validate the default message in English so I can make the translations for others languages ? It's quite a repetitive task to translate in all supported languages, so I would like to avoid doing it twice as possible ;)
Note that I will use Deepl for this, tell me if you have a better solution/strategy ;)

---

I have another interrogation about deprecating errorText in BasePhoneFormField.
I did not find any other solution than removing default value and make errorText nullable to ensure it will still work on both previous and new manner of handling validator.
However this seems like a breaking change as a standalone BasePhoneFormField may not flag the field as invalid when :

• supplied without errorText
• and if a custom validator is not properly written (ie: not returning the String that user want to be displayed)

But as it seems to be like a bug on previous version (ie: the validator returned value was ignored due to the usage of errorText property), I think this an acceptable BC. Curious to hear what's your opinion about that ;)

[cedvdb]

Sorry, for some reason I did not see your last message:

For your other PR I made a change to the translation files which are now .arb files and need to be generated with the command -flutter gen-l10n . The property keys must be valid dart property names though. So please either use valid names in the json files so it will be easy for me to make the merge, or rebase on the localization_arb_fix branch and update the .arb files located in lib/l10n. The later being the ideal case. Alternatively we wait and see what comes out of the flutter issue, but I doubt it is going to be fixed fast flutter/flutter#90079 . I managed to replicate the issue with only flutter code so the issue is on their end.

In any case I believe that the property names should be the same whether we use a localization strategy or another.

Therefor the properties should look like this (camelCased):

  "phoneNumberRequired": "Phone number required",
  "phoneNumberInvalid": "Phone number invalid",
  "invalidCountry": "Invalid country", // although I'm not sure where that is displayed ?
  "invalidMobilePhoneNumber": "Invalid mobile phone number",
  "invalidFixedLinePhoneNumber": "Invalid fixed line phone number",

For the error text issue, tbh I did not understand exactly what you were saying. Does the field not become invalid when the errorText is not provided by the user ? If the field is marked as invalid when it is invalid I don't see an issue with a BC especially if it's a bug.

I'm eager to merge this PR and the other one as the library seems to be getting some traction currently. So ask me if help is needed somewhere. I have updates of my own in the making also.

[emri99]

Sorry for the delay, this was a busy week.

• For the localization strings keys, I will update this and perform a rebase. Be aware that it can introduce breaking change for english application as the PhoneFormLocalization was optional to get corrects invalid message. Updating this will make it required otherwise the camelCased strings will be used instead ;)

• For the errorText, if the errorText is not provided, it still flag the field as invalid with the previous default errorText as expected. The problem is only with the usage of BasePhoneFormField when supplying validator property. Previously, the validator output was not used because the getErrorText method was returning the widget errorText property and not the errorText state info. I maintain this weird behavior (seems more like a bug imho) to avoid breaking change. This may however create kind of a breaking change if validator was defined manually and not used properly.

Here's an example of a previous code that will be affected:

BasePhoneFormField(
  validator: (...) {
    if (/* check for invalid */) {
      return 'fake message not expected to be used as widget.errorText ' 
        'was displayed (validator output was ignored)';
    }
  } 
  //errorText not defined
)

With the above example :

• previously: the default previous constructor argument errorText was displayed (ie: Invalid phone number)
• now: it will display the fake message above as I have removed the default errorText value and change it to a nullable String

[emri99]

Hmmm, due to the recently introduced localization changes (from localization_arb_fix branch), this PR is already breaking backward compatibility.
Thus, I don't see the interest anymore in deprecating errorText & phoneNumberType, only cons (more complex, more error prone, more confusing for end user). What's your opinion about this ?

---

About new translations, 2 solutions proposed:

• you update the missing 5 translations strings in all supported languages
• you validate/updates the english ones proposed and I perform the updates for others supported languages

In any case, I would really appreciate if you can share the tool used (if you have one ;)) as my original idea of using deepl seems to be impossible as it does not support all the languages presents in this package. Thanks !

[cedvdb]

Yeah just go ahead and remove the unnecessary code, let's keep things simple.

For the translation, When I did it the first time for all countries I used google translation. However I'm don't recall how I managed to keep the keys untranslated. Maybe I wrote a short script in the browser console ? I don't know.

It's not as long as it sounds though, it's just translating from english and switching through the languages and copy pasting. I can do it if needed.

[emri99]

I don't think translations keys are problematics as they are not reals words anymore. At least in deepl, they aren't translated.

FYI, at work, according to our team mates who are native from others countries, we use deepl because they find it more efficient and it produces less word by word translations than google translations.

[emri99]

Ouch, I don't think the add-favorites feature should be included here. I've rebased on localization_arb_fix and it seems that this branch includes WIP code. I've squash my commits after rebasing so it looks like only my last commit and yours addressing l10n issues should be included here.

Take a look and tell me if you want me to rebase/squash for a proper git history and/or separated features branches.

I've also updated CHANGELOG & README, don't hesitate to reword for better comprehension/english ;)

[cedvdb]

Ho damn, yeah I might have started from your other branch for that fix to see if it fixed your issue.

As you might have seen I'm not a git history maniac, quite the contrary. Previously as lead dev I received some slack about that from my team, so eventually someone else dealt with it. However since this project is growing in size of contributors it will become increasingly important to have a standard clean git strategy.

So for future PR I'll adhere to the standard of squashing for pull request and not mixing things up.

Concerning this one, the git history here just has too many files to do a clean review. If you already rebased, fixed the comments and the test pass I'm okay with either (merging this as is or merge before your rebase).

Whatever gets the result done faster, as I'm awaiting for those 2 PR to incorporate some changes.

What I propose though, is merging the other merge request first. The one with validation (I merged it on a separate branch in order to create the flutter issue) but it eventually needs to be merged in dev. That flutter issue has been reproduced by the flutter team btw.

[emri99]

I can merge my last changes done yesterday about favorites countries in this one (I've added few tests), resolve conflicts with current dev branch and ensure that all tests are passing so you have only this PR to accept.

Is this ok for you ?
I hope I can do this in the evening/night or tomorrow at most :)

[cedvdb]

Yes that would be perfect. Then a new major version could be released beginning of next week. I've a breaking change I need to introduce as well and now is a good time.

What about the validators though ? We merge it first or you plan on just submitting this one with both (which is fine for me, as long as we keep the history clean going forward) ? In any case those two features are not likely to be reverted so I don't think that's going to be an issue.

[emri99]

I just notice that the BasePhoneFormField is not a FormField anymore in your last refactored branch. I'm not sure of the consequences of this change, but I'm very worried about this and usage of this widget inside a form, as well for submitting data and retrieving it, data validation, autovalidation features, and all the handling previously done by form field.
The validation I've add seems almost useless, at least obsolete with this way of handling data. Either the package/widget name are pretty confusing too. Sure you wan't to refactor this in the upcoming next version ? (I think this needs more tests in a Form context to ensure that the package will still be usable)

[cedvdb]

Before PhoneFormField was a StatefulWidget and base was FormField now it's the opposite. Making them both FormField is an option though but base is not supposed to be exposed so I don't see the use of doing it.

No the refactor does not have to be in the upcoming version but it has some neat fixes in it though. I'm still trying to fix a bug that somehow pass unit test but not manual testing.

I don't think the validation you made is useless though, it just has to be displaced onto PhoneFormField, no?

[emri99]

I miss the fact that it has been swapped between PhoneFormField and BasePhoneFormField.
Thanks for this clarification ;)

[emri99]

I don't understand the produced diff, a lots of files are fully modified but contents is the same. Did you change formatting ? spaces/tabs ?

[cedvdb]

I have merged the changes from this branch onto my other branch. There is just 2 files with errors regarding validation.

I ve done it on that branch validation_favs, does that help ?

The main issue is specifying a default validator when no context is available.

In other words I'm having trouble with this line:

PhoneNumberInputValidator validator = PhoneValidator.invalid(context),

and about the same one in the test

[emri99]

When I switch onto validations_favs I've got 16 errors on my side

[cedvdb]

there should be 5 now (related to what I mentioned previously).

As far as I understand, I'm not sure the translation can live inside the Validators side. Which is not a bad thing (separation of concerns).

PhoneValidator.invalid(context)

This is not doable where you do not have access to Context (FormField constructor)

[emri99]

Got 6 ;) but I'm still on flutter 2.2.3 because my work app cannot be upgraded until all dependencies have been updated.
Currently upgrading to 2.5.1 for this project only...

---

But this can be set to null in constructor and used later, that's what's I've done I think. Not for context, but validator is not a constant

[emri99]

[✓] Flutter (Channel stable, 2.2.3, on macOS 11.3 20E232 darwin-x64, locale fr-FR)
    • Flutter version 2.2.3
    • Framework revision f4abaa0735 (3 months ago), 2021-07-01 12:46:11 -0700
    • Engine revision 241c87ad80
    • Dart version 2.13.4

---

Got 5 errors too once flutter upgraded to 2.5.1

[emri99]

I notice that you have removed usage of the light parser in the validator class, but not the argument useLightParser. Is this just a missed point or is this voluntary ? ;)

Noticed too, I forgot to remove a comment I made to validate a test (phone_validator.dart:61-62) ;)

[cedvdb]

That is a missed point. light parser is not used anymore. Actually in the way it had no effect.
We can discuss somewhere else sure.

[cedvdb]

https://gitter.im/cedvdb/phone_form_field I've created a gitter for discussion