[Console] Fix hasParameterOption / getParameterOption when used with multiple flags #25893
Conversation
|
AppVeyor build has an error in code not modified by this PR: Must be a transient failure, since https://ci.appveyor.com/project/fabpot/symfony/build/1.0.32162 passed. |
| @@ -168,12 +168,12 @@ private function parseArgument($token) | |||
| $arg = $this->definition->getArgument($c); | |||
| $this->arguments[$arg->getName()] = $arg->isArray() ? array($token) : $token; | |||
|
|
|||
| // if last argument isArray(), append token to last argument | |||
| // if last argument isArray(), append token to last argument | |||
There was a problem hiding this comment.
please revert this one, it's a wrong behaviour of fabbot
| } elseif ($this->definition->hasArgument($c - 1) && $this->definition->getArgument($c - 1)->isArray()) { | ||
| $arg = $this->definition->getArgument($c - 1); | ||
| $this->arguments[$arg->getName()][] = $token; | ||
|
|
||
| // unexpected argument | ||
| // unexpected argument |
There was a problem hiding this comment.
please revert this one, it's a wrong behaviour of fabbot
| // If hasParameterOption('-fh') is supported for 'cli.php -fh', then | ||
| // one might also expect that it should also be supported for | ||
| // 'cli.php -f -h'. However, this is not supported. | ||
| $this->assertFalse($input->hasParameterOption('-fh'), '->hasParameterOption() returns true if the given short option is in the raw input'); |
There was a problem hiding this comment.
So you break the behaviour that was working with the patch IIUC?
There was a problem hiding this comment.
If the patch breaks the existing and we can't manage to make both work, better revert it for now.
There was a problem hiding this comment.
The patch does indeed have serious consequences for Drush, Robo, and potentially other clients as well. It is regrettable that get/hasParameterOptions do not have enough information available to handle multiple short options correctly.
2f5853d to
542cf6d
Compare
|
Reverted the mistaken style patch. |
|
👎 I'd rather revert the problematic patch (#24987) and see if we can fix that problem properly, or live with the limitation. |
|
@fabpot Thanks for backing out the previous commits. That will help considerably. Note that this PR also fixes a bug in getParameterOption that has existed for a long time. Would you like that re-submitted as a separate PR, or would you prefer to live with the limitation? |
|
@greg-1-anderson Can you rebase on current 2.7? |
Add one test to demonstrate getParameterOption fix. Add several tests to demonstrate current limitations in hasParameterOption.
542cf6d to
35f98e2
Compare
|
Rebased. The only remaining failure is the mistaken code style change suggested by fabbot, as mentioned above. |
|
Thank you @greg-1-anderson. |
… used with multiple flags (greg-1-anderson) This PR was merged into the 2.7 branch. Discussion ---------- [Console] Fix hasParameterOption / getParameterOption when used with multiple flags | Q | A | ------------- | --- | Branch? | 2.7 | Bug fix? | yes | New feature? | no | BC breaks? | no (Fixes BC break in #24987) | Deprecations? | no | Tests pass? | yes | Fixed tickets | #25825 | License | MIT | Doc PR | n/a Proposed resolution to #25825: - Back out #24987 - Fix getParameterOption for short options with values, e.g. `-edev` Commits ------- 35f98e2 Follow-on to #25825: Fix edge case in getParameterOption.
Proposed resolution to #25825:
-edev