-
-
Notifications
You must be signed in to change notification settings - Fork 9.6k
[Console] Fix check of color support on Windows #26609
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
Conversation
If the stream is redirected, the script should behave the same on Windows and on POSIX systems.
61feb7d
to
f7f8189
Compare
I have a doubt about this PR: if (function_exists('stream_isatty') && !@stream_isatty($this->stream)) {
return false;
}
if (DIRECTORY_SEPARATOR === '\\') {
if (function_exists('sapi_windows_vt100_support')) {
return @sapi_windows_vt100_support($this->stream);
}
return
'10.0.10586' === PHP_WINDOWS_VERSION_MAJOR.'.'.PHP_WINDOWS_VERSION_MINOR.'.'.PHP_WINDOWS_VERSION_BUILD
|| false !== getenv('ANSICON')
|| 'ON' === getenv('ConEmuANSI')
|| 'xterm' === getenv('TERM');
}
return function_exists('posix_isatty') && @posix_isatty($this->stream); |
Thank you @mlocati. |
This PR was merged into the 2.7 branch. Discussion ---------- [Console] Fix check of color support on Windows | Q | A | ------------- | --- | Branch? | 2.7 | Bug fix? | yes | New feature? | no | BC breaks? | no | Deprecations? | no | Tests pass? | yes | Fixed tickets | | License | MIT | Doc PR | If the stream is redirected, `StreamOutput::hasColorSupport()` returns `false` on POSIX systems. On Windows, this is not always true. Before PHP 7.2 we can't say if the stream is redirected, but since PHP 7.2 we have the `stream_isatty` function that works on Windows too: let's use it. Sure, `sapi_windows_vt100_support` should return `false` if the stream is redirected, but it's in `or` with the other conditions, so the logic was flawed. Commits ------- f7f8189 Fix check of color support on Windows
Hello, i use cygwin with mintty terminal on windows and up to 4.0.6 i have got colored output, but after update package to 4.0.7, the colors are lost. :/ |
@mu4ddi3 If seems that php -r "var_export(stream_isatty(STDERR));" |
@mlocati thx for answer. its "false" in cygwin but i can write to STDERR: it's "true" in windows shell (cmd). //edit |
I asked two developers (@weltling and @johnstevenson) that have a deep knowledge of the PHP source code. |
I understand that the cygwin minnty is some kind of emulation and can not work (sad that before this fix it worked), but normal Git bash shell from windows instalation of git (which is btw also using mintty) didnt work with colors now as well (even if the command you requested to check returns true). I need to live with it. |
Im running gitbash and recently ran into this issue that color support is not detected as true. Is there a way to detect gitbash and set OutputInterface::setDecorated(true)? I dont know enough about how these emulated terminals interact with PHP. |
@fabpot Unfortunately, this is a BC break, because color output is now broken in Windows unixy terminals/bash, which were tested using an environment variable (because of the named pipes issue) and now they are not. Please see https://github.com/composer/xdebug-handler/blob/master/src/Process.php#L111 which uses the new PHP 7.2 functionality without breaking bc. @mlocati @mu4ddi3 Git Bash works fine, but it does depend on how you have set it up (using the native conhost or mintty) and how you are running it (standalone terminal, in Console/Console2, ConEmu etc) and on what version of Windows, ie. does it support vt100 natively (Windows 10 generally, or later than November 2015 if never updated). Likewise with Cygwin. There is no problem if you invoke it using The bottom line is that this has been a known feature/issue for years (mintty/mintty#56) and there are many ways to solve it, but no one-size fits all solution. I think that the symfony approach (as was) provides a clean solution for the majority of developers. If output contains unwanted vt100 sequences then there is the option to either use the |
@mlocati would you like to send a PR that would fix the behavior change? |
I can't see a way to understand when the output is redirected (ie |
We don't need to make the difference: since years, we didn't, and nobody complained (on Windows). |
Just nick my code (perhaps credit it so that we can keep it in sync?). |
Thing is, you have no way of knowing what is happening outside mintty (whether a program has a hidden console window to get round the pseudo-terminal stuff, or whether it is doing cunning tricks like Git that use bash to access /dev/tty. In fact Git seems to change quite frequently, as more problems/issues come up with various setups and WSL. I see that it is now setting the TERM variable to That's why I think that the previous symfony solution is the best. If there are problems then the user will have to find out what the current solution is, because these implementations are going to change (especially as the native conhost.exe improves and gets more unixy functionality ). |
So, the solution is to simply remove these lines from the method: if (function_exists('stream_isatty') && !@stream_isatty($this->stream)) {
return false;
} |
actually, I'd suggest to move them after the Windows checks, like the Process class does. |
|
If this helps, this is why my code is as it is: if (defined('PHP_WINDOWS_VERSION_BUILD')) {
return (function_exists('sapi_windows_vt100_support')
&& sapi_windows_vt100_support($output))
|| false !== getenv('ANSICON')
|| 'ON' === getenv('ConEmuANSI')
|| 'xterm' === getenv('TERM');
} There is no need to check for a specific build of Windows from Nov 2015 that accidentally enabled vt100 by default, because it will have been replaced by (automatic updates). Also, as this was obviously a Windows bug (it was never documented to do this) this functionality may have been removed in an intermediate fix - we don't know. if (function_exists('stream_isatty')) {
return stream_isatty($output);
} elseif (function_exists('posix_isatty')) {
return posix_isatty($output);
}
$stat = fstat($output);
// Check if formatted mode is S_IFCHR
return $stat ? 0020000 === ($stat['mode'] & 0170000) : false; The new |
Please read my explanation above (I think both our posts arrived at the same time!) |
You are right (as always ;) ) |
Ha ha. You wrote it in PHP source! |
+1 for @johnstevenson code inspecting the env first Edit - passing the --ansi argument will force color output. |
I'm using bash with standard after-install settings (mintty in standalone terminal) and i'm currently using windows 7 (business pc).
When i invoke it in my env it just starts in standard windows terminal and also without colors. So, guys what is the conclusion? Is there something to be done to make it work again as is suppose to work? |
I think that the code from @johnstevenson implements the best solution we can have so far. I don't want to steal code or credits, so I'd let him create another pull request with his own code... |
@mlocati Ah okay, I'll do that if that's what you want |
Workaround for issues caused by symfony/symfony#26609 Fixes #7264
This PR was squashed before being merged into the 2.7 branch (closes #26910). Discussion ---------- Use new PHP7.2 functions in hasColorSupport | Q | A | ------------- | --- | Branch? | 2.7 | Bug fix? | yes | New feature? | no | BC breaks? | no | Deprecations? | no | Tests pass? | yes | Fixed tickets | | License | MIT | Doc PR | Fixes bc break in #26609 Reference: https://github.com/composer/xdebug-handler/blob/master/src/Process.php#L111 Commits ------- b0c9225 Use new PHP7.2 functions in hasColorSupport
Lost colors in MinTTY from default Git distribution for Windows. How to fix it? |
@artemdigi Problem is known and fix is also ready. You need to wait for upcoming version. |
If the stream is redirected,
StreamOutput::hasColorSupport()
returnsfalse
on POSIX systems.On Windows, this is not always true. Before PHP 7.2 we can't say if the stream is redirected, but since PHP 7.2 we have the
stream_isatty
function that works on Windows too: let's use it.Sure,
sapi_windows_vt100_support
should returnfalse
if the stream is redirected, but it's inor
with the other conditions, so the logic was flawed.