Php_codesniffer: PSR2 is not checking for blank lines at the start and end of control structures

[llaville]

I've recently used the version 2.5.1 of phar distrib to detect code violation following PSR-2 standard.

It gave me no errors, and I was pretty happy, but I understand why a Tracis-CI job detect an error until I see it used an old version 2.3.0

With this code :

        foreach ($keywords as $ident) {

            $letters = str_split($ident);

            if ($letters[0] == ':' && $letters[1] == ':' && end($letters) == '(') {
                $this->staticMethods[] = substr($ident, 2, -1);

            } elseif ($letters[0] == '-' && $letters[1] == '>' && end($letters) == '(') {
                $this->concreteMethods[] = substr($ident, 2, -1);

            } elseif (end($letters) == '(') {
                $this->functions[] = substr($ident, 0, -1);
            }
        }

On 2.5.1, I got

Processing NodeVisitor.php [PHP => 671 tokens in 88 lines]... DONE in 140ms (0 errors, 0 warnings)

On 2.3.0 I got

----------------------------------------------------------------------
FOUND 1 ERROR AFFECTING 1 LINE
----------------------------------------------------------------------
 23 | ERROR | [x] Expected 1 newline after opening brace; 2 found
----------------------------------------------------------------------
PHPCBF CAN FIX THE 1 MARKED SNIFF VIOLATIONS AUTOMATICALLY
----------------------------------------------------------------------

Line 23 is related to foreach statement.

[gsherwood]

I've done some digging and figured out the problem.

The root cause is the fix for bug #782, at which point I stopped this sniff from checking for newlines at the start of a control structure. This was released in 2.4.0.

What I forgot was that PSR2 was relying on this sniff to check that, whereas the Squiz standard was using a special sniff to do this (and to check for empty lines at the end of control structures, which PSR2 has never done, but should).

The solution is to get the PSR2 standard to also use this sniff. Then the errors will appear again, although they are generated and fixed by a different sniff.

Thanks for finding this change.

[gsherwood]

That's done now. So now you'll actually get these errors (ignore line numbers):

FILE: temp.php
----------------------------------------------------------------------
FOUND 3 ERRORS AFFECTING 3 LINES
----------------------------------------------------------------------
  2 | ERROR | [x] Blank line found at start of control structure
  8 | ERROR | [x] Blank line found at end of control structure
 11 | ERROR | [x] Blank line found at end of control structure
----------------------------------------------------------------------
PHPCBF CAN FIX THE 3 MARKED SNIFF VIOLATIONS AUTOMATICALLY
----------------------------------------------------------------------

and the file will be fixed like this:

foreach ($keywords as $ident) {
    $letters = str_split($ident);

    if ($letters[0] == ':' && $letters[1] == ':' && end($letters) == '(') {
        $this->staticMethods[] = substr($ident, 2, -1);
    } elseif ($letters[0] == '-' && $letters[1] == '>' && end($letters) == '(') {
        $this->concreteMethods[] = substr($ident, 2, -1);
    } elseif (end($letters) == '(') {
        $this->functions[] = substr($ident, 0, -1);
    }
}

[llaville]

Thanks Greg for very quick answer and fixe :)

[Rican7]

@gsherwood After upgrading to PHPCS 2.6 and using the PSR-2 standard, I'm getting a lot of errors for blank lines before the end of a control structure or after the start of a control structure. This aligns with your most recent comment.

Unfortunately, though, this doesn't seem to actually comply with the PSR-2 standard. The PSR-2 standard explicitly mentions space requirements in control structures, but says nothing about blank lines. In fact quite the contrary, PSR-2 states:

Blank lines MAY be added to improve readability and to indicate related blocks of code.

Unless I'm misinterpreting something, it seems that the changes made in b88fb32ab49f10a952bd2b874a99f476a3880954 actually broke compliance with PSR-2.

[gsherwood]

Unfortunately, though, this doesn't seem to actually comply with the PSR-2 standard.

Initially discussed here: https://github.com/squizlabs/PHP_CodeSniffer/issues/520

A post was made on the mailing list over a year ago: https://groups.google.com/forum/#!topic/php-fig/W0MBLw84dHg

Unfortunately, it only got 2 responses, although both were from PHP-FIG members. One was for, one was against. So that wasn't super helpful and I left the rule in there.

The commit you reference was just re-adding that rule as it was accidentally removed. It also added the additional "Blank line found at end of control structure" because PSR-2 states that The closing brace MUST be on the next line after the body. It feels like the opening brace rule just wasn't properly documented, but is inferred from the example code.

If you disagree, you might want to post on the mailing list again and see if you can get some consensus this time. PSR-12 actually makes some parts of the standard more explicit, including banning blank lines at the top and bottom of class definitions, but hasn't managed to do that to all sections yet. So there is possibly a discussion to be had there as well, if you are keen to do so.

[Rican7]

I see what you're getting at, but you/squizlabs are essentially interpreting the PSR-2 rules in a way that isn't explicit, therefore going beyond (or arguably against) the standard's definition.

The PHP-FIG mailing-list isn't a place to ask about clarification for an existing standard. Any opinion or response made by the FIG simply isn't in the standard, and therefore doesn't represent the standard without an amendment containing errata.

If the rule is ambiguous or not explicitly defined, then it shouldn't be enforced. Otherwise the PSR-2 "standard" in PHP_CodeSniffer becomes it's own standard itself. And then it's lost much of it's utility.

[gsherwood]

The PHP-FIG mailing-list isn't a place to ask about clarification for an existing standard. Any opinion or response made by the FIG simply isn't in the standard, and therefore doesn't represent the standard without an amendment containing errata.

How do you think that errata got there? From people (including myself) asking on the mailing list.

Concepts are only ambiguous because people don't agree on the interpretation. Instead of simply ignoring the rule all together, the correct way to resolve ambiguity is to ask for the original intention of the authors.

[Isinlor]

If you disagree, you might want to post on the mailing list again and see if you can get some consensus this time. PSR-12 actually makes some parts of the standard more explicit, including banning blank lines at the top and bottom of class definitions, but hasn't managed to do that to all sections yet. So there is possibly a discussion to be had there as well, if you are keen to do so.

Why not to move this two controversial rules to the PSR-12 standard? Or make PSR-2-controversial standard separate, but extending from PSR-2?