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

[Bug] PSR2.ControlStructure.ControlStructureSpacing inconsistent behaviour #2499

Closed
VincentLanglet opened this issue May 9, 2019 · 4 comments
Milestone

Comments

@VincentLanglet
Copy link
Contributor

I saw many discussion about PSR2.ControlStructure.ControlStructureSpacingSniff since this rule report an error with this code

// Case 1
if (
    $expr1
    && $expr2
) {
    // if body
}

The PSR2 rule is the following

Opening parentheses for control structures MUST NOT have a space after them, and closing > parentheses for control structures MUST NOT have a space before.

The fact newline are not allowed was not clear and is debatable ; but the phpcs rule made a choice and that's fine.

Then, why this rule does not report error with

// Case 2
if ($expr1
    && $expr2
) {
    // if body
}

I don't see any reason to ban newline after the opening parentheses and not for closing parentheses. The way the PSR2 is written is the same for both opening and closing parentheses.

Worst, since there is a check ($tokens[$parenOpener]['line'] === $tokens[$parenCloser]['line']), the Phpcs rule does not give error for this code, which does not follow the PSR2 standard.

// Case 3
if ($expr1
    && $expr2             ) {
    // if body
}

In my opinion :

  • Case 3 should be an error, it's a bug.
  • Case 1 should be allowed OR Case 2 should be forbidden, the actual situation is not consistent.
@gsherwood
Copy link
Member

Long ago, I asked about this here: https://groups.google.com/d/msg/php-fig-cs/9yc7Go8dJkM/8Lp4n7ZNpxQJ

There are some links at the bottom that might explain how things came to be.

Nobody is really interested in enforcing PSR-2 to the letter of the law given it disregards a lot of common ways of doing things. Given PHPCS has behaved as it has for many years, I'm not going to change it now.

@VincentLanglet
Copy link
Contributor Author

I can understand the choice made @gsherwood.

At least, maybe we should fix

// Case 3
if ($expr1
    && $expr2             ) {
    // if body
}

For other cases, the changes can wait a PSR12.ControlStructure.ControlStructureSpacing

@SharkMachine
Copy link

@gsherwood What about https://github.com/php-fig/fig-standards/blob/master/proposed/extended-coding-style-guide.md#51-if-elseif-else ? Has the upcoming extended coding style guide been considered?

Expressions in parentheses MAY be split across multiple lines, where each subsequent line is indented at least once. When doing so, the first condition MUST be on the next line. The closing parenthesis and opening brace MUST be placed together on their own line with one space between them. Boolean operators between conditions MUST always be at the beginning or at the end of the line, not a mix of both.

@gsherwood gsherwood added this to the 3.5.1 milestone Sep 23, 2019
@gsherwood gsherwood modified the milestones: 3.5.1, 3.5.2 Oct 1, 2019
gsherwood added a commit that referenced this issue Nov 11, 2019
…before the closing parenthesis of multi-line control structures (ref #2499)
@gsherwood
Copy link
Member

At least, maybe we should fix

I've committed a change so that the sniff checks whitespace before the closing parenthesis of multi-line structures. It was intentionally performing the check for single-line structures only, but I think PSR2 is clear enough to extend that check.

The new PSR-12 standard is more clearer in this area, and the included PSR12 standard already banned this type of formatting. Where possible, I'd recommend moving to PSR12, or at least picking the PSR12.ControlStructures.ControlStructureSpacing sniff over the PSR2 one.

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

No branches or pull requests

3 participants