Report Full: fix two line wrapping bugs

[anomiex]

Description

This is a re-creation of squizlabs/PHP_CodeSniffer#3925:

The first is that the ANSI escape codes applied to bold the message when -s is used were not being taken into account when wrapping the lines for width, causing some lines to be wrapped unnecessarily.

The second is that when lines were wrapped in the middle of a long message, the | characters making up the table were being bolded along with the message.

Suggested changelog entry

• Avoid unnecessarily wrapping lines in the full report when -s is used.
• Fix incorrect bolding of pipes in the full report tables when -s is used and messages wrap.

Related issues/external references

Fixes #124
Fixes squizlabs/PHP_CodeSniffer#3924
Closes squizlabs/PHP_CodeSniffer#3925

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 see no existing tests for any of the reporting formats.
• 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]

@anomiex Thanks for this PR. I'm looking at it now. To keep this moving forward, could you please rebase the PR ? I made some changes to the workflows since you created this branch and it would be great to see a passing build using the current workflows.

[jrfnl]

@anomiex Thanks for reporting this issue and your work on this patch.

I've now looked at and tested this PR extensively, including with the test code from a previous fix/iteration on this code which was merged in squizlabs/PHP_CodeSniffer#2093.

Findings:

• Your code fixes the issue, but does still contain an "off by one" error somewhere.
  This can be seen when running the test code using the exact max length:

  phpcs -s ./test.php --report-width=110 --standard=PSR12
  phpcs -s ./test.php --report-width=111 --standard=PSR12

  Using the first command the line for the first message will still be wrapped, while when using the second command, it won't but will have a one space difference with the delimiter line.

From #124:

A more complex fix might be to wordwrap() only the message, and then manually calculate the length of the last line to decide whether to append the source directly or to append it as a new line.

As I was looking at this now anyway, I've prepared an update to your fix, which does just that and surprisingly, actually simplifies the code instead of making it more complex.

If you are okay with this, I can push the update in a separate commit to this PR for you to have a look at ?

As a future iteration, I would really like to safeguard these fixes (both this one as well as the ones from squizlabs/PHP_CodeSniffer#2093) with tests, but as testing the reports is greenfields, I consider that "future scope" and not something which needs to be addressed in this PR.

I also discovered another bug while testing this fix. I will pull that as a separate PR, but wouldn't mind a review from you on that PR.

[jrfnl]

I also discovered another bug while testing this fix. I will pull that as a separate PR, but wouldn't mind a review from you on that PR.

FYI: I've opened PR #154 for this other bug.

[anomiex]

Using the first command the line for the first message will still be wrapped, while when using the second command, it won't but will have a one space difference with the delimiter line.

That's how the existing code works, it comes from the - 1 at

PHP_CodeSniffer/src/Reports/Full.php

Line 116 in 7fb9515

$maxErrorSpace = ($width - $paddingLength - 1);

Try a file like this, for example:

<?php

$x = TRUE;

$ vendor/bin/phpcs --report-width=83 --standard=PSR12 test.php 

FILE: /tmp/test/test.php
-----------------------------------------------------------------------------------
FOUND 1 ERROR AFFECTING 1 LINE
-----------------------------------------------------------------------------------
 3 | ERROR | [x] TRUE, FALSE and NULL must be lowercase; expected "true" but found
   |       |     "TRUE"
-----------------------------------------------------------------------------------
PHPCBF CAN FIX THE 1 MARKED SNIFF VIOLATIONS AUTOMATICALLY
-----------------------------------------------------------------------------------

$ vendor/bin/phpcs --report-width=82 --standard=PSR12 test.php 

FILE: /tmp/test/test.php
----------------------------------------------------------------------------------
FOUND 1 ERROR AFFECTING 1 LINE
----------------------------------------------------------------------------------
 3 | ERROR | [x] TRUE, FALSE and NULL must be lowercase; expected "true" but
   |       |     found "TRUE"
----------------------------------------------------------------------------------
PHPCBF CAN FIX THE 1 MARKED SNIFF VIOLATIONS AUTOMATICALLY
----------------------------------------------------------------------------------

As I was looking at this now anyway, I've prepared an update to your fix, which does just that and surprisingly, actually simplifies the code instead of making it more complex.

If you are okay with this, I can push the update in a separate commit to this PR for you to have a look at ?

I'll be interested to see your take on it.

[jrfnl]

Using the first command the line for the first message will still be wrapped, while when using the second command, it won't but will have a one space difference with the delimiter line.

That's how the existing code works, it comes from the - 1 at

PHP_CodeSniffer/src/Reports/Full.php

Line 116 in 7fb9515

$maxErrorSpace = ($width - $paddingLength - 1);

Good catch! I've updated my change to take that into account now.

As I was looking at this now anyway, I've prepared an update to your fix, which does just that and surprisingly, actually simplifies the code instead of making it more complex.
If you are okay with this, I can push the update in a separate commit to this PR for you to have a look at ?

I'll be interested to see your take on it.

Pushing it now.

[anomiex]

Ah, wordwrap() already handles embedded newlines, so the code doesn't have to handle them itself. Good catch!

[jrfnl]

Ah, wordwrap() already handles embedded newlines, so the code doesn't have to handle them itself. Good catch!

It does indeed, though I have to admit I needed to test that out myself as well (which I did when creating the update): https://3v4l.org/7ENWc

[jrfnl]

As far as I'm concerned this is ready for merge. @anomiex All okay from your perspective too ?

[anomiex]

It does indeed, though I have to admit I needed to test that out myself as well (which I did when creating the update): https://3v4l.org/7ENWc

I did much the same thing! 😀 https://3v4l.org/knYT4

As far as I'm concerned this is ready for merge. @anomiex All okay from your perspective too ?

Looks good to me too!

[jrfnl]

Thanks @anomiex !