Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly
Sign up
Appearance settings

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

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

Open
MatmaRex wants to merge 3 commits into PHPCSStandards:3.x
base: 3.x
Choose a base branch
Loading
from MatmaRex:windows-colors

Conversation

@MatmaRex
Copy link
Contributor

@MatmaRex MatmaRex commented Nov 9, 2024 •
edited
Loading

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.

Copy link
Member

jrfnl commented Nov 10, 2024

@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.

Copy link
Contributor Author

@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
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.

Copy link
Member

jrfnl commented Nov 10, 2024

@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
image image

Not so much on PHP 7.0....:

Before After
image image

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.

Copy link
Contributor Author

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?

Copy link
Member

jrfnl commented Nov 11, 2024

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).

Copy link
Contributor Author

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.
Copy link
Contributor Author

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

@MatmaRex 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 
...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
Copy link
Contributor Author

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 :)

Copy link
Contributor Author

MatmaRex commented Dec 1, 2024

@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!

Copy link
Member

jrfnl commented Dec 4, 2024 •
edited
Loading

@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.

Copy link
Member

@jrfnl jrfnl left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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:
image

After:
image

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 ?

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

}
// Colour runs of invisible characters.
$match = implode('', array_keys($replacements));
$content = preg_replace("/([$match]+)/", "033円[30;1m1ドル033円[0m", $content);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

jrfnl commented Mar 22, 2025

@MatmaRex Just checking in - do you intend to continue with this PR ? If not, I suggest closing it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@jrfnl jrfnl jrfnl requested changes

Requested changes must be addressed to merge this pull request.

Assignees

No one assigned

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

AltStyle によって変換されたページ (->オリジナル) /