PSR-2 v CodeSniffer PSR-2

0-intro.md

This is a list of issues or discrepencies between the wording or intention of PSR-2 itself and the CodeSniffer PSR-2 ruleset.

Add suggestions in the comments or tweet me (@philsturgeon) if you have more inconsistencies to report.

1-multi-line-args.md

1.) Multi-line Arguments

Here is one I call "The Multi-line Argument Conundrum".

Status: Fixed

Example #1

A normal function declaration with some arguments.

somefunction($foo, $bar, $baz, $ban);

PSR-2: Valid
CodeSniffer "PSR-2": Valid

Example #2

As PSR-2 Section 4.6 says, if you'd like to split this argument list onto multiple lines (with the intended meaning being to avoid super-wide declarations), you can chose to drop each argument onto its own line, and indent each one with an extra paragraph.

somefunction(
    $foo,
    $bar,
    $baz,
    $ban
);

PSR-2: Valid
CodeSniffer "PSR-2": Valid

Example #3

What SHOULD be a perfectly valid example of PSR-2 code (and is considered by many PSR-2 members to be perfectly valid), is the option of one of your arguments being a multi-line argument.

somefunction($foo, $bar, [
  // ...
], $ban);

PSR-2: Valid
CodeSniffer "PSR-2": Invalid

Or, the same type of example taken right from the Silex homepage:

$app->get('/hello/{name}', function($name) use($app) { 
    return 'Hello '.$app->escape($name); 
});

PSR-2: Valid
CodeSniffer "PSR-2": Invalid

Now, a multi-line argument is assumed to be treated as a single argument and so this example is technically no different from Example #1, and as such should not automatically trigger 4.6. By triggering 4.6 you are forced against your will to suddenly drop everything onto new lines and indent everything, and then it looks like...:

Example #4

This nasty-ass shit which is forced upon you by using CodeSniffer/CS-Fixer. This is ugly as sin, and not ever what anyone was talking about when PSR-2 was written.

somefunction(
    $foo,
    $bar,
    [
      // ...
    ],
    $ban
);

PSR-2: Valid
CodeSniffer "PSR-2": Valid

$app->get(
    '/hello/{name}',
    function($name) use($app) { 
        return 'Hello '.$app->escape($name); 
    }
);

PSR-2: Valid
CodeSniffer "PSR-2": Valid

Check this one out:

Fuck that noise.

Example #5

Avoid PHP CodeSniffers wrath by assigning multi-line arguments to their own variable, which essentially makes Example #5 into Example #1 or Example #2:

$arr = [
  // ...
];

somefunction(
    $foo,
    $bar,
    $arr,
    $ban
);

PSR-2: Valid
CodeSniffer "PSR-2": Valid

Yay for uneccessary code.

Short-Term Fix

You can avoid all of this with a standards file:

<?xml version="1.0"?>
<ruleset name="PHP_CodeSniffer">

  <description>PHP_CodeSniffer configuration</description>

    <rule ref="PSR2">
        <exclude name="PEAR.Functions.FunctionCallSignature"/>
        <exclude name="PEAR.Functions.FunctionCallSignature.SpaceAfterCloseBracket"/>
    </rule>

</ruleset>

Shove it in Jenkins, or reference it in Sublime Text with:

"phpcs_additional_args": {
    "--standard": "/Users/foo/phpcs.xml",
    "-n": ""
},

Further Reading

• Should an anonymous function be able to be passed like this and still be valid PSR-2?
• PSR-2 : clarification on multi-line function calls

2-aligning-arguments.md

2.) Aligning Arguments in Signature

This is another "PSR-2 doesn't care but CodeSniffer says it does" situation.

Status: Fixed

<?php
 
namespace Test;
 
class MyClass
{
    public function someFunction(
        MyType       $param1,
        LongTypeName $param2,
        array        $param3
    ) {
        // code ...
    }
}

CodeSniffer should have no reason to care about anything in this code. The text is here:

4.4. Method Arguments

In the argument list, there MUST NOT be a space before each comma, and there MUST be one space after each comma.

Method arguments with default values MUST go at the end of the argument list.

...some code...

Argument lists MAY be split across multiple lines, where each subsequent line is indented once. When doing so, the first item in the list MUST be on the next line, and there MUST be only one argument per line.

When the argument list is split across multiple lines, the closing parenthesis and opening brace MUST be placed together on their own line with one space between them.

Expected Output

Nothing.

Actual Output

--------------------------------------------------------------------------------
FOUND 2 ERROR(S) AFFECTING 2 LINE(S)
--------------------------------------------------------------------------------
  8 | ERROR | Expected 1 space between type hint and argument "$param1"; 7
    |       | found
 10 | ERROR | Expected 1 space between type hint and argument "$param3"; 8
    |       | found
--------------------------------------------------------------------------------

Resolution

Report as CodeSniffer Bug

Thanks

Props to Tom Oram for catching this one.

3-comment-indentation.md

3.) Comment Alignment

This is another "PSR-2 doesn't care but CodeSniffer says it does" situation.

Status: Waiting on confirmation of being fixed

<?php

// Do something if yes
if ($foo === true) {
    echo "Yep"

// Do something if nope
} elseif ($foo !== true) {
    echo "Nope";

// Fuck knows
} else {
    die("Universe Implosion");
}

Sadly CodeSniffer is not letting comments sit next to their appropriate constrol structure keyword.

Expected Output

Nothing.

Actual Output

--------------------------------------------------------------------------------
FOUND 2 ERROR(S) AFFECTING 2 LINE(S)
--------------------------------------------------------------------------------
  7 | ERROR | Line indented incorrectly; expected at least 4 spaces, found 0
 11 | ERROR | Line indented incorrectly; expected at least 4 spaces, found 0
--------------------------------------------------------------------------------

Resolution

Greg is on the job in the comments.

4-class-name-resolution.md

4.) Class name resolution

When using the new Foo::class CodeSniffer gets all confused.

Status: Fixed

<?php

call_user_func(Foo::class);

Expected Output

Nothing.

Actual Output

---------------------------------------------------------------------------
-----
FOUND 2 ERROR(S) AND 2 WARNING(S) AFFECTING 2 LINE(S)
---------------------------------------------------------------------------
-----
 1 | ERROR   | The closing brace for the class must go on the 
next line after
   |         | the body
 3 | WARNING | Possible parse error: class missing opening or 
closing brace
 3 | WARNING | Possible parse error: class missing opening or 
closing brace
 3 | ERROR   | Each class must be in a namespace of at least one 
level (a
   |         | top-level vendor name)
---------------------------------------------------------------------------
-----

Resolution

Reported as CodeSniffer Bug

5-multi-line-interface-extends.md

5.) Multi-Line Interface Extends

PSR-2 didn't ever mention what to do if you want to extend multiple interfaces in an interface, but CodeSniffer will tell you you're wrong.

Status: Waiting on Errata Pull Request

<?php

namespace Test;

interface MyInterface extends
    Interface1,
    Interface2
{
}

Expected Output

Not entirely sure. We'll work it out on the mailing list.

Actual Output

--------------------------------------------------------------------------------
FOUND 2 ERROR(S) AFFECTING 2 LINE(S)
--------------------------------------------------------------------------------
 6 | ERROR | Expected 1 space before "Interface1"; 4 found
 7 | ERROR | Expected 1 space before "Interface2"; 4 found
--------------------------------------------------------------------------------

Resolution

Poll the FIG to see if we can (or need to) make this Errata. If so, make errata. If not... bug?

@peterjmit: Thanks for linking that ML thread up, I remember that well as it was happening last time I tried implementing CS in Sublime. I linked it in a more reading section before I noticed your comment, so we're thinking along the same lines.

The relevant sections of PSR-2 are sections 4.6 and 6. Section 4.6
does not address closures as arguments. Section 6 dictates the format
of a closure. My interpretation is therefore that a closure may be a
single argument and can therefore start on the same line as the
bracket opening the argument list. It is not a "multiple argument" by
itself. The last example of Section 6 shows that the format is
required in an argument list (so presumably also when a single
argument).

-- Pádraic Brady, FIG representative for Zend

As you say, the intention of the FIG was for this to be intentionally vague. It could mean either, but the wording was done badly.

Seeing as it COULD mean either, we simply need PHP-CS to allow either.

That is the crux of this conversation.

Here is an excellent discussion on the topic, between @padraic and @gsherwood:

zendframework/zendframework#4390

A vote has started on the FIG ML about getting Errata added. Once that is in we can make a new PR to clarify the intention of 4.6 and explain that multi-line arguments are cool. THEN we can bug the CodeSniffer team about making the PSR-2 ruleset work the way we expect.

The Errata vote passed and the Multi-Lingual PSR-2 Errata Vote is off to a strong start.

Here's another issue I found with PHP CS's PSR-2 Standard. For some reason it wants 1 space indentation rather than 4 in this situation.

<?php

namespace Test;

interface MyInterface extends
    Interface1,
    Interface2
{
}

PHP CS OUTPUT

phpcs --standard=psr2 MyInterface.php 

FILE: /home/tom/phpcs/MyInterface.php
--------------------------------------------------------------------------------
FOUND 2 ERROR(S) AFFECTING 2 LINE(S)
--------------------------------------------------------------------------------
 6 | ERROR | Expected 1 space before "Interface1"; 4 found
 7 | ERROR | Expected 1 space before "Interface2"; 4 found
--------------------------------------------------------------------------------

Time: 0 seconds, Memory: 2.75Mb

Although now looking through PSR-2 it looks like this case isn't mentioned. It talks about extends and implements for classes but not extends for interfaces, even though it says class refers to interfaces and traits also, I think this is missing the fact than an interface can extend multiple interfaces and therefore PSR-2 should allow multiple lines here.

Maybe a discussion for PHP-FIG?

Re: Comment Alignment

There are 2 ways to do things here:

Ignore the alignment of all comments (PSR-2 doesn't explicitly say that comments must be aligned in the same way as code).

OR

Allow a very specific case where a comment can come directly before a control structure (not even a blank line between them?) as long as it is indented to the same column as the control structure opening line.

I think the first way is maybe more technically correct given PSR-2 ignores comment rules, although it does lead to some pretty bad code if people stop indenting comments. The second option is much nicer and seems like that's how you'd want people contributing to a project to write code, but would require an explicit rule in the PSR to enforce that. Otherwise it is just "best practice", which PSR-2 excludes as well.

Just to confirm, PHPCS will allow the following code if comment alignment is no longer checked:

if ($foo) {
    if ($bar) {
// Do somerthing.
        echo 'something';
// Else error.
    } else if ($baz) {
        echo 'error';
    }
    // Else, set to false.
} else {
    $foo = false;
}

With regards to my point on interface extending I have posted this to the PHP-FIG mailing list https://groups.google.com/forum/?fromgroups#!topic/php-fig/EAtkRv2xqZc

@gsherwood I'd go with 2. A line break would make it part of the code block above. Strict is fine in this instance.

@tomphp added that as #5.

@gsherwood Number 1. As tempting as it is to cram all the best practices we think we want into the sniff, comments aren't actually part of PSR-1/2. Intentionally. Don't enforce anything about them.

@philsturgeon @bobthecow It probably needs some discussion on the ML, or even a change to the errata. Otherwise, it just seems like best practice.

Also, number 4 (Class Name Resolution) is already fixed in the master branch. I'm just waiting on the result of the PR vote for number 1 before releasing. I assume the vote will pass, so I've made the change but haven't pushed it yet.

It would be great to resolve the commenting issue at the same time, if the FIG can come to an agreement over what should be checked for.

@gsherwood I was wrong to suggest 2. It's a personal preference and @bobthecow is right, it is nothing to do with PSR1/2. CodeSniffer should just completely ignore comments, so if somebody puts them in a stupid place then yay for them.

I can ask the group for a poll on this if you want, but it definitely doesn't need errata. PSR-2 doesnt mention comments, so you shouldn't enforce comments.

@philsturgeon I've finally managed to get back to this and will take a look at completely ignoring comment lines in the indentation checks. Thanks for figuring out what is needed here.

Edit: that's done now. Will be released in the next version, although I don't know when that will be yet. Soonish. Commit is here: squizlabs/PHP_CodeSniffer@d8ed4e3

Just a minor addition to the Short-Term Fix for the Multi-line Arguments as I had some serious trouble getting this to work: As it seems the "fix" with the custom ruleset.xml seems not to work with version 1.4.7 of CodeSniffer. I had to explicitly downgrade to version 1.4.6. With version 1.4.7 I got a few errors stating "Closing parenthesis of a multi-line function call must be on a line by itself" which for me seems to indicate that the exclude statement in the ruleset file is ignored as the error is thrown in the PEAR/Sniffs/Functions/FunctionCallSignatureSniff.php file which I excluded as described above.

@philsturgeon is there any movement on the number 3, comment alignment issue?

We totally agree at work, but are stuck with it being broken every time, especially when using cs fixer. Does anyone have a ruleset that would allow this to be overridden in the meantime?

I prefer the following style of comment alignment (using the exact example above):

<?php

if ($foo === true) {
    // Do something if yes
    echo "Yep"
} elseif ($foo !== true) {
    // Do something if nope
    echo "Nope";
} else {
    // Fuck knows
    die("Universe Implosion");
}

PSR-2: Valid (Doesn't care)
CodeSniffer: Valid