Movatterモバイル変換


[0]ホーム

URL:


Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

Sign up
Appearance settings

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

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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Merged
chalasr merged 1 commit intosymfony:7.3fromkbond:feat/console-optional-value
May 9, 2025

Conversation

kbond
Copy link
Member

@kbondkbond commentedJan 23, 2025
edited
Loading

QA
Branch?7.3
Bug fix?no
New feature?no
Deprecations?no
Issuesn/a
LicenseMIT
  1. Adds tests for the rules described inthis table
  2. Adds exceptions/tests for the restrictions described inthis table

@carsonbotcarsonbot added this to the7.3 milestoneJan 23, 2025
@carsonbotcarsonbot changed the title[WIP][Console] Invokable CommandInputOption::VALUE_OPTIONAL[Console] [WIP] Invokable CommandInputOption::VALUE_OPTIONALJan 23, 2025
@kbondkbond added the RFCRFC = Request For Comments (proposals about features that you want to be discussed) labelJan 23, 2025
@GromNaN
Copy link
Member

GromNaN commentedJan 23, 2025
edited
Loading

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
Copy link
MemberAuthor

kbond commentedJan 23, 2025
edited
Loading

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 beVALUE_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 thedoctrine:schema:update command).

@stof
Copy link
Member

@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
Copy link
MemberAuthor

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 thannull, trigger a deprecation?

@chalasr
Copy link
Member

chalasr commentedMay 3, 2025
edited
Loading

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 thefalse 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
Copy link
MemberAuthor

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 thatnull would be passed here if no option is passed.

@kbond
Copy link
MemberAuthor

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
Copy link
Member

chalasr commentedMay 3, 2025
edited
Loading

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 anull 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
Copy link
MemberAuthor

kbond commentedMay 3, 2025
edited
Loading

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

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

@GromNaN
Copy link
Member

GromNaN commentedMay 3, 2025
edited
Loading

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

@chalasr
Copy link
Member

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

  1. #[Option] bool $arg default to false but make the option negatable?
  2. #[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
Copy link
MemberAuthor

kbond commentedMay 3, 2025
edited
Loading

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
Copy link
Member

chalasr commentedMay 3, 2025
edited
Loading

Default false (when omitted) matches with whatInput::getOption() does so that could work. I was more thinking about usingnull as the general marker for "not passed" as you don't have thehasOption() check, which basically means all invoke parameters configured as input options must be nullable.
If we go withfalse 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
Copy link
MemberAuthor

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

@yceruto
Copy link
Member

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 istrue, 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 settingnull as default.

@yceruto
Copy link
Member

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

@kbond
Copy link
MemberAuthor

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
Copy link
Member

#[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
Copy link
Member

yceruto commentedMay 3, 2025
edited
Loading

[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
Copy link
MemberAuthor

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 betweenno option passed and--arg="". (I don't believe this to be a real scenario but maybe I'm wrong)

@yceruto
Copy link
Member

yceruto commentedMay 3, 2025
edited
Loading

Nothing new there regarding the current implementation, right?

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

that case is actually tested intestBinaryInputOptions the$c option.

kbond reacted with thumbs up emoji

@yceruto
Copy link
Member

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.

chalasr reacted with thumbs up emoji

@yceruto
Copy link
Member

yceruto commentedMay 3, 2025
edited
Loading

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:

  1. 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.
  2. Passing an option with a value--opt=value hydrates that value (only valid for non-boolean options);
  3. Passing an option without a value--opt hydrates:
    • true for boolean options
    • null for others (only valid for nullable options)
kbond reacted with thumbs up emoji

@yceruto
Copy link
Member

yceruto commentedMay 5, 2025
edited
Loading

Found some signatures that probably deserve aLogicException:

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

    Since boolean options are defined asVALUE_NONE, using--opt sets the value totrue, omitting the option sets the value totrue orfalse (depending on its default value), and negating--no-opt sets the value tofalse, there is no way you can receive anull value in this case. So, the exception message should suggest removing the nullable type from the signature or setnull as default value (which will be used when omitted).

  2. #[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.

  3. ?string $opt = null (maybe this one too? focus on thenull as default value)

    Since omitting the option sets the value tonull (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 reacted with thumbs up emoji

@chalasr
Copy link
Member

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

@kbond
Copy link
MemberAuthor

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

yceruto, OskarStark, and chalasr reacted with heart emoji

@kbondkbondforce-pushed thefeat/console-optional-value branch fromcfe06ba to06c2eceCompareMay 7, 2025 23:21
@kbondkbond changed the title[Console] [WIP] Invokable CommandInputOption::VALUE_OPTIONAL[Console]#[Option] rules & restrictionsMay 7, 2025
@kbond
Copy link
MemberAuthor

kbond commentedMay 7, 2025
edited
Loading

Ok, PR updated:

  1. Adds tests for the rules described inthis table (they all worked w/o changes)
  2. Adds exceptions/tests for the restrictions described inthis 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 inthis table awkward to reason about (btw, 5 works but 6 throws exception). For 5, it's really not obvious that this will beVALUE_OPTIONAL and thatnull 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 howVALUE_OPTIONAL works - when the option is used w/o a value,null is given. I don't believe we can change this sothis is not possible (unless I'm mistaken).

@kbondkbondforce-pushed thefeat/console-optional-value branch from06c2ece to41a5462CompareMay 8, 2025 13:52
Copy link
Member

@ycerutoyceruto left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

LGTM! thanks Kevin!

@chalasr
Copy link
Member

Thank you@kbond.

@chalasrchalasr merged commite532750 intosymfony:7.3May 9, 2025
11 checks passed
symfony-splitter pushed a commit to symfony/console that referenced this pull requestMay 9, 2025
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       | MIT1. 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
@fabpotfabpot mentioned this pull requestMay 10, 2025
@kbondkbond deleted the feat/console-optional-value branchMay 10, 2025 17:01
@tacman
Copy link
Contributor

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

publicfunction__construct(        #[Autowire('%env(DEFAULT_TRANSPORT)%')]private ?string$defaultTransport =null,    ) {    }publicfunction__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 =newChoiceQuestion('Transition?',$transitions,0                );$transition =$io->askQuestion($question);            }

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

publicfunction __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
Copy link
Member

chalasr commentedMay 11, 2025
edited
Loading

@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:

SignatureDefinitionUsageValue
?string|bool $opt = nullVALUE_OPTIONAL<omitted>
--opt=val
--opt
null (default)
"val"
true

? Not sure it's already supported.

@kbond
Copy link
MemberAuthor

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

SignatureDefinitionUsageValue
?string $opt = nullVALUE_REQUIRED<omitted>
--opt=val
null (default)
"val"

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

SignatureDefinitionUsageValue
?string $opt = ''VALUE_OPTIONAL<omitted>
--opt=val
--opt
'' (default)
"val"
null

@chalasr@yceruto, wdyt?

@chalasr
Copy link
Member

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

@kbond
Copy link
MemberAuthor

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 howVALUE_OPTIONAL works, I don't believe it's possible - see the last part of#59602 (comment)

@kbond
Copy link
MemberAuthor

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

privatefunctiongetParameters(InputInterface$input,OutputInterface$output):array

I can give this another try

chalasr reacted with thumbs up emoji

fabpot added a commit that referenced this pull requestMay 14, 2025
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       | MITThe *only* way to make an option `VALUE_OPTIONAL`.| Signature | Definition | Usage | Value ||--|--|--|--|| `string\|bool $opt = false` | `VALUE_OPTIONAL` | `<omitted>` <br> `--opt=val` <br> `--opt` | `false` (default)<br>`"val"` <br>`true` |Now throws an exception:| Signature | Definition | Usage | Value ||--|--|--|--|| ~`?string $opt = ''`~ | ~`VALUE_OPTIONAL`~ | ~`<omitted>` <br> `--opt=val` <br> `--opt`~ | ~`''` (default)<br>`"val"` <br>`null`~ |Now is valid:| Signature | Definition | Usage | Value ||--|--|--|--|| `?string $opt = null` | `VALUE_REQUIRED` | `<omitted>` <br> `--opt=val` | `null` (default)<br>`"val"` || `?array $opt = null` | `VALUE_ARRAY` & `VALUE_REQUIRED` | `<omitted>` <br> `--opt=val1` | `null` (default)<br>`['val1']` |Commits-------59a4ae9 [Console] Invokable command `#[Option]` adjustments
Sign up for freeto join this conversation on GitHub. Already have an account?Sign in to comment
Reviewers

@ycerutoycerutoyceruto approved these changes

@chalasrchalasrchalasr approved these changes

Assignees
No one assigned
Labels
ConsoleRFCRFC = Request For Comments (proposals about features that you want to be discussed)❄️ Feature FreezeImportant Pull Requests to finish before the next Symfony "feature freeze"Status: Reviewed
Projects
None yet
Milestone
7.3
Development

Successfully merging this pull request may close these issues.

8 participants
@kbond@GromNaN@stof@chalasr@yceruto@OskarStark@tacman@carsonbot

[8]ページ先頭

©2009-2025 Movatter.jp