Skip to content
New issue

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

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

Already on GitHub? # to your account

SelectPrompt $required is not allowed to be false #98

Closed
code-distortion opened this issue Oct 25, 2023 · 4 comments · Fixed by #99
Closed

SelectPrompt $required is not allowed to be false #98

code-distortion opened this issue Oct 25, 2023 · 4 comments · Fixed by #99
Assignees

Comments

@code-distortion
Copy link

Laravel Prompts Version

v0.1.12

Laravel Version

v10.29.0

PHP Version

8.2.11

Operating System & Version

Debian GNU/Linux 12 (bookworm) (php:8.2.11-fpm docker image)

Terminal Application

GNOME Terminal

Description

Hi Jess and Co. Prompts is a useful package and I appreciate the effort put in to it.

The relatively recent change f48bc94 forces SelectPrompt's $required bool|string to not be false anymore.

This broke the prompts for me because of the way I use the SelectPrompt.

In my project, I give the user a number of options to choose from. The last option is sometimes called "Back" and sometimes called "Quit", and has a key of '' (an empty string). For me an empty string is a consistent way of checking that the user wants to return from the current prompt / go back / exit / quit etc without having to worry about the wording. e.g.

$options = [
    'database' => 'Database Management',
    'deploy' => 'Deployment',
    '' => 'Quit',
];

$choice = select(
    label: "Main",
    options: $options,
    required: false, // <<<< This would allow '' (i.e. Quit) to be chosen
);

if ($choice === '') {
    break;
}

(My actual code is abstracted and the $options are passed in, so it's less hard-coded than this).

I can get around it by setting $required to '', but it seems odd to solve it that way.

The commit explicitly checks to make sure that $required is not false, so I thought I'd ask about the reason behind that? Or am I simply not using it as intended?

Steps To Reproduce

$options = [
    'database' => 'Database Management',
    'deploy' => 'Deployment',
    '' => 'Quit',
];

$choice = select(
    label: "Main",
    options: $options,
);

if ($choice === '') {
    break;
}

The Quit option can't be picked.

 ┌ Main ────────────────────────────────────────────────────────┐
 │   ○ Database Management                                      │
 │   ○ Deployment                                               │
 │ › ● Quit                                                     │
 └──────────────────────────────────────────────────────────────┘
  ⚠ Required.
@jessarcher
Copy link
Member

Hey @code-distortion,

The commit explicitly checks to make sure that $required is not false, so I thought I'd ask about the reason behind that? Or am I simply not using it as intended?

Before the change, $required could not be configured for the select prompt because it's inherently required to select one of the configured options. The $required parameter was added to allow customisation of the error message when the select prompt is run in non-interactive mode without a default specified. Because it was always true previously, it seemed to make sense to prevent it from being set to false to ensure that the return value would always be one of the configured options regardless of whether it's run interactively or non-interactively.

It wasn't intended for users to ever get a required validation error when running the select prompt interactively because all configured options should be valid.

I'm curious how you were able to set $required to false before f48bc94?

I think the real issue is that the validation check should allow empty-strings for the select prompt. The current check is very generic for all prompts and the empty-string check is really just for prompts like text where an empty string should not be valid.

I'll work up a PR to fix this.

I can get around it by setting $required to '', but it seems odd to solve it that way.

Agreed. It's a bit of a hack that relies on a falsy check.

@code-distortion
Copy link
Author

Hi Jess. Thank you, that's great. I understand now.

Sorry for the confusion. In the first example I showed required: false, to indicate what would help solve the problem if it worked. In my actual code before the update, I omitted the required field when calling select().

Your proposed solution of allowing the empty-string option to be chosen for select inputs seems like the right idea to me too.

@jessarcher
Copy link
Member

Great! #99 should solve this.

Thanks for your help!

@code-distortion
Copy link
Author

Yes, that's fixed it up for me. Thank you.

# for free to join this conversation on GitHub. Already have an account? # to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants