[General]: non-required fields became required in v11.0.0

[techouse]

Is there an existing issue for this?

• I have searched the existing issues

Package/Plugin version

11.0.0

Platforms

• Android
• iOS
• Linux
• MacOS
• Web
• Windows

Flutter doctor

Flutter doctor

[✓] Flutter (Channel stable, 3.22.2, on macOS 14.4 23E214 darwin-arm64, locale en-GB)
    • Flutter version 3.22.2 on channel stable at /Users/techouse/fvm/versions/3.22.2
    • Upstream repository https://github.com/flutter/flutter.git
    • Framework revision 761747bfc5 (6 weeks ago), 2024-06-05 22:15:13 +0200
    • Engine revision edd8546116
    • Dart version 3.4.3
    • DevTools version 2.34.3

[✓] Android toolchain - develop for Android devices (Android SDK version 34.0.0)
    • Android SDK at /Users/techouse/Library/Android/sdk
    • Platform android-34, build-tools 34.0.0
    • ANDROID_HOME = /Users/techouse/Library/Android/sdk
    • Java binary at: /Users/techouse/Applications/Android Studio.app/Contents/jbr/Contents/Home/bin/java
    • Java version OpenJDK Runtime Environment (build 17.0.11+0-17.0.11b1207.24-11852314)
    • All Android licenses accepted.

[✓] Xcode - develop for iOS and macOS (Xcode 15.4)
    • Xcode at /Applications/Xcode.app/Contents/Developer
    • Build 15F31d
    • CocoaPods version 1.15.2

[✓] Chrome - develop for the web
    • Chrome at /Applications/Google Chrome.app/Contents/MacOS/Google Chrome

[✓] Android Studio (version 2024.1)
    • Android Studio at /Users/techouse/Applications/Android Studio.app/Contents
    • Flutter plugin can be installed from:
      🔨 https://plugins.jetbrains.com/plugin/9212-flutter
    • Dart plugin can be installed from:
      🔨 https://plugins.jetbrains.com/plugin/6351-dart
    • Java version OpenJDK Runtime Environment (build 17.0.11+0-17.0.11b1207.24-11852314)

[✓] IntelliJ IDEA Community Edition (version 2024.1.4)
    • IntelliJ at /Users/techouse/Applications/IntelliJ IDEA Community Edition.app
    • Flutter plugin version 80.0.2
    • Dart plugin version 241.17890.8

[✓] Connected device (5 available)
    • Pixel 6 (mobile)                • xxx                       • android-arm64  • Android 14 (API 34)
    • techouse’s iPhone (mobile)      • xxx                       • ios            • iOS 17.5.1 21F90
    • macOS (desktop)                 • macos                     • darwin-arm64   • macOS 14.4 23E214 darwin-arm64
    • Mac Designed for iPad (desktop) • mac-designed-for-ipad     • darwin         • macOS 14.4 23E214 darwin-arm64
    • Chrome (web)                    • chrome                    • web-javascript • Google Chrome 126.0.6478.127

[✓] Network resources
    • All expected network resources are available.

• No issues found!

Minimal code example

Code sample

FormBuilderTextField(
  name: 'middle_name',
  initialValue: context.store.middleName,
  autocorrect: false,
  autofillHints: const [AutofillHints.middleName],
  inputFormatters: [
    LengthLimitingTextInputFormatter(64),
  ],
  textCapitalization: TextCapitalization.words,
  decoration: const InputDecoration(
    labelText: 'Middle name(s)',
    hintMaxLines: 1,
  ),
  validator: FormBuilderValidators.compose([
    FormBuilderValidators.maxLength(64),
  ]),
  textInputAction: TextInputAction.next,
  onSubmitted: (_) => debugPrint('onSubmitted'),
);

Current Behavior

With v11 this field became required even though FormBuilderValidators.required() was not one of the validators.

Expected Behavior

Before v11 this field was not required and would allow the user to skip it.

Steps To Reproduce

1. create non-required field
2. try to submit form with non-required field
3. validator throws error

Aditional information

With regards to the breaking changes in v11, I would have thought that non-required fields would have behaved the same.

Does this now mean that checkNullOrEmpty effectively replaces FormBuilderValidators.required()? If so, could the documentation highlight this change?

[deandreamatias]

Yes @techouse, the checkNullOrEmpty by default is true and that is the breaking change on 11.0.0 version.
Can set checkNullOrEmpty : false to avoid this behaviour that do you not want.

What do you think that are the best way to write this on README? We are open to change

[deandreamatias]

Maybe @martijn00 can join to this discussion

[techouse]

What do you think that are the best way to write this on README? We are open to change

I'm not quite sure why this change was even done as we already had FormBuilderValidators.required() which did the job, just as an opt-in instead of an opt-out.

If nothing else some sort of migration document should be created highlighting the change.

[martijn00]

This change was done because null check behaviour was inconstant between a lot of the validators. It made sense for me to have checkNullOrEmpty set to true to avoid have all kinds of checks again which would be different per validator.

Feel free to submit a PR to clarify the behaviour.

[techouse]

This change was done because null check behaviour was inconstant between a lot of the validators.

So in other words, an internal implementation detail is the culprit of this external API change. 😧

Right, I'll do my best to write up a migration guide.

[deandreamatias]

Sorry for that @techouse. We try to improve the package but maybe this behaviour isn't the best.

What do you think about checkNullOrEmpty set false by default? Do you think that .required validator maybe don't make sense anymore?

[techouse]

We try to improve the package but maybe this behaviour isn't the best.

This will break A LOT of logic. A breaking change like this should not be a footnote in a wall of text. 😅

What do you think about checkNullOrEmpty set false by default?

Possibly, but that defeats @martijn00's work. 😅 Could you maybe add an option to set this boolean globally or via source_gen in build.yaml, i.e.

targets:
  $default:
    builders:
      form_builder_validators:
        options:
          check_null_or_empty: false

or some other way of global delivery?

Do you think that .required validator maybe don't make sense anymore?

Well, in v11 it certainly doesn't make any logical sense to have both checkNullOrEmpty and a dedicated FormBuilderValidators.required() validator, however, semantically something like required should exist as it is one of the most basic building blocks of form validation.

[martijn00]

It's not just an implementation detail. Half of the validators would just blow up and crash when using null or empty. That's a big bug and solved by this implementation. I could look into making the behaviour the other way around, but that has side effects as well. This was a major version update so it is acceptable to make a breaking change that benefits future users.

Let's have an open discussion about the best way to handle this generically and then see if it needs to be implemented.

[techouse]

Let's have an open discussion about the best way to handle this generically and then see if it needs to be implemented.

Agreed. There is no simple answer here.

[techouse]

It just dawned on me that this could be mitigated with dart fix as described in Data-driven Fixes.

What do you guys think?

[martijn00]

It could maybe be migrated with dart fix, but that would probably confuse usage. If you think about the expected behaviour of validators, how would it be? Would all accept a null or empty value as valid? In most cases you don't want that, but depending on your app on some you do. That's why there is a property for it.

There is definitely a need for better documentation on this. I'm just struggling to find a different way to handle this in the code. On itself the Required validators is still a valid one and works fine. Just when combining with other validators it has no use anymore because they would already check it.

[igor-shtein]

I think the intuitive behaviour here would be to let required validator decide if null or empty value is passed to the rest of the validators. Only if the required validator is present - the empty value is passed to the rest of the validators. Otherwise it should be assumed that the field is empty and there is nothing to validate.

And I guess it then would be fine to leave it default to true and present on each validator. I would remove the parameter in this case though. Every validator should be able to valiadte any value, including empty ones.

It is just that a required validator is now feels useless, but it also the one every developer new to the package is looking for when implementing such functionality, so it cannot be removed.