Fixer::fixFile(): bug fix for incorrect return value

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();

tests/Core/Fixer/FixFileReturnValueAllGoodTest.xml

@@ -0,0 +1,8 @@
+<?xml version="1.0"?>
+<ruleset xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance" name="FixFileReturnValueTest" xsi:noNamespaceSchemaLocation="https://raw.githubusercontent.com/PHPCSStandards/PHP_CodeSniffer/master/phpcs.xsd">
+
+    <config name="installed_paths" value="./tests/Core/Fixer/Fixtures/TestStandard/"/>
+
+    <rule ref="TestStandard.FixFileReturnValue.AllGood"/>
+
+</ruleset>

tests/Core/Fixer/FixFileReturnValueConflictTest.xml

@@ -0,0 +1,8 @@
+<?xml version="1.0"?>
+<ruleset xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance" name="FixFileReturnValueTest" xsi:noNamespaceSchemaLocation="https://raw.githubusercontent.com/PHPCSStandards/PHP_CodeSniffer/master/phpcs.xsd">
+
+    <config name="installed_paths" value="./tests/Core/Fixer/Fixtures/TestStandard/"/>
+
+    <rule ref="TestStandard.FixFileReturnValue.Conflict"/>
+
+</ruleset>

tests/Core/Fixer/FixFileReturnValueNotEnoughLoopsTest.xml

@@ -0,0 +1,8 @@
+<?xml version="1.0"?>
+<ruleset xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance" name="FixFileReturnValueTest" xsi:noNamespaceSchemaLocation="https://raw.githubusercontent.com/PHPCSStandards/PHP_CodeSniffer/master/phpcs.xsd">
+
+    <config name="installed_paths" value="./tests/Core/Fixer/Fixtures/TestStandard/"/>
+
+    <rule ref="TestStandard.FixFileReturnValue.NotEnoughLoops"/>
+
+</ruleset>

tests/Core/Fixer/FixFileReturnValueTest.php

@@ -0,0 +1,89 @@
+<?php
+/**
+ * Tests for the Fixer::fixFile() return value.
+ *
+ * @copyright 2025 PHPCSStandards and contributors
+ * @license   https://github.com/PHPCSStandards/PHP_CodeSniffer/blob/master/licence.txt BSD Licence
+ */
+
+namespace PHP_CodeSniffer\Tests\Core\Fixer;
+
+use PHP_CodeSniffer\Files\LocalFile;
+use PHP_CodeSniffer\Ruleset;
+use PHP_CodeSniffer\Tests\ConfigDouble;
+use PHPUnit\Framework\TestCase;
+
+/**
+ * Tests for the Fixer::fixFile() return value.
+ *
+ * @covers PHP_CodeSniffer\Fixer::fixFile
+ */
+final class FixFileReturnValueTest extends TestCase
+{
+
+
+    /**
+     * Test that the return value of the fixFile() method is true when the file was completely fixed.
+     *
+     * @return void
+     */
+    public function testReturnValueIsTrueWhenFileWasFixed()
+    {
+        $standard = __DIR__.'/FixFileReturnValueAllGoodTest.xml';
+        $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->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<string, array<string, string>>
+     */
+    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

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

@@ -0,0 +1,37 @@
+<?php
+/**
+ * Test fixture.
+ *
+ * @see \PHP_CodeSniffer\Tests\Core\Fixer\FixFileReturnValueTest
+ */
+
+namespace Fixtures\TestStandard\Sniffs\FixFileReturnValue;
+
+use PHP_CodeSniffer\Files\File;
+use PHP_CodeSniffer\Sniffs\Sniff;
+
+class AllGoodSniff implements Sniff
+{
+
+    public function register()
+    {
+        return [T_ECHO];
+    }
+
+    public function process(File $phpcsFile, $stackPtr)
+    {
+        $tokens = $phpcsFile->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));
+        }
+    }
+}

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

@@ -0,0 +1,101 @@
+<?php
+/**
+ * Test fixture.
+ *
+ * This sniff deliberately causes a fixer conflict **with no fixes applied in loop 50**.
+ * This last part is important as that's the exact situation which needs testing.
+ *
+ * Loop 1 applies the fix for `BlankLineAfterOpen` and then bows out.
+ * Loop 2 applies a fix for `NewlineEOF`.
+ * Loop 3 applies a fix for `NoNewlineEOF`.
+ * Loop 4 will try to apply the `NewlineEOF` fix again, but sees this causes a conflict and skips.
+ * Loop 5 will try to apply the `NewlineEOF` fix again, but sees this causes a conflict and skips.
+ * Loop 6 applies a fix for `NewlineEOF`.
+ * Loop 7 will try to apply the `NoNewlineEOF` fix again, but sees this causes a conflict and skips.
+ * Loop 8 applies a fix for `NoNewlineEOF`.
+ * Loop 9 - 13 repeat loop 4 - 8.
+ * Loop 14 - 18 repeat loop 4 - 8.
+ * Loop 19 - 23 repeat loop 4 - 8.
+ * Loop 24 - 28 repeat loop 4 - 8.
+ * Loop 29 - 33 repeat loop 4 - 8.
+ * Loop 34 - 38 repeat loop 4 - 8.
+ * Loop 39 - 43 repeat loop 4 - 8.
+ * Loop 44 - 48 repeat loop 4 - 8.
+ * Loop 49 = loop 4.
+ * Loop 50 = loop 5, i.e. applies no fixes.
+ *
+ * @see \PHP_CodeSniffer\Tests\Core\Fixer\FixFileReturnValueTest
+ */
+
+namespace Fixtures\TestStandard\Sniffs\FixFileReturnValue;
+
+use PHP_CodeSniffer\Files\File;
+use PHP_CodeSniffer\Sniffs\Sniff;
+
+class ConflictSniff implements Sniff
+{
+
+    public function register()
+    {
+        return [T_OPEN_TAG];
+    }
+
+    public function process(File $phpcsFile, $stackPtr)
+    {
+        $tokens = $phpcsFile->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;
+    }
+}

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

@@ -0,0 +1,40 @@
+<?php
+/**
+ * Test fixture.
+ *
+ * This sniff deliberately causes a "failed to fix" situation by causing the fixer to run out of loops.
+ *
+ * @see \PHP_CodeSniffer\Tests\Core\Fixer\FixFileReturnValueTest
+ */
+
+namespace Fixtures\TestStandard\Sniffs\FixFileReturnValue;
+
+use PHP_CodeSniffer\Files\File;
+use PHP_CodeSniffer\Sniffs\Sniff;
+
+class NotEnoughLoopsSniff implements Sniff
+{
+
+    public function register()
+    {
+        return [T_ECHO];
+    }
+
+    public function process(File $phpcsFile, $stackPtr)
+    {
+        $tokens = $phpcsFile->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, ' ');
+        }
+    }
+}

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

@@ -0,0 +1,4 @@
+<?xml version="1.0"?>
+<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">
+
+</ruleset>

tests/Core/Fixer/Fixtures/test.inc

@@ -0,0 +1,2 @@
+<?php
+echo 'this is a simple file which will have a fixer conflict when the TestStandard.FixFileReturnValue.Conflict sniff is run against it';