squizlabs / PHP_CodeSniffer

PHP_CodeSniffer tokenizes PHP files and detects violations of a defined set of coding standards.
BSD 3-Clause "New" or "Revised" License
10.66k stars 1.48k forks source link

PSR12 - foreach - ControlStructure #3047

Open SpazzMarticus opened 4 years ago

SpazzMarticus commented 4 years ago

Hi,

in the PSR-12 standard foreach is defined only as:

A foreach statement looks like the following. Note the placement of parentheses, spaces, and braces.

<?php

foreach ($iterable as $key => $value) {
    // foreach body
}

Splitting - like for if, elseif, else, switch, case, while, do-while and for - is not defined:

... Expressions in parentheses MAY be split across multiple lines, where each subsequent line is indented at least once. ...

Given these three formatting styles only the first one yields no errors:

<?php

$array = ['a', 'b', 'c'];
foreach (
    array_filter($array, function ($a) {
        return $a === 'b';
    }) as $data
) {
    //code
}

foreach (array_filter($array, function ($a) { //line 12
    return $a === 'b'; 
}) as $data) { //line 14
    //code
}

foreach (array_filter( //line 18
    $array,
    function ($a) {
        return $a === 'b';
    }) as $data) { //line 22
    //code
}

PHP 7.4.7 on Windows, with CodeSniffer 3.5.5:

 12 | ERROR | [x] The first expression of a multi-line control structure must be on the line after
    |       |     the opening parenthesis
    |       |     (PSR12.ControlStructures.ControlStructureSpacing.FirstExpressionLine)
 14 | ERROR | [x] Each line in a multi-line control structure must be indented at least once;
    |       |     expected at least 4 spaces, but found 0
    |       |     (PSR12.ControlStructures.ControlStructureSpacing.LineIndent)
 14 | ERROR | [x] The closing parenthesis of a multi-line control structure must be on the line after
    |       |     the last expression
    |       |     (PSR12.ControlStructures.ControlStructureSpacing.CloseParenthesisLine)
 18 | ERROR | [x] The first expression of a multi-line control structure must be on the line after
    |       |     the opening parenthesis
    |       |     (PSR12.ControlStructures.ControlStructureSpacing.FirstExpressionLine)
 22 | ERROR | [x] Multi-line function call not indented correctly; expected 0 spaces but found 4
    |       |     (PSR2.Methods.FunctionCallSignature.Indent)
 22 | ERROR | [x] Closing parenthesis of a multi-line function call must be on a line by itself
    |       |     (PSR2.Methods.FunctionCallSignature.CloseBracketLine)
 22 | ERROR | [x] The closing parenthesis of a multi-line control structure must be on the line after
    |       |     the last expression
    |       |     (PSR12.ControlStructures.ControlStructureSpacing.CloseParenthesisLine)

Is the first formatting style valid PSR-12 code?

gsherwood commented 3 years ago

I don't know why the MR (https://github.com/php-fig/fig-standards/pull/900) didn't include this for foreach statements as it didn't explicitly exclude them and the discussion revolved around IF statements mainly.

I'm not sure I want to back out of allowing this for all control structures as it's likely to cause some BC breaks and a discussion about the author's intention, which I can't speak for.

Will leave this open in case anyone cares to comment, but I wont take any action at this time. Thanks for bringing it up though.

jwittorf commented 1 year ago

I'll dig up this issue because I'm currently comparing different tools like PHP_CodeSniffer and PHP CS Fixer. I'm using PSR-12 as a reference to check how precise the tools check for and fix the code according to PSR-12.

Since there is nothing saying "foreach multi-line statements MUST follow the same guidelines and principles as if statements, see examples below", neither of @SpazzMarticus code-snippets would be correct. Furthermore, they are missing the => $value part.

According to the currently accepted PSR-12, the only valid way is using foreach ($iterable as $key => $value) - period.

I therefore propose to add some input to the foreach section as described in the second paragraph.

Some of my personal ideas:

<?php

$iterableArray = [
    "one",
    "two",
    "three",
];

// this makes no sense (1-2)
foreach (
    $iterableArray as $i => $value) {
    // code
}

foreach (
    $iterableArray as $i => $value
) {
    // code
}

// this could make sense (3-4)
foreach (
    $iterableArray
    as $i => $value
) {
    // code
}

foreach (
    $iterableArray as $i
    => $value
) {
    // code
}

// how about this? (5-6)
// a different approach, similar to the for-loop
foreach (
    $iterableArray
    as $i =>
    $value
) {
    // code
}

foreach (
    $iterableArray
    as $i
    => $value
) {
    // code
}

I'm curious about further input, although the issue is a bit older.

/edit: Whops, I though this was already in the PSR-repository ... I'll start a discussion on their mailing list.