Generic.Files.LineLength false positive for non-breakable strings at exactly the soft limit

[ghostal]

Version:

alexuser@46419a0ca528:/var/www/html$ vendor/bin/phpcs --version           
PHP_CodeSniffer version 3.5.3 (stable) by Squiz (http://www.squiz.net)

My phpcs.xml:

<?xml version="1.0"?>
<ruleset name="PHP_CodeSniffer">
    <description>The coding standard for our project.</description>
    <rule ref="PSR2"/>

    <file>app</file>
    <file>bootstrap</file>
    <file>config</file>
    <file>database</file>
    <file>resources</file>
    <file>routes</file>
    <file>tests</file>

    <exclude-pattern>bootstrap/cache/*</exclude-pattern>
    <exclude-pattern>bootstrap/autoload.php</exclude-pattern>
    <exclude-pattern>*/migrations/*</exclude-pattern>
    <exclude-pattern>*/seeds/*</exclude-pattern>
    <exclude-pattern>*.blade.php</exclude-pattern>
    <exclude-pattern>*.js</exclude-pattern>

    <!-- Show progression -->
    <arg value="p"/>
</ruleset>

With my phpcs.xml I'm using PSR2, so the line length soft limit is 120.

Starting point

I found that a particular comment from the Laravel Doctrine project looks like this:

            /*
            | ...
            | http://symfony.com/doc/current/cookbook/doctrine/dbal.html#registering-custom-mapping-types-in-the-schematool
            | ...
            */

The indent: 12 spaces
The left margin: Two chars (| )
The URL itself: 109 chars
Total length without indent: 111 chars
Total length: 123 chars

Full file here: https://github.com/laravel-doctrine/orm/blob/52a2a673a457c331d0957da152d0d35e56998adb/config/doctrine.php#L71

CodeSniffer complains about the line with the URL:

FILE: /var/www/html/config/doctrine.php
----------------------------------------------------------------------
FOUND 0 ERRORS AND 1 WARNING AFFECTING 1 LINE
----------------------------------------------------------------------
 71 | WARNING | Line exceeds 120 characters; contains 123 characters
----------------------------------------------------------------------

Time: 78ms; Memory: 8MB

Next step

I remove the left margin characters:

            http://symfony.com/doc/current/cookbook/doctrine/dbal.html#registering-custom-mapping-types-in-the-schematool

The indent: 12 spaces
The URL itself: 109 chars
Total length: 121 chars

W 1 / 1 (100%)



FILE: /var/www/html/config/doctrine.php
----------------------------------------------------------------------
FOUND 0 ERRORS AND 1 WARNING AFFECTING 1 LINE
----------------------------------------------------------------------
 71 | WARNING | Line exceeds 120 characters; contains 121 characters
----------------------------------------------------------------------

Time: 79ms; Memory: 8MB

Next step

I remove a character from the end of the URL:

            http://symfony.com/doc/current/cookbook/doctrine/dbal.html#registering-custom-mapping-types-in-the-schematoo

The indent: 12 spaces
The URL itself: 108 chars
Total length: 120 chars

. 1 / 1 (100%)


Time: 79ms; Memory: 8MB

Here's where it gets weird

If, instead of removing a single character to bring the URL down to 120 characters, I add a character to bring it up to 122 characters, the sniff is happy again:

            http://symfony.com/doc/current/cookbook/doctrine/dbal.html#registering-custom-mapping-types-in-the-schematoola

The indent: 12 spaces
The URL itself: 110 chars
Total length: 122 chars

The sniff is (confusingly?) happy again!

. 1 / 1 (100%)


Time: 79ms; Memory: 8MB

My findings

I've been staring at the LineLengthSniff.php code for a while but I can't really get my head around its decision making process, and what the logic is for when it should complain and when it shouldn't. I have noticed what I think is a small bug though:

                $oldLength = $tokens[$stackPtr]['length'];
                $newLength = strlen(ltrim($tokens[$stackPtr]['content'], "/#\t "));

The $tokens[$stackPtr]['length'] is less than strlen($tokens[$stackPtr]['content']) if the token is part of a multi-line comment (the extra character(s) in $tokens[$stackPtr]['content'] are EOL characters). For the purposes of counting line length, I think these should be ignored, and either way, when comparing strings to calculate $indent, we need to compare apples with apples.

So, with the current code, for a multi-line comment, $indent is calculated as 11 for the lines above, when it should be 12.

Is my understanding here correct?

So I've committed what I think is a fix for that here:

ghostal@4d37172

The tests all pass, so that's nice.

But I can't recreate the problem behaviour I'm solving with a test, nor the weird behaviour described above, and I've no idea why, and so now I'm stuck 😞

[gsherwood]

Thanks for reporting this. The problem was visible when the line length was exactly matching that soft limit. The reason adding characters got the error to go away was because the comment can't be broken up over multiple lines and so should be ignored, but the EOL char was tripping it up.

I used your solution (an added strlen call) although I normally wouldn't do this because strlen isn't multi-byte safe whereas the length index is (assuming you have iconv). But in this case, the sniff isn't using that figure to try and determine the length of the comment - it is just trying to determine the length of the indent (a list of known characters). So it's actually more accurate now.

Thanks a lot for the detailed report and the patch. It made it very easy to resolve.

[ghostal]

No problem, glad I could help. Thanks very much for your hard work maintaining PHP_CodeSniffer!