From 604c25f9720c41f9fe2c08eb0a107ba93130e33c Mon Sep 17 00:00:00 2001 From: jrfnl Date: Thu, 9 Jan 2025 10:31:44 +0100 Subject: [PATCH 01/14] Squiz/FunctionDeclarationArgumentSpacing: improve `SpacingBetween` fixer The fixer for the `SpacingBetween` error code would only handle one whitespace token at a time, which is inefficient and could also lead to more complex fixer conflicts. This commit changes the fixer to handle all whitespace between the parentheses in one go. For the code sample added as a test, this means the difference between the fixer needing 7 loops (including two skipped loops due to possible conflicts) versus 2 loops.
Old: ``` => Fixing file: 1/1 violations remaining Squiz.Functions.FunctionDeclarationArgumentSpacing:133 replaced token 6 (T_WHITESPACE on line 3) "\n\n" => "\n" => Fixing file: 1/1 violations remaining [made 1 pass]... * fixed 1 violations, starting loop 2 * Squiz.Functions.FunctionDeclarationArgumentSpacing:133 replaced token 6 (T_WHITESPACE on line 3) "\n\n" => "\n" => Fixing file: 1/1 violations remaining [made 2 passes]... * fixed 1 violations, starting loop 3 * **** Squiz.Functions.FunctionDeclarationArgumentSpacing:133 has possible conflict with another sniff on loop 1; caused by the following change **** **** replaced token 6 (T_WHITESPACE on line 3) "\n " => " " **** **** ignoring all changes until next loop **** => Fixing file: 0/1 violations remaining [made 3 passes]... * fixed 0 violations, starting loop 4 * Squiz.Functions.FunctionDeclarationArgumentSpacing:133 replaced token 6 (T_WHITESPACE on line 3) "\n " => " " => Fixing file: 1/1 violations remaining [made 4 passes]... * fixed 1 violations, starting loop 5 * **** Squiz.Functions.FunctionDeclarationArgumentSpacing:133 has possible conflict with another sniff on loop 3; caused by the following change **** **** replaced token 6 (T_WHITESPACE on line 3) " )" => ")" **** **** ignoring all changes until next loop **** => Fixing file: 0/1 violations remaining [made 5 passes]... * fixed 0 violations, starting loop 6 * Squiz.Functions.FunctionDeclarationArgumentSpacing:133 replaced token 6 (T_WHITESPACE on line 3) " )" => ")" => Fixing file: 1/1 violations remaining [made 6 passes]... * fixed 1 violations, starting loop 7 * => Fixing file: 0/1 violations remaining [made 7 passes]... ```
New: ``` => Fixing file: 1/1 violations remaining => Changeset started by Squiz.Functions.FunctionDeclarationArgumentSpacing:133 Q: Squiz.Functions.FunctionDeclarationArgumentSpacing:135 replaced token 6 (T_WHITESPACE on line 3) "\n\n" => "\n" Q: Squiz.Functions.FunctionDeclarationArgumentSpacing:135 replaced token 7 (T_WHITESPACE on line 4) "\n\n" => "\n" Q: Squiz.Functions.FunctionDeclarationArgumentSpacing:135 replaced token 8 (T_WHITESPACE on line 5) "\n " => " " Q: Squiz.Functions.FunctionDeclarationArgumentSpacing:135 replaced token 9 (T_WHITESPACE on line 6) " )" => ")" A: Squiz.Functions.FunctionDeclarationArgumentSpacing:137 replaced token 6 (T_WHITESPACE on line 3) "\n\n" => "\n" A: Squiz.Functions.FunctionDeclarationArgumentSpacing:137 replaced token 7 (T_WHITESPACE on line 4) "\n\n" => "\n" A: Squiz.Functions.FunctionDeclarationArgumentSpacing:137 replaced token 8 (T_WHITESPACE on line 5) "\n " => " " A: Squiz.Functions.FunctionDeclarationArgumentSpacing:137 replaced token 9 (T_WHITESPACE on line 6) " )" => ")" => Changeset ended: 4 changes applied => Fixing file: 4/1 violations remaining [made 1 pass]... * fixed 4 violations, starting loop 2 * => Fixing file: 0/1 violations remaining [made 2 passes]... ```
--- .../FunctionDeclarationArgumentSpacingSniff.php | 9 +++++++-- .../FunctionDeclarationArgumentSpacingUnitTest.inc | 5 +++++ .../FunctionDeclarationArgumentSpacingUnitTest.inc.fixed | 2 ++ .../FunctionDeclarationArgumentSpacingUnitTest.php | 1 + 4 files changed, 15 insertions(+), 2 deletions(-) diff --git a/src/Standards/Squiz/Sniffs/Functions/FunctionDeclarationArgumentSpacingSniff.php b/src/Standards/Squiz/Sniffs/Functions/FunctionDeclarationArgumentSpacingSniff.php index 11cdba8d2e..5da366c6d5 100644 --- a/src/Standards/Squiz/Sniffs/Functions/FunctionDeclarationArgumentSpacingSniff.php +++ b/src/Standards/Squiz/Sniffs/Functions/FunctionDeclarationArgumentSpacingSniff.php @@ -131,13 +131,18 @@ public function processBracket($phpcsFile, $openBracket) $data = [$found]; $fix = $phpcsFile->addFixableError($error, $openBracket, 'SpacingBetween', $data); if ($fix === true) { - $phpcsFile->fixer->replaceToken(($openBracket + 1), ''); + $phpcsFile->fixer->beginChangeset(); + for ($i = ($openBracket + 1); $tokens[$i]['code'] === T_WHITESPACE; $i++) { + $phpcsFile->fixer->replaceToken($i, ''); + } + + $phpcsFile->fixer->endChangeset(); } } // No params, so we don't check normal spacing rules. return; - } + }//end if }//end if foreach ($params as $paramNumber => $param) { diff --git a/src/Standards/Squiz/Tests/Functions/FunctionDeclarationArgumentSpacingUnitTest.inc b/src/Standards/Squiz/Tests/Functions/FunctionDeclarationArgumentSpacingUnitTest.inc index 07a6d452f5..e26782f940 100644 --- a/src/Standards/Squiz/Tests/Functions/FunctionDeclarationArgumentSpacingUnitTest.inc +++ b/src/Standards/Squiz/Tests/Functions/FunctionDeclarationArgumentSpacingUnitTest.inc @@ -113,3 +113,8 @@ fn ($a,$b = null) => $a($b); function multipleWhitespaceTokensAfterType(int $number) {} + +function spacingBetweenParenthesesShouldBeFixedInOneGo( + + + ) {} diff --git a/src/Standards/Squiz/Tests/Functions/FunctionDeclarationArgumentSpacingUnitTest.inc.fixed b/src/Standards/Squiz/Tests/Functions/FunctionDeclarationArgumentSpacingUnitTest.inc.fixed index cb4265057c..da8bd13702 100644 --- a/src/Standards/Squiz/Tests/Functions/FunctionDeclarationArgumentSpacingUnitTest.inc.fixed +++ b/src/Standards/Squiz/Tests/Functions/FunctionDeclarationArgumentSpacingUnitTest.inc.fixed @@ -111,3 +111,5 @@ $a = function ($var1, $var2=false) use ( fn ($a, $b=null) => $a($b); function multipleWhitespaceTokensAfterType(int $number) {} + +function spacingBetweenParenthesesShouldBeFixedInOneGo() {} diff --git a/src/Standards/Squiz/Tests/Functions/FunctionDeclarationArgumentSpacingUnitTest.php b/src/Standards/Squiz/Tests/Functions/FunctionDeclarationArgumentSpacingUnitTest.php index 391463bbea..60d67ddda5 100644 --- a/src/Standards/Squiz/Tests/Functions/FunctionDeclarationArgumentSpacingUnitTest.php +++ b/src/Standards/Squiz/Tests/Functions/FunctionDeclarationArgumentSpacingUnitTest.php @@ -69,6 +69,7 @@ public function getErrorList() 107 => 2, 111 => 3, 113 => 1, + 117 => 1, ]; }//end getErrorList() From 399b1b382f9b0c539a8078fe18727f1ed54f1757 Mon Sep 17 00:00:00 2001 From: jrfnl Date: Thu, 9 Jan 2025 10:52:14 +0100 Subject: [PATCH 02/14] Squiz/FunctionDeclarationArgumentSpacing: bug fix / `SpacingAfterReference` does not flag new lines A `T_WHITESPACE` token only containing new line characters will have its token `'length'` set to `0`. This means that if there was a new line after a reference token, the sniff would not report it. Fixed now. Also note that, as the sniff demands no whitespace after a reference operator, we don't need the extra check for `$gap !== 0` as we already know that the next token is a whitespace token and needs to be flagged. Includes tests. --- .../FunctionDeclarationArgumentSpacingSniff.php | 6 +++--- .../FunctionDeclarationArgumentSpacingUnitTest.inc | 12 ++++++++++++ ...ctionDeclarationArgumentSpacingUnitTest.inc.fixed | 9 +++++++++ .../FunctionDeclarationArgumentSpacingUnitTest.php | 2 ++ 4 files changed, 26 insertions(+), 3 deletions(-) diff --git a/src/Standards/Squiz/Sniffs/Functions/FunctionDeclarationArgumentSpacingSniff.php b/src/Standards/Squiz/Sniffs/Functions/FunctionDeclarationArgumentSpacingSniff.php index 5da366c6d5..7598eb8950 100644 --- a/src/Standards/Squiz/Sniffs/Functions/FunctionDeclarationArgumentSpacingSniff.php +++ b/src/Standards/Squiz/Sniffs/Functions/FunctionDeclarationArgumentSpacingSniff.php @@ -149,12 +149,12 @@ public function processBracket($phpcsFile, $openBracket) if ($param['pass_by_reference'] === true) { $refToken = $param['reference_token']; - $gap = 0; if ($tokens[($refToken + 1)]['code'] === T_WHITESPACE) { $gap = $tokens[($refToken + 1)]['length']; - } + if ($tokens[$refToken]['line'] !== $tokens[($refToken + 2)]['line']) { + $gap = 'newline'; + } - if ($gap !== 0) { $error = 'Expected 0 spaces after reference operator for argument "%s"; %s found'; $data = [ $param['name'], diff --git a/src/Standards/Squiz/Tests/Functions/FunctionDeclarationArgumentSpacingUnitTest.inc b/src/Standards/Squiz/Tests/Functions/FunctionDeclarationArgumentSpacingUnitTest.inc index e26782f940..d1755b2f15 100644 --- a/src/Standards/Squiz/Tests/Functions/FunctionDeclarationArgumentSpacingUnitTest.inc +++ b/src/Standards/Squiz/Tests/Functions/FunctionDeclarationArgumentSpacingUnitTest.inc @@ -118,3 +118,15 @@ function spacingBetweenParenthesesShouldBeFixedInOneGo( ) {} + +function newlineAfterReferenceShouldBeFlaggedAndFixed( + & + + $param +) {} + +function newlineAfterReferenceFixerRespectsComment( + & + // comment + $param +) {} diff --git a/src/Standards/Squiz/Tests/Functions/FunctionDeclarationArgumentSpacingUnitTest.inc.fixed b/src/Standards/Squiz/Tests/Functions/FunctionDeclarationArgumentSpacingUnitTest.inc.fixed index da8bd13702..35355c13d6 100644 --- a/src/Standards/Squiz/Tests/Functions/FunctionDeclarationArgumentSpacingUnitTest.inc.fixed +++ b/src/Standards/Squiz/Tests/Functions/FunctionDeclarationArgumentSpacingUnitTest.inc.fixed @@ -113,3 +113,12 @@ fn ($a, $b=null) => $a($b); function multipleWhitespaceTokensAfterType(int $number) {} function spacingBetweenParenthesesShouldBeFixedInOneGo() {} + +function newlineAfterReferenceShouldBeFlaggedAndFixed( + &$param +) {} + +function newlineAfterReferenceFixerRespectsComment( + &// comment + $param +) {} diff --git a/src/Standards/Squiz/Tests/Functions/FunctionDeclarationArgumentSpacingUnitTest.php b/src/Standards/Squiz/Tests/Functions/FunctionDeclarationArgumentSpacingUnitTest.php index 60d67ddda5..f40c0e66b9 100644 --- a/src/Standards/Squiz/Tests/Functions/FunctionDeclarationArgumentSpacingUnitTest.php +++ b/src/Standards/Squiz/Tests/Functions/FunctionDeclarationArgumentSpacingUnitTest.php @@ -70,6 +70,8 @@ public function getErrorList() 111 => 3, 113 => 1, 117 => 1, + 123 => 1, + 129 => 1, ]; }//end getErrorList() From 30319c4966738a9f142c427864b6405208b93c61 Mon Sep 17 00:00:00 2001 From: jrfnl Date: Thu, 9 Jan 2025 10:55:56 +0100 Subject: [PATCH 03/14] Squiz/FunctionDeclarationArgumentSpacing: improve `SpacingAfterReference` fixer The fixer for the `SpacingAfterReference` error code would only handle one whitespace token at a time, which is inefficient and could also lead to more complex fixer conflicts. This commit changes the fixer to handle all whitespace tokens after a reference token in one go. For the test code sample added in the previous commit, this means the difference between the fixer needing 5 loops (including one skipped loop due to possible conflicts) versus 2 loops.
Old: ``` => Fixing file: 1/1 violations remaining Squiz.Functions.FunctionDeclarationArgumentSpacing:164 replaced token 9 (T_WHITESPACE on line 4) "\n\n" => "\n" => Fixing file: 1/1 violations remaining [made 1 pass]... * fixed 1 violations, starting loop 2 * Squiz.Functions.FunctionDeclarationArgumentSpacing:164 replaced token 9 (T_WHITESPACE on line 4) "\n " => " " => Fixing file: 1/1 violations remaining [made 2 passes]... * fixed 1 violations, starting loop 3 * **** Squiz.Functions.FunctionDeclarationArgumentSpacing:164 has possible conflict with another sniff on loop 1; caused by the following change **** **** replaced token 9 (T_WHITESPACE on line 4) " $param" => "$param" **** **** ignoring all changes until next loop **** => Fixing file: 0/1 violations remaining [made 3 passes]... * fixed 0 violations, starting loop 4 * Squiz.Functions.FunctionDeclarationArgumentSpacing:164 replaced token 9 (T_WHITESPACE on line 4) " $param" => "$param" => Fixing file: 1/1 violations remaining [made 4 passes]... * fixed 1 violations, starting loop 5 * => Fixing file: 0/1 violations remaining [made 5 passes]... ```
New: ``` => Fixing file: 1/1 violations remaining => Changeset started by Squiz.Functions.FunctionDeclarationArgumentSpacing:164 Q: Squiz.Functions.FunctionDeclarationArgumentSpacing:166 replaced token 9 (T_WHITESPACE on line 4) "\n \n" => " \n" Q: Squiz.Functions.FunctionDeclarationArgumentSpacing:166 replaced token 10 (T_WHITESPACE on line 5) " \n " => " " Q: Squiz.Functions.FunctionDeclarationArgumentSpacing:166 replaced token 11 (T_WHITESPACE on line 6) " $param" => "$param" A: Squiz.Functions.FunctionDeclarationArgumentSpacing:169 replaced token 9 (T_WHITESPACE on line 4) "\n \n" => " \n" A: Squiz.Functions.FunctionDeclarationArgumentSpacing:169 replaced token 10 (T_WHITESPACE on line 5) " \n " => " " A: Squiz.Functions.FunctionDeclarationArgumentSpacing:169 replaced token 11 (T_WHITESPACE on line 6) " $param" => "$param" => Changeset ended: 3 changes applied => Fixing file: 3/1 violations remaining [made 1 pass]... * fixed 3 violations, starting loop 2 * => Fixing file: 0/1 violations remaining [made 2 passes]... ```
--- .../FunctionDeclarationArgumentSpacingSniff.php | 9 +++++++-- 1 file changed, 7 insertions(+), 2 deletions(-) diff --git a/src/Standards/Squiz/Sniffs/Functions/FunctionDeclarationArgumentSpacingSniff.php b/src/Standards/Squiz/Sniffs/Functions/FunctionDeclarationArgumentSpacingSniff.php index 7598eb8950..6aae3c2349 100644 --- a/src/Standards/Squiz/Sniffs/Functions/FunctionDeclarationArgumentSpacingSniff.php +++ b/src/Standards/Squiz/Sniffs/Functions/FunctionDeclarationArgumentSpacingSniff.php @@ -162,9 +162,14 @@ public function processBracket($phpcsFile, $openBracket) ]; $fix = $phpcsFile->addFixableError($error, $refToken, 'SpacingAfterReference', $data); if ($fix === true) { - $phpcsFile->fixer->replaceToken(($refToken + 1), ''); + $phpcsFile->fixer->beginChangeset(); + for ($i = ($refToken + 1); $tokens[$i]['code'] === T_WHITESPACE; $i++) { + $phpcsFile->fixer->replaceToken($i, ''); + } + + $phpcsFile->fixer->endChangeset(); } - } + }//end if }//end if if ($param['variable_length'] === true) { From 17006012cdf72053edc9d794167b346664455318 Mon Sep 17 00:00:00 2001 From: jrfnl Date: Thu, 9 Jan 2025 11:02:03 +0100 Subject: [PATCH 04/14] Squiz/FunctionDeclarationArgumentSpacing: bug fix / `SpacingAfterVariadic` does not flag new lines A `T_WHITESPACE` token only containing new line characters will have its token `'length'` set to `0`. This means that if there was a new line after an ellipsis token, the sniff would not report it. Fixed now. Also note that, as the sniff demands no whitespace after a reference operator, we don't need the extra check for `$gap !== 0` as we already know that the next token is a whitespace token and needs to be flagged. Includes test. --- .../FunctionDeclarationArgumentSpacingSniff.php | 6 +++--- .../FunctionDeclarationArgumentSpacingUnitTest.inc | 12 ++++++++++++ ...ctionDeclarationArgumentSpacingUnitTest.inc.fixed | 9 +++++++++ .../FunctionDeclarationArgumentSpacingUnitTest.php | 2 ++ 4 files changed, 26 insertions(+), 3 deletions(-) diff --git a/src/Standards/Squiz/Sniffs/Functions/FunctionDeclarationArgumentSpacingSniff.php b/src/Standards/Squiz/Sniffs/Functions/FunctionDeclarationArgumentSpacingSniff.php index 6aae3c2349..f3e26bf12a 100644 --- a/src/Standards/Squiz/Sniffs/Functions/FunctionDeclarationArgumentSpacingSniff.php +++ b/src/Standards/Squiz/Sniffs/Functions/FunctionDeclarationArgumentSpacingSniff.php @@ -175,12 +175,12 @@ public function processBracket($phpcsFile, $openBracket) if ($param['variable_length'] === true) { $variadicToken = $param['variadic_token']; - $gap = 0; if ($tokens[($variadicToken + 1)]['code'] === T_WHITESPACE) { $gap = $tokens[($variadicToken + 1)]['length']; - } + if ($tokens[$variadicToken]['line'] !== $tokens[($variadicToken + 2)]['line']) { + $gap = 'newline'; + } - if ($gap !== 0) { $error = 'Expected 0 spaces after variadic operator for argument "%s"; %s found'; $data = [ $param['name'], diff --git a/src/Standards/Squiz/Tests/Functions/FunctionDeclarationArgumentSpacingUnitTest.inc b/src/Standards/Squiz/Tests/Functions/FunctionDeclarationArgumentSpacingUnitTest.inc index d1755b2f15..6e53c071e6 100644 --- a/src/Standards/Squiz/Tests/Functions/FunctionDeclarationArgumentSpacingUnitTest.inc +++ b/src/Standards/Squiz/Tests/Functions/FunctionDeclarationArgumentSpacingUnitTest.inc @@ -130,3 +130,15 @@ function newlineAfterReferenceFixerRespectsComment( // comment $param ) {} + +function newlineAfterVariadicShouldBeFlaggedAndFixed( + ... + + $param +) {} + +function newlineAfterVariadicFixerRespectsComment( + ... + //comment + $param +) {} diff --git a/src/Standards/Squiz/Tests/Functions/FunctionDeclarationArgumentSpacingUnitTest.inc.fixed b/src/Standards/Squiz/Tests/Functions/FunctionDeclarationArgumentSpacingUnitTest.inc.fixed index 35355c13d6..d54645db14 100644 --- a/src/Standards/Squiz/Tests/Functions/FunctionDeclarationArgumentSpacingUnitTest.inc.fixed +++ b/src/Standards/Squiz/Tests/Functions/FunctionDeclarationArgumentSpacingUnitTest.inc.fixed @@ -122,3 +122,12 @@ function newlineAfterReferenceFixerRespectsComment( &// comment $param ) {} + +function newlineAfterVariadicShouldBeFlaggedAndFixed( + ...$param +) {} + +function newlineAfterVariadicFixerRespectsComment( + ...//comment + $param +) {} diff --git a/src/Standards/Squiz/Tests/Functions/FunctionDeclarationArgumentSpacingUnitTest.php b/src/Standards/Squiz/Tests/Functions/FunctionDeclarationArgumentSpacingUnitTest.php index f40c0e66b9..2e44b548f3 100644 --- a/src/Standards/Squiz/Tests/Functions/FunctionDeclarationArgumentSpacingUnitTest.php +++ b/src/Standards/Squiz/Tests/Functions/FunctionDeclarationArgumentSpacingUnitTest.php @@ -72,6 +72,8 @@ public function getErrorList() 117 => 1, 123 => 1, 129 => 1, + 135 => 1, + 141 => 1, ]; }//end getErrorList() From 432e5b9b4a928ee94f5b4eda48effce2a5430c33 Mon Sep 17 00:00:00 2001 From: jrfnl Date: Thu, 9 Jan 2025 11:03:16 +0100 Subject: [PATCH 05/14] Squiz/FunctionDeclarationArgumentSpacing: improve `SpacingAfterVariadic` fixer The fixer for the `SpacingAfterVariadic` error code would only handle one whitespace token at a time, which is inefficient and could also lead to more complex fixer conflicts. This commit changes the fixer to handle all whitespace tokens after a variadic token in one go. For the test code sample added in the previous commit, this means the difference between the fixer needing 5 loops (including one skipped loop due to possible conflicts) versus 2 loops.
Old: ``` => Fixing file: 1/1 violations remaining Squiz.Functions.FunctionDeclarationArgumentSpacing:190 replaced token 9 (T_WHITESPACE on line 4) "\n\n" => "\n" => Fixing file: 1/1 violations remaining [made 1 pass]... * fixed 1 violations, starting loop 2 * Squiz.Functions.FunctionDeclarationArgumentSpacing:190 replaced token 9 (T_WHITESPACE on line 4) "\n " => " " => Fixing file: 1/1 violations remaining [made 2 passes]... * fixed 1 violations, starting loop 3 * **** Squiz.Functions.FunctionDeclarationArgumentSpacing:190 has possible conflict with another sniff on loop 1; caused by the following change **** **** replaced token 9 (T_WHITESPACE on line 4) " $param" => "$param" **** **** ignoring all changes until next loop **** => Fixing file: 0/1 violations remaining [made 3 passes]... * fixed 0 violations, starting loop 4 * Squiz.Functions.FunctionDeclarationArgumentSpacing:190 replaced token 9 (T_WHITESPACE on line 4) " $param" => "$param" => Fixing file: 1/1 violations remaining [made 4 passes]... * fixed 1 violations, starting loop 5 * => Fixing file: 0/1 violations remaining [made 5 passes]... ```
New: ``` => Fixing file: 1/1 violations remaining => Changeset started by Squiz.Functions.FunctionDeclarationArgumentSpacing:190 Q: Squiz.Functions.FunctionDeclarationArgumentSpacing:192 replaced token 9 (T_WHITESPACE on line 4) "\n\n" => "\n" Q: Squiz.Functions.FunctionDeclarationArgumentSpacing:192 replaced token 10 (T_WHITESPACE on line 5) "\n " => " " Q: Squiz.Functions.FunctionDeclarationArgumentSpacing:192 replaced token 11 (T_WHITESPACE on line 6) " $param" => "$param" A: Squiz.Functions.FunctionDeclarationArgumentSpacing:195 replaced token 9 (T_WHITESPACE on line 4) "\n\n" => "\n" A: Squiz.Functions.FunctionDeclarationArgumentSpacing:195 replaced token 10 (T_WHITESPACE on line 5) "\n " => " " A: Squiz.Functions.FunctionDeclarationArgumentSpacing:195 replaced token 11 (T_WHITESPACE on line 6) " $param" => "$param" => Changeset ended: 3 changes applied => Fixing file: 3/1 violations remaining [made 1 pass]... * fixed 3 violations, starting loop 2 * => Fixing file: 0/1 violations remaining [made 2 passes]... ```
--- .../FunctionDeclarationArgumentSpacingSniff.php | 9 +++++++-- 1 file changed, 7 insertions(+), 2 deletions(-) diff --git a/src/Standards/Squiz/Sniffs/Functions/FunctionDeclarationArgumentSpacingSniff.php b/src/Standards/Squiz/Sniffs/Functions/FunctionDeclarationArgumentSpacingSniff.php index f3e26bf12a..5609fab368 100644 --- a/src/Standards/Squiz/Sniffs/Functions/FunctionDeclarationArgumentSpacingSniff.php +++ b/src/Standards/Squiz/Sniffs/Functions/FunctionDeclarationArgumentSpacingSniff.php @@ -188,9 +188,14 @@ public function processBracket($phpcsFile, $openBracket) ]; $fix = $phpcsFile->addFixableError($error, $variadicToken, 'SpacingAfterVariadic', $data); if ($fix === true) { - $phpcsFile->fixer->replaceToken(($variadicToken + 1), ''); + $phpcsFile->fixer->beginChangeset(); + for ($i = ($variadicToken + 1); $tokens[$i]['code'] === T_WHITESPACE; $i++) { + $phpcsFile->fixer->replaceToken($i, ''); + } + + $phpcsFile->fixer->endChangeset(); } - } + }//end if }//end if if (isset($param['default_equal_token']) === true) { From 0e271eba6f878f90c2825b325569b38109fb7929 Mon Sep 17 00:00:00 2001 From: jrfnl Date: Thu, 9 Jan 2025 11:21:30 +0100 Subject: [PATCH 06/14] Squiz/FunctionDeclarationArgumentSpacing: bug fix / `SpaceBeforeEquals` + `SpaceAfterEquals` do not flag new lines for spacing 0 A `T_WHITESPACE` token only containing new line characters will have its token `'length'` set to `0`. This means that if there was a new line before/after the equal sign and the `$equalsSpacing` property was set to the default value of `0`, the sniff would not report it. When the `$equalsSpacing` property was set to `1`, the sniff already flagged this correctly (though there are other problems with that in the fixer - see later commit). Fixed now. Includes tests. --- .../Functions/FunctionDeclarationArgumentSpacingSniff.php | 8 ++++++-- .../FunctionDeclarationArgumentSpacingUnitTest.inc | 8 ++++++++ .../FunctionDeclarationArgumentSpacingUnitTest.inc.fixed | 4 ++++ .../FunctionDeclarationArgumentSpacingUnitTest.php | 1 + 4 files changed, 19 insertions(+), 2 deletions(-) diff --git a/src/Standards/Squiz/Sniffs/Functions/FunctionDeclarationArgumentSpacingSniff.php b/src/Standards/Squiz/Sniffs/Functions/FunctionDeclarationArgumentSpacingSniff.php index 5609fab368..31994cd813 100644 --- a/src/Standards/Squiz/Sniffs/Functions/FunctionDeclarationArgumentSpacingSniff.php +++ b/src/Standards/Squiz/Sniffs/Functions/FunctionDeclarationArgumentSpacingSniff.php @@ -202,7 +202,9 @@ public function processBracket($phpcsFile, $openBracket) $equalToken = $param['default_equal_token']; $spacesBefore = 0; - if (($equalToken - $param['token']) > 1) { + if ($tokens[$param['token']]['line'] !== $tokens[$equalToken]['line']) { + $spacesBefore = 'newline'; + } else if ($tokens[($param['token'] + 1)]['code'] === T_WHITESPACE) { $spacesBefore = $tokens[($param['token'] + 1)]['length']; } @@ -225,7 +227,9 @@ public function processBracket($phpcsFile, $openBracket) }//end if $spacesAfter = 0; - if ($tokens[($equalToken + 1)]['code'] === T_WHITESPACE) { + if ($tokens[$equalToken]['line'] !== $tokens[$param['default_token']]['line']) { + $spacesAfter = 'newline'; + } else if ($tokens[($equalToken + 1)]['code'] === T_WHITESPACE) { $spacesAfter = $tokens[($equalToken + 1)]['length']; } diff --git a/src/Standards/Squiz/Tests/Functions/FunctionDeclarationArgumentSpacingUnitTest.inc b/src/Standards/Squiz/Tests/Functions/FunctionDeclarationArgumentSpacingUnitTest.inc index 6e53c071e6..06c8927046 100644 --- a/src/Standards/Squiz/Tests/Functions/FunctionDeclarationArgumentSpacingUnitTest.inc +++ b/src/Standards/Squiz/Tests/Functions/FunctionDeclarationArgumentSpacingUnitTest.inc @@ -142,3 +142,11 @@ function newlineAfterVariadicFixerRespectsComment( //comment $param ) {} + +function newlineBeforeAndAfterEqualsSignShouldBeFixedForSpacing0( + $param + + = + + true +) {} diff --git a/src/Standards/Squiz/Tests/Functions/FunctionDeclarationArgumentSpacingUnitTest.inc.fixed b/src/Standards/Squiz/Tests/Functions/FunctionDeclarationArgumentSpacingUnitTest.inc.fixed index d54645db14..2136bd7bcf 100644 --- a/src/Standards/Squiz/Tests/Functions/FunctionDeclarationArgumentSpacingUnitTest.inc.fixed +++ b/src/Standards/Squiz/Tests/Functions/FunctionDeclarationArgumentSpacingUnitTest.inc.fixed @@ -131,3 +131,7 @@ function newlineAfterVariadicFixerRespectsComment( ...//comment $param ) {} + +function newlineBeforeAndAfterEqualsSignShouldBeFixedForSpacing0( + $param=true +) {} diff --git a/src/Standards/Squiz/Tests/Functions/FunctionDeclarationArgumentSpacingUnitTest.php b/src/Standards/Squiz/Tests/Functions/FunctionDeclarationArgumentSpacingUnitTest.php index 2e44b548f3..89e3fd8c06 100644 --- a/src/Standards/Squiz/Tests/Functions/FunctionDeclarationArgumentSpacingUnitTest.php +++ b/src/Standards/Squiz/Tests/Functions/FunctionDeclarationArgumentSpacingUnitTest.php @@ -74,6 +74,7 @@ public function getErrorList() 129 => 1, 135 => 1, 141 => 1, + 149 => 2, ]; }//end getErrorList() From b2a78ddb5167ee57957f34a53777a66197666adf Mon Sep 17 00:00:00 2001 From: jrfnl Date: Thu, 9 Jan 2025 12:29:44 +0100 Subject: [PATCH 07/14] Squiz/FunctionDeclarationArgumentSpacing: bug fix / `SpaceBeforeEquals` + `SpaceAfterEquals` vs comments When there is a comment between the parameter variable and the equal sign or the equal sign and the default value, the fixer would previously unintentionally and incorrectly remove most of these comments. For the tests added in this commit, the fixer would previously result in the following - note that three out of the four comments have been removed.. ```php function newlineBeforeAndAfterEqualsSignFixedRespectsComments( $param=true ) {} function commentBeforeAndAfterEqualsSignFixedRespectsComments( $param=/*comment*/ true ) {} ``` In this commit, I'm changing the sniff to still allow for flagging the spacing issue when there are comments, but to no longer auto-fix these to prevent the sniff removing the comments. Includes tests. --- ...unctionDeclarationArgumentSpacingSniff.php | 38 ++++++++++++------- ...tionDeclarationArgumentSpacingUnitTest.inc | 16 ++++++++ ...clarationArgumentSpacingUnitTest.inc.fixed | 16 ++++++++ ...tionDeclarationArgumentSpacingUnitTest.php | 2 + 4 files changed, 58 insertions(+), 14 deletions(-) diff --git a/src/Standards/Squiz/Sniffs/Functions/FunctionDeclarationArgumentSpacingSniff.php b/src/Standards/Squiz/Sniffs/Functions/FunctionDeclarationArgumentSpacingSniff.php index 31994cd813..c99bba0956 100644 --- a/src/Standards/Squiz/Sniffs/Functions/FunctionDeclarationArgumentSpacingSniff.php +++ b/src/Standards/Squiz/Sniffs/Functions/FunctionDeclarationArgumentSpacingSniff.php @@ -215,13 +215,18 @@ public function processBracket($phpcsFile, $openBracket) $spacesBefore, ]; - $fix = $phpcsFile->addFixableError($error, $equalToken, 'SpaceBeforeEquals', $data); - if ($fix === true) { - $padding = str_repeat(' ', $this->equalsSpacing); - if ($spacesBefore === 0) { - $phpcsFile->fixer->addContentBefore($equalToken, $padding); - } else { - $phpcsFile->fixer->replaceToken(($equalToken - 1), $padding); + $nextNonWhitespace = $phpcsFile->findNext(T_WHITESPACE, ($param['token'] + 1), $equalToken, true); + if ($nextNonWhitespace !== false) { + $phpcsFile->addError($error, $equalToken, 'SpaceBeforeEquals', $data); + } else { + $fix = $phpcsFile->addFixableError($error, $equalToken, 'SpaceBeforeEquals', $data); + if ($fix === true) { + $padding = str_repeat(' ', $this->equalsSpacing); + if ($spacesBefore === 0) { + $phpcsFile->fixer->addContentBefore($equalToken, $padding); + } else { + $phpcsFile->fixer->replaceToken(($equalToken - 1), $padding); + } } } }//end if @@ -240,13 +245,18 @@ public function processBracket($phpcsFile, $openBracket) $spacesAfter, ]; - $fix = $phpcsFile->addFixableError($error, $equalToken, 'SpaceAfterEquals', $data); - if ($fix === true) { - $padding = str_repeat(' ', $this->equalsSpacing); - if ($spacesAfter === 0) { - $phpcsFile->fixer->addContent($equalToken, $padding); - } else { - $phpcsFile->fixer->replaceToken(($equalToken + 1), $padding); + $nextNonWhitespace = $phpcsFile->findNext(T_WHITESPACE, ($equalToken + 1), $param['default_token'], true); + if ($nextNonWhitespace !== false) { + $phpcsFile->addError($error, $equalToken, 'SpaceAfterEquals', $data); + } else { + $fix = $phpcsFile->addFixableError($error, $equalToken, 'SpaceAfterEquals', $data); + if ($fix === true) { + $padding = str_repeat(' ', $this->equalsSpacing); + if ($spacesAfter === 0) { + $phpcsFile->fixer->addContent($equalToken, $padding); + } else { + $phpcsFile->fixer->replaceToken(($equalToken + 1), $padding); + } } } }//end if diff --git a/src/Standards/Squiz/Tests/Functions/FunctionDeclarationArgumentSpacingUnitTest.inc b/src/Standards/Squiz/Tests/Functions/FunctionDeclarationArgumentSpacingUnitTest.inc index 06c8927046..8cae7f4004 100644 --- a/src/Standards/Squiz/Tests/Functions/FunctionDeclarationArgumentSpacingUnitTest.inc +++ b/src/Standards/Squiz/Tests/Functions/FunctionDeclarationArgumentSpacingUnitTest.inc @@ -150,3 +150,19 @@ function newlineBeforeAndAfterEqualsSignShouldBeFixedForSpacing0( true ) {} + +function commentBeforeOrAfterEqualsSignShouldBeFlaggedNotFixed( + $param /*comment*/ = /*comment*/ true +) {} + +function newlineAndCommentBeforeAndAfterEqualsSignShouldBeFlaggedNotFixed( + $param + + //comment + + = + + //comment + + true +) {} diff --git a/src/Standards/Squiz/Tests/Functions/FunctionDeclarationArgumentSpacingUnitTest.inc.fixed b/src/Standards/Squiz/Tests/Functions/FunctionDeclarationArgumentSpacingUnitTest.inc.fixed index 2136bd7bcf..d9fe39c869 100644 --- a/src/Standards/Squiz/Tests/Functions/FunctionDeclarationArgumentSpacingUnitTest.inc.fixed +++ b/src/Standards/Squiz/Tests/Functions/FunctionDeclarationArgumentSpacingUnitTest.inc.fixed @@ -135,3 +135,19 @@ function newlineAfterVariadicFixerRespectsComment( function newlineBeforeAndAfterEqualsSignShouldBeFixedForSpacing0( $param=true ) {} + +function commentBeforeOrAfterEqualsSignShouldBeFlaggedNotFixed( + $param /*comment*/ = /*comment*/ true +) {} + +function newlineAndCommentBeforeAndAfterEqualsSignShouldBeFlaggedNotFixed( + $param + + //comment + + = + + //comment + + true +) {} diff --git a/src/Standards/Squiz/Tests/Functions/FunctionDeclarationArgumentSpacingUnitTest.php b/src/Standards/Squiz/Tests/Functions/FunctionDeclarationArgumentSpacingUnitTest.php index 89e3fd8c06..c5206d7563 100644 --- a/src/Standards/Squiz/Tests/Functions/FunctionDeclarationArgumentSpacingUnitTest.php +++ b/src/Standards/Squiz/Tests/Functions/FunctionDeclarationArgumentSpacingUnitTest.php @@ -75,6 +75,8 @@ public function getErrorList() 135 => 1, 141 => 1, 149 => 2, + 155 => 2, + 163 => 2, ]; }//end getErrorList() From 40a0f6b6f86b1ed14fb80fe424ec056d9ba7a4f5 Mon Sep 17 00:00:00 2001 From: jrfnl Date: Thu, 9 Jan 2025 12:46:18 +0100 Subject: [PATCH 08/14] Squiz/FunctionDeclarationArgumentSpacing: fix fixer conflict / improve `SpaceBeforeEquals` + `SpaceAfterEquals` fixers The fixers for the `SpaceBeforeEquals` and `SpaceAfterEquals` error codes would only handle one whitespace token at a time, which is inefficient. While this worked correctly when `$equalsSpacing` was set to `0`, as covered by the `newlineBeforeAndAfterEqualsSignShouldBeFixedForSpacing0()` test, it would take four loops instead of two to make the fixes. However, when `$equalsSpacing` was set to `1` AND there were new lines to deal with, the fixer would get into a fixer conflict with itself as it would keep trying to add a space before the equal sign. ``` Squiz.Functions.FunctionDeclarationArgumentSpacing:227 replaced token 31 (T_WHITESPACE on line 13) " =" => " =" ``` This commit changes both fixers to handle all whitespace tokens in one go. This makes the fixers more efficient, but also fixes the fixer conflict which was identified. Tested via the pre-existing `newlineBeforeAndAfterEqualsSignShouldBeFixedForSpacing0()` test case, as well as the new `newlineBeforeAndAfterEqualsSignShouldBeFixedForSpacing1()` test case. --- ...unctionDeclarationArgumentSpacingSniff.php | 24 ++++++++++++------- ...tionDeclarationArgumentSpacingUnitTest.inc | 10 ++++++++ ...clarationArgumentSpacingUnitTest.inc.fixed | 6 +++++ ...tionDeclarationArgumentSpacingUnitTest.php | 1 + 4 files changed, 33 insertions(+), 8 deletions(-) diff --git a/src/Standards/Squiz/Sniffs/Functions/FunctionDeclarationArgumentSpacingSniff.php b/src/Standards/Squiz/Sniffs/Functions/FunctionDeclarationArgumentSpacingSniff.php index c99bba0956..6d999cfb60 100644 --- a/src/Standards/Squiz/Sniffs/Functions/FunctionDeclarationArgumentSpacingSniff.php +++ b/src/Standards/Squiz/Sniffs/Functions/FunctionDeclarationArgumentSpacingSniff.php @@ -222,11 +222,15 @@ public function processBracket($phpcsFile, $openBracket) $fix = $phpcsFile->addFixableError($error, $equalToken, 'SpaceBeforeEquals', $data); if ($fix === true) { $padding = str_repeat(' ', $this->equalsSpacing); - if ($spacesBefore === 0) { - $phpcsFile->fixer->addContentBefore($equalToken, $padding); - } else { - $phpcsFile->fixer->replaceToken(($equalToken - 1), $padding); + + $phpcsFile->fixer->beginChangeset(); + $phpcsFile->fixer->addContent($param['token'], $padding); + + for ($i = ($param['token'] + 1); $tokens[$i]['code'] === T_WHITESPACE; $i++) { + $phpcsFile->fixer->replaceToken($i, ''); } + + $phpcsFile->fixer->endChangeset(); } } }//end if @@ -252,11 +256,15 @@ public function processBracket($phpcsFile, $openBracket) $fix = $phpcsFile->addFixableError($error, $equalToken, 'SpaceAfterEquals', $data); if ($fix === true) { $padding = str_repeat(' ', $this->equalsSpacing); - if ($spacesAfter === 0) { - $phpcsFile->fixer->addContent($equalToken, $padding); - } else { - $phpcsFile->fixer->replaceToken(($equalToken + 1), $padding); + + $phpcsFile->fixer->beginChangeset(); + $phpcsFile->fixer->addContent($equalToken, $padding); + + for ($i = ($equalToken + 1); $tokens[$i]['code'] === T_WHITESPACE; $i++) { + $phpcsFile->fixer->replaceToken($i, ''); } + + $phpcsFile->fixer->endChangeset(); } } }//end if diff --git a/src/Standards/Squiz/Tests/Functions/FunctionDeclarationArgumentSpacingUnitTest.inc b/src/Standards/Squiz/Tests/Functions/FunctionDeclarationArgumentSpacingUnitTest.inc index 8cae7f4004..ee843d1873 100644 --- a/src/Standards/Squiz/Tests/Functions/FunctionDeclarationArgumentSpacingUnitTest.inc +++ b/src/Standards/Squiz/Tests/Functions/FunctionDeclarationArgumentSpacingUnitTest.inc @@ -166,3 +166,13 @@ function newlineAndCommentBeforeAndAfterEqualsSignShouldBeFlaggedNotFixed( true ) {} + +// phpcs:set Squiz.Functions.FunctionDeclarationArgumentSpacing equalsSpacing 1 +function newlineBeforeAndAfterEqualsSignShouldBeFixedForSpacing1( + $param + + = + + true +) {} +// phpcs:set Squiz.Functions.FunctionDeclarationArgumentSpacing equalsSpacing 0 diff --git a/src/Standards/Squiz/Tests/Functions/FunctionDeclarationArgumentSpacingUnitTest.inc.fixed b/src/Standards/Squiz/Tests/Functions/FunctionDeclarationArgumentSpacingUnitTest.inc.fixed index d9fe39c869..f03a98044a 100644 --- a/src/Standards/Squiz/Tests/Functions/FunctionDeclarationArgumentSpacingUnitTest.inc.fixed +++ b/src/Standards/Squiz/Tests/Functions/FunctionDeclarationArgumentSpacingUnitTest.inc.fixed @@ -151,3 +151,9 @@ function newlineAndCommentBeforeAndAfterEqualsSignShouldBeFlaggedNotFixed( true ) {} + +// phpcs:set Squiz.Functions.FunctionDeclarationArgumentSpacing equalsSpacing 1 +function newlineBeforeAndAfterEqualsSignShouldBeFixedForSpacing1( + $param = true +) {} +// phpcs:set Squiz.Functions.FunctionDeclarationArgumentSpacing equalsSpacing 0 diff --git a/src/Standards/Squiz/Tests/Functions/FunctionDeclarationArgumentSpacingUnitTest.php b/src/Standards/Squiz/Tests/Functions/FunctionDeclarationArgumentSpacingUnitTest.php index c5206d7563..e4af44769c 100644 --- a/src/Standards/Squiz/Tests/Functions/FunctionDeclarationArgumentSpacingUnitTest.php +++ b/src/Standards/Squiz/Tests/Functions/FunctionDeclarationArgumentSpacingUnitTest.php @@ -77,6 +77,7 @@ public function getErrorList() 149 => 2, 155 => 2, 163 => 2, + 174 => 2, ]; }//end getErrorList() From 93b7e2de609b76fa92c304da81bf6f0a25d7d487 Mon Sep 17 00:00:00 2001 From: jrfnl Date: Thu, 9 Jan 2025 17:29:36 +0100 Subject: [PATCH 09/14] Squiz/FunctionDeclarationArgumentSpacing: bug fix / unclear `SpaceBeforeComma` message When there would be a new line between the end of the previous parameter and the comma, the error message was not always descriptive enough. For example, for the test added in this commit, the messages would be: ``` 182 | ERROR | [x] Expected 0 spaces between argument "$paramA" and comma; 0 found | | (Squiz.Functions.FunctionDeclarationArgumentSpacing.SpaceBeforeComma) 185 | ERROR | [x] Expected 0 spaces between argument "$paramB" and comma; 4 found | | (Squiz.Functions.FunctionDeclarationArgumentSpacing.SpaceBeforeComma) ``` In particular, take not of the "Expected 0 spaces... 0 found". This commit improves the messaging to take new lines into account. Includes tests. --- .../FunctionDeclarationArgumentSpacingSniff.php | 13 +++++++++++-- .../FunctionDeclarationArgumentSpacingUnitTest.inc | 9 +++++++++ ...tionDeclarationArgumentSpacingUnitTest.inc.fixed | 6 ++++++ .../FunctionDeclarationArgumentSpacingUnitTest.php | 2 ++ 4 files changed, 28 insertions(+), 2 deletions(-) diff --git a/src/Standards/Squiz/Sniffs/Functions/FunctionDeclarationArgumentSpacingSniff.php b/src/Standards/Squiz/Sniffs/Functions/FunctionDeclarationArgumentSpacingSniff.php index 6d999cfb60..f1d149e196 100644 --- a/src/Standards/Squiz/Sniffs/Functions/FunctionDeclarationArgumentSpacingSniff.php +++ b/src/Standards/Squiz/Sniffs/Functions/FunctionDeclarationArgumentSpacingSniff.php @@ -312,11 +312,20 @@ public function processBracket($phpcsFile, $openBracket) } if ($commaToken !== false) { - if ($tokens[($commaToken - 1)]['code'] === T_WHITESPACE) { + $endOfPreviousParam = $phpcsFile->findPrevious(Tokens::$emptyTokens, ($commaToken - 1), null, true); + + $spaceBeforeComma = 0; + if ($tokens[$endOfPreviousParam]['line'] !== $tokens[$commaToken]['line']) { + $spaceBeforeComma = 'newline'; + } else if ($tokens[($commaToken - 1)]['code'] === T_WHITESPACE) { + $spaceBeforeComma = $tokens[($commaToken - 1)]['length']; + } + + if ($spaceBeforeComma !== 0) { $error = 'Expected 0 spaces between argument "%s" and comma; %s found'; $data = [ $params[($paramNumber - 1)]['name'], - $tokens[($commaToken - 1)]['length'], + $spaceBeforeComma, ]; $fix = $phpcsFile->addFixableError($error, $commaToken, 'SpaceBeforeComma', $data); diff --git a/src/Standards/Squiz/Tests/Functions/FunctionDeclarationArgumentSpacingUnitTest.inc b/src/Standards/Squiz/Tests/Functions/FunctionDeclarationArgumentSpacingUnitTest.inc index ee843d1873..8aeb4f2d5c 100644 --- a/src/Standards/Squiz/Tests/Functions/FunctionDeclarationArgumentSpacingUnitTest.inc +++ b/src/Standards/Squiz/Tests/Functions/FunctionDeclarationArgumentSpacingUnitTest.inc @@ -176,3 +176,12 @@ function newlineBeforeAndAfterEqualsSignShouldBeFixedForSpacing1( true ) {} // phpcs:set Squiz.Functions.FunctionDeclarationArgumentSpacing equalsSpacing 0 + +function newlineBeforeCommaShouldBeFixedInOneGo( + $paramA +, + $paramB + + , + $paramC +) {} diff --git a/src/Standards/Squiz/Tests/Functions/FunctionDeclarationArgumentSpacingUnitTest.inc.fixed b/src/Standards/Squiz/Tests/Functions/FunctionDeclarationArgumentSpacingUnitTest.inc.fixed index f03a98044a..0db5f328f0 100644 --- a/src/Standards/Squiz/Tests/Functions/FunctionDeclarationArgumentSpacingUnitTest.inc.fixed +++ b/src/Standards/Squiz/Tests/Functions/FunctionDeclarationArgumentSpacingUnitTest.inc.fixed @@ -157,3 +157,9 @@ function newlineBeforeAndAfterEqualsSignShouldBeFixedForSpacing1( $param = true ) {} // phpcs:set Squiz.Functions.FunctionDeclarationArgumentSpacing equalsSpacing 0 + +function newlineBeforeCommaShouldBeFixedInOneGo( + $paramA, + $paramB, + $paramC +) {} diff --git a/src/Standards/Squiz/Tests/Functions/FunctionDeclarationArgumentSpacingUnitTest.php b/src/Standards/Squiz/Tests/Functions/FunctionDeclarationArgumentSpacingUnitTest.php index e4af44769c..85deb94bf9 100644 --- a/src/Standards/Squiz/Tests/Functions/FunctionDeclarationArgumentSpacingUnitTest.php +++ b/src/Standards/Squiz/Tests/Functions/FunctionDeclarationArgumentSpacingUnitTest.php @@ -78,6 +78,8 @@ public function getErrorList() 155 => 2, 163 => 2, 174 => 2, + 182 => 1, + 185 => 1, ]; }//end getErrorList() From 0e0e0fbd79526ec121b9e12ab237670ec0d686b4 Mon Sep 17 00:00:00 2001 From: jrfnl Date: Thu, 9 Jan 2025 17:52:04 +0100 Subject: [PATCH 10/14] Squiz/FunctionDeclarationArgumentSpacing: improve `SpaceBeforeComma` fixer The fixer for the `SpaceBeforeComma` error code would only handle one whitespace token at a time, which is inefficient and could also lead to more complex fixer conflicts. This commit changes the fixer to handle all whitespace tokens in one go. For the test code sample added in the previous commit, this means the difference between the fixer needing 4 loops versus 2 loops. Additionally, this commit updates the fixer to cleanly handle moving the comma in the case of a trailing comment between the end of the previous parameter and the comma. Tested via the `newlineBeforeCommaShouldBeFixedInOneGo()` test case and the new `newlineBeforeCommaFixerRespectsComments()` test case. --- ...unctionDeclarationArgumentSpacingSniff.php | 32 +++++++++++++++++-- ...tionDeclarationArgumentSpacingUnitTest.inc | 10 ++++++ ...clarationArgumentSpacingUnitTest.inc.fixed | 8 +++++ ...tionDeclarationArgumentSpacingUnitTest.php | 4 +++ 4 files changed, 51 insertions(+), 3 deletions(-) diff --git a/src/Standards/Squiz/Sniffs/Functions/FunctionDeclarationArgumentSpacingSniff.php b/src/Standards/Squiz/Sniffs/Functions/FunctionDeclarationArgumentSpacingSniff.php index f1d149e196..6758cf9395 100644 --- a/src/Standards/Squiz/Sniffs/Functions/FunctionDeclarationArgumentSpacingSniff.php +++ b/src/Standards/Squiz/Sniffs/Functions/FunctionDeclarationArgumentSpacingSniff.php @@ -330,9 +330,35 @@ public function processBracket($phpcsFile, $openBracket) $fix = $phpcsFile->addFixableError($error, $commaToken, 'SpaceBeforeComma', $data); if ($fix === true) { - $phpcsFile->fixer->replaceToken(($commaToken - 1), ''); - } - } + $startOfCurrentParam = $phpcsFile->findNext(Tokens::$emptyTokens, ($commaToken + 1), null, true); + + $phpcsFile->fixer->beginChangeset(); + $phpcsFile->fixer->addContent($endOfPreviousParam, ','); + $phpcsFile->fixer->replaceToken($commaToken, ''); + + if ($tokens[$commaToken]['line'] === $tokens[$startOfCurrentParam]['line']) { + for ($i = ($commaToken + 1); $tokens[$i]['code'] === T_WHITESPACE; $i++) { + $phpcsFile->fixer->replaceToken($i, ''); + } + } else { + for ($i = ($commaToken - 1); + $tokens[$i]['code'] === T_WHITESPACE && $tokens[$endOfPreviousParam]['line'] !== $tokens[$i]['line']; + $i-- + ) { + $phpcsFile->fixer->replaceToken($i, ''); + } + + for ($i = ($commaToken + 1); + $tokens[$i]['code'] === T_WHITESPACE && $tokens[$commaToken]['line'] === $tokens[$i]['line']; + $i++ + ) { + $phpcsFile->fixer->replaceToken($i, ''); + } + } + + $phpcsFile->fixer->endChangeset(); + }//end if + }//end if // Don't check spacing after the comma if it is the last content on the line. $checkComma = true; diff --git a/src/Standards/Squiz/Tests/Functions/FunctionDeclarationArgumentSpacingUnitTest.inc b/src/Standards/Squiz/Tests/Functions/FunctionDeclarationArgumentSpacingUnitTest.inc index 8aeb4f2d5c..b52bd83a2d 100644 --- a/src/Standards/Squiz/Tests/Functions/FunctionDeclarationArgumentSpacingUnitTest.inc +++ b/src/Standards/Squiz/Tests/Functions/FunctionDeclarationArgumentSpacingUnitTest.inc @@ -185,3 +185,13 @@ function newlineBeforeCommaShouldBeFixedInOneGo( , $paramC ) {} + +function newlineBeforeCommaFixerRespectsComments( + $paramA // comment + , + $paramB=10 /* comment */ +, + $paramC=20 # comment + , $paramC=30 + , string $paramC='foo' +) {} diff --git a/src/Standards/Squiz/Tests/Functions/FunctionDeclarationArgumentSpacingUnitTest.inc.fixed b/src/Standards/Squiz/Tests/Functions/FunctionDeclarationArgumentSpacingUnitTest.inc.fixed index 0db5f328f0..98a981e9ba 100644 --- a/src/Standards/Squiz/Tests/Functions/FunctionDeclarationArgumentSpacingUnitTest.inc.fixed +++ b/src/Standards/Squiz/Tests/Functions/FunctionDeclarationArgumentSpacingUnitTest.inc.fixed @@ -163,3 +163,11 @@ function newlineBeforeCommaShouldBeFixedInOneGo( $paramB, $paramC ) {} + +function newlineBeforeCommaFixerRespectsComments( + $paramA, // comment + $paramB=10, /* comment */ + $paramC=20, # comment + $paramC=30, + string $paramC='foo' +) {} diff --git a/src/Standards/Squiz/Tests/Functions/FunctionDeclarationArgumentSpacingUnitTest.php b/src/Standards/Squiz/Tests/Functions/FunctionDeclarationArgumentSpacingUnitTest.php index 85deb94bf9..f0988627a7 100644 --- a/src/Standards/Squiz/Tests/Functions/FunctionDeclarationArgumentSpacingUnitTest.php +++ b/src/Standards/Squiz/Tests/Functions/FunctionDeclarationArgumentSpacingUnitTest.php @@ -80,6 +80,10 @@ public function getErrorList() 174 => 2, 182 => 1, 185 => 1, + 191 => 1, + 193 => 1, + 195 => 1, + 196 => 1, ]; }//end getErrorList() From 7463c78cf86842b9ba93108e2687d99acd9c9f49 Mon Sep 17 00:00:00 2001 From: jrfnl Date: Fri, 10 Jan 2025 08:57:06 +0100 Subject: [PATCH 11/14] Squiz/FunctionDeclarationArgumentSpacing: rename test case file ... to allow for adding additional test case files. --- ...nDeclarationArgumentSpacingUnitTest.1.inc} | 0 ...rationArgumentSpacingUnitTest.1.inc.fixed} | 0 ...tionDeclarationArgumentSpacingUnitTest.php | 120 ++++++++++-------- 3 files changed, 64 insertions(+), 56 deletions(-) rename src/Standards/Squiz/Tests/Functions/{FunctionDeclarationArgumentSpacingUnitTest.inc => FunctionDeclarationArgumentSpacingUnitTest.1.inc} (100%) rename src/Standards/Squiz/Tests/Functions/{FunctionDeclarationArgumentSpacingUnitTest.inc.fixed => FunctionDeclarationArgumentSpacingUnitTest.1.inc.fixed} (100%) diff --git a/src/Standards/Squiz/Tests/Functions/FunctionDeclarationArgumentSpacingUnitTest.inc b/src/Standards/Squiz/Tests/Functions/FunctionDeclarationArgumentSpacingUnitTest.1.inc similarity index 100% rename from src/Standards/Squiz/Tests/Functions/FunctionDeclarationArgumentSpacingUnitTest.inc rename to src/Standards/Squiz/Tests/Functions/FunctionDeclarationArgumentSpacingUnitTest.1.inc diff --git a/src/Standards/Squiz/Tests/Functions/FunctionDeclarationArgumentSpacingUnitTest.inc.fixed b/src/Standards/Squiz/Tests/Functions/FunctionDeclarationArgumentSpacingUnitTest.1.inc.fixed similarity index 100% rename from src/Standards/Squiz/Tests/Functions/FunctionDeclarationArgumentSpacingUnitTest.inc.fixed rename to src/Standards/Squiz/Tests/Functions/FunctionDeclarationArgumentSpacingUnitTest.1.inc.fixed diff --git a/src/Standards/Squiz/Tests/Functions/FunctionDeclarationArgumentSpacingUnitTest.php b/src/Standards/Squiz/Tests/Functions/FunctionDeclarationArgumentSpacingUnitTest.php index f0988627a7..ea783e0b35 100644 --- a/src/Standards/Squiz/Tests/Functions/FunctionDeclarationArgumentSpacingUnitTest.php +++ b/src/Standards/Squiz/Tests/Functions/FunctionDeclarationArgumentSpacingUnitTest.php @@ -26,65 +26,73 @@ final class FunctionDeclarationArgumentSpacingUnitTest extends AbstractSniffUnit * The key of the array should represent the line number and the value * should represent the number of errors that should occur on that line. * + * @param string $testFile The name of the file being tested. + * * @return array */ - public function getErrorList() + public function getErrorList($testFile='') { - return [ - 3 => 1, - 5 => 2, - 7 => 2, - 8 => 2, - 9 => 2, - 11 => 2, - 13 => 7, - 14 => 2, - 15 => 2, - 16 => 4, - 18 => 2, - 35 => 2, - 36 => 2, - 44 => 2, - 45 => 1, - 46 => 1, - 51 => 2, - 53 => 2, - 55 => 1, - 56 => 1, - 58 => 1, - 73 => 7, - 76 => 1, - 77 => 1, - 81 => 1, - 89 => 2, - 92 => 1, - 93 => 1, - 94 => 1, - 95 => 1, - 99 => 11, - 100 => 2, - 101 => 2, - 102 => 2, - 106 => 1, - 107 => 2, - 111 => 3, - 113 => 1, - 117 => 1, - 123 => 1, - 129 => 1, - 135 => 1, - 141 => 1, - 149 => 2, - 155 => 2, - 163 => 2, - 174 => 2, - 182 => 1, - 185 => 1, - 191 => 1, - 193 => 1, - 195 => 1, - 196 => 1, - ]; + switch ($testFile) { + case 'FunctionDeclarationArgumentSpacingUnitTest.1.inc': + return [ + 3 => 1, + 5 => 2, + 7 => 2, + 8 => 2, + 9 => 2, + 11 => 2, + 13 => 7, + 14 => 2, + 15 => 2, + 16 => 4, + 18 => 2, + 35 => 2, + 36 => 2, + 44 => 2, + 45 => 1, + 46 => 1, + 51 => 2, + 53 => 2, + 55 => 1, + 56 => 1, + 58 => 1, + 73 => 7, + 76 => 1, + 77 => 1, + 81 => 1, + 89 => 2, + 92 => 1, + 93 => 1, + 94 => 1, + 95 => 1, + 99 => 11, + 100 => 2, + 101 => 2, + 102 => 2, + 106 => 1, + 107 => 2, + 111 => 3, + 113 => 1, + 117 => 1, + 123 => 1, + 129 => 1, + 135 => 1, + 141 => 1, + 149 => 2, + 155 => 2, + 163 => 2, + 174 => 2, + 182 => 1, + 185 => 1, + 191 => 1, + 193 => 1, + 195 => 1, + 196 => 1, + ]; + + default: + return []; + }//end switch }//end getErrorList() From d62f08d52dd604cfce470c6d993975e2b2781d3f Mon Sep 17 00:00:00 2001 From: jrfnl Date: Fri, 10 Jan 2025 09:06:07 +0100 Subject: [PATCH 12/14] Squiz/FunctionDeclarationArgumentSpacing: add parse error/live coding tests --- .../FunctionDeclarationArgumentSpacingUnitTest.2.inc | 6 ++++++ .../FunctionDeclarationArgumentSpacingUnitTest.3.inc | 6 ++++++ .../FunctionDeclarationArgumentSpacingUnitTest.4.inc | 6 ++++++ .../FunctionDeclarationArgumentSpacingUnitTest.5.inc | 6 ++++++ .../FunctionDeclarationArgumentSpacingUnitTest.6.inc | 6 ++++++ 5 files changed, 30 insertions(+) create mode 100644 src/Standards/Squiz/Tests/Functions/FunctionDeclarationArgumentSpacingUnitTest.2.inc create mode 100644 src/Standards/Squiz/Tests/Functions/FunctionDeclarationArgumentSpacingUnitTest.3.inc create mode 100644 src/Standards/Squiz/Tests/Functions/FunctionDeclarationArgumentSpacingUnitTest.4.inc create mode 100644 src/Standards/Squiz/Tests/Functions/FunctionDeclarationArgumentSpacingUnitTest.5.inc create mode 100644 src/Standards/Squiz/Tests/Functions/FunctionDeclarationArgumentSpacingUnitTest.6.inc diff --git a/src/Standards/Squiz/Tests/Functions/FunctionDeclarationArgumentSpacingUnitTest.2.inc b/src/Standards/Squiz/Tests/Functions/FunctionDeclarationArgumentSpacingUnitTest.2.inc new file mode 100644 index 0000000000..1b572336b2 --- /dev/null +++ b/src/Standards/Squiz/Tests/Functions/FunctionDeclarationArgumentSpacingUnitTest.2.inc @@ -0,0 +1,6 @@ + Date: Fri, 10 Jan 2025 09:08:53 +0100 Subject: [PATCH 13/14] Squiz/FunctionDeclarationArgumentSpacing: add test for the `NoSpaceBeforeHint` error ... which was so far not covered by tests. --- .../Functions/FunctionDeclarationArgumentSpacingUnitTest.1.inc | 2 +- .../FunctionDeclarationArgumentSpacingUnitTest.1.inc.fixed | 2 +- .../Functions/FunctionDeclarationArgumentSpacingUnitTest.php | 1 + 3 files changed, 3 insertions(+), 2 deletions(-) diff --git a/src/Standards/Squiz/Tests/Functions/FunctionDeclarationArgumentSpacingUnitTest.1.inc b/src/Standards/Squiz/Tests/Functions/FunctionDeclarationArgumentSpacingUnitTest.1.inc index b52bd83a2d..764e47331d 100644 --- a/src/Standards/Squiz/Tests/Functions/FunctionDeclarationArgumentSpacingUnitTest.1.inc +++ b/src/Standards/Squiz/Tests/Functions/FunctionDeclarationArgumentSpacingUnitTest.1.inc @@ -100,7 +100,7 @@ function functionName( ?string $arg1 = 'foo' , ?int & $arg2 , $arg3 ) {} function functionName(string $arg1, ... $arg2) {} function functionName(string $arg1, int ... $arg2) {} function functionName(string $arg1, & ... $arg2) {} - +function functionName(string $arg1,int $arg2) {} $a = function ($var1, $var2=false) use ( $longVar1, & $longerVar1, diff --git a/src/Standards/Squiz/Tests/Functions/FunctionDeclarationArgumentSpacingUnitTest.1.inc.fixed b/src/Standards/Squiz/Tests/Functions/FunctionDeclarationArgumentSpacingUnitTest.1.inc.fixed index 98a981e9ba..1de259a75d 100644 --- a/src/Standards/Squiz/Tests/Functions/FunctionDeclarationArgumentSpacingUnitTest.1.inc.fixed +++ b/src/Standards/Squiz/Tests/Functions/FunctionDeclarationArgumentSpacingUnitTest.1.inc.fixed @@ -100,7 +100,7 @@ function functionName(?string $arg1='foo', ?int &$arg2, $arg3) {} function functionName(string $arg1, ...$arg2) {} function functionName(string $arg1, int ...$arg2) {} function functionName(string $arg1, &...$arg2) {} - +function functionName(string $arg1, int $arg2) {} $a = function ($var1, $var2=false) use ( $longVar1, &$longerVar1, diff --git a/src/Standards/Squiz/Tests/Functions/FunctionDeclarationArgumentSpacingUnitTest.php b/src/Standards/Squiz/Tests/Functions/FunctionDeclarationArgumentSpacingUnitTest.php index ea783e0b35..79854ca9b1 100644 --- a/src/Standards/Squiz/Tests/Functions/FunctionDeclarationArgumentSpacingUnitTest.php +++ b/src/Standards/Squiz/Tests/Functions/FunctionDeclarationArgumentSpacingUnitTest.php @@ -69,6 +69,7 @@ public function getErrorList($testFile='') 100 => 2, 101 => 2, 102 => 2, + 103 => 1, 106 => 1, 107 => 2, 111 => 3, From d8924e387cac8a543520c8e9dc858aa310615feb Mon Sep 17 00:00:00 2001 From: jrfnl Date: Fri, 10 Jan 2025 11:03:53 +0100 Subject: [PATCH 14/14] Squiz/FunctionDeclarationArgumentSpacing: minor simplification No need to recreate the type hint when it's already available in the return value of the `getMethodParameters()` method. --- .../Functions/FunctionDeclarationArgumentSpacingSniff.php | 5 +---- 1 file changed, 1 insertion(+), 4 deletions(-) diff --git a/src/Standards/Squiz/Sniffs/Functions/FunctionDeclarationArgumentSpacingSniff.php b/src/Standards/Squiz/Sniffs/Functions/FunctionDeclarationArgumentSpacingSniff.php index 6758cf9395..2621a43615 100644 --- a/src/Standards/Squiz/Sniffs/Functions/FunctionDeclarationArgumentSpacingSniff.php +++ b/src/Standards/Squiz/Sniffs/Functions/FunctionDeclarationArgumentSpacingSniff.php @@ -396,10 +396,7 @@ public function processBracket($phpcsFile, $openBracket) } }//end if } else { - $hint = $phpcsFile->getTokensAsString($param['type_hint_token'], (($param['type_hint_end_token'] - $param['type_hint_token']) + 1)); - if ($param['nullable_type'] === true) { - $hint = '?'.$hint; - } + $hint = $param['type_hint']; if ($tokens[($commaToken + 1)]['code'] !== T_WHITESPACE) { $error = 'Expected 1 space between comma and type hint "%s"; 0 found';