Fixer::fixFile(): bug fix for incorrect return value #1048
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
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.
jrfnl
force-pushed
the
feature/fixer-bugfix-incorrect-return-value
branch
from
2f85471 to
fda2bf6
Compare
rodrigoprimo
approved these changes
Apr 25, 2025
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description
Over the years, I'd noticed on numerous occasions that
phpcbfcould 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
Fixerfile reverted:Now check the
./tests/Core/Fixer/Fixtures/directory and take note that there is notest.inc.fixedfile present.Next, run:
Now check the
./tests/Core/Fixer/Fixtures/directory again and see that thetest.inc.fixedfile has been created.If you run these commands with
-vvyou 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.fixedfile)Turns out that if the last loop of the Fixer didn't make any fixes, the
Fixer::fixFile()method always returnedtrue(= 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
CBFreport, which triggers the fixer and checks the return value of thefixFile()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.
Suggested changelog entry
Fixed: the file under scan would sometimes be updated with partial fixes, even though the file "failed to fix".
Types of changes