Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork9.7k
Detect CLI color support for Windows 10 build 10586#18385
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Uh oh!
There was an error while loading.Please reload this page.
Conversation
Newer Windows 10 versions (builds older than 10586) offer VT100 color support.Seehttp://www.nivot.org/blog/post/2016/02/04/Windows-10-TH2-(v1511)-Console-Host-Enhancements
nicolas-grekas commentedMar 31, 2016
note that the VarDumper component has similar logic that should be patched also |
| if (DIRECTORY_SEPARATOR ==='\\') { | ||
| returnfalse !==getenv('ANSICON') ||'ON' ===getenv('ConEmuANSI') ||'xterm' ===getenv('TERM'); | ||
| return | ||
| defined('PHP_WINDOWS_VERSION_MAJOR') |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
AFAIK,PHP_WINDOWS_VERSION_MAJOR goves the version of Windows where PHP was compiled, not the version where it runs
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
AFAIK, PHP_WINDOWS_VERSION_MAJOR goves the version of Windows where PHP was compiled, not the version where it runs
@stof Whoops, you're right!
stof commentedMar 31, 2016
not a bugfix IMO (especially given that we can already force it with a CLI flag) |
mlocati commentedApr 1, 2016
No, it's not true. They aredetermined at runtime. I diggereda bit into these I tested exactly the same PHP executables under these different PCs:
And here's the values of the three constants (concatenated with dots for simplicity) for every PHP version and every PC:
For these versions:
the detected Windows version is wrong because ofthis Windows change. These versions:
havethis patch, so those versions have a reliable value for the constants. Given all that, I think that this pull request is still valid, but it will work only for PHP 5.5 >= 5.5.27, PHP 5.6 >= 5.6.11 and PHP 7+ (older versions will continue to work as before). So,@stof, shall I reopen this PR? |
mlocati commentedApr 1, 2016
PS: a more reliable way to determine the exact Windows version would be to use
|
stof commentedApr 1, 2016
it is fine if the autodetection works fine only in uptodate PHP versions. Not detecting color support won't break the console (it will just fallback to non colored output by default) |
Newer Windows 10 versions (builds older than 10586) offer VT100 color support.Seehttp://www.nivot.org/blog/post/2016/02/04/Windows-10-TH2-(v1511)-Console-Host-Enhancements
mlocati commentedApr 1, 2016
I updated this PR, adding the Windows 10 detection code to |
| if ('\\' ===DIRECTORY_SEPARATOR) { | ||
| returnfalse !==getenv('ANSICON') ||'ON' ===getenv('ConEmuANSI') ||'xterm' ===getenv('TERM'); | ||
| return | ||
| defined('PHP_WINDOWS_VERSION_MAJOR') |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
is this part actually needed ? We already detect windows just before
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
We just detected that the directory separator in\... I'm not 100% sure it's only used in Windows (there are more OSs out there than just *nix, Windows and Mac... What about OS2? Others?....)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
I think we can safely state that only Windows does'\\' === DIRECTORY_SEPARATOR
nicolas-grekas commentedApr 3, 2016
👍, as a bug fix on 2.3 to me... |
fabpot commentedApr 3, 2016
I don't think it qualifies as a bug fix (and this PR does not apply cleanly on 2.3 as two files did not exist back then). For such a PR, I propose to merge it on master and be done with it. |
nicolas-grekas commentedApr 4, 2016
Thank you@mlocati. |
This PR was submitted for the master branch but it was merged into the 2.3 branch instead (closes#18385).Discussion----------Detect CLI color support for Windows 10 build 10586Newer Windows 10 versions (builds starting from 10586) offer VT100 color support.Seehttp://www.nivot.org/blog/post/2016/02/04/Windows-10-TH2-(v1511)-Console-Host-Enhancements| Q | A| ------------- | ---| Branch? | master - Maybe it could be backported to other branches too| Bug fix? | maybe 😉 - Do you you consider a bug not having colors on Windows?| New feature? | yes| BC breaks? | no| Deprecations? | no| Tests pass? | yes| Fixed tickets || License | MIT| Doc PR |Commits-------472a7bf Detect CLI color support for Windows 10 build 10586
Newer Windows 10 versions (builds starting from 10586) offer VT100 color support.
Seehttp://www.nivot.org/blog/post/2016/02/04/Windows-10-TH2-(v1511)-Console-Host-Enhancements