Fixed bug #2763 : PSR12 standard reports errors for multi-line FOR definitions

gsherwood · gsherwood · commit 692237d7ad70 · 2020-01-06T15:48:57.000+11:00
This was done by adding a new ignoreNewlines property to Squiz.ControlStructures.ForLoopDeclaration so that the PSR12 standard could use it. In the case of newlines, PSR12 should ignore the statement and let other sniffs enforce the multi-line rules. But when there is no newline, it needs this sniff to enforce a single space.
diff --git a/package.xml b/package.xml
@@ -40,6 +40,9 @@ http://pear.php.net/dtd/package-2.0.xsd">
     -- Thanks to Vincent Langlet for the patch
   - PSR12.Files.ImportStatement now auto-fixes import statements by removing the leading slash
     -- Thanks to Michał Bundyra for the patch
+  - Squiz.ControlStructures.ForLoopDeclaration now has a setting to ignore newline characters
+    -- Default remains FALSE, so newlines are not allowed within FOR definitions
+    -- Override the "ignoreNewlines" setting in a ruleset.xml file to change
   - Squiz.PHP.InnerFunctions now handles multiple nested anon classes correctly
   - Fixed bug #2688 : Case statements not tokenized correctly when switch is contained within ternary
   - Fixed bug #2698 : PHPCS throws errors determining auto report width when shell_exec is disabled
@@ -48,6 +51,7 @@ http://pear.php.net/dtd/package-2.0.xsd">
     -- Thanks to Michał Bundyra for the patch
   - Fixed bug #2751 : Autoload relative paths first to avoid confusion with files from the global include path
     -- Thanks to Klaus Purer for the patch
+  - Fixed bug #2763 : PSR12 standard reports errors for multi-line FOR definitions
   - Fixed bug #2768 : Generic.Files.LineLength false positive for non-breakable strings at exactly the soft limit
     -- Thanks to Alex Miles for the patch
   - Fixed bug #2791 : PSR12.Functions.NullableTypeDeclaration false positive when ternary operator used with instanceof
diff --git a/src/Standards/PSR12/ruleset.xml b/src/Standards/PSR12/ruleset.xml
@@ -228,7 +228,11 @@
     <rule ref="Squiz.WhiteSpace.ControlStructureSpacing.SpacingBeforeClose"/>
     <rule ref="Squiz.WhiteSpace.ScopeClosingBrace"/>
     <rule ref="Squiz.ControlStructures.ForEachLoopDeclaration"/>
-    <rule ref="Squiz.ControlStructures.ForLoopDeclaration"/>
+    <rule ref="Squiz.ControlStructures.ForLoopDeclaration">
+        <properties>
+            <property name="ignoreNewlines" value="true"/>
+        </properties>
+    </rule>
     <rule ref="Squiz.ControlStructures.ForLoopDeclaration.SpacingAfterOpen">
         <severity>0</severity>
     </rule>
diff --git a/src/Standards/Squiz/Sniffs/ControlStructures/ForLoopDeclarationSniff.php b/src/Standards/Squiz/Sniffs/ControlStructures/ForLoopDeclarationSniff.php
@@ -30,6 +30,13 @@ class ForLoopDeclarationSniff implements Sniff
      */
     public $requiredSpacesBeforeClose = 0;
 
+    /**
+     * Allow newlines instead of spaces.
+     *
+     * @var boolean
+     */
+    public $ignoreNewlines = false;
+
     /**
      * A list of tokenizers this sniff supports.
      *
@@ -77,20 +84,27 @@ public function process(File $phpcsFile, $stackPtr)
 
         $closingBracket = $tokens[$openingBracket]['parenthesis_closer'];
 
-        if ($this->requiredSpacesAfterOpen === 0 && $tokens[($openingBracket + 1)]['code'] === T_WHITESPACE) {
-            $error = 'Whitespace found after opening bracket of FOR loop';
-            $fix   = $phpcsFile->addFixableError($error, $openingBracket, 'SpacingAfterOpen');
-            if ($fix === true) {
-                $phpcsFile->fixer->beginChangeset();
-                for ($i = ($openingBracket + 1); $i < $closingBracket; $i++) {
-                    if ($tokens[$i]['code'] !== T_WHITESPACE) {
-                        break;
+        if ($this->requiredSpacesAfterOpen === 0
+            && $tokens[($openingBracket + 1)]['code'] === T_WHITESPACE
+        ) {
+            $nextNonWhiteSpace = $phpcsFile->findNext(T_WHITESPACE, ($openingBracket + 1), $closingBracket, true);
+            if ($this->ignoreNewlines === false
+                || $tokens[$nextNonWhiteSpace]['line'] === $tokens[$openingBracket]['line']
+            ) {
+                $error = 'Whitespace found after opening bracket of FOR loop';
+                $fix   = $phpcsFile->addFixableError($error, $openingBracket, 'SpacingAfterOpen');
+                if ($fix === true) {
+                    $phpcsFile->fixer->beginChangeset();
+                    for ($i = ($openingBracket + 1); $i < $closingBracket; $i++) {
+                        if ($tokens[$i]['code'] !== T_WHITESPACE) {
+                            break;
+                        }
+
+                        $phpcsFile->fixer->replaceToken($i, '');
                     }
 
-                    $phpcsFile->fixer->replaceToken($i, '');
+                    $phpcsFile->fixer->endChangeset();
                 }
-
-                $phpcsFile->fixer->endChangeset();
             }
         } else if ($this->requiredSpacesAfterOpen > 0) {
             $nextNonWhiteSpace = $phpcsFile->findNext(T_WHITESPACE, ($openingBracket + 1), $closingBracket, true);
@@ -101,7 +115,10 @@ public function process(File $phpcsFile, $stackPtr)
                 $spaceAfterOpen = $tokens[($openingBracket + 1)]['length'];
             }
 
-            if ($spaceAfterOpen !== $this->requiredSpacesAfterOpen) {
+            if ($spaceAfterOpen !== $this->requiredSpacesAfterOpen
+                && ($this->ignoreNewlines === false
+                || $spaceAfterOpen !== 'newline')
+            ) {
                 $error = 'Expected %s spaces after opening bracket; %s found';
                 $data  = [
                     $this->requiredSpacesAfterOpen,
@@ -133,7 +150,11 @@ public function process(File $phpcsFile, $stackPtr)
             $beforeClosefixable = false;
         }
 
-        if ($this->requiredSpacesBeforeClose === 0 && $tokens[($closingBracket - 1)]['code'] === T_WHITESPACE) {
+        if ($this->requiredSpacesBeforeClose === 0
+            && $tokens[($closingBracket - 1)]['code'] === T_WHITESPACE
+            && ($this->ignoreNewlines === false
+            || $tokens[$prevNonWhiteSpace]['line'] === $tokens[$closingBracket]['line'])
+        ) {
             $error = 'Whitespace found before closing bracket of FOR loop';
 
             if ($beforeClosefixable === false) {
@@ -161,7 +182,10 @@ public function process(File $phpcsFile, $stackPtr)
                 $spaceBeforeClose = $tokens[($closingBracket - 1)]['length'];
             }
 
-            if ($this->requiredSpacesBeforeClose !== $spaceBeforeClose) {
+            if ($this->requiredSpacesBeforeClose !== $spaceBeforeClose
+                && ($this->ignoreNewlines === false
+                || $spaceBeforeClose !== 'newline')
+            ) {
                 $error = 'Expected %s spaces before closing bracket; %s found';
                 $data  = [
                     $this->requiredSpacesBeforeClose,
@@ -264,7 +288,10 @@ public function process(File $phpcsFile, $stackPtr)
                         $spaces = 'newline';
                     }
 
-                    if ($spaces !== 1) {
+                    if ($spaces !== 1
+                        && ($this->ignoreNewlines === false
+                        || $spaces !== 'newline')
+                    ) {
                         $error     = 'Expected 1 space after %s semicolon of FOR loop; %s found';
                         $errorCode = 'SpacingAfter'.$humanReadableCode;
                         $data[]    = $spaces;
diff --git a/src/Standards/Squiz/Tests/ControlStructures/ForLoopDeclarationUnitTest.inc b/src/Standards/Squiz/Tests/ControlStructures/ForLoopDeclarationUnitTest.inc
@@ -115,5 +115,15 @@ for (
 for ($i = function() { return $this->i  ;    }; $i < function() { return $this->max; }; $i++) {}
 for ($i = function() { return $this->i; }; $i < function() { return $this->max; }  ;   $i++) {}
 
+// phpcs:set Squiz.ControlStructures.ForLoopDeclaration ignoreNewlines true
+for (
+    $i = 0;
+    $i < 5;
+    $i++
+) {
+    // body here
+}
+// phpcs:set Squiz.ControlStructures.ForLoopDeclaration ignoreNewlines false
+
 // This test has to be the last one in the file! Intentional parse error check.
 for
diff --git a/src/Standards/Squiz/Tests/ControlStructures/ForLoopDeclarationUnitTest.inc.fixed b/src/Standards/Squiz/Tests/ControlStructures/ForLoopDeclarationUnitTest.inc.fixed
@@ -81,5 +81,15 @@ for ( $i = 0; $i < 10; $i++ ) {}
 for ($i = function() { return $this->i  ;    }; $i < function() { return $this->max; }; $i++) {}
 for ($i = function() { return $this->i; }; $i < function() { return $this->max; }; $i++) {}
 
+// phpcs:set Squiz.ControlStructures.ForLoopDeclaration ignoreNewlines true
+for (
+    $i = 0;
+    $i < 5;
+    $i++
+) {
+    // body here
+}
+// phpcs:set Squiz.ControlStructures.ForLoopDeclaration ignoreNewlines false
+
 // This test has to be the last one in the file! Intentional parse error check.
 for
diff --git a/src/Standards/Squiz/Tests/ControlStructures/ForLoopDeclarationUnitTest.php b/src/Standards/Squiz/Tests/ControlStructures/ForLoopDeclarationUnitTest.php
@@ -117,7 +117,7 @@ public function getWarningList($testFile='ForLoopDeclarationUnitTest.inc')
     {
         switch ($testFile) {
         case 'ForLoopDeclarationUnitTest.inc':
-            return [119 => 1];
+            return [129 => 1];
 
         case 'ForLoopDeclarationUnitTest.js':
             return [125 => 1];

Original file line number	Diff line number	Diff line change
`@@ -117,7 +117,7 @@ public function getWarningList($testFile='ForLoopDeclarationUnitTest.inc')`
`117`	`117`	`{`
`118`	`118`	`switch ($testFile) {`
`119`	`119`	`case 'ForLoopDeclarationUnitTest.inc':`
`120`		`- return [119 => 1];`
	`120`	`+ return [129 => 1];`
`121`	`121`
`122`	`122`	`case 'ForLoopDeclarationUnitTest.js':`
`123`	`123`	`return [125 => 1];`