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] Explicitly passed options without value (or empty) should remain empty#21228

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

Conversation

@chalasr
Copy link
Member

@chalasrchalasr commentedJan 10, 2017
edited
Loading

QA
Branch?master
Bug fix?yes
New feature?no
BC breaks?no
Deprecations?no
Tests pass?yes
Fixed tickets#21215#11572#12773
LicenseMIT
Doc PRn/a (maybe look at updating the existing one)

This conserves empty values for options instead of returning their default values.

Code:

// cli.php$application =newApplication();$application    ->register('echo')    ->addOption('prefix',null, InputOption::VALUE_OPTIONAL,null,'my-default')    ->addArgument('value', InputArgument::REQUIRED)    ->setCode(function ($input,$output) {var_dump($input->getOption('prefix'));    });$application->run();

Before:
before

After:
after

jvasseur, ogizanagi, dakess, and julienfalque reacted with thumbs up emojiro0NL reacted with hooray emoji
@javiereguiluz
Copy link
Member

javiereguiluz commentedJan 10, 2017
edited
Loading

I don't know if it makes sense, but I expected an empty string in the third example (instead ofnull).

In fact, that was my original problem: there is a command that defines a default value for the "prefix" option. I want to use an empty string as the prefix, but there's no way to tell the command that I want an empty string.

@ro0NL
Copy link
Contributor

ro0NL commentedJan 10, 2017
edited
Loading

👍 for empty string when given empty..

cmd --foo     => ""cmd           => nullcmd --foo bar => "bar"

@chalasr
Copy link
MemberAuthor

chalasr commentedJan 10, 2017
edited
Loading

Yeah, problem is that the conversion of empty strings to null is a feature (https://github.com/symfony/symfony/blob/master/src/Symfony/Component/Console/Input/ArgvInput.php#L224).
So, should we change this behavior? If we agree, I'll try to do it here.

@ro0NL
Copy link
Contributor

ro0NL commentedJan 10, 2017
edited
Loading

Imo. the distinction betweennull/default and"" makes sense when compared to the command line.

@javiereguiluz
Copy link
Member

@ro0NL I think we are talking about different behaviors.

You say:

command            ==> foo = nullcommand --foo      ==> foo = empty stringcommand --foo=""   ==> foo = empty string

And this is what I say:

command            ==> foo = nullcommand --foo      ==> foo = nullcommand --foo=""   ==> foo = empty string

@ro0NL
Copy link
Contributor

ro0NL commentedJan 10, 2017
edited
Loading

Yep.. you're right. Maybe even better 👍

However i considered it (for my own usecase 👼) as follow

  • cmd (foo is default)
  • cmd --foo="" (foo is given)
  • cmd --foo="bar" (foo is given)
  • cmd --foo (foo is null)

In the last scenario i'd like to throw if the command is running non-interactively (--foo requires a value by design), otherwise, if interactive, and--foo is passed, i'd like toask a value.

But you're right, this is a different look at it.

@ro0NL
Copy link
Contributor

ro0NL commentedJan 10, 2017
edited
Loading

We actually talking about the same :)--foo (null) and--foo="" ("") I wrote it down wrong :)

👍

edit: depends though.. i vote for--foo (when given) beingNULL at all time,not the default value (which can be null).

@chalasr
Copy link
MemberAuthor

chalasr commentedJan 10, 2017
edited
Loading

i vote for --foo (when given) being NULL at all time

It's what this actually fixes:

// beforecmd// $input->getOption('foo') gives the default valuecmd --foo// $input->getOption('foo') gives the default valuecmd --foo=""// $input->getOption('foo') gives the default value// aftercmd// $input->getOption('foo') gives the default valuecmd --foo//  $input->getOption('foo') gives NULLcmd --foo=""// $input->getOption('foo') gives NULL (remaining problem, should give an empty string)

@ro0NL
Copy link
Contributor

ro0NL commentedJan 10, 2017
edited
Loading

My bad :), your lastafter example sounds really good (with the correct empty string handling).

Maybe for the input definition we could/should make things opt-in?InputOption::VALUE_NULL or so, otherwise im 👍 for the behavior change.

Ie.--foo being NULL vs. default will be a never ending discussion i guess. One can expect both actually..

@ogizanagi
Copy link
Contributor

Ie. --foo being NULL vs. default will be a never ending discussion i guess. One can expect both actually..

IMHO it must benull. You already get the default by callingcmd without the option. Why would you add the option then if you expect the default?

chalasr and parkertr reacted with thumbs up emoji

@ro0NL
Copy link
Contributor

ro0NL commentedJan 10, 2017
edited
Loading

I tend to agree null makes more sense as well.. as it's rather explicit. However, i could understand one saying "no value means the default value", as we technically did not pass a value. Other thennull of course.. or is it void? ;-)

Anyway.. concerning BC.. what about making the behavior opt-in?VALUE_NULL could differ between givingnull (new behavior), or default (old behavior).

@chalasr
Copy link
MemberAuthor

chalasr commentedJan 10, 2017
edited
Loading

what about making the behavior opt-in?

It would add too much complexity to something already quite complex imho, from a code pov as well as from a usage one: readingVALUE_NULL is quite confusing to me, since other constantsVALUE_OPTIONAL andVALUE_REQUIRED are not about using or not the default value but only that the option must take a value or not.
I would say it's a minor behavior break which is acceptable and quite easy to fix (i.e. don't pass--prefix with no value if your option has a default value, no code to change).

About empty strings, I think we can't handle them while keeping a consistent behavior.
Fact is that runningcmd --foo="" orcmd --foo= gives thesame result when looking at$argv, i.e.array('cmd', '--foo=');. So what do we do for the later, null or empty string?
It looks like a PHP limitation (don't know about other languages) and we have to make a choice, giving alwaysnull or always an empty string. I prefernull, as the current implementation gives, fixable using a--no-prefix option.

This fixes the fact that default value is used when giving an empty value, another issue could be opened for the fact that null is returned for an empty string, but I'm afraid we can't fix without totally changing the way we get and parse the input (and I don't see how actually).
/cc@javiereguiluz@fabpot

ogizanagi reacted with thumbs up emoji

@ro0NL
Copy link
Contributor

I would say it's a minor behavior break which is acceptable and quite easy to fix

👍

Imo.--foo= equals--foo="". The trailing= is trivial here.

@javiereguiluz
Copy link
Member

@ro0NL I don't think it's the same: An empty string is not the same as null. To me,--foo and--foo= means that there is no value, sonull. However,--foo='' is different because we are passing an empty string as the value, so there is a value and it's an empty string.

@ro0NL
Copy link
Contributor

ro0NL commentedJan 10, 2017
edited
Loading

But quotes are optional..--foo=bar equals--foo="bar" as well (and equals--foo bar).

chalasr reacted with thumbs up emoji

@chalasr
Copy link
MemberAuthor

chalasr commentedJan 10, 2017
edited
Loading

However, --foo='' is different because we are passing an empty string as the value, so there is a value and it's an empty string

Sadly, regarding php it is not:

// php cli.php --foo='bar'dump(getopt('', ['foo::']));// ["foo" => "bar"]dump($argv);// [0 => "cli.php", 1 => "--foo=bar"]//  php cli.php  --foo=dump(getopt('', ['foo::']));// []dump($argv);// [0 => "cli.php", 1 => "--foo="]//  php cli.php --foo=''dump(getopt('', ['foo::']));// []dump($argv);// [0 => "cli.php", 1 => "--foo="]

But:

//  php cli.php --foo="''"dump(getopt('', ['foo::']));// []dump($argv);// [0 => "cli.php", 1 => "--foo=''"]

edit: For the record, I tried some of the most used CLI packages (hoa/console,auraphp/Aura.Cli,nategood/commando andc9s/CLIFramework) and none of them behave "perfectly" regarding our need. The fact we usenull for no-value and not an empty string looks debatable and could be reconsidered, both have pros and cons.

@javiereguiluz
Copy link
Member

@chalasr OK. It's clear now that this problem has no solution because of PHP. We should instead add a note in the docs explaining that you cannot pass an empty string as the value of an option. Thanks for investigating this!

@chalasr
Copy link
MemberAuthor

chalasr commentedJan 14, 2017
edited
Loading

@javiereguiluz As explained in#21215 (comment), there is actually 2 different issues:

  • Passing an option with no value or an empty value actually returns the default value of the option, so we can't know if the option was explicitly specified or not
  • Passing an option with only quotes (empty string) is not detectable when using= as separator

The former can be solved by returningnull instead of the default value (or any other empty value, but the current implementation usesnull), unlocking the ability to know that the option was specified but empty, letting up to the developer to changenull to an empty value of any primitive type, or use the default value. That is what is proposed here.

The latter is either full white or full black: we have to use an empty value of a predefined primitive type (currentlynull) to return when the option is specified with no value or an empty string.
We could have returned an empty string instead of null (as we are forced to return the same in all cases i.e.cmd --foo,--foo=,--foo="" and--foo ""),but I do think it's not a big deal (fixed), as one would complain that he wantsnull when using--foo, for sure.
However, it is no more a blocking issue as soon as we have the fix made here, because we are aware of that the option has been specified empty, so we can easily convertnull to the value of our choice (some CLI libraries allow to set a type on the option beforehand for handling these cases differently depending on the need, it could be a way to improve the situation in a next time, not sure it's worth it though).

@ro0NL
Copy link
Contributor

The implicit empty string to null conversion as mentioned before, should be fixed at least imo.; looking athttps://github.com/symfony/symfony/blob/v3.2.2/src/Symfony/Component/Console/Input/ArgvInput.php#L145 we already detect the difference between a value an no value depending on=. So this works counter-wise.

Including this PR the only drawback seems we cant differ between--foo and--foo "", which right now doesnt work either (gives default, after it gives null).

@chalasr
Copy link
MemberAuthor

chalasr commentedJan 15, 2017
edited
Loading

I looked at improving the situation for empty strings, here is what I end up with:

Code

$application =newApplication();$application    ->register('echo')    ->addOption('prefix',null, InputOption::VALUE_OPTIONAL,null,'my-default')    ->addArgument('value', InputArgument::REQUIRED)    ->setCode(function ($input,$output) {var_dump($input->getOption('prefix'));    });$application->run();

Before this PR
before

After
after

We get the default value only when don't passing the option explicitly.
I think we are good. What do you think?

jvasseur reacted with thumbs up emojiro0NL reacted with heart emoji

@chalasrchalasrforce-pushed theconsole/keep-empty-value-for-options branch 2 times, most recently from5fa7221 to06b5e7fCompareJanuary 15, 2017 11:37
@ro0NL
Copy link
Contributor

Looks really good :) can you confirm--opt-long --opt-long2 are bothnull? Or vice-versa... does--opt-long "" --opt-long2 works out?

I think the latter is an edge case where you getnull for--opt-long right?

chalasr reacted with thumbs up emoji

@chalasr
Copy link
MemberAuthor

Any thought here?

@chalasrchalasrforce-pushed theconsole/keep-empty-value-for-options branch from5a6ca14 to6502cb6CompareFebruary 28, 2017 15:24
@chalasr
Copy link
MemberAuthor

changelog updated, ping deciders

@chalasrchalasrforce-pushed theconsole/keep-empty-value-for-options branch from6502cb6 to4c3c06eCompareFebruary 28, 2017 16:10
CHANGELOG
=========

2.7.25
Copy link
Member

Choose a reason for hiding this comment

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

Should be removed, bug fixes are documented in the main CHANGELOG, which is automatically generated when I make a new release.

@fabpot
Copy link
Member

This looks like a BC break to me (and the fact that you added a note in the CHANGELOG confirms that feeling). So, I think that's not something that can be merged in 2.7, master would be ok if clearly documented.

@chalasrchalasrforce-pushed theconsole/keep-empty-value-for-options branch from4c3c06e to776653aCompareFebruary 28, 2017 22:35
@chalasrchalasr changed the base branch from2.7 tomasterFebruary 28, 2017 22:35
@chalasrchalasrforce-pushed theconsole/keep-empty-value-for-options branch 2 times, most recently fromd37516e to259e8beCompareFebruary 28, 2017 23:17
@chalasr
Copy link
MemberAuthor

@fabpot rebased on master and documented.

@chalasrchalasrforce-pushed theconsole/keep-empty-value-for-options branch 2 times, most recently fromaf4719e tof3d678fCompareFebruary 28, 2017 23:31
@chalasrchalasrforce-pushed theconsole/keep-empty-value-for-options branch fromf3d678f to8086742CompareFebruary 28, 2017 23:37
@fabpot
Copy link
Member

Thank you@chalasr.

@fabpotfabpot merged commit8086742 intosymfony:masterMar 1, 2017
fabpot added a commit that referenced this pull requestMar 1, 2017
…empty) should remain empty (chalasr)This PR was merged into the 3.3-dev branch.Discussion----------[Console] Explicitly passed options without value (or empty) should remain empty| Q             | A| ------------- | ---| Branch?       | master| Bug fix?      | yes| New feature?  | no| BC breaks?    | no| Deprecations? | no| Tests pass?   | yes| Fixed tickets |#21215#11572#12773| License       | MIT| Doc PR        | n/a (maybe look at updating the existing one)This conserves empty values for options instead of returning their default values.Code:```php// cli.php$application = new Application();$application    ->register('echo')    ->addOption('prefix', null, InputOption::VALUE_OPTIONAL, null, 'my-default')    ->addArgument('value', InputArgument::REQUIRED)    ->setCode(function ($input, $output) {        var_dump($input->getOption('prefix'));    });$application->run();```Before:![before](http://image.prntscr.com/image/157d9c6c054240da8b0dce54c9ce24d6.png)After:![after](http://image.prntscr.com/image/4aeded77f8084d3c985687fc8cc7b54e.png)Commits-------8086742 [Console] Explicitly passed options without value (or empty) should remain empty
@ro0NL
Copy link
Contributor

Thanks@chalasr. Made my day 😊

chalasr reacted with heart emoji

@chalasrchalasr deleted the console/keep-empty-value-for-options branchMarch 1, 2017 08:30
@fabpotfabpot mentioned this pull requestMay 1, 2017
fabpot added a commit that referenced this pull requestJun 14, 2017
… (chalasr)This PR was merged into the 3.4 branch.Discussion----------[FrameworkBundle] Deprecate useless --no-prefix option| Q             | A| ------------- | ---| Branch?       | 3.4| Bug fix?      | no| New feature?  | no| BC breaks?    | no| Deprecations? | yes| Tests pass?   | yes| Fixed tickets | n/a| License       | MIT| Doc PR        | n/aIt was a workaround, not needed since#21228. Let's deprecate it and remove it in 4.0.Commits-------f7afa77 [FrameworkBundle] Deprecate useless --no-prefix option
symfony-splitter pushed a commit to symfony/framework-bundle that referenced this pull requestJun 14, 2017
… (chalasr)This PR was merged into the 3.4 branch.Discussion----------[FrameworkBundle] Deprecate useless --no-prefix option| Q             | A| ------------- | ---| Branch?       | 3.4| Bug fix?      | no| New feature?  | no| BC breaks?    | no| Deprecations? | yes| Tests pass?   | yes| Fixed tickets | n/a| License       | MIT| Doc PR        | n/aIt was a workaround, not needed sincesymfony/symfony#21228. Let's deprecate it and remove it in 4.0.Commits-------f7afa777d8 [FrameworkBundle] Deprecate useless --no-prefix option
Sign up for freeto join this conversation on GitHub. Already have an account?Sign in to comment

Reviewers

@fabpotfabpotfabpot left review comments

Assignees

No one assigned

Projects

None yet

Milestone

2.7

Development

Successfully merging this pull request may close these issues.

7 participants

@chalasr@javiereguiluz@ro0NL@ogizanagi@fabpot@nicolas-grekas@carsonbot

[8]ページ先頭

©2009-2025 Movatter.jp