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

[Console] Prevent ArgvInput::getFirstArgument() from returning an option value #30277

Merged
merged 1 commit into from Feb 21, 2019

Conversation

Projects
None yet
5 participants
@chalasr
Copy link
Member

commented Feb 17, 2019

Q A
Branch? 3.4
Bug fix? yes
New feature? no
BC breaks? no
Deprecations? no
Tests pass? yes
Fixed tickets #23343
License MIT
Doc PR n/a

Fixes the case where the passed input string contains no command name but one or more global (i.e. application-defined) options accepting values.

@chalasr chalasr added this to the 3.4 milestone Feb 17, 2019

@chalasr chalasr force-pushed the chalasr:cli-opts-val-are-not-args branch 4 times, most recently from 5076733 to e4d4a92 Feb 17, 2019

@nicolas-grekas

This comment has been minimized.

Copy link
Member

commented Feb 20, 2019

Do we really need that logic? To me, an approach like #30244 will achieve the same with greater flexibility.

@chalasr

This comment has been minimized.

Copy link
Member Author

commented Feb 20, 2019

I think yes, it fixes a real bug. ArrayInput does not suffer from this issue.
#30244 looks like a feature to me as it requires code change for being used, we would need to document it. And the fact it involves to remove an option from the input makes it less flexible to me, IMHO it is not the desired behavior for the 80% use case, keeping in mind that this is not only about --env but any option defined at the application level which are generally meant to be accessed from the final input object at command runtime.

@stof

This comment has been minimized.

Copy link
Member

commented Feb 20, 2019

how does this handle cases where multiple shortcut gets grouped together ?

@chalasr chalasr force-pushed the chalasr:cli-opts-val-are-not-args branch from e4d4a92 to 46461e9 Feb 20, 2019

@chalasr

This comment has been minimized.

Copy link
Member Author

commented Feb 20, 2019

@stof in case the token is a short option (group), this checks that the last char of the token has a value and, if it has one, the next token is skipped. Just added a test + inline comment.

@fabpot

fabpot approved these changes Feb 21, 2019

@fabpot

This comment has been minimized.

Copy link
Member

commented Feb 21, 2019

Thank you @chalasr.

@fabpot fabpot merged commit 46461e9 into symfony:3.4 Feb 21, 2019

3 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
fabbot.io Your code looks good.
Details

fabpot added a commit that referenced this pull request Feb 21, 2019

bug #30277 [Console] Prevent ArgvInput::getFirstArgument() from retur…
…ning an option value (chalasr)

This PR was merged into the 3.4 branch.

Discussion
----------

[Console] Prevent ArgvInput::getFirstArgument() from returning an option value

| Q             | A
| ------------- | ---
| Branch?       | 3.4
| Bug fix?      | yes
| New feature?  | no
| BC breaks?    | no
| Deprecations? | no
| Tests pass?   | yes
| Fixed tickets | #23343
| License       | MIT
| Doc PR        | n/a

Fixes the case where the passed input string contains no command name but one or more global (i.e. application-defined) options accepting values.

Commits
-------

46461e9 [Console] Prevent ArgvInput::getFirstArgument() from returning an option value

@chalasr chalasr deleted the chalasr:cli-opts-val-are-not-args branch Feb 21, 2019

This was referenced Mar 3, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.