From fda2bf66183f75c6ce49b73be9a61b4172121667 Mon Sep 17 00:00:00 2001 From: jrfnl Date: Fri, 25 Apr 2025 05:01:45 +0200 Subject: [PATCH 1/2] Fixer::fixFile(): bug fix for incorrect return value Over the years, I'd noticed on numerous occasions that `phpcbf` could update the file under scan, even when the end-result of the run was a "FAILED TO FIX". In my opinion, if the fixer didn't manage to fix the file completely, the file should not be updated as the end-result may be worse than before. The trouble was that this didn't _always_ happen, but only some of the time, so this needed some debugging to figure out under what exact conditions the file write would happen (and when it wouldn't happen). To demonstrate the issue, run the following commands with the branch for this PR checked out and the change in the `Fixer` file reverted: ```bash phpcbf --suffix=.fixed ./tests/Core/Fixer/Fixtures/test.inc --standard=./tests/Core/Fixer/FixFileReturnValueNotEnoughLoopsTest.xml ``` Now check the `./tests/Core/Fixer/Fixtures/` directory and take note that there is no `test.inc.fixed` file present. Next, run: ```bash phpcbf --suffix=.fixed ./tests/Core/Fixer/Fixtures/test.inc --standard=./tests/Core/Fixer/FixFileReturnValueConflictTest.xml ``` Now check the `./tests/Core/Fixer/Fixtures/` directory again and see that the `test.inc.fixed` file has been created. If you run these commands with `-vv` you can see what is happening in the fixer, including seeing a _"=> Fixed file written to test.inc.fixed"_ debug notice at the end of the debug output for the second command, but not seeing it for the first command. _(you can now delete the `test.inc.fixed` file)_ Turns out that if the last loop of the Fixer didn't make any fixes, the `Fixer::fixFile()` method always returned `true` (= everything fixed), even when there were no fixes made _due to the fixer having discarded the fixes as it detected a fixer conflict..._. This, in turn, would then cause the `CBF` report, which triggers the fixer and checks the return value of the `fixFile()` method, to do a file write with the "fixed" content of the file. This PR fixes this snafu by also checking the conflict state when determining the return value for the method. Includes tests specifically for this issue. --- src/Fixer.php | 2 +- .../Fixer/FixFileReturnValueAllGoodTest.xml | 8 ++ .../Fixer/FixFileReturnValueConflictTest.xml | 8 ++ .../FixFileReturnValueNotEnoughLoopsTest.xml | 8 ++ tests/Core/Fixer/FixFileReturnValueTest.php | 89 +++++++++++++++ .../FixFileReturnValue/AllGoodSniff.php | 37 +++++++ .../FixFileReturnValue/ConflictSniff.php | 101 ++++++++++++++++++ .../NotEnoughLoopsSniff.php | 40 +++++++ .../Fixer/Fixtures/TestStandard/ruleset.xml | 4 + tests/Core/Fixer/Fixtures/test.inc | 2 + 10 files changed, 298 insertions(+), 1 deletion(-) create mode 100644 tests/Core/Fixer/FixFileReturnValueAllGoodTest.xml create mode 100644 tests/Core/Fixer/FixFileReturnValueConflictTest.xml create mode 100644 tests/Core/Fixer/FixFileReturnValueNotEnoughLoopsTest.xml create mode 100644 tests/Core/Fixer/FixFileReturnValueTest.php create mode 100644 tests/Core/Fixer/Fixtures/TestStandard/Sniffs/FixFileReturnValue/AllGoodSniff.php create mode 100644 tests/Core/Fixer/Fixtures/TestStandard/Sniffs/FixFileReturnValue/ConflictSniff.php create mode 100644 tests/Core/Fixer/Fixtures/TestStandard/Sniffs/FixFileReturnValue/NotEnoughLoopsSniff.php create mode 100644 tests/Core/Fixer/Fixtures/TestStandard/ruleset.xml create mode 100644 tests/Core/Fixer/Fixtures/test.inc diff --git a/src/Fixer.php b/src/Fixer.php index 093e4fce5d..3e1f482a72 100644 --- a/src/Fixer.php +++ b/src/Fixer.php @@ -201,7 +201,7 @@ public function fixFile() $this->enabled = false; - if ($this->numFixes > 0) { + if ($this->numFixes > 0 || $this->inConflict === true) { if (PHP_CODESNIFFER_VERBOSITY > 1) { if (ob_get_level() > 0) { ob_end_clean(); diff --git a/tests/Core/Fixer/FixFileReturnValueAllGoodTest.xml b/tests/Core/Fixer/FixFileReturnValueAllGoodTest.xml new file mode 100644 index 0000000000..6f4e4996eb --- /dev/null +++ b/tests/Core/Fixer/FixFileReturnValueAllGoodTest.xml @@ -0,0 +1,8 @@ + + + + + + + + diff --git a/tests/Core/Fixer/FixFileReturnValueConflictTest.xml b/tests/Core/Fixer/FixFileReturnValueConflictTest.xml new file mode 100644 index 0000000000..5e2ddb473d --- /dev/null +++ b/tests/Core/Fixer/FixFileReturnValueConflictTest.xml @@ -0,0 +1,8 @@ + + + + + + + + diff --git a/tests/Core/Fixer/FixFileReturnValueNotEnoughLoopsTest.xml b/tests/Core/Fixer/FixFileReturnValueNotEnoughLoopsTest.xml new file mode 100644 index 0000000000..25810c04e6 --- /dev/null +++ b/tests/Core/Fixer/FixFileReturnValueNotEnoughLoopsTest.xml @@ -0,0 +1,8 @@ + + + + + + + + diff --git a/tests/Core/Fixer/FixFileReturnValueTest.php b/tests/Core/Fixer/FixFileReturnValueTest.php new file mode 100644 index 0000000000..fdffed3dfc --- /dev/null +++ b/tests/Core/Fixer/FixFileReturnValueTest.php @@ -0,0 +1,89 @@ +process(); + $fixed = $phpcsFile->fixer->fixFile($phpcsFile); + + $this->assertTrue($fixed); + + }//end testReturnValueIsTrueWhenFileWasFixed() + + + /** + * Test that the return value of the fixFile() method is false when the file failed to make all fixes. + * + * @param string $standard The ruleset file to use for the test. + * + * @dataProvider dataReturnValueIsFalse + * + * @return void + */ + public function testReturnValueIsFalse($standard) + { + $config = new ConfigDouble(["--standard=$standard"]); + $ruleset = new Ruleset($config); + + $testCaseFile = __DIR__.'/Fixtures/test.inc'; + $phpcsFile = new LocalFile($testCaseFile, $ruleset, $config); + $phpcsFile->process(); + $fixed = $phpcsFile->fixer->fixFile($phpcsFile); + + $this->assertFalse($fixed); + + }//end testReturnValueIsFalse() + + + /** + * Data provider. + * + * @return array> + */ + public static function dataReturnValueIsFalse() + { + return [ + 'when there is a fixer conflict' => [ + 'standard' => __DIR__.'/FixFileReturnValueConflictTest.xml', + ], + 'when the fixer ran out of loops before all fixes could be applied' => [ + 'standard' => __DIR__.'/FixFileReturnValueNotEnoughLoopsTest.xml', + ], + ]; + + }//end dataReturnValueIsFalse() + + +}//end class diff --git a/tests/Core/Fixer/Fixtures/TestStandard/Sniffs/FixFileReturnValue/AllGoodSniff.php b/tests/Core/Fixer/Fixtures/TestStandard/Sniffs/FixFileReturnValue/AllGoodSniff.php new file mode 100644 index 0000000000..20dbe2fe22 --- /dev/null +++ b/tests/Core/Fixer/Fixtures/TestStandard/Sniffs/FixFileReturnValue/AllGoodSniff.php @@ -0,0 +1,37 @@ +getTokens(); + + if ($tokens[($stackPtr + 1)]['code'] !== T_WHITESPACE + || $tokens[($stackPtr + 1)]['length'] > 51 + ) { + return; + } + + $error = 'There should be 52 spaces after an ECHO keyword'; + $fix = $phpcsFile->addFixableError($error, ($stackPtr + 1), 'ShortSpace'); + if ($fix === true) { + $phpcsFile->fixer->replaceToken(($stackPtr + 1), str_repeat(' ', 52)); + } + } +} diff --git a/tests/Core/Fixer/Fixtures/TestStandard/Sniffs/FixFileReturnValue/ConflictSniff.php b/tests/Core/Fixer/Fixtures/TestStandard/Sniffs/FixFileReturnValue/ConflictSniff.php new file mode 100644 index 0000000000..ca7afb9159 --- /dev/null +++ b/tests/Core/Fixer/Fixtures/TestStandard/Sniffs/FixFileReturnValue/ConflictSniff.php @@ -0,0 +1,101 @@ +getTokens(); + + // Demand a blank line after the PHP open tag. + // This error is here to ensure something will be fixed in the file. + $nextNonWhitespace = $phpcsFile->findNext(T_WHITESPACE, ($stackPtr + 1), null, true); + + if (($tokens[$nextNonWhitespace]['line'] - $tokens[$stackPtr]['line']) !== 2) { + $error = 'There must be a single blank line after the PHP open tag'; + $fix = $phpcsFile->addFixableError($error, $stackPtr, 'BlankLineAfterOpen'); + if ($fix === true) { + $phpcsFile->fixer->addNewline($stackPtr); + + // This return is here deliberately to force a new loop. + // This should ensure that loop 50 does *NOT* apply any fixes. + return; + } + } + + // Skip to the end of the file. + $stackPtr = ($phpcsFile->numTokens - 1); + + $eolCharLen = strlen($phpcsFile->eolChar); + $lastChars = substr($tokens[$stackPtr]['content'], ($eolCharLen * -1)); + + // Demand a newline at the end of a file. + if ($lastChars !== $phpcsFile->eolChar) { + $error = 'File must end with a newline character'; + $fix = $phpcsFile->addFixableError($error, $stackPtr, 'NoNewlineEOF'); + if ($fix === true) { + $phpcsFile->fixer->addNewline($stackPtr); + } + } + + // Demand NO newline at the end of a file. + if ($lastChars === $phpcsFile->eolChar) { + $error = 'File must not end with a newline character'; + $fix = $phpcsFile->addFixableError($error, $stackPtr, 'NewlineEOF'); + if ($fix === true) { + $phpcsFile->fixer->beginChangeset(); + + for ($i = $stackPtr; $i > 0; $i--) { + $newContent = rtrim($tokens[$i]['content'], $phpcsFile->eolChar); + $phpcsFile->fixer->replaceToken($i, $newContent); + + if ($newContent !== '') { + break; + } + } + + $phpcsFile->fixer->endChangeset(); + } + } + + // Ignore the rest of the file. + return $phpcsFile->numTokens; + } +} diff --git a/tests/Core/Fixer/Fixtures/TestStandard/Sniffs/FixFileReturnValue/NotEnoughLoopsSniff.php b/tests/Core/Fixer/Fixtures/TestStandard/Sniffs/FixFileReturnValue/NotEnoughLoopsSniff.php new file mode 100644 index 0000000000..e42d3fd7ed --- /dev/null +++ b/tests/Core/Fixer/Fixtures/TestStandard/Sniffs/FixFileReturnValue/NotEnoughLoopsSniff.php @@ -0,0 +1,40 @@ +getTokens(); + + if ($tokens[($stackPtr + 1)]['code'] !== T_WHITESPACE + || $tokens[($stackPtr + 1)]['length'] > 60 + ) { + return; + } + + $error = 'There should be 60 spaces after an ECHO keyword'; + $fix = $phpcsFile->addFixableError($error, ($stackPtr + 1), 'ShortSpace'); + if ($fix === true) { + // The fixer deliberately only adds one space in each loop to ensure it runs out of loops before the file complies. + $phpcsFile->fixer->addContent($stackPtr, ' '); + } + } +} diff --git a/tests/Core/Fixer/Fixtures/TestStandard/ruleset.xml b/tests/Core/Fixer/Fixtures/TestStandard/ruleset.xml new file mode 100644 index 0000000000..345c4837eb --- /dev/null +++ b/tests/Core/Fixer/Fixtures/TestStandard/ruleset.xml @@ -0,0 +1,4 @@ + + + + diff --git a/tests/Core/Fixer/Fixtures/test.inc b/tests/Core/Fixer/Fixtures/test.inc new file mode 100644 index 0000000000..76195b04c4 --- /dev/null +++ b/tests/Core/Fixer/Fixtures/test.inc @@ -0,0 +1,2 @@ + Date: Sun, 27 Apr 2025 15:43:41 +0200 Subject: [PATCH 2/2] FixFileReturnValueTest: remove redundant parameter --- tests/Core/Fixer/FixFileReturnValueTest.php | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/tests/Core/Fixer/FixFileReturnValueTest.php b/tests/Core/Fixer/FixFileReturnValueTest.php index fdffed3dfc..76e56364c4 100644 --- a/tests/Core/Fixer/FixFileReturnValueTest.php +++ b/tests/Core/Fixer/FixFileReturnValueTest.php @@ -36,7 +36,7 @@ public function testReturnValueIsTrueWhenFileWasFixed() $testCaseFile = __DIR__.'/Fixtures/test.inc'; $phpcsFile = new LocalFile($testCaseFile, $ruleset, $config); $phpcsFile->process(); - $fixed = $phpcsFile->fixer->fixFile($phpcsFile); + $fixed = $phpcsFile->fixer->fixFile(); $this->assertTrue($fixed); @@ -60,7 +60,7 @@ public function testReturnValueIsFalse($standard) $testCaseFile = __DIR__.'/Fixtures/test.inc'; $phpcsFile = new LocalFile($testCaseFile, $ruleset, $config); $phpcsFile->process(); - $fixed = $phpcsFile->fixer->fixFile($phpcsFile); + $fixed = $phpcsFile->fixer->fixFile(); $this->assertFalse($fixed);