Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add requiredIf() and requiredUnless() validation methods #6666

Merged
merged 3 commits into from
Jun 4, 2023

Conversation

bakerkretzmar
Copy link
Contributor

@bakerkretzmar bakerkretzmar commented Jun 1, 2023

This PR adds requiredIf() and requiredUnless() validation methods. I know that performing just the actual underlying validation here is technically already possible using $get(), but that approach results in incorrect error messages.

I've seen in other issues/PRs and in Discord that the suggested way to accomplish this is manually with ->required() and a Closure and $get(), but the error messages produced that way don't accurately describe the actual validation rules. It may be possible to manually override the messages, but that's still extra work and code that this PR makes unnecessary.

Before

TextInput::make('status')
    ->in(['pending', 'completed', 'other'])

TextInput::make('status_detail')
    ->required(fn (Closure $get) => $get('status') === 'other')

This validates correctly (validation fails if status is set to other and status_detail is left empty), but the error message for the status_detail field is just "The status detail is required." That may or may not be true depending on the other input in the form, and it could be misleading because it implies that the field is always required, which it isn't.

It's possible to get a better error message by specifying the rule manually with a string, but that's kind of annoying because you have to prefix it with data. (and I saw this discouraged in Discord because it requires knowledge of Filament's internals and could be confusing):

TextInput::make('status_detail')
    ->rule('required_if:data.status,other')

Finally, if you want the required state of the field to react to other fields and you want an accurate error message, you have to define your rules twice:

TextInput::make('status')
    ->in(['pending', 'completed', 'other'])
    ->reactive()

TextInput::make('status_detail')
    ->required(fn (Closure $get) => $get('status') === 'other')
    ->rule('required_if:data.status,other')

After

Adding dedicated methods for both these rules cleans this up nicely:

TextInput::make('status')
    ->in(['pending', 'completed', 'other']),

TextInput::make('status_detail')
    ->requiredIf('status', 'other');

This is a lot easier to read and write (in my opinion), and more importantly it provides a much more accurate and helpful error message out of the box: "The status detail field is required when status is other."

The new multiFieldValueComparisonRule() method I added here is based on multiFieldComparisonRule() but may be able to be simplified further.

See #3682 and #4373.

  • Changes have been thoroughly tested to not break existing functionality.
  • New functionality has been documented or existing documentation has been updated to reflect changes.

@what-the-diff
Copy link
Contributor

what-the-diff bot commented Jun 1, 2023

PR Summary

  • Added requiredIf and requiredUnless validation rules
    New validation rules have been introduced to improve form validation capabilities.
  • Updated documentation
    Documentation has been updated to reflect the new validation rules.

@zepfietje zepfietje added the enhancement New feature or request label Jun 1, 2023
@zepfietje zepfietje added this to the v2 milestone Jun 1, 2023
@danharrin
Copy link
Member

Hey! Thank you very much for this very thorough PR, the effort is much appreciated.

Do you think it's worth making the $stateValues optional, so you can apply a requiredIf() simply if the other field is not null?

@bakerkretzmar
Copy link
Contributor Author

@danharrin thanks! Good question, I think that case is covered by the required_with rule, which you have a dedicated ->requiredWith() method for already, but either way I don't think we can make $stateValues optional here because the value is actually required by that rule, doing something like required_if:foo will error.

@danharrin danharrin merged commit 8fd0557 into filamentphp:2.x Jun 4, 2023
11 of 12 checks passed
@danharrin
Copy link
Member

Alright, thanks!

@bakerkretzmar bakerkretzmar deleted the required-if branch June 4, 2023 17:49
@bakerkretzmar
Copy link
Contributor Author

Looks like this doesn't work exactly how I assumed it would. The validation works but using just requiredIf() on its own doesn't show and hide the asterisk and make the HTML input required dynamically, you still have to add required() and use a closure for that.

Adding something like this inside multiFieldValueComparisonRule() 'fixes' it but I'm not sure if that's the best solution, because then the new validation messages this PR enables will never appear, since it's impossible to submit the form without the requiredIf() value:

$this->required(static function (Field $component, Closure $get) use ($statePath, $stateValues) {
    $statePath = $component->evaluate($statePath);
    $stateValues = $component->evaluate($stateValues);

    return in_array($get($statePath), (array) $stateValues);
});

@danharrin thoughts? The current behaviour does work, is consistent with requiredWith() etc., and is still an improvement over ->rule('required_if:data.status,other') I think, but to really get it reactive you still need two lines:

TextInput::make('status_detail')
    ->required(fn (Closure $get) => $get('status') === 'other')
    ->requiredIf('status', 'other');

@danharrin
Copy link
Member

Maybe, we should use required() internally and then work out how to change the error message to be more appropriate?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants