Multiline control structure and PHP CodeSniffer

[Maurus Cuelenaere]

Hi all,

if I understand the PSR-2 correctly, the following code is valid PSR-2:

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

However, PHP CodeSniffer thinks differently about this and reports it as an error:

--------------------------------------------------------------------------------
FOUND 1 ERROR(S) AFFECTING 1 LINE(S)
--------------------------------------------------------------------------------
 1 | ERROR | Expected 0 spaces after opening bracket; 1 found
--------------------------------------------------------------------------------

( tracking issue at https://github.com/squizlabs/PHP_CodeSniffer/pull/188 )

Is this code snippet valid PSR-2 and if not, how should this construct be expressed?

PS: I know it is frowned upon to have complex conditions in control structures, but I'm not looking to solve that issue.

Thanks,

--
Maurus Cuelenaere

[Paul M. Jones]

On Feb 13, 2014, at 7:38 AM, Maurus Cuelenaere <mcuel...@gmail.com> wrote:

> Hi all,
>
> if I understand the PSR-2 correctly, the following code is valid PSR-2:
>
> if (
> $foo === "bar" &&
> $bar === "foo"
> ) {
> // ...
> }
>
> However, PHP CodeSniffer thinks differently about this and reports it as an error:
>
> --------------------------------------------------------------------------------
> FOUND 1 ERROR(S) AFFECTING 1 LINE(S)
> --------------------------------------------------------------------------------
> 1 | ERROR | Expected 0 spaces after opening bracket; 1 found
> --------------------------------------------------------------------------------
>
>
> ( tracking issue at https://github.com/squizlabs/PHP_CodeSniffer/pull/188 )
>
> Is this code snippet valid PSR-2 and if not, how should this construct be expressed?

I believe PHPCS is correct in this case. There should be no space after the opening parenthesis. The solution is to extract the condition to an explaining variable like so:

$foobar = $foo === "bar" &&
$bar === "foo";

if ($foobar) {
// ...
}

That will make it compliant. Alternatively, keep the conditions all on the same line.


--
Paul M. Jones
pmjo...@gmail.com
http://paul-m-jones.com

Modernizing Legacy Applications in PHP
http://mlaphp.com

[Pádraic Brady]

Hi Marcus,

On 13 February 2014 13:38, Maurus Cuelenaere <mcuel...@gmail.com> wrote:
> Hi all,
>
> if I understand the PSR-2 correctly, the following code is valid PSR-2:
>
>> if (
>> $foo === "bar" &&
>> $bar === "foo"
>> ) {
>> // ...
>> }
>
>
> However, PHP CodeSniffer thinks differently about this and reports it as an
> error:
>
>>
>> --------------------------------------------------------------------------------
>> FOUND 1 ERROR(S) AFFECTING 1 LINE(S)
>>
>> --------------------------------------------------------------------------------
>> 1 | ERROR | Expected 0 spaces after opening bracket; 1 found
>>
>> --------------------------------------------------------------------------------
>
> ( tracking issue at https://github.com/squizlabs/PHP_CodeSniffer/pull/188 )
>
> Is this code snippet valid PSR-2 and if not, how should this construct be
> expressed?

Control structures are covered by Section 5 of PSR-2. The rules state:

- There MUST be one space after the control structure keyword
- There MUST NOT be a space after the opening parenthesis
- There MUST NOT be a space before the closing parenthesis
- There MUST be one space between the closing parenthesis and the opening brace
- The structure body MUST be indented once
- The closing brace MUST be on the next line after the body

The word “space” is not defined in rule 2 or 3, but can only be taken
literally in rule 4, so it must be taken literally for all rules. The
standard does not, therefore, preclude your example in my opinion.

> PS: I know it is frowned upon to have complex conditions in control
> structures, but I'm not looking to solve that issue.

Yes, it is frowned on ;). It’s generally more readable to extract them
into a separate variable assignment or format starting on the same
line as the starting parenthesis which is the generic rule
PHP_CodeSniffer is falling back on I presume. That is irrelevant in
this case since PSR-2 is effectively silent on how to format control
statement conditions over multiple lines.

If Greg doesn’t buy the obvious explanation, the only other way to get
PHP_CodeSniffer changed is to have another PSR-2 errata voted for by
PHP-FIG to formalise its position on this particular case.

Paddy

--
Pádraic Brady

http://blog.astrumfutura.com
http://www.survivethedeepend.com
Zend Framework Community Review Team
Zend Framework PHP-FIG Representative

[Paul M. Jones]

On Feb 13, 2014, at 8:55 AM, Pádraic Brady <padrai...@gmail.com> wrote:

>> Is this code snippet valid PSR-2 and if not, how should this construct be
>> expressed?
>
> Control structures are covered by Section 5 of PSR-2. The rules state:
>
> - There MUST be one space after the control structure keyword
> - There MUST NOT be a space after the opening parenthesis
> - There MUST NOT be a space before the closing parenthesis
> - There MUST be one space between the closing parenthesis and the opening brace
> - The structure body MUST be indented once
> - The closing brace MUST be on the next line after the body
>
> The word “space” is not defined in rule 2 or 3, but can only be taken
> literally in rule 4, so it must be taken literally for all rules. The
> standard does not, therefore, preclude your example in my opinion.

As the guy who shepherded PSR-2 through the acceptance process, I contend that "space" is a well-known word indicating, well, a space. If it had meant "a single contiguous section of whitespace whether spaces tabs or newlines" then we'd have said so. Ancillary documents, such as the survey, back this up. So rule 1 saying "there MUST NOT be a space after the opening parenthesis" does preclude his example.

[Paul M. Jones]

As a followup, a strict reading of PSR-2 might not preclude this ...

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

... since there is technically no space after the opening paren, only a newline, but I would hope for some level of sensibility to prevail here. ;-)

[Pádraic Brady]

Hi Paul,

> As a followup, a strict reading of PSR-2 might not preclude this ...
>
> if (
> $foo === "bar" &&
> $bar === "foo")
> {
> // ...
> }
>
> ... since there is technically no space after the opening paren, only a newline, but I would hope for some level of sensibility to prevail here. ;-)

I’m with you on the sensibility bit, but we’re stuck with the current
wording. Rather than leaving you short a head, and transplanting your
brain cells into all PHP programmers, we’ll need an errata or
something to swing things to where you intended.

You sure you wouldn’t part with the head? ;)

[Paul M. Jones]

On Feb 13, 2014, at 9:24 AM, Pádraic Brady <padrai...@gmail.com> wrote:

> Hi Paul,
>
>> As a followup, a strict reading of PSR-2 might not preclude this ...
>>
>> if (
>> $foo === "bar" &&
>> $bar === "foo")
>> {
>> // ...
>> }
>>
>> ... since there is technically no space after the opening paren, only a newline, but I would hope for some level of sensibility to prevail here. ;-)
>
> I’m with you on the sensibility bit, but we’re stuck with the current
> wording. Rather than leaving you short a head, and transplanting your
> brain cells into all PHP programmers, we’ll need an errata or
> something to swing things to where you intended.

I think for people of good will and a collegial attitude, there's not an issue here. For those of ill will with exploitation in mind, well, no set of rules will ever be good enough.

I'm pretty sure that any modifications at this point will be beyond errata, since a change might render code that is *currently* compliant (in some sense) *not* compliant. :-/

But if others here think errata are a reasonable solution, I'm on board, if only to quell frequently-asked and totally reasonable questions such as Maurus'.

> You sure you wouldn’t part with the head? ;)

My bet is that some would opine that happened long ago. ;-)

[Jason Judge]

On Thursday, 13 February 2014 15:09:12 UTC, pmjones wrote:

> As the guy who shepherded PSR-2 through the acceptance process, I contend that "space" is a well-known word indicating, well, a space. If it had meant "a single contiguous section of whitespace whether spaces tabs or newlines" then we'd have said so. Ancillary documents, such as the survey, back this up. So rule 1 saying "there MUST NOT be a space after the opening parenthesis" does preclude his example.

As a followup, a strict reading of PSR-2 might not preclude this ...

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

... since there is technically no space after the opening paren, only a newline, but I would hope for some level of sensibility to prevail here. ;-)

--
Paul M. Jones

So if the closing brace is on a line of its own, as above, does that closing brace have "a space" in front of it, or does it have the required level of indentation and NO space between that indentation and the closing brace? I would argue that the spaces are counted only AFTER the indentation is taken into account, and that the above closing brace does not have "a space" in front of it.

The whole reason for that rule was to prevent putting needless spaces around the arguments within a parameter list, al-la WordPress style. The spaces in indentation is another thing altogether, and spaces should not be counted as two completely different things at the same time. IMO :-)

-- Jason

[Jason Judge]

Google stripped out the code I pasted in the digest email. Here it is:

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

My argument is that the ")" does not have "a space" in front of it, because the spaces in front of it on that line are already taken into account as indentation. So putting that ")" on the previous line after "foo" is not needed just to meet the letter of the law in the PSR. That's how I see it, at least.

-- Jason