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

Support for custom validation and aliases #102

Merged
merged 10 commits into from
Dec 29, 2023

Conversation

cerbero90
Copy link
Contributor

@cerbero90 cerbero90 commented Dec 27, 2023

Following up the improvement for checking the Prompts validation when testing, this PR introduces the ability to globally set a custom validation logic for Laravel Prompts.

The end goal is to be able to validate Laravel Prompts by leveraging the existing Laravel validation rules, while keeping Laravel Prompts framework agnostic.

To achieve this, the static method Prompt::validateUsing() can be called to determine the global logic to validate Laravel Prompts. If a prompt has its own validation logic set via the parameter validate, such logic has the precedence over the global validation logic.

If this PR gets merged, we can then set the global validation logic in the Laravel framework by updating the ConfiguresPrompts trait like this draft PR is currently doing:

protected function configurePrompts(InputInterface $input)
{
    // ...
    Prompt::validateUsing($this->validatePrompt(...));
    // ...
}

protected function validatePrompt(Prompt $prompt)
{
    if (! $rules = $prompt->validate) {
        return null;
    }

    [$alias, $value] = [$prompt->alias(), $prompt->value()];

    $validator = Validator::make([$alias => $value], [$alias => $rules], $this->messages(), $this->attributes());

    return $validator->errors()->first();
}

protected function messages()
{
    return [];
}

protected function attributes()
{
    return [];
}

The above logic would let the user pass the Laravel validation rules to the validate parameter like this:

text('What is your name?', validate: 'required|min:2');

and take full advantage of the validator features like setting custom error messages or custom attributes.

This PR also introduces aliases so that each prompt can be uniquely differentiated. Useful for both validation and any future use-case that needs to distinguish a prompt in a short and unique way.

The alias can be set via the as parameter, e.g.:

text('What is your name?', validate: 'required|min:2', as: 'name');

Otherwise the alias is automatically inferred by counting each prompt i.e. prompt_1, prompt_2, etc.

Ultimately this PR is designed to keep Laravel Prompts framework agnostic and let the frameworks decide their own preferred way to validate Laravel Prompts.

@cerbero90
Copy link
Contributor Author

@jessarcher please note that there is a PHPStan error on the Progress class that doesn't depend on the changes of this PR 👍

@jessarcher
Copy link
Member

Hey @cerbero90,

I dig this approach for adding Laravel's validation rules without increasing the dependencies.

What are your thoughts on leaving out the as stuff for now, to reduce the changes and complexity a little? I understand one of the challenges with using the validator is that validation messages often refer to the field name, but I think we could initially just set the field name as something like "answer" on the Validator instance so the messages still make sense. I think this would also be a nicer default than prompt_1, etc, and because we display validation errors immediately alongside the prompt, I don't see the need to make them unique like you would with a standard web form.

@jessarcher jessarcher self-requested a review December 27, 2023 03:58
@jessarcher jessarcher marked this pull request as draft December 27, 2023 03:59
@cerbero90
Copy link
Contributor Author

cerbero90 commented Dec 27, 2023

Thanks for your quick reply @jessarcher :)

I'm always in favour of simplicity 👍 the reason for the aliases is merely for being able to also define custom error messages and attributes for each prompt individually.

Having answer as the default field name would mean that:

  • we can't have custom error messages/attributes per prompt
  • the value of :attribute for the Laravel validator will always be answer, e.g. The answer field is required.

An alternative to the current default aliasing may be converting the label into snake case. That would work well when we use something like Full name as a label (the alias would become full_name, which is then displayed as full name by the validator), but it would not work so well if we use labels like What is your full name?. In the latter case it would make sense to set a custom alias.

If you prefer, we can get rid of the aliases altogether but we would encounter the 2 problems listed above 🤔

@jessarcher
Copy link
Member

I think I'm okay with those problems, at least for the initial version of this feature. We can always add them in a follow-up. But just to make sure I fully understand, can you please elaborate a little more on the problems and specifically what type of customization you'd want to make that wouldn't be possible? Are you just wanting to be able to make the message say "The full name field is required" instead of "the answer field is required"?

I've also fixed the PHPStan error in main if you'd like to rebase.

@cerbero90
Copy link
Contributor Author

cerbero90 commented Dec 27, 2023

Sure, no worries :) and yes, the issue with dropping the aliases is only related to the possibility to customise the name of the field and the related error messages. If we want to have an error completely different from the default provided by the validator, we can't. Without aliases we lose the ability to completely being able to customise our output.

Another drawback is for all the validation rules that depend on other fields, like the following:

  • accepted_if
  • declined_if
  • different
  • in_array
  • missing_if
  • missing_unless
  • present_if
  • present_unless
  • prohibited_if
  • prohibited_unless
  • prohibits
  • required_if
  • required_if_accepted
  • required_unless
  • same

But if you are ok with that at this stage, I'll remove the aliases from this PR 👍

@jessarcher
Copy link
Member

Thanks! Would interdependent validation even make sense when each prompt is validated in isolation?

@cerbero90
Copy link
Contributor Author

Yes you are right, I didn't think about that :)

@cerbero90
Copy link
Contributor Author

Aliases removed 👍

@jessarcher jessarcher marked this pull request as ready for review December 27, 2023 06:22
@cerbero90
Copy link
Contributor Author

@jessarcher if it's possible to bump the version of Prompts once this PR is merged, I'll make sure to require the new version in this PR :)

@jessarcher
Copy link
Member

Sure thing - thanks!

@taylorotwell taylorotwell merged commit d814a27 into laravel:main Dec 29, 2023
4 checks passed
@cerbero90 cerbero90 deleted the feature/configurable-validation branch December 30, 2023 10:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants