Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

PSR2: fix newline character being seen as superfluous spacing in multiline control structure #188

Merged

Conversation

mcuelenaere
Copy link
Contributor

This fixes the following code snippet:

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

@gsherwood
Copy link
Member

I don't think this is valid PSR2 code. The standard states "There MUST NOT be a space after the opening parenthesis".

If you don't consider a newline character to be a space character, then it's best if you go onto the PHP-FIG mailing list and ask about this specific case. I've had them review the PHP_CodeSniffer standard and this has not been brought up as an issue. I also never change the PSR1 or PSR2 standards without them first clarifying their position, unless I've very clearly made a mistake. I don't think that is the case here.

If anything, the standard would need a piece of errata to explain that newlines are allowed at various points, even though every example of a control structure doesn't use them, because it would be ambiguous if both interpretations were allowed.

@mcuelenaere
Copy link
Contributor Author

I believe it is, but the standard just doesn't mention anything regarding multilines in control structures.

4.6. Method and Function Calls states "there MUST NOT be a space after the opening parenthesis", while it also has an addition which says "Argument lists MAY be split across multiple lines".

I believe that this also (implicitly) applies to 5. Control Structures.

When looking for other references, I found this post (continues here) and https://github.com/php-fig/fig-standards/issues/181.

However, they mainly sidestep the issue by extracting the condition into a separate variable. I couldn't find an official stance on this.

Perhaps I should just ask php-fig-cs to clear this up.

@gsherwood
Copy link
Member

Yep, I think asking about it is the best way to clear it up.

@padraic
Copy link

padraic commented Feb 13, 2014

Just to summarise here, Paul M. Jones and I discussed this on the php-fig-cs mailing list and we concluded that the given code meets PSR-2. This is to the letter of the PSR-2 text.

The original intention would have been that this code NOT meet the standard, but short of a DeLorean, a fusion reactor and a Quantum Flux Capacitor...we reap what we sow. In the interests of backwards compatibility, an errata will not be proposed to restate the rule.

The wording interpretation hinges on the meaning of "space" versus the technically correct term "whitespace". Since one of these rules is unavoidably using "space" to mean a single lone space character, that definition must take precedence in the preceding rules. The term "space" does not therefore include any other vertical or horizontal whitespace character other than a space character.

Unfortunately, this is one of those times where we can't expect readers to know the original intent of the author, and cannot allow their lack of psychic ability to be penalised for something that's just not their fault.

@gsherwood
Copy link
Member

If I didn't have this standard in PHP_CodeSniffer I'd have nothing to do :)

Thanks for figuring out your position. Given that multi-line conditions are not defined anywhere in PSR-2 and are now acceptable, I imagine it will lead to some interesting formatting (PEAR has a particular standard for this if people want something to follow) but I'll make sure PHPCS just ignores them.

@gsherwood gsherwood merged commit 4ef9f3a into squizlabs:master Feb 20, 2014
@jfcherng jfcherng mentioned this pull request Feb 20, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants