-
Notifications
You must be signed in to change notification settings - Fork 107
Allow static validator to run before prompt's validator. #186
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
base: main
Are you sure you want to change the base?
Allow static validator to run before prompt's validator. #186
Conversation
21f1a8b
to
3d0cd21
Compare
3d0cd21
to
4fa40f6
Compare
@@ -249,7 +249,7 @@ public static function terminal(): Terminal | |||
/** | |||
* Set the custom validation callback. | |||
*/ | |||
public static function validateUsing(Closure $callback): void | |||
public static function validateUsing(?Closure $callback): void |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Note that $validateUsing
property already allows to be set to NULL
.
I think this will cause issues with a feature in The text('What is your name?', validate: 'required|min:2'); Anyone using Prompts with Laravel will have In Laravel, we enable the testing of Prompts by disabling interactive mode and defining fallbacks that are used when the Prompt isn't interactive. E.g: Prompt::interactive(false); // Only called when running tests
TextPrompt::fallbackUsing(function (TextPrompt $prompt) {
// ...
}); This almost entirely bypasses the Prompts package, but it requires that each fallback handles the various signatures and behaviours of every Prompt. The I'm open to improving Prompts testability, but I think it should be done with a new API specifically designed to assist in testing code that uses Prompts (rather than testing Prompts itself). I.e. allow asserting that a given Prompt was called (and with the correct arguments) and to mock the return value. E.g. TextPrompt::expects('What is your name')->andReturns('Jess'); |
@jessarcher I've looked through the functionality you are referring to in laravel and can see that the I agree that this change will break that functionality if But from the point of Prompts, Laravel is a consumer project and should be able to adapt to changes in Prompts.
While I agree with the notion of building a better testing API, this PR addresses an existing API introduced in another PR. Specifically, Since this current test API is the only test API we, the consumer project authors, currently have, the proposed change to re-order the |
Summary
Prompts allow to specify a validator for a prompt using
validate()
closure or static$validateUsing
closure.Prompt::validate()
contains amatch()
with:This will call the Prompt's validator before the static validator.
Problem
When testing prompts (via PHPUnit or Pest) in consumer projects using interactive inputs, the prompt that has failed the validation will halt the test execution expecting an input.
To work around this, a test executor (PHPUnit in the example below), could override the handling of the incorrect validator to, for example, throw an exception instead of re-asking for an input, and then assert for that exception in the test:
Before this PR, the static
$validateUsing
will not be assessed if the Prompt'svalidate()
closure is set, making it impossible to intercept the validation handling.After this PR is merged, the static
$validateUsing
will take precedence and will allow to intercept the validation.Changes
static::$validateUsing
validateUsing()
method to allow acceptingNULL
as a callback to allow resetting the value of$validateUsing
. Note that the property value already allows to have aNULL
'ified callback, so the changes to this setter follow that property type definition.static::$validateUsing
and then erroneously resetting the value usingfn() => null
to clear it: this was setting the value to an empty closure that would return aNULL
instead of unsetting the callback completely, so the intended "resetting" was not working correctly resulting in "bleeding" of thestatic::$validateUsing
into other tests; this was not picked up by tests until a change was made in this PR.