[Console] Selectively output to STDOUT or OUTPUT stream #4148
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Addresses issues with writing console output for IBM i5 Series (OS400). The normal QP2TERM shell outputs garbage text when attempting to write directly to STDOUT, likely because of EBCDIC character-encoding used on IBM platforms. Writing to the OUTPUT mimics using 'echo' or 'print' and prints properly in the console. Fixes symfony#1434
| */ | ||
| protected function hasStdoutSupport() | ||
| { | ||
| // @codeCoverageIgnoreStart |
There was a problem hiding this comment.
we don't use these annotations in the Symfony source code AFAIK. Please remove them
There was a problem hiding this comment.
Symfony/Component/Console/Output/StreamOutput used these for method hasColorSupport() which I tried to model when adding this, but I'll remove them. https://github.com/johnkary/symfony/blob/450a9c03ff7a928aa385274228a43957c6c5c92b/src/Symfony/Component/Console/Output/StreamOutput.php#L105
| */ | ||
| protected function hasStdoutSupport() | ||
| { | ||
| return !('OS400' === php_uname('s')); |
There was a problem hiding this comment.
use !==. It would be more readable :)
|
Can you create a PR for the 2.0 branch as this is a bug fix? Thanks. |
|
Opened new PR targeting 2.0: #4152 Closing this PR. |
craigmarvelley
pushed a commit
to craigmarvelley/symfony
that referenced
this pull request
Nov 26, 2013
Commits ------- 5b92b9e [Console] Selectively output to STDOUT or OUTPUT stream Discussion ---------- [Console] Selectively output to STDOUT or OUTPUT stream Originally opened in this PR targeting master, but asked to target 2.0 instead: symfony#4148 Bug fix: yes Feature addition: no Backwards compatibility break: no Symfony2 tests pass: yes Fixes the following tickets: symfony#1434 Todo: - As noted in the ticket discussion and linked discussion threads, IBM i5 Series has issues with writing output to STDOUT when viewed via their QP2TERM console. The output is likely not being converted to the correct character-encoding on the system level. This PR changes the default output stream from `php://stdout` to `php://output` for OS400 environments, which does not exhibit this issue. I'm using `php_uname('s')` to check for the presence of "OS400", which is at least one of the IBM OS's exhibiting this issue. This check is only done once when executing a Console task and shouldn't see any adverse affects in speed on the 99% other platforms using Symfony. I'm not a native to the OS400 platform so I can't really anticipate any other possible regressions that might occur from switching output streams for that platform. On my Mac, this change would strip all color output, but the PR code only changes output for OS400 environment. To my knowledge the QP2TERM console doesn't even support color, so no loss there. I think this change is best to make the Console component at least usable out of the box for anyone else trying to build CLI applications for use on OS400. --------------------------------------------------------------------------- by igorw at 2012-04-29T19:41:21Z symfony#4146 might also need a fix for this. --------------------------------------------------------------------------- by johnkary at 2012-04-29T20:21:39Z @igorw Hmm. In this case for symfony#4152 when creating a CLI application, `Symfony\Component\Console\Output\ConsoleOutput` is the [default implementation](https://github.com/johnkary/symfony/blob/5b92b9ed432ea1d5f2c596517fc7f8aa825b1977/src/Symfony/Component/Console/Application.php#L113) used by `Symfony\Component\Console\Application` when not specifying your own `OutputInterface`. Our hard-coded defaults were causing problems out of the box. I haven't looked too closely at the PRs surrounding the additions of `StreamingResponse` and your recent `OutputStream` but are we assuming anywhere that `php://stdout` is the default stream used when creating a streaming response? If so it MAY require a check similar to what I implemented for Console. My addition was only necessary because the output was being sent to a CLI console. If output is sent to a browser, I don't believe this would be an issue. If you have something that needs testing on OS400 just ping me.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Bug fix: yes
Feature addition: no
Backwards compatibility break: no
Symfony2 tests pass: yes
Fixes the following tickets: #1434
Todo: -
As noted in the ticket discussion and linked discussion threads, IBM i5 Series has issues with writing output to STDOUT when viewed via their QP2TERM console. The output is likely not being converted to the correct character-encoding on the system level.
This PR changes the default output stream from
php://stdouttophp://outputfor OS400 environments, which does not exhibit this issue.I'm using
php_uname('s')to check for the presence of "OS400", which is at least one of the IBM OS's exhibiting this issue. This check is only done once when executing a Console task and shouldn't see any adverse affects in speed on the 99% other platforms using Symfony.I'm not a native to the OS400 platform so I can't really anticipate any other possible regressions that might occur from switching output streams for that platform. On my Mac, this change would strip all color output, but the PR code only changes output for OS400 environment. To my knowledge the QP2TERM console doesn't even support color, so no loss there.
I think this change is best to make the Console component at least usable out of the box for anyone else trying to build CLI applications for use on OS400.