Skip to content

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

Draft
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

AlexSkrypnyk
Copy link
Contributor

@AlexSkrypnyk AlexSkrypnyk commented Mar 8, 2025

Summary

Prompts allow to specify a validator for a prompt using validate() closure or static $validateUsing closure.

Prompt::validate() contains a match() with:

$error = match (true) {
    is_callable($this->validate) => ($this->validate)($value),
    isset(static::$validateUsing) => (static::$validateUsing)($this),
    default => throw new RuntimeException('The validation logic is missing.'),
};

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:

// Fake input.
Prompt::fake(['t', 'e', 's', 't', Key::ENTER]);

// Intercept handling of the validation by throwing an exception.
Prompt::validateUsing(function (Prompt $prompt) {
    if (is_callable($prompt->validate)) {
        $error = ($prompt->validate)($prompt->value()); // <--- similar to what Prompt::validate() does.
        
        // Intercepting and throwing an exception that will later be picked up by a test and would fail an assertion.
        if ($error) {
            throw new \RuntimeException(sprintf('Validation for "%s" failed with error "%s".', $prompt->label, $error));
        }
    }

    return NULL;
  });
  
// call the prompt  
...

// Assert for the exception.
...

Before this PR, the static $validateUsing will not be assessed if the Prompt's validate() 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

  1. Updated the order of validation callback checks to check for static::$validateUsing
  2. Updated validateUsing() method to allow accepting NULL as a callback to allow resetting the value of $validateUsing. Note that the property value already allows to have a NULL'ified callback, so the changes to this setter follow that property type definition.
  3. Updated tests that were setting the static::$validateUsing and then erroneously resetting the value using fn() => null to clear it: this was setting the value to an empty closure that would return a NULL instead of unsetting the callback completely, so the intended "resetting" was not working correctly resulting in "bleeding" of the static::$validateUsing into other tests; this was not picked up by tests until a change was made in this PR.

@AlexSkrypnyk AlexSkrypnyk force-pushed the feature/allow-static-validator-to-run-first branch from 21f1a8b to 3d0cd21 Compare March 8, 2025 01:52
@AlexSkrypnyk AlexSkrypnyk force-pushed the feature/allow-static-validator-to-run-first branch from 3d0cd21 to 4fa40f6 Compare March 8, 2025 02:02
@@ -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
Copy link
Contributor Author

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.

@taylorotwell taylorotwell requested a review from jessarcher March 10, 2025 17:33
@taylorotwell taylorotwell marked this pull request as draft March 10, 2025 17:33
@jessarcher
Copy link
Member

I think this will cause issues with a feature in laravel/framework.

The validateUsing method was introduced in #102 to allow Laravel users to pass a string or array of Laravel validation rules to Prompts in laravel/framework#49497. E.g:

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

Anyone using Prompts with Laravel will have $validateUsing defined by the framework, and currently, that implementation expects to be used only as a fallback. With this change, if someone passes a validation callback directly to a Prompt, that callback will be passed to Laravel's validation fallback, which doesn't handle closures.

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 Prompt::fake() method was only intended for internally testing Prompts handling of individual keypresses and render cycles and in my opinion isn't a great DX for folks to test their apps' usage of Prompts.

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');

@AlexSkrypnyk
Copy link
Contributor Author

@jessarcher
Thank you for reviewing the PR.

I've looked through the functionality you are referring to in laravel and can see that the validatePrompt() expects rules to be passed as an array.

I agree that this change will break that functionality if validate is callable.

But from the point of Prompts, Laravel is a consumer project and should be able to adapt to changes in Prompts.

validatePrompt() in Laravel can be updated to either evaluate the validate callback or to pass it further.


While I agree with the notion of building a better testing API, this PR addresses an existing API introduced in another PR. Specifically, $validateUsing is NULL by default and the change here just follows that up to allow re-setting the value from the setter.

Since this current test API is the only test API we, the consumer project authors, currently have, the proposed change to re-order the match() would allow us to use it in our tests without halting and waiting for input.

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.

2 participants