LanguageConstructSpacingSniff changes spaces to ·· (utf-8 middot)

[Mrten]

The LanguageConstructSpacingSniff from the Squiz standard outputs the whitespace it conflicts via an extra step:

    $data  = [Util\Common::prepareForOutput($content)];

which adds some terminal specific ANSI escape sequences

    $content = str_replace(' ', "\033[30;1m·\033[0m", $content);

This makes my subversion pre-commit hook error out with

Committing transaction...
svn: E165001: Commit failed (details follow):
svn: E165001: Commit blocked by pre-commit hook (exit code 1) with output:
[Error output could not be translated from the native locale to UTF-8.]

which is kinda hard to debug :(

[Mrten]

I've fixed this for now by filtering escape strings in a custom report:

+ $message = preg_replace("/\x1B[@-_][0-?]*[ -\/]*[@-~]/",'',$message);

[Mrten]

er, rephrase that, not fixed.

[Mrten]

this does fix my problem:

$message = preg_replace("/\x1B[@-_][0-?]*[ -\/]*[@-~]/",'',$message);
$message = iconv("UTF-8","ASCII//TRANSLIT//IGNORE",$message);

changes the · to ? but that's acceptable to me.

[jrfnl]

@Mrten Sounds like you found a solution to your problem. Can this issue be closed ?

[Mrten]

Solution, solution, I'd classify iconv() as a workaround.... I really don't know. As Squiz is usually quite strict about code-quality, I would be tempted to say no 😄

[jrfnl]

@Mrten Okay. Let's see if we can figure this out.

A lot of the reports offer colorized output in one way or another. It is rare that the code for this is in a sniff error message, but not unheard of (like the above).

Now, the Reporter class already contains code to remove the colorization if --no-colors was passed on the command line or via the ruleset.

Now you say above that you are using PHPCS in a subversion pre-commit hook. So, I'm now wondering how you are calling PHPCS from the pre-commit hook and if it would be possible for you to pass --no-colors ?

Is that possible ? Would it solve your problem ?

[Mrten]

Looking at the error message, I think the problem isn't so much the escape sequences for the colors as it is the UTF-8 middot. I'm in the process of finding out whether the workaround above is even in use any more, I'll let you know :)

Thanks for thinking along by the way!

[Mrten]

I think we've indeed set the Reporter config option colors to false in the time since we've reported this bug. So we are in the clear, however, the initial problem lies in assuming the terminal and the complete pipeline supports UTF-8.

I don't remember quite clearly anymore but if I interpret my first message correctly you may assume that it took me a lot of time to figure the root problem out. I would suggest removing the middot (change it to a .?), or having a searchable documentation somewhere warning of this problem, or perhaps even checking if the pipeline supports UTF-8 as you're already checking the width of the terminal via stty.

[jrfnl]

Well, any call to exec() will slow the Config processing further down, so I'm not sure that's the way to go.

I wonder if we shouldn't just change the wording of the error message to be more in line with other, similar error messages, i.e. "found # spaces/new line instead".

As far as I can see, there are three sniffs which use the Common::prepareForOutput() method:

• Generic.PHP.LowercaseKeyword - I'm not sure this sniff should use it at all.
• Generic.WhiteSpace.LanguageConstructSpacing
• Squiz.WhiteSpace.LanguageConstructSpacing

[rodrigoprimo]

I spent some time checking this issue. First, the list of sniffs that use Common::prepareForOutput() needs to be updated:

Squiz.WhiteSpace.LanguageConstructSpacing no longer exists. It was removed in PHPCS 4.0. Additionally, Squiz.Functions.FunctionDeclarationArgumentSpacing started using prepareForOutput() in PHPCSStandards/PHP_CodeSniffer#620.

Here is the updated list:

• Generic.PHP.LowercaseKeyword
• Generic.WhiteSpace.LanguageConstructSpacing
• Squiz.Functions.FunctionDeclarationArgumentSpacing

I've submitted a PR for Generic.PHP.LowerCaseKeyword (PHPCSStandards/PHP_CodeSniffer#1379), which was the most straightforward case. For the remaining two sniffs, I'd like to get input on the expected behavior before submitting PRs.

I wonder if we shouldn't just change the wording of the error message to be more in line with other, similar error messages, i.e. "found # spaces/new line instead".

@jrfnl, do you happen to have a suggestion of which similar error messages should be used as a reference? I'm asking because I checked Squiz.WhiteSpace.OperatorSpacing and Squiz.ControlStructures.ControlSignature and they don't distinguish between spaces and tabs in the error message while the sniffs using Common::prepareForOutput() do make this distinction. I'm inclined to think it is better to distinguish between spaces and tabs.

For example, here is what Generic.WhiteSpace.LanguageConstructSpacing currently outputs for the IncorrectSingle error code using prepareForOutput() vs the proposed change using the pattern from Squiz.WhiteSpace.OperatorSpacing:

Tab after echo:

// Current:
Language constructs must be followed by a single space; expected 1 space but found "\t"

// Proposed:
Language constructs must be followed by a single space; expected 1 space but found 1