Use ANSI color codes and Unicode characters on Windows as well #662

MatmaRex · 2024-11-09T23:54:37Z

Description

Use ANSI color codes and Unicode characters on Windows as well, when possible. See commit messages for details.

Suggested changelog entry

Output now has fewer redundant ANSI color codes.
On Windows, when using the --colors option, the output now has more colors.
On Windows, when using PHP 7.1+, the output now has more Unicode characters.

Related issues/external references

(none)

Types of changes

Bug fix (non-breaking change which fixes an issue)
New feature (non-breaking change which adds functionality)
Breaking change (fix or feature that would cause existing functionality to change)
- This change is only breaking for integrators, not for external standards or end-users.
Documentation improvement

PR checklist

I have checked there is no other PR open for the same change.
I have read the Contribution Guidelines.
I grant the project the right to include and distribute the code under the BSD-3-Clause license (and I have the right to grant these rights).
I have added tests to cover my changes.
I have verified that the code complies with the projects coding standards.
[Required for new sniffs] I have added XML documentation for the sniff.

jrfnl · 2024-11-10T00:58:14Z

@MatmaRex Thanks for the PR.

However, similar to the other PR, I need to ask you to please either provide a reproduction scenario, screenshots or tests to demonstrate what you are trying to solve. Preferably all three of these should be provided with any PR which changes things in the UI.

I can take a guess what you are trying to solve, but my guess may be wrong, so without any of the above, I can't even begin to properly evaluate the PR.

PHP enables this by default since PHP 7.2. https://www.php.net/manual/en/function.sapi-windows-vt100-support.php

PHPCS 3.x still has a minimum supported PHP version of PHP 5.4, so we cannot rely on features from PHP 7.2.
Please test your changes with PHP < 7.2 and see what breaks (and yes, things will break).

I could tentatively earmark this PR for PHPCS 4.0 , which will have a PHP 7.2 minimum, but that still doesn't negate my remarks above.

MatmaRex · 2024-11-10T21:26:44Z

@MatmaRex Thanks for the PR.

However, similar to the other PR, I need to ask you to please either provide a reproduction scenario, screenshots or tests to demonstrate what you are trying to solve. Preferably all three of these should be provided with any PR which changes things in the UI.

I can take a guess what you are trying to solve, but my guess may be wrong, so without any of the above, I can't even begin to properly evaluate the PR.

While using Windows 10 or newer, and PHP 7.2 or newer, run PHPCS with --colors e.g. on this file: https://github.com/MatmaRex/phpcs-pull-654-demo/blob/master/demo.php

Before	After

Note:

On PHP 7.2 and newer, the inline "\n" is now highlighted using a different color. This is the same behavior as you'd see on Linux.
On PHP older than 7.2, or on Windows older than 10, the --colors mode is already not working correctly. As a result of this change a few additional artifacts appear.

PHP enables this by default since PHP 7.2. https://www.php.net/manual/en/function.sapi-windows-vt100-support.php

PHPCS 3.x still has a minimum supported PHP version of PHP 5.4, so we cannot rely on features from PHP 7.2. Please test your changes with PHP < 7.2 and see what breaks (and yes, things will break).

I could tentatively earmark this PR for PHPCS 4.0 , which will have a PHP 7.2 minimum, but that still doesn't negate my remarks above.

The --colors mode is already broken before my patch, as shown in the screenshots above (tested on 7.1). The --no-colors mode is unaffected by my patch. I believe nothing new breaks.

If you really want to support these old systems, the only thing to do would be to disable --colors entirely. I could write a patch for that, if you promise you won't ask me for any test cases for it. I'm not sure what the point would be since they wouldn't run in CI anyway.

jrfnl · 2024-11-10T22:45:17Z

@MatmaRex

The --no-colors mode is unaffected by my patch. I believe nothing new breaks.

Given the following simple file:

<?php

    // This is a space indented line.
    $a = 10;
	// This is a tab indented line.

And running the following command on the Windows 10 command line:

phpcs ./phpcs-662.php --report=code --tab-width=4 --no-colors --standard=generic

☝️ Take note of the use of --report=code as that's the report which is being adjusted in this PR (amongst other things). Also note that this is in no-colors mode.

On PHP 8.4, everything looks swell:

Before	After

Not so much on PHP 7.0....:

Before	After

Which only goes to demonstrate why I insist on tests and will continue to do so...

If you really want to support these old systems, the only thing to do would be to disable --colors entirely. I could write a patch for that, if you promise you won't ask me for any test cases for it. I'm not sure what the point would be since they wouldn't run in CI anyway.

Proper tests will run in CI and should have caught the above. If not, the tests would not be good enough.

MatmaRex · 2024-11-11T01:07:53Z

Oh, right, the "visible" characters are still applied in --no-colors mode. Thanks for pointing that out.

The encoding issue is interesting – it happens with PHP 7.0, but not with PHP 7.1, which I tested with.

Proper tests will run in CI and should have caught the above. If not, the tests would not be good enough.

I'm not very familiar with GitHub's CI – can you point out which of the jobs run on Windows?

jrfnl · 2024-11-11T01:28:48Z

Oh, right, the "visible" characters are still applied in --no-colors mode. Thanks for pointing that out.

The encoding issue is interesting – it happens with PHP 7.0, but not with PHP 7.1, which I tested with.

Might just be that PHP added support for the Windows ANSI codes in PHP 7.1 and only added the function related to it in PHP 7.2.

Proper tests will run in CI and should have caught the above. If not, the tests would not be good enough.

I'm not very familiar with GitHub's CI – can you point out which of the jobs run on Windows?

At this time, there are no runs on Windows as there are no tests covering code which has special conditions for Windows. As soon as such tests would be added, the Windows OS would have to be added to the test matrix/workflow. (No worries about that, I can do that).

MatmaRex · 2024-11-11T01:43:22Z

Might just be that PHP added support for the Windows ANSI codes in PHP 7.1 and only added the function related to it in PHP 7.2.

The ANSI codes and the Unicode characters actually appear unrelated. I found a PHP doc page that documents the latter as a change in PHP 7.1. https://www.php.net/manual/en/migration71.windows-support.php

At this time, there are no runs on Windows as there are no tests covering code which has special conditions for Windows. As soon as such tests would be added, the Windows OS would have to be added to the test matrix/workflow. (No worries about that, I can do that).

I guess I can try adding some. Probably in yet another pull request.

This reduces the size of the output, and lessens weird effects of the ANSI color code characters on text wrapping, when the result of this method is used in a sniff error message (e.g. see LanguageConstructSpacingSniff).

Windows' console supports ANSI escape sequences since Windows 10. https://learn.microsoft.com/en-us/windows/console/console-virtual-terminal-sequences PHP enables this by default since PHP 7.2. https://www.php.net/manual/en/function.sapi-windows-vt100-support.php On older PHP versions, the output with --colors will not be correct, but this is already the case when using this option. PHP CodeSniffer special-cased Windows in this code in 2014 (bfd095d). This workaround is no longer needed today in 2024. Note that the --colors option was already supported on Windows. This only affects inline highlighting in error and debug messages.

MatmaRex · 2024-11-17T21:09:23Z

I extended the changes here slightly, adding two additional commits (see commit messages for details):

Update tests, which were added in Tests: Add unit tests for Common::prepareForOutput() #663
Improve the use of control characters in general
Use the Unicode characters on PHP 7.1+ on Windows, per our discussion above

…PHP 7.1+ PHP 7.1 fixed problems with printing Unicode characters to Windows console that forced us to avoid using them. https://www.php.net/manual/en/migration71.windows-support.php

MatmaRex · 2024-11-17T21:42:10Z

Sorry about the noise, I didn't realize at first that PHPUnit 4.8 (used on PHP 5.4) does not support the @requires PHP < 7.1 syntax. And thanks for enabling automatic test runs for me :)

MatmaRex · 2024-12-01T14:06:01Z

@jrfnl I greatly appreciate your reviews so far. I'd love to hear your thoughts on these patches again whenever you have the time. Please do not feel hurried though, I just want to make it clear that I'm still interested in completing these changes!

jrfnl · 2024-12-04T08:58:12Z

@jrfnl I greatly appreciate your reviews so far. I'd love to hear your thoughts on these patches again whenever you have the time. Please do not feel hurried though, I just want to make it clear that I'm still interested in completing these changes!

@MatmaRex Sorry for the delay, looking at it again now.

jrfnl

@MatmaRex Okay, so I have looked at this again.

Aside from the PR being blocked by the lack of tests for the Code report (which may be fixed via a future iteration on #664), I'm still concerned about the side-effects of this PR.

I've left a number of comments inline for you to consider.

I'm also a little concerned about the performance impact of the liberal use of array_* functions in this utility function. On modern PHP versions, it appears to be ~25% slower, while on older PHP versions, it appears to be about ~200% slower.
Before: https://3v4l.org/E3bVW/perf
After: https://3v4l.org/7NQUU/perf

Hope this helps.

jrfnl · 2024-12-04T09:35:31Z

src/Tokenizers/PHP.php

I'm not sure I understand the reasoning for this change ?

I mean, in contrast to the "Output now has fewer redundant ANSI color codes" changelog suggestion, this ends up creating more redundant ANSI color codes (at least on Windows).

Before:

After:

Screenshots generated using the following command on the previously posted code snippet on PHP 7.1 on Windows.

phpcs ./phpcs-662.php --tab-width=4 --standard=generic -vv

Also note (not necessarily to be addressed in this PR, probably should be a separate one): the --no-colors option can also be used on non-Windows OSes and the previous implementation already didn't respect that.
The PHP class does have access to the Config though via the Tokenizer::$config option, so maybe it should respect the colors setting ?

jrfnl · 2024-12-04T09:46:29Z

src/Util/Common.php

+            "\t" => '\t',
+            " "  => '·',
+        ];
+        if (stripos(PHP_OS, 'WIN') === 0 && PHP_VERSION_ID < 70100) {


Is this condition sufficient ?

If I read the https://www.php.net/manual/en/migration71.windows-support.php page, I get the impression an additional check may be needed against the value of the internal_encoding ini setting - and if that's empty, the default_charset ini setting - to verify it is set to UTF-8 ?

Also note that internal_encoding and default_charset were both only introduced in PHP 5.6: https://www.php.net/manual/en/migration56.deprecated.php#migration56.deprecated.iconv-mbstring-encoding

Same remark applies to the Code report change.

jrfnl · 2024-12-04T09:50:15Z

src/Util/Common.php

-            }
+        // Colour runs of invisible characters.
+        $match   = implode('', array_keys($replacements));
+        $content = preg_replace("/([$match]+)/", "\033[30;1m$1\033[0m", $content);


This ends up adding more redundant colour codes on Windows for PHP < 7.1, similar to what is happening in the PHP tokenizer.

github-actions bot added Type: bug Type: enhancement Core Component: Reports Core Component: Tokenizer Core Component: Utils Status: triage labels Nov 9, 2024

MatmaRex mentioned this pull request Nov 9, 2024

Reports: Fix word wrapping edge cases #654

Open

11 tasks

jrfnl added Status: awaiting feedback and removed Type: bug Core Component: Tokenizer labels Nov 10, 2024

MatmaRex force-pushed the windows-colors branch from 2ec254b to e64ea14 Compare November 11, 2024 01:39

MatmaRex force-pushed the windows-colors branch from e64ea14 to 294252d Compare November 17, 2024 20:17

MatmaRex added 2 commits November 17, 2024 22:05

MatmaRex force-pushed the windows-colors branch from 294252d to 9a33b8c Compare November 17, 2024 21:07

MatmaRex changed the title ~~Use ANSI color codes on Windows as well~~ Use ANSI color codes and Unicode characters on Windows as well Nov 17, 2024

MatmaRex force-pushed the windows-colors branch from 9a33b8c to ff05ec9 Compare November 17, 2024 21:20

Common: prepareForOutput(): Use Unicode output on Windows as well on …

8e976bc

…PHP 7.1+ PHP 7.1 fixed problems with printing Unicode characters to Windows console that forced us to avoid using them. https://www.php.net/manual/en/migration71.windows-support.php

MatmaRex force-pushed the windows-colors branch from ff05ec9 to 8e976bc Compare November 17, 2024 21:38

jrfnl removed the Status: awaiting feedback label Nov 25, 2024

jrfnl requested changes Dec 4, 2024

View reviewed changes

jrfnl added the Status: awaiting feedback label Dec 4, 2024

jrfnl force-pushed the master branch from e06559a to aab0862 Compare December 7, 2024 10:54

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use ANSI color codes and Unicode characters on Windows as well #662

Use ANSI color codes and Unicode characters on Windows as well #662

MatmaRex commented Nov 9, 2024 •

edited

Loading

jrfnl commented Nov 10, 2024

MatmaRex commented Nov 10, 2024

jrfnl commented Nov 10, 2024

MatmaRex commented Nov 11, 2024

jrfnl commented Nov 11, 2024

MatmaRex commented Nov 11, 2024

MatmaRex commented Nov 17, 2024

MatmaRex commented Nov 17, 2024

MatmaRex commented Dec 1, 2024

jrfnl commented Dec 4, 2024 •

edited

Loading

jrfnl left a comment

jrfnl Dec 4, 2024

jrfnl Dec 4, 2024

jrfnl Dec 4, 2024

Use ANSI color codes and Unicode characters on Windows as well #662

Are you sure you want to change the base?

Use ANSI color codes and Unicode characters on Windows as well #662

Conversation

MatmaRex commented Nov 9, 2024 • edited Loading

Description

Suggested changelog entry

Related issues/external references

Types of changes

PR checklist

jrfnl commented Nov 10, 2024

MatmaRex commented Nov 10, 2024

jrfnl commented Nov 10, 2024

MatmaRex commented Nov 11, 2024

jrfnl commented Nov 11, 2024

MatmaRex commented Nov 11, 2024

MatmaRex commented Nov 17, 2024

MatmaRex commented Nov 17, 2024

MatmaRex commented Dec 1, 2024

jrfnl commented Dec 4, 2024 • edited Loading

jrfnl left a comment

Choose a reason for hiding this comment

jrfnl Dec 4, 2024

Choose a reason for hiding this comment

jrfnl Dec 4, 2024

Choose a reason for hiding this comment

jrfnl Dec 4, 2024

Choose a reason for hiding this comment

MatmaRex commented Nov 9, 2024 •

edited

Loading

jrfnl commented Dec 4, 2024 •

edited

Loading