[Console] [Enhancement] fixes #5316 #5676
Conversation
| } | ||
|
|
||
| if (count($leftOptions)) { | ||
| $messages[] = '<comment>Global options:</comment>'.PHP_EOL.implode(PHP_EOL, $leftOptions).PHP_EOL; |
There was a problem hiding this comment.
The help is using \n in previous places, not PHP_EOL
There was a problem hiding this comment.
btw, it looks weird to me that this part does not require changing any test
There was a problem hiding this comment.
actually the test fixtures did change
There was a problem hiding this comment.
but only the XML ones. And here, I'm commenting on the plain text help
|
I simplified the change. And the reason why tests for text help do not need changes is because in CommandTest, the commands are executed first which also merges app definition before invoking asText or asXml . While in ApplicationTest, commands are never run therefore app definition is not being merged. |
| @@ -265,15 +265,17 @@ public function setCode(\Closure $code) | |||
| /** | |||
| * Merges the application definition with the command definition. | |||
| */ | |||
| private function mergeApplicationDefinition() | |||
| private function mergeApplicationDefinition($merge_args = true) | |||
There was a problem hiding this comment.
Please use camelCased names for the arguments
and please add the @param annotation
There was a problem hiding this comment.
applied your suggestion @stof
|
@fabpot This looks ready to me. Anything left ? |
This PR was merged into the master branch. Commits ------- 63b480e [Console] fixed #5316 Discussion ---------- [Console] [Enhancement] fixes #5316 Bug fix: no Feature addition: yes Backwards compatibility break: no Symfony2 tests pass: yes Fixes the following tickets: #5316 --------------------------------------------------------------------------- by marfillaster at 2012-10-05T02:14:55Z I simplified the change. And the reason why tests for text help do not need changes is because in CommandTest, the commands are executed first which also merges app definition before invoking asText or asXml . While in ApplicationTest, commands are never run therefore app definition is not being merged. --------------------------------------------------------------------------- by stof at 2012-10-13T23:13:52Z @fabpot This looks ready to me. Anything left ?
Bug fix: no
Feature addition: yes
Backwards compatibility break: no
Symfony2 tests pass: yes
Fixes the following tickets: #5316