Use new PHP7.2 functions in hasColorSupport #26910

johnstevenson · 2018-04-12T20:27:47Z

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

nicolas-grekas

There are more places that check for color support (grep ConEmuANSI src/ -r)
could you please sync them all?

src/Symfony/Bridge/PhpUnit/DeprecationErrorHandler.php
src/Symfony/Component/VarDumper/Dumper/CliDumper.php

johnstevenson · 2018-04-14T18:40:31Z

Sure. But I could do with some info regarding CliDumper:

The constructor checks for Windows color support (so it can set a restricted style set), but only uses environment variables and not the output.

I guess it is safe to use $this->outputStream here?
Or would it be better to set the styles within the supportsColors method later?

The supportsColors method does one type of check if $this->outputStream is not the default (is_resource && posix_isatty, which doesn't include windows) and a different, more stringent one if it is. An explanation why would be really useful. Likewise why if it is not the default, the method returns before checking for color options in argv.

I was wondering if the output needs to be differentiated, and if so, then what are the criteria for each type.

nicolas-grekas · 2018-04-14T19:45:46Z

Which color set does new versions of Windows support? If it doesn't need the reduced set, then the current logic in the CliDumper constructor should be fine, so that only supportsColors should be updated.

johnstevenson · 2018-04-14T20:30:47Z

Windows 10 has supported 24-bit color for over a year now, but you can obviously still run PHP7.2 on earlier versions.

stof · 2018-04-16T09:49:55Z

src/Symfony/Component/Console/Output/StreamOutput.php

-        return function_exists('posix_isatty') && @posix_isatty($this->stream);
+        if (function_exists('stream_isatty')) {
+            return @stream_isatty($this->stream);
+        } elseif (function_exists('posix_isatty')) {


just if, not elseif as the previous case returns

johnstevenson · 2018-04-16T20:07:58Z

I've synced DeprecationErrorHandler.php, but without any info about CliDumper.php I have no idea if my changes are correct.

I've kept the current logic in the CliDumper constructor, which is not ideal since it won't show 24-bit color output in cmd.exe on Windows 10. Incidentally, the Windows tests pass for me locally, but not on Appveyor for some reason:

There were 3 failures:
1) Symfony\Component\HttpKernel\Tests\DataCollector\DumpDataCollectorTest::testCollectDefault
Failed asserting that two strings are identical.
--- Expected
+++ Actual
@@ @@
-DumpDataCollectorTest.php on line 65:
-123
+DumpDataCollectorTest.php on line 65:
+123

C:\projects\symfony\src\Symfony\Component\HttpKernel\Tests\DataCollector\DumpDataCollectorTest.php:73

2) Symfony\Component\HttpKernel\Tests\DataCollector\DumpDataCollectorTest::testFlush
Failed asserting that two strings are identical.
--- Expected
+++ Actual
@@ @@
-DumpDataCollectorTest.php on line 123:
-456
+DumpDataCollectorTest.php on line 123:
+456

C:\projects\symfony\src\Symfony\Component\HttpKernel\Tests\DataCollector\DumpDataCollectorTest.php:129

3) Symfony\Component\HttpKernel\Tests\DataCollector\DumpDataCollectorTest::testFlushNothingWhenDataDumperIsProvided
Failed asserting that two strings are identical.
--- Expected
+++ Actual
@@ @@
-DumpDataCollectorTest.php on line 142:
-456
+DumpDataCollectorTest.php on line 142:
+456

C:\projects\symfony\src\Symfony\Component\HttpKernel\Tests\DataCollector

I've also added references to the source of the code, but I'll remove them if not considered desirable.

nicolas-grekas

LGTM, with one minor comment.
About the CliDumper constructor, we have to use 16 colors only on some terminals, which is what we do by default. Do you think we can improve the logic and somehow detect when the native Windows terminal supports 24b colors? That's what missing there.

nicolas-grekas · 2018-04-16T20:33:12Z

src/Symfony/Bridge/PhpUnit/DeprecationErrorHandler.php

+     * Reference: Composer\XdebugHandler\Process::supportsColor
+     * https://github.com/composer/xdebug-handler
+     *
+     * @return bool


this annotation should be removed and replaced by a return-type on the hasColorsSupport (same in other files below)

?? Sorry, you've lost me!

Oups sorry, branch 2.7 doesn't support scalar type hints.

johnstevenson · 2018-04-16T20:43:58Z

Do you think we can improve the logic and somehow detect when the native Windows terminal supports 24b colors?

Probably, but I need you to answer my original question:

The constructor checks for Windows color support (so it can set a restricted style set), but only uses environment variables and not the output.

I guess it is safe to use $this->outputStream here?
Or would it be better to set the styles within the supportsColors method later?

nicolas-grekas · 2018-04-16T20:45:59Z

It is not safe to use $this->outputStream. The check should be done against a global stream, STDOUT basically.

nicolas-grekas

(with one minor comment)

nicolas-grekas · 2018-04-20T09:30:24Z

src/Symfony/Component/VarDumper/Dumper/CliDumper.php

+     */
+    private function isWindowsTrueColor()
+    {
+        $result = strval(getenv('ANSICON_VER')) >= '183'


thanks to PHP type juggling, this should be simplified to 183 <= getenv('ANSICON_VER')

Ah yes, that is a much better way of doing it.

fixed in 9234171

fabpot · 2018-04-22T05:55:21Z

Thank you @johnstevenson.

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

johnstevenson · 2018-04-22T10:41:10Z

Thanks for fixing this remaining item, and apologies for not fixing it myself (life/time problem!)

…ts (chalasr) This PR was merged into the 2.7 branch. Discussion ---------- [HttpKernel] Remove decoration from actual output in tests | Q | A | ------------- | --- | Branch? | 2.7 | Bug fix? | no | New feature? | no | BC breaks? | no | Deprecations? | no | Tests pass? | yes green again | Fixed tickets | n/a | License | MIT | Doc PR | n/a AppVeyor has color support since #26910, that breaks the build. Fixes it by removing decoration from tested DumpDataCollector CLI outputs, same as what's already done for HTML dumps Commits ------- c4daef9 [VarDumper] Remove decoration from actual output in tests

carsonbot added Status: Needs Review Bug labels Apr 12, 2018

nicolas-grekas added this to the 2.7 milestone Apr 14, 2018

carsonbot added Status: Needs Work and removed Status: Needs Review labels Apr 14, 2018

nicolas-grekas requested changes Apr 14, 2018

View reviewed changes

stof reviewed Apr 16, 2018

View reviewed changes

nicolas-grekas reviewed Apr 16, 2018

View reviewed changes

nicolas-grekas approved these changes Apr 20, 2018

View reviewed changes

carsonbot added Status: Reviewed and removed Status: Needs Work labels Apr 20, 2018

fabpot approved these changes Apr 22, 2018

View reviewed changes

Use new PHP7.2 functions in hasColorSupport

b0c9225

fabpot force-pushed the fix-output-hascolorsupport branch from 72482c5 to b0c9225 Compare April 22, 2018 05:55

fabpot merged commit b0c9225 into symfony:2.7 Apr 22, 2018

johnstevenson deleted the fix-output-hascolorsupport branch April 22, 2018 10:41

chalasr mentioned this pull request Apr 25, 2018

[HttpKernel] Remove decoration from actual output in tests #27046

Merged

This was referenced Apr 27, 2018

Release v2.7.46 #27070

Merged

Release v2.8.39 #27095

Merged

Release v3.4.9 #27102

Merged

fabpot mentioned this pull request Apr 30, 2018

Release v4.0.9 #27103

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use new PHP7.2 functions in hasColorSupport #26910

Use new PHP7.2 functions in hasColorSupport #26910

johnstevenson commented Apr 12, 2018

nicolas-grekas left a comment •

edited

Loading

johnstevenson commented Apr 14, 2018

nicolas-grekas commented Apr 14, 2018

johnstevenson commented Apr 14, 2018

stof Apr 16, 2018

johnstevenson Apr 16, 2018

johnstevenson commented Apr 16, 2018

nicolas-grekas left a comment

nicolas-grekas Apr 16, 2018

johnstevenson Apr 16, 2018

nicolas-grekas Apr 16, 2018 •

edited

Loading

johnstevenson commented Apr 16, 2018

nicolas-grekas commented Apr 16, 2018 •

edited

Loading

nicolas-grekas left a comment

nicolas-grekas Apr 20, 2018

johnstevenson Apr 20, 2018

fabpot Apr 22, 2018

fabpot commented Apr 22, 2018

johnstevenson commented Apr 22, 2018

Use new PHP7.2 functions in hasColorSupport #26910

Use new PHP7.2 functions in hasColorSupport #26910

Conversation

johnstevenson commented Apr 12, 2018

nicolas-grekas left a comment • edited Loading

Choose a reason for hiding this comment

johnstevenson commented Apr 14, 2018

nicolas-grekas commented Apr 14, 2018

johnstevenson commented Apr 14, 2018

stof Apr 16, 2018

Choose a reason for hiding this comment

johnstevenson Apr 16, 2018

Choose a reason for hiding this comment

johnstevenson commented Apr 16, 2018

nicolas-grekas left a comment

Choose a reason for hiding this comment

nicolas-grekas Apr 16, 2018

Choose a reason for hiding this comment

johnstevenson Apr 16, 2018

Choose a reason for hiding this comment

nicolas-grekas Apr 16, 2018 • edited Loading

Choose a reason for hiding this comment

johnstevenson commented Apr 16, 2018

nicolas-grekas commented Apr 16, 2018 • edited Loading

nicolas-grekas left a comment

Choose a reason for hiding this comment

nicolas-grekas Apr 20, 2018

Choose a reason for hiding this comment

johnstevenson Apr 20, 2018

Choose a reason for hiding this comment

fabpot Apr 22, 2018

Choose a reason for hiding this comment

fabpot commented Apr 22, 2018

johnstevenson commented Apr 22, 2018

nicolas-grekas left a comment •

edited

Loading

nicolas-grekas Apr 16, 2018 •

edited

Loading

nicolas-grekas commented Apr 16, 2018 •

edited

Loading