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

[11.x] Fix prompting for missing array arguments on artisan command #50850

Merged

Conversation

macocci7
Copy link
Contributor

This PR fixes the bug below:

When an Artisan Command has array arguments configuration, like:

protected $signature = 'app:foo {args*}';

promptForMissingArgumentsUsing() doesn't work,

because $input->getArgument($argument->getName()) returns empty array, and judgement in promptForMissingArguments() method of Illuminate\Console\Concerns\PromptsForMissingInput uses is_null(), which should be replaced with empty() .
empty() method also covers string argument, like:

protected $signature = 'app:foo {arg}';

* fixed prompting for missing array arguments on artisan command
fixed:tests/Console/ConsoleApplicationTest.php
@taylorotwell taylorotwell requested a review from jessarcher April 1, 2024 00:33
@taylorotwell taylorotwell marked this pull request as draft April 1, 2024 00:34
@macocci7 macocci7 marked this pull request as ready for review April 1, 2024 09:25
@taylorotwell taylorotwell marked this pull request as draft April 1, 2024 13:48
@taylorotwell
Copy link
Member

I'm leaving in draft until @jessarcher reviews it.

@macocci7
Copy link
Contributor Author

macocci7 commented Apr 1, 2024

I understand.

Co-authored-by: Jess Archer <jess@jessarcher.com>
@macocci7
Copy link
Contributor Author

macocci7 commented Apr 2, 2024

OK.That's right.
Thank you for your suggestion.

@jessarcher jessarcher marked this pull request as ready for review April 3, 2024 06:07
@taylorotwell
Copy link
Member

Personally I think this needs more thought. If you provide the argument on the CLI, you would get the argument as an array using $this->argument. With this, the argument would be a string. That feels inconsistent.

@taylorotwell taylorotwell marked this pull request as draft April 3, 2024 15:02
@jessarcher
Copy link
Member

Personally I think this needs more thought. If you provide the argument on the CLI, you would get the argument as an array using $this->argument. With this, the argument would be a string. That feels inconsistent.

Good catch. I've updated it to wrap the response in an array. There isn't a good prompt for entering multiple arbitrary values, so with the default behaviour, you'd always get an array of one item.

It's possible for someone to provide their own prompt callback that can return multiple values using whatever implementation they like (multiselect of known options, multisearch against the DB, text prompts in a loop that continues until an empty response, or some sort of space/comma/newline separated answer).

@jessarcher jessarcher marked this pull request as ready for review April 4, 2024 00:04
@taylorotwell taylorotwell merged commit 29acd21 into laravel:11.x Apr 4, 2024
28 checks passed
@taylorotwell
Copy link
Member

Thanks!

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