[Console] `#[Option]` rules & restrictions #59602

kbond · 2025-01-23T15:28:19Z

Q	A
Branch?	7.3
Bug fix?	no
New feature?	no
Deprecations?	no
Issues	n/a
License	MIT

Adds tests for the rules described in this table
Adds exceptions/tests for the restrictions described in this table

GromNaN · 2025-01-23T16:04:36Z

There are also cases where the parameter doesn't have a default value, or where the default value is inconsistent.

#[Option] bool $arg or #[Option] bool $arg = true: same behavior as #[Option] bool $arg = false: VALUE_NONE
#[Option] string $arg: same behavior as #[Option] ?string $arg = null: VALUE_REQUIRED

kbond · 2025-01-23T16:15:27Z

I suggest we make options require a default in their argument definition:

#[Option] ?string $foo: exception
#[Option] ?string $foo = null: VALUE_REQUIRED
#[Option] bool $arg: exception

#[Option] bool $arg = true

I think, right now, that would be VALUE_NONE | VALUE_NEGATABLE but this feels unintuitive to me. @yceruto pointed out to me that this could be useful for deprecating default values though (like what was done with --complete/--force in the doctrine:schema:update command).

stof · 2025-01-23T17:58:56Z

@kbond for a no-op boolean flag, you still need to keep the same default value than before, because you still need to detect the explicit usage of the flag to display deprecation warning (but you would then ignore that flag for the actual logic)

kbond · 2025-05-03T15:03:58Z

for a no-op boolean flag, you still need to keep the same default value than before, because you still need to detect the explicit usage of the flag to display deprecation warning (but you would then ignore that flag for the actual logic)

@stof, wouldn't #[Option] ?bool $arg = null work for this? If anything other than null, trigger a deprecation?

chalasr · 2025-05-03T16:26:07Z

For instance, #[Option] ?bool $arg = false should be invalid and throw an exception because it doesn't make sense.

I agree, better expose a negatable option for such cases and then the false default is pointless.

I suggest we make options require a default in their argument definition:

Not sure as using #[Option] ?bool $colors works well to me.

#[Option] bool $arg = true.
I think, right now, that would be VALUE_NONE | VALUE_NEGATABLE but this feels unintuitive to me.

Same as above, I think it should be #[Option] ?bool $colors resulting in

not passed : null
--colors: true
--no-colors: false

No? And #[Option] ?bool $colors = null would be strictly equivalent.

wouldn't #[Option] ?bool $arg = null work for this? If anything other than null, trigger a deprecation?

Makes sense to me.

kbond · 2025-05-03T16:30:50Z

Thanks for the review!

Not sure as using #[Option] ?bool $colors works well to me.

I'm sure it's fine but it isn't crystal clear that null would be passed here if no option is passed.

kbond · 2025-05-03T16:41:22Z

What about #[Option] array $arg = []? Are you thinking #[Option] array $arg should be valid (and an empty array passed)?

I sort of thought it was easier to reason about if we have a hard "all options require a default" rule.

chalasr · 2025-05-03T17:27:43Z

I'm sure it's fine but it isn't crystal clear that null would be passed here if no option is passed.

No blocker but I feel like it is clear enough that ? is what actually handles the case where the option is absent and so you'll get a null value.

What about #[Option] array $arg = []? Are you thinking #[Option] array $arg should be valid (and an empty array passed)?

I think yes.

I sort of thought it was easier to reason about if we have a hard "all options require a default" rule

And I don't strongly disagree, I'm just leaning towards leaving this up to implementors as I can't think of any issue it could cause and one may be happy to lighten their signatures.

#[Option] array $arg = ['default']: VALUE_REQUIRED | VALUE_ARRAY (maybe? it feels ambiguous to me - are passed options appended?)

If by appended you mean that you end up with the default values + the user values in the array then I would say no. As soon as one set such array option once the default is lost IMHO

kbond · 2025-05-03T17:42:56Z

Ok, I'm good with this, couple of remaining questions, what should the following do?

#[Option] bool $arg default to false but make the option negatable?
#[Option] bool $arg = true make the option negatable or just noop?

GromNaN · 2025-05-03T17:47:42Z

If there is some combination of parameter type, default value and input type that seems illogical, we can throw a LogicException.

chalasr · 2025-05-03T17:50:56Z

Ok, I'm good with this, couple of remaining questions, what should the following do?

#[Option] bool $arg default to false but make the option negatable?

#[Option] bool $arg = true make the option negatable or just noop?

What about no default value means negatable, default means not?

@GromNaN I’m not sure to understand, could you illustrate?

kbond · 2025-05-03T17:56:02Z

If there is some combination of parameter type, default value and input type seems illogical, we can throw a Logo exception.

That's kind of what I'm trying to do here: detailing things that don't make sense and throw logic exceptions.

What about no default value means negatable, default means not?

I'm ok with that. But when there's no default (#[Option] bool $arg), what should the default be (if no option is passed), false I think. Again, though, like ?bool $arg, this isn't totally clear imo.

chalasr · 2025-05-03T18:56:54Z

Default false (when omitted) matches with what Input::getOption() does so that could work. I was more thinking about using null as the general marker for "not passed" as you don't have the hasOption() check, which basically means all invoke parameters configured as input options must be nullable.
If we go with false as default for not passed, I'm starting to think we may need to make negatable configurable opt-in through the Attribute... too much assumption. wdyt?

kbond · 2025-05-03T19:58:38Z

That all sounds good. I'll adjust this PR to enforce/accommodate these rules (hopefully in a way that makes it clear).

yceruto · 2025-05-03T20:41:29Z

Hey there! here are my thoughts about some points

I like the rule that options must define a default value, it's aligned with the option definition and nature. Assuming default value will lead to implicit behavior that’s harder to reason about, debug, and document, especially when options are optional by design. A clear default forces the developer to make an intentional decision.
#[Option] ?bool $arg = null: VALUE_NONE | VALUE_NEGATABLE I'm more to throw an exception in this case than open it to assumptions. Cause that only boolean options are negatable, and passing --no-opt makes sense when the default value is true, otherwise you don't need to negate.
#[Option] ?string $arg = null: VALUE_REQUIRED this one is confusing to me, ppl can think the opposite as they are setting null as default.

yceruto · 2025-05-03T20:45:05Z

#[Option] array $arg = ['default']: VALUE_REQUIRED | VALUE_ARRAY (maybe? it feels ambiguous to me - are passed options appended?) IMO it should never append

kbond · 2025-05-03T20:54:48Z

I like the rule that options must define a default value,

This was my thinking also - you articulated it better 😉

#[Option] ?bool $arg = null: VALUE_NONE | VALUE_NEGATABLE I'm more to throw an exception

How else could we allow negatable options (where you need to know if it was explicitly passed)?

[Option] ?string $arg = null: VALUE_REQUIRED this one is confusing to me, ppl can think the opposite as they are setting null as default.

Again, how else could we handle this case?

yceruto · 2025-05-03T21:16:13Z

#[Option] ?bool $arg = null: VALUE_NONE | VALUE_NEGATABLE I'm more to throw an exception
How else could we allow negatable options (where you need to know if it was explicitly passed)?

Forget that comment, I just saw that it's the current case, and allow to check the option was explicitly passed. Nothing new there regarding the current implementation, right?

yceruto · 2025-05-03T21:20:50Z

[Option] ?string $arg = null: VALUE_REQUIRED this one is confusing to me, ppl can think the opposite as they are setting null as default.

Again, how else could we handle this case?

Here IMO it has a different meaning, a nullable option should not require a value (seeing it from the usage perspective). I think this case can be currently handled with [Option] string $opt = '': VALUE_REQUIRED instead

kbond · 2025-05-03T22:05:24Z

Nothing new there regarding the current implementation, right?

I don't believe so but I'll double check.

I think this case can be currently handled with [Option] string $opt = '': VALUE_REQUIRED instead

That's fair. I guess the only thing I don't like is not being able to distinguish between no option passed and --arg="". (I don't believe this to be a real scenario but maybe I'm wrong)

yceruto · 2025-05-03T22:37:05Z

Nothing new there regarding the current implementation, right?

I don't believe so but I'll double check.

that case is actually tested in testBinaryInputOptions the $c option.

yceruto · 2025-05-03T22:49:07Z

I think this case can be currently handled with [Option] string $opt = '': VALUE_REQUIRED instead

That's fair. I guess the only thing I don't like is not being able to distinguish between no option passed and --arg="". (I don't believe this to be a real scenario but maybe I'm wrong)

I don’t see a real scenario either, but you can still use $input->hasParameterOption('--arg') to check whether the option was passed or not.

yceruto · 2025-05-03T23:04:30Z

I’d like to revisit these general rules from a usage perspective, not because they’re ideal, but because they reflect how we’ve implemented the current behavior:

Omitting an option hydrates its default value (which must be defined); If necessary, use $input->hasParameterOption('--opt') to check whether the option was explicitly passed and matches the default value.
Passing an option with a value --opt=value hydrates that value (only valid for non-boolean options);
Passing an option without a value --opt hydrates:
- true for boolean options
- null for others (only valid for nullable options)

yceruto · 2025-05-05T23:01:59Z

Found some signatures that probably deserve a LogicException:

#[Option] ?bool $opt = true/false (focus on the nullable type ? when default value is not null)

Since boolean options are defined as VALUE_NONE, using --opt sets the value to true, omitting the option sets the value to true or false (depending on its default value), and negating --no-opt sets the value to false, there is no way you can receive a null value in this case. So, the exception message should suggest removing the nullable type from the signature or set null as default value (which will be used when omitted).
#[Option] ?array $opt = [] (focus on the nullable type ?)

Since an array of values is expected (or the default value if omitted), I can't find a use case for passing the options without a value, e.g. --opt or --opt --opt for an array option. The exception should suggest removing the nullable type from the signature, so --option=value is always required in this case.
?string $opt = null (maybe this one too? focus on the null as default value)

Since omitting the option sets the value to null (default), the intent behind passing the option without a value results in (null) the same outcome as omitting the option, making this intent unclear. Here we could suggest setting a non-null default value instead.

chalasr · 2025-05-05T23:12:43Z

Had to stare at 3 for a while but yes, that all sounds good to me.

kbond · 2025-05-06T03:08:30Z

Thanks for all the feedback! I'll finalize this PR this week.

kbond · 2025-05-07T23:35:57Z

Ok, PR updated:

Adds tests for the rules described in this table (they all worked w/o changes)
Adds exceptions/tests for the restrictions described in this table (I had to add some checks for two of these)

There's likely other logic checks we could make but I didn't want to go overboard.

I have to admit, I find the nuance between 5 & 6 in this table awkward to reason about (btw, 5 works but 6 throws exception). For 5, it's really not obvious that this will be VALUE_OPTIONAL and that null will be the value when the option is used w/o a value. It feels like it should throw an exception as it's similar to ?bool = false (nullable parameter with non-null default).

~~I'd suggest making #59602 (comment) possible before 7.3 and use this to document this option type. (I can do this in a follow-up PR)~~

I've discovered this is just how VALUE_OPTIONAL works - when the option is used w/o a value, null is given. I don't believe we can change this so this is not possible (unless I'm mistaken).

src/Symfony/Component/Console/Tests/Command/InvokableCommandTest.php

yceruto

LGTM! thanks Kevin!

chalasr · 2025-05-09T14:50:38Z

Thank you @kbond.

This PR was merged into the 7.3 branch. Discussion ---------- [Console] `#[Option]` rules & restrictions | Q | A | ------------- | --- | Branch? | 7.3 | Bug fix? | no | New feature? | no | Deprecations? | no | Issues | n/a | License | MIT 1. Adds tests for the rules described in [this table](symfony/symfony#59602 (comment)) 2. Adds exceptions/tests for the restrictions described in [this table](symfony/symfony#59602 (comment)) Commits ------- 41a5462f0d4 [Console] `#[Option]` rules & restrictions

tacman · 2025-05-11T14:04:14Z

I just upgraded to beta-2 and wanted to give feedback as now most of my CLI commands won't even compile anymore, because I depend on null strings in options.

For example, I have a default transport that in my dev environment is set to sync, in production it is async. I inject that information in the contructor, and then use it in the commen

    public function __construct(
        #[Autowire('%env(DEFAULT_TRANSPORT)%')] private ?string $defaultTransport = null,
    ) {
    }

    public function __invoke(
        SymfonyStyle                                                                                $io,
        #[Option(description: 'message transport')] ?string $transport = null,
   ) {
         $transport ?== $this->defaultTransport;
   }

Similarly, I rely on null to mean that an option wasn't pass it, so I can prompt for it.

            if ($transition) {
                assert(in_array($transition, $transitions), "invalid transition:\n\n$transition: use\n\n" . join("\n", $transitions));
            } else {
                $question = new ChoiceQuestion(
                    'Transition?',
                    $transitions,
                    0
                );

                $transition = $io->askQuestion($question);

            }

This worked fine in zenstruck/console-extra and beta1, and I use it all the time.

    public function __invoke(
        SymfonyStyle                                                                                $io,
        #[Argument(description: 'class name')] ?string                  $className = null,
        # to override the default
        #[Option(description: 'message transport')] ?string $transport = null,
        #[Option(description: 'workflow transition')] ?string $transition = null,
        #[Option(name: 'workflow', description: 'workflow (if multiple on class)')] ?string $workflowName = null,
        // marking CAN be null, which is why we should set it when inserting
        #[Option(description: 'workflow marking')] ?string $marking = null,
        #[Option(description: 'tags (for listeners)')] ?string $tags = null,
        #[Option(name: 'index', description: 'grid:index after flush?')] ?bool $indexAfterFlush = false,
        #[Option(description: 'show stats only')] ?bool $stats = false,
        #[Option] int $limit = 0,
        #[Option(description: "use this count for progressBar")] int $count = 0,
        #[Option] string $dump = '',
    ): int {

I realize I could set all the string options to blank, but I use null to mean that nothing was pass it (the option wasn't set).

I realize nullable booleans have their own meaning (because you can have --no-option and --option), but a nullable string seems like it should mean the option wasn't passed at all.

chalasr · 2025-05-11T17:24:34Z

@tacman thanks for testing beta features. Quoting #59602 (comment):

?string $opt = null (maybe this one too? focus on the null as default value)
Since omitting the option sets the value to null (default), the intent behind passing the option without a value results in (null) the same outcome as omitting the option, making this intent unclear. Here we could suggest setting a non-null default value instead.

That's why currently using an empty string as default for such options is the way to go. Maybe this would make sense and help though:

Signature	Definition	Usage	Value
`?string\|bool $opt = null`	`VALUE_OPTIONAL`	`<omitted>` `--opt=val` `--opt`	`null` (default) `"val"` `true`

? Not sure it's already supported.

kbond · 2025-05-12T13:55:42Z

I tend to agree with @tacman - I think it'll be common to want ?string $arg = null to === VALUE_REQUIRED. So:

Signature	Definition	Usage	Value
`?string $opt = null`	`VALUE_REQUIRED`	`<omitted>` `--opt=val`	`null` (default) `"val"`

The only hesitation I have it it'd be super similar to VALUE_OPTIONAL which requires the default to be '':

Signature	Definition	Usage	Value
`?string $opt = ''`	`VALUE_OPTIONAL`	`<omitted>` `--opt=val` `--opt`	`''` (default) `"val"` `null`

@chalasr @yceruto, wdyt?

chalasr · 2025-05-12T14:48:37Z

I agree. To avoid the ambiguity, what about enforcing string|bool as the only mean to get VALUE_OPTIONAL as proposed in #59602 (comment)? (The omitted behavior is debatable)

kbond · 2025-05-12T14:54:56Z

To avoid the ambiguity, what about enforcing string|bool as the only mean to get VALUE_OPTIONAL as proposed in #59602 (comment)? (The omitted behavior is debatable)

I tried this but because of how VALUE_OPTIONAL works, I don't believe it's possible - see the last part of #59602 (comment)

kbond · 2025-05-12T14:57:23Z

Hmm, actually, maybe I can adjust the value here:

symfony/src/Symfony/Component/Console/Command/InvokableCommand.php

Line 108 in db8e84d

    
           private function getParameters(InputInterface $input, OutputInterface $output): array

I can give this another try

This PR was merged into the 7.3 branch. Discussion ---------- [Console] Invokable command `#[Option]` adjustments | Q | A | ------------- | --- | Branch? | 7.3 | Bug fix? | no | New feature? | yes | Deprecations? | no | Issues | Fix #59602 (comment) | License | MIT The *only* way to make an option `VALUE_OPTIONAL`. | Signature | Definition | Usage | Value | |--|--|--|--| | `string\|bool $opt = false` | `VALUE_OPTIONAL` | `<omitted>` `--opt=val` `--opt` | `false` (default) `"val"` `true` | Now throws an exception: | Signature | Definition | Usage | Value | |--|--|--|--| | ~`?string $opt = ''`~ | ~`VALUE_OPTIONAL`~ | ~`<omitted>` `--opt=val` `--opt`~ | ~`''` (default) `"val"` `null`~ | Now is valid: | Signature | Definition | Usage | Value | |--|--|--|--| | `?string $opt = null` | `VALUE_REQUIRED` | `<omitted>` `--opt=val` | `null` (default) `"val"` | | `?array $opt = null` | `VALUE_ARRAY` & `VALUE_REQUIRED` | `<omitted>` `--opt=val1` | `null` (default) `['val1']` | Commits ------- 59a4ae9 [Console] Invokable command `#[Option]` adjustments

kbond requested a review from chalasr as a code owner January 23, 2025 15:28

carsonbot added Status: Needs Work Status: Needs Review Console labels Jan 23, 2025

carsonbot added this to the 7.3 milestone Jan 23, 2025

carsonbot changed the title ~~[WIP][Console] Invokable Command InputOption::VALUE_OPTIONAL~~ [Console] [WIP] Invokable Command InputOption::VALUE_OPTIONAL Jan 23, 2025

kbond added the RFC RFC = Request For Comments (proposals about features that you want to be discussed) label Jan 23, 2025

yceruto mentioned this pull request May 5, 2025

[Console] Document invokable command symfony/symfony-docs#20932

Open

6 tasks

kbond force-pushed the feat/console-optional-value branch from cfe06ba to 06c2ece Compare May 7, 2025 23:21

kbond changed the title ~~[Console] [WIP] Invokable Command InputOption::VALUE_OPTIONAL~~ [Console] #[Option] rules & restrictions May 7, 2025

yceruto reviewed May 8, 2025

View reviewed changes

src/Symfony/Component/Console/Tests/Command/InvokableCommandTest.php Show resolved Hide resolved

[Console] #[Option] rules & restrictions

41a5462

kbond force-pushed the feat/console-optional-value branch from 06c2ece to 41a5462 Compare May 8, 2025 13:52

yceruto approved these changes May 8, 2025

View reviewed changes

carsonbot added Status: Reviewed and removed Status: Needs Review Status: Needs Work labels May 8, 2025

chalasr approved these changes May 9, 2025

View reviewed changes

chalasr merged commit e532750 into symfony:7.3 May 9, 2025
11 checks passed

chalasr mentioned this pull request May 9, 2025

[Console] Add support for invokable commands and input attributes symfony/symfony-docs#20553

Open

fabpot mentioned this pull request May 10, 2025

Release v7.3.0-BETA2 #60397

Merged

kbond deleted the feat/console-optional-value branch May 10, 2025 17:01

This was referenced May 12, 2025

[Console] #[Option] ?string $opt = null as VALUE_REQUIRED #60406

Closed

[Console] Invokable command #[Option] adjustments #60407

Merged

[Console] #[Option] rules & restrictions #59602

[Console] #[Option] rules & restrictions #59602

Conversation

kbond commented Jan 23, 2025 • edited Loading

GromNaN commented Jan 23, 2025 • edited Loading

kbond commented Jan 23, 2025 • edited Loading

stof commented Jan 23, 2025

kbond commented May 3, 2025

chalasr commented May 3, 2025 • edited Loading

kbond commented May 3, 2025

kbond commented May 3, 2025

chalasr commented May 3, 2025 • edited Loading

kbond commented May 3, 2025 • edited Loading

GromNaN commented May 3, 2025 • edited Loading

chalasr commented May 3, 2025

kbond commented May 3, 2025 • edited Loading

chalasr commented May 3, 2025 • edited Loading

kbond commented May 3, 2025

yceruto commented May 3, 2025

yceruto commented May 3, 2025

kbond commented May 3, 2025

yceruto commented May 3, 2025

yceruto commented May 3, 2025 • edited Loading

kbond commented May 3, 2025

yceruto commented May 3, 2025 • edited Loading

yceruto commented May 3, 2025

yceruto commented May 3, 2025 • edited Loading

yceruto commented May 5, 2025 • edited Loading

chalasr commented May 5, 2025

kbond commented May 6, 2025

kbond commented May 7, 2025 • edited Loading

yceruto left a comment

Choose a reason for hiding this comment

chalasr commented May 9, 2025

tacman commented May 11, 2025

chalasr commented May 11, 2025 • edited Loading

kbond commented May 12, 2025

chalasr commented May 12, 2025

kbond commented May 12, 2025

kbond commented May 12, 2025

[Console] `#[Option]` rules & restrictions #59602

[Console] `#[Option]` rules & restrictions #59602

kbond commented Jan 23, 2025 •

edited

Loading

GromNaN commented Jan 23, 2025 •

edited

Loading

kbond commented Jan 23, 2025 •

edited

Loading

chalasr commented May 3, 2025 •

edited

Loading

chalasr commented May 3, 2025 •

edited

Loading

kbond commented May 3, 2025 •

edited

Loading

GromNaN commented May 3, 2025 •

edited

Loading

kbond commented May 3, 2025 •

edited

Loading

chalasr commented May 3, 2025 •

edited

Loading

yceruto commented May 3, 2025 •

edited

Loading

yceruto commented May 3, 2025 •

edited

Loading

yceruto commented May 3, 2025 •

edited

Loading

yceruto commented May 5, 2025 •

edited

Loading

kbond commented May 7, 2025 •

edited

Loading

chalasr commented May 11, 2025 •

edited

Loading