PSR2: Multiline conditionals

[Pittiplatsch]

As already stated in #454 (comment), code like this:

if (
    true
    || false
) {
    1;
}

is valid for PSR2 and should therefore not throw errors like Expected 0 spaces after opening bracket; newline found.

[edited link to issue]

[jian-wu]

Looks like I'm not alone - #459. 😄

[Pittiplatsch]

Ups, yes. However I believe that CS should not throw an error for a rule which actually is not broken.
Even if current behaviour is caused by the fixer, this doesn't justify throwing non-existant errors IMO.

[jian-wu]

Agreed. I've requested it to be a warning at the most.

[gsherwood]

is valid for PSR2 and should therefore not throw errors

What makes you think this code is valid in PSR-2? The example code looks pretty clear and states Note the placement of parentheses, spaces, and braces: https://github.com/php-fig/fig-standards/blob/master/accepted/PSR-2-coding-style-guide.md#51-if-elseif-else

[Pittiplatsch]

As already stated by several people all over the web, PSR2 does not make clear assumptions on each and every statement on how to format them when it comes to excessing single line length.

After all, PSR2 longs for good readability, and it's obvious for me to utilize the same rules that are explicitly given for other use cases, e.g. for method calls:
https://github.com/php-fig/fig-standards/blob/master/accepted/PSR-2-coding-style-guide.md#46-method-and-function-calls

PSR2 states definitive rules of where to place spaces (IMO in terms of ASCII 32) to unify overall styling of braces, i.e. encourage if ($foo over if($foo, if ( $foo, if($foo and the like.

However, I don't think that PSR2's term "space" is to be understood as regex-ish \w (i.e. ANY whitespace).

Don't want to argue too much, but IMO when even PSR's people don't make it to achieve some agreement for all the years the problem exists (see https://github.com/php-fig/fig-standards/issues?q=is%3Aissue+condition) than any tool aiming to follow PSR2 should not make any additional assumptions, as this obviously will throw errors not coverd by PSR2, and moreover will lead to discussions like this one.
What if I want to use two competing code style tools at the same time, but they make contrary assumptions on vague rules like that one?

Btw.: Even ZF2 will fail with many errors once they upgrade to CS2.2 - because there simply is no clear rule...

[gsherwood]

any tool aiming to follow PSR2 should not make any additional assumptions

I'm specifically trying to not make assumptions. I will ask on the PHP-FIG-CS mailing list and see if someone can clear this one up.

[gsherwood]

I will ask on the PHP-FIG-CS mailing list and see if someone can clear this one up.

Actually, it seems like @pmjones has already indicated that there should not be a newline.

Firstly here: https://github.com/php-fig/fig-standards/issues/181
Then much more recently here: https://github.com/php-fig/fig-standards/issues/261

I'll still ask on the mailing list for a bit more clarification.

[Pittiplatsch]

Hope you're luckier than all the guys before...
By now, all I found were prevarications, suggesting to refactor code like this to completely avoid these situations...

[Pittiplatsch]

Actually, it seems like @pmjones has already indicated that there should not be a newline.

I saw those ones too, actually I had these posts in mind when I wrote my last comment.
However, I can't see any reliable hint to the one or the other direction. On the contrary:

This comes up so often that we should add it to the meta document.
https://github.com/php-fig/fig-standards/issues/181#issuecomment-24595159

So, thx for your efforts to gather more concrete information. :-)

[jian-wu]

Honestly, I really don't like creating explaining variables. That to me is no different than breaking up multi-conditional if statement into nested single if statements.

Anyway, 👍 for trying to getting more concrete answer to this issue.

Thanks a bunch.

[gsherwood]

I've posted here: https://groups.google.com/forum/#!topic/php-fig-cs/9yc7Go8dJkM

Personally, I think that treating the word "space" to mean a literal space character makes things much easier, so I'm interested to see what other people think on that mailing list.

I'd be very happy to make this change, although I do think it would be a lot better to have a really clear multi-line control structure format defined in the standard. But I know this will never happen because PHP-FIG have already stated that PSRs are never changed once released.

[jian-wu]

Well, they can always release PSR-5. 😄

Thanks and good luck to us all.

[gsherwood]

It's been over a month and I've had zero replies to my post. Because of this, I'll leave PHPCS as it is based on the previous php-fig issues I linked to.

If anyone has more success getting something concrete documented, please reopen this issue (or just open a new one) so we can discuss it again.

[wodka]

Is there at least a way to ignore those additional spaces?

I really do not want to introduce new variables simply to make the "Ifs" work -> multiline cases usually would result in lines longer than 120 chars...

[white-gecko]

I thought it was already the result of #188 that a newline after the opening bracket for an if-condition is fine for PSR-2. The pull-request #188 was already merged, so I wonder why codesniffer tells me

Expected 0 spaces after opening bracket; newline found

in version 2.3.1

[gsherwood]

It was changed here: ccf1f70

Here is my problem:

If the following code is valid PSR2 code:

if (
    $foo === "bar" &&
    $bar === "foo"
) {
    // ...
}

Then so is:

if (
        $foo === "bar" &&
    $bar === "foo"
) {
    // ...
}

And also:

if (
$foo === "bar" &&
    $bar === "foo"
) {
    // ...
}

Because PSR2 was not written to support this formatting, and so does not put rules in place to govern what it should look like when you use a newline after the opening brace.

So people were complaining to me that the auto-fixing feature of PHPCS was not fixing these other 2 code blocks, which was correct because nothing in PSR2 actually complains about them as errors. So I'd have to add a completely new error into PSR2 to enforce that the conditions need to be indented 4 spaces, which is completely reasonable but not actually part of the standard.

Going back, it was clear that PSR2 was never written with this type of code block in mind (that was admitted in #188 as well) and that other PSR issues (see previous comments) stated that this type of code was not allowed. So what can I do? I can't both fix this code for people and adhere to PSR2 unless I make the assumption that a newline after the opening brace is not allowed.

I really suggest that people with a clear coding standard in mind take PSR2 and then customise it to meet your needs. Even some PHP-FIG member projects who say they adhere to PSR2 don't actually adhere to the standard. They pick and choose the bits they want and then ignore the rest. That's perfectly fine as well, and PHPCS lets you customise PSR2 using a ruleset.xml file. In this case, you can mute the PSR2.ControlStructures.ControlStructureSpacing.SpacingAfterOpenBrace message to do this:

<?xml version="1.0"?>
<ruleset name="MyStandard">
    <description>My custom coding standard.</description>
    <rule ref="PSR2">
        <exclude name="PSR2.ControlStructures.ControlStructureSpacing.SpacingAfterOpenBrace"/>
    </rule>
</ruleset>

But then I'd also suggest writing a sniff to enforce whatever specific rules you want in place for multi-line conditions as well. Things like indentation, how many conditions per line, and where the logical operators should be placed (start or end of line).

[nkovacs]

The problem with your suggestion is that it also mutes errors about spaces after the opening brace, so I had to copy the whole sniff and undo your change in addition to writing a separate sniff that checks indentation in multi-line conditions.

[chrisemerson]

Yeh, this is old now, but I really think that a set of standards that claim to be PSR-2 shouldn't be imposing additional rules that aren't in the standard just because it makes some other bits tidier. You say that the behaviour is undefined with regards to indentation after a newline - yes, it probably is. PSR-2 has all kinds of stuff missing, it's not by any means perfect, but putting in additional rules that aren't in the standard just to keep things tidy in your head really shouldn't happen. If it claims to be a PSR-2 standard, then it should enforce only the rules within the standard, and like you say - people can add additional sniffs if they want to enforce a specific indentation style after a new line.

So people were complaining to me that the auto-fixing feature of PHPCS was not fixing these other 2 code blocks, which was correct because nothing in PSR2 actually complains about them as errors.

So you've added a rule that isn't in the standard just so you don't get complaints from people that are wrong anyway?

I can easily modify the provided standard myself to get the behaviour I want, but my issue is with a set of rules claiming to be PSR-2 that in fact impose additional rules not in the standard.

[white-gecko]

To ensure a strict interpretation of PSR-2 on the one hand and additionally have a set of practical PSR-2+ sniffs on the other hand would sound like a solution, right?

[chrisemerson]

Yes, I think that would be good

[tiagofrancafernandes]

<rule ref="PSR2">
    <exclude name="PSR2.ControlStructures.ControlStructureSpacing.SpacingAfterOpenBrace"/>
 </rule>