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
Appearance settings

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

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 1 commit into from
Feb 21, 2019
Merged

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

merged 1 commit into from
Feb 21, 2019

Conversation

chalasr
Copy link
Member

@chalasr chalasr 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 cli-opts-val-are-not-args branch 4 times, most recently from 5076733 to e4d4a92 Compare February 18, 2019 18:44
@nicolas-grekas
Copy link
Member

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

@chalasr
Copy link
Member Author

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

stof commented Feb 20, 2019

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

@chalasr chalasr force-pushed the cli-opts-val-are-not-args branch from e4d4a92 to 46461e9 Compare February 20, 2019 20:43
@chalasr
Copy link
Member Author

chalasr commented Feb 20, 2019 •
edited
Loading

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

fabpot commented Feb 21, 2019

Thank you @chalasr.

@fabpot fabpot merged commit 46461e9 into symfony:3.4 Feb 21, 2019
fabpot added a commit that referenced this pull request Feb 21, 2019
@fabpot
bug #30277 [Console] Prevent ArgvInput::getFirstArgument() from retur…
ff4b1d4 
…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 cli-opts-val-are-not-args branch February 21, 2019 08:55
This was referenced Mar 3, 2019
Merged
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@fabpot fabpot fabpot approved these changes

@nicolas-grekas nicolas-grekas nicolas-grekas approved these changes

Assignees
No one assigned
Labels
Bug Console Status: Reviewed
Projects
None yet
Milestone
3.4
Development

Successfully merging this pull request may close these issues.
5 participants
@chalasr @nicolas-grekas @stof @fabpot @carsonbot
Morty Proxy This is a proxified and sanitized view of the page, visit original site.