Skip to content

Commit e4c1f2f

Browse files
jrfnljrfnl
authored
Fixer::fixFile(): bug fix for incorrect return value (#1048)
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.
1 parent 23feb19 commit e4c1f2f

File tree

10 files changed

+298
-1
lines changed

10 files changed

+298
-1
lines changed

‎src/Fixer.php

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -201,7 +201,7 @@ public function fixFile()
201201

202202
$this->enabled = false;
203203

204-
if ($this->numFixes > 0) {
204+
if ($this->numFixes > 0 || $this->inConflict === true) {
205205
if (PHP_CODESNIFFER_VERBOSITY > 1) {
206206
if (ob_get_level() > 0) {
207207
ob_end_clean();

‎tests/Core/Fixer/FixFileReturnValueAllGoodTest.xml

Lines changed: 8 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,8 @@
1+
<?xml version="1.0"?>
2+
<ruleset xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance" name="FixFileReturnValueTest" xsi:noNamespaceSchemaLocation="https://raw.githubusercontent.com/PHPCSStandards/PHP_CodeSniffer/master/phpcs.xsd">
3+
4+
<config name="installed_paths" value="./tests/Core/Fixer/Fixtures/TestStandard/"/>
5+
6+
<rule ref="TestStandard.FixFileReturnValue.AllGood"/>
7+
8+
</ruleset>

‎tests/Core/Fixer/FixFileReturnValueConflictTest.xml

Lines changed: 8 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,8 @@
1+
<?xml version="1.0"?>
2+
<ruleset xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance" name="FixFileReturnValueTest" xsi:noNamespaceSchemaLocation="https://raw.githubusercontent.com/PHPCSStandards/PHP_CodeSniffer/master/phpcs.xsd">
3+
4+
<config name="installed_paths" value="./tests/Core/Fixer/Fixtures/TestStandard/"/>
5+
6+
<rule ref="TestStandard.FixFileReturnValue.Conflict"/>
7+
8+
</ruleset>

‎tests/Core/Fixer/FixFileReturnValueNotEnoughLoopsTest.xml

Lines changed: 8 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,8 @@
1+
<?xml version="1.0"?>
2+
<ruleset xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance" name="FixFileReturnValueTest" xsi:noNamespaceSchemaLocation="https://raw.githubusercontent.com/PHPCSStandards/PHP_CodeSniffer/master/phpcs.xsd">
3+
4+
<config name="installed_paths" value="./tests/Core/Fixer/Fixtures/TestStandard/"/>
5+
6+
<rule ref="TestStandard.FixFileReturnValue.NotEnoughLoops"/>
7+
8+
</ruleset>

‎tests/Core/Fixer/FixFileReturnValueTest.php

Lines changed: 89 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,89 @@
1+
<?php
2+
/**
3+
* Tests for the Fixer::fixFile() return value.
4+
*
5+
* @copyright 2025 PHPCSStandards and contributors
6+
* @license https://github.com/PHPCSStandards/PHP_CodeSniffer/blob/master/licence.txt BSD Licence
7+
*/
8+
9+
namespace PHP_CodeSniffer\Tests\Core\Fixer;
10+
11+
use PHP_CodeSniffer\Files\LocalFile;
12+
use PHP_CodeSniffer\Ruleset;
13+
use PHP_CodeSniffer\Tests\ConfigDouble;
14+
use PHPUnit\Framework\TestCase;
15+
16+
/**
17+
* Tests for the Fixer::fixFile() return value.
18+
*
19+
* @covers PHP_CodeSniffer\Fixer::fixFile
20+
*/
21+
final class FixFileReturnValueTest extends TestCase
22+
{
23+
24+
25+
/**
26+
* Test that the return value of the fixFile() method is true when the file was completely fixed.
27+
*
28+
* @return void
29+
*/
30+
public function testReturnValueIsTrueWhenFileWasFixed()
31+
{
32+
$standard = __DIR__.'/FixFileReturnValueAllGoodTest.xml';
33+
$config = new ConfigDouble(["--standard=$standard"]);
34+
$ruleset = new Ruleset($config);
35+
36+
$testCaseFile = __DIR__.'/Fixtures/test.inc';
37+
$phpcsFile = new LocalFile($testCaseFile, $ruleset, $config);
38+
$phpcsFile->process();
39+
$fixed = $phpcsFile->fixer->fixFile();
40+
41+
$this->assertTrue($fixed);
42+
43+
}//end testReturnValueIsTrueWhenFileWasFixed()
44+
45+
46+
/**
47+
* Test that the return value of the fixFile() method is false when the file failed to make all fixes.
48+
*
49+
* @param string $standard The ruleset file to use for the test.
50+
*
51+
* @dataProvider dataReturnValueIsFalse
52+
*
53+
* @return void
54+
*/
55+
public function testReturnValueIsFalse($standard)
56+
{
57+
$config = new ConfigDouble(["--standard=$standard"]);
58+
$ruleset = new Ruleset($config);
59+
60+
$testCaseFile = __DIR__.'/Fixtures/test.inc';
61+
$phpcsFile = new LocalFile($testCaseFile, $ruleset, $config);
62+
$phpcsFile->process();
63+
$fixed = $phpcsFile->fixer->fixFile();
64+
65+
$this->assertFalse($fixed);
66+
67+
}//end testReturnValueIsFalse()
68+
69+
70+
/**
71+
* Data provider.
72+
*
73+
* @return array<string, array<string, string>>
74+
*/
75+
public static function dataReturnValueIsFalse()
76+
{
77+
return [
78+
'when there is a fixer conflict' => [
79+
'standard' => __DIR__.'/FixFileReturnValueConflictTest.xml',
80+
],
81+
'when the fixer ran out of loops before all fixes could be applied' => [
82+
'standard' => __DIR__.'/FixFileReturnValueNotEnoughLoopsTest.xml',
83+
],
84+
];
85+
86+
}//end dataReturnValueIsFalse()
87+
88+
89+
}//end class

‎tests/Core/Fixer/Fixtures/TestStandard/Sniffs/FixFileReturnValue/AllGoodSniff.php

Lines changed: 37 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,37 @@
1+
<?php
2+
/**
3+
* Test fixture.
4+
*
5+
* @see \PHP_CodeSniffer\Tests\Core\Fixer\FixFileReturnValueTest
6+
*/
7+
8+
namespace Fixtures\TestStandard\Sniffs\FixFileReturnValue;
9+
10+
use PHP_CodeSniffer\Files\File;
11+
use PHP_CodeSniffer\Sniffs\Sniff;
12+
13+
class AllGoodSniff implements Sniff
14+
{
15+
16+
public function register()
17+
{
18+
return [T_ECHO];
19+
}
20+
21+
public function process(File $phpcsFile, $stackPtr)
22+
{
23+
$tokens = $phpcsFile->getTokens();
24+
25+
if ($tokens[($stackPtr + 1)]['code'] !== T_WHITESPACE
26+
|| $tokens[($stackPtr + 1)]['length'] > 51
27+
) {
28+
return;
29+
}
30+
31+
$error = 'There should be 52 spaces after an ECHO keyword';
32+
$fix = $phpcsFile->addFixableError($error, ($stackPtr + 1), 'ShortSpace');
33+
if ($fix === true) {
34+
$phpcsFile->fixer->replaceToken(($stackPtr + 1), str_repeat(' ', 52));
35+
}
36+
}
37+
}

‎tests/Core/Fixer/Fixtures/TestStandard/Sniffs/FixFileReturnValue/ConflictSniff.php

Lines changed: 101 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,101 @@
1+
<?php
2+
/**
3+
* Test fixture.
4+
*
5+
* This sniff deliberately causes a fixer conflict **with no fixes applied in loop 50**.
6+
* This last part is important as that's the exact situation which needs testing.
7+
*
8+
* Loop 1 applies the fix for `BlankLineAfterOpen` and then bows out.
9+
* Loop 2 applies a fix for `NewlineEOF`.
10+
* Loop 3 applies a fix for `NoNewlineEOF`.
11+
* Loop 4 will try to apply the `NewlineEOF` fix again, but sees this causes a conflict and skips.
12+
* Loop 5 will try to apply the `NewlineEOF` fix again, but sees this causes a conflict and skips.
13+
* Loop 6 applies a fix for `NewlineEOF`.
14+
* Loop 7 will try to apply the `NoNewlineEOF` fix again, but sees this causes a conflict and skips.
15+
* Loop 8 applies a fix for `NoNewlineEOF`.
16+
* Loop 9 - 13 repeat loop 4 - 8.
17+
* Loop 14 - 18 repeat loop 4 - 8.
18+
* Loop 19 - 23 repeat loop 4 - 8.
19+
* Loop 24 - 28 repeat loop 4 - 8.
20+
* Loop 29 - 33 repeat loop 4 - 8.
21+
* Loop 34 - 38 repeat loop 4 - 8.
22+
* Loop 39 - 43 repeat loop 4 - 8.
23+
* Loop 44 - 48 repeat loop 4 - 8.
24+
* Loop 49 = loop 4.
25+
* Loop 50 = loop 5, i.e. applies no fixes.
26+
*
27+
* @see \PHP_CodeSniffer\Tests\Core\Fixer\FixFileReturnValueTest
28+
*/
29+
30+
namespace Fixtures\TestStandard\Sniffs\FixFileReturnValue;
31+
32+
use PHP_CodeSniffer\Files\File;
33+
use PHP_CodeSniffer\Sniffs\Sniff;
34+
35+
class ConflictSniff implements Sniff
36+
{
37+
38+
public function register()
39+
{
40+
return [T_OPEN_TAG];
41+
}
42+
43+
public function process(File $phpcsFile, $stackPtr)
44+
{
45+
$tokens = $phpcsFile->getTokens();
46+
47+
// Demand a blank line after the PHP open tag.
48+
// This error is here to ensure something will be fixed in the file.
49+
$nextNonWhitespace = $phpcsFile->findNext(T_WHITESPACE, ($stackPtr + 1), null, true);
50+
51+
if (($tokens[$nextNonWhitespace]['line'] - $tokens[$stackPtr]['line']) !== 2) {
52+
$error = 'There must be a single blank line after the PHP open tag';
53+
$fix = $phpcsFile->addFixableError($error, $stackPtr, 'BlankLineAfterOpen');
54+
if ($fix === true) {
55+
$phpcsFile->fixer->addNewline($stackPtr);
56+
57+
// This return is here deliberately to force a new loop.
58+
// This should ensure that loop 50 does *NOT* apply any fixes.
59+
return;
60+
}
61+
}
62+
63+
// Skip to the end of the file.
64+
$stackPtr = ($phpcsFile->numTokens - 1);
65+
66+
$eolCharLen = strlen($phpcsFile->eolChar);
67+
$lastChars = substr($tokens[$stackPtr]['content'], ($eolCharLen * -1));
68+
69+
// Demand a newline at the end of a file.
70+
if ($lastChars !== $phpcsFile->eolChar) {
71+
$error = 'File must end with a newline character';
72+
$fix = $phpcsFile->addFixableError($error, $stackPtr, 'NoNewlineEOF');
73+
if ($fix === true) {
74+
$phpcsFile->fixer->addNewline($stackPtr);
75+
}
76+
}
77+
78+
// Demand NO newline at the end of a file.
79+
if ($lastChars === $phpcsFile->eolChar) {
80+
$error = 'File must not end with a newline character';
81+
$fix = $phpcsFile->addFixableError($error, $stackPtr, 'NewlineEOF');
82+
if ($fix === true) {
83+
$phpcsFile->fixer->beginChangeset();
84+
85+
for ($i = $stackPtr; $i > 0; $i--) {
86+
$newContent = rtrim($tokens[$i]['content'], $phpcsFile->eolChar);
87+
$phpcsFile->fixer->replaceToken($i, $newContent);
88+
89+
if ($newContent !== '') {
90+
break;
91+
}
92+
}
93+
94+
$phpcsFile->fixer->endChangeset();
95+
}
96+
}
97+
98+
// Ignore the rest of the file.
99+
return $phpcsFile->numTokens;
100+
}
101+
}

‎tests/Core/Fixer/Fixtures/TestStandard/Sniffs/FixFileReturnValue/NotEnoughLoopsSniff.php

Lines changed: 40 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,40 @@
1+
<?php
2+
/**
3+
* Test fixture.
4+
*
5+
* This sniff deliberately causes a "failed to fix" situation by causing the fixer to run out of loops.
6+
*
7+
* @see \PHP_CodeSniffer\Tests\Core\Fixer\FixFileReturnValueTest
8+
*/
9+
10+
namespace Fixtures\TestStandard\Sniffs\FixFileReturnValue;
11+
12+
use PHP_CodeSniffer\Files\File;
13+
use PHP_CodeSniffer\Sniffs\Sniff;
14+
15+
class NotEnoughLoopsSniff implements Sniff
16+
{
17+
18+
public function register()
19+
{
20+
return [T_ECHO];
21+
}
22+
23+
public function process(File $phpcsFile, $stackPtr)
24+
{
25+
$tokens = $phpcsFile->getTokens();
26+
27+
if ($tokens[($stackPtr + 1)]['code'] !== T_WHITESPACE
28+
|| $tokens[($stackPtr + 1)]['length'] > 60
29+
) {
30+
return;
31+
}
32+
33+
$error = 'There should be 60 spaces after an ECHO keyword';
34+
$fix = $phpcsFile->addFixableError($error, ($stackPtr + 1), 'ShortSpace');
35+
if ($fix === true) {
36+
// The fixer deliberately only adds one space in each loop to ensure it runs out of loops before the file complies.
37+
$phpcsFile->fixer->addContent($stackPtr, ' ');
38+
}
39+
}
40+
}

‎tests/Core/Fixer/Fixtures/TestStandard/ruleset.xml

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,4 @@
1+
<?xml version="1.0"?>
2+
<ruleset xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance" name="TestStandard" namespace="Fixtures\TestStandard" xsi:noNamespaceSchemaLocation="https://raw.githubusercontent.com/PHPCSStandards/PHP_CodeSniffer/master/phpcs.xsd">
3+
4+
</ruleset>

‎tests/Core/Fixer/Fixtures/test.inc

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,2 @@
1+
<?php
2+
echo 'this is a simple file which will have a fixer conflict when the TestStandard.FixFileReturnValue.Conflict sniff is run against it';

0 commit comments

Comments
 (0)