Cache makes phpcbf fix unexpected sniff

[takaram]

Describe the bug I ran phpcbf with --sniffs option.

$ vendor/bin/phpcbf --sniffs=Generic.PHP.DisallowShortOpenTag /path/to/files

I expected this command to only fix Generic.PHP.DisallowShortOpenTag, but some other errors are fixed too.

Code sample

<?

function test() {
if (true) {
  echo 1;
}
}

Custom ruleset

<?xml version="1.0" ?>
<ruleset>
  <rule ref="Generic.PHP.DisallowShortOpenTag"/>
  <rule ref="Generic.WhiteSpace.ScopeIndent"/>
</ruleset>

To reproduce Steps to reproduce the behavior:

1. Create a file called test.php with the code sample above...
2. Run phpcs --report=diff --sniffs=Generic.PHP.DisallowShortOpenTag --cache=.phpcs-cache test.php

3. See the result

   --- test.php
   +++ PHP_CodeSniffer
   @@ -1,7 +1,7 @@
   -<?
   +<?php
   
   function test() {
   if (true) {
   -  echo 1;
   +          echo 1;
   }
   }

Expected behavior Indentation of line 5 should not be fixed.

Removing --cache solves the problem.

Versions (please complete the following information):

• OS: Ubuntu 18.04 on Windows 10 WSL 2
• PHP: 8.2
• PHPCS: 3.7.2
• Standard: Custom

[fredden]

I can confirm this bug exists in the latest version of PHP_CodeSniffer, and also in the master branch at https://github.com/PHPCSStandards/PHP_CodeSniffer/commit/b0bb13380d7cd4109867837e40d613899155f0fd (which is current at time of writing). The steps to reproduce provided are sufficient to demonstrate the bug.

When running with --no-cache, only the Generic.PHP.DisallowShortOpenTag sniff gets registered in the ruleset, so only that sniff runs. This is normal, and there is no problem here.

When running with --cache, all sniffs in the ruleset get registered. This is normal, as the cache then gets all the errors/warnings recorded. After running all sniffs and recording all errors in the cache, the returned errors are then filtered down to only those being requested (by replaying the errors without the "record all" flag set).

So far so good. When the "full" report is used, all is well as the cache gets all the errors and the "full" report only shows the errors/warnings for the requested sniff(s) (not the whole ruleset).

https://github.com/squizlabs/PHP_CodeSniffer/blob/c6c65ca0dc8608ba87631523b97b2f8d5351a854/src/Ruleset.php#L198-L202

https://github.com/squizlabs/PHP_CodeSniffer/blob/c6c65ca0dc8608ba87631523b97b2f8d5351a854/src/Files/LocalFile.php#L145-L151

In order to generate the "diff" report, or when running phpcbf, the fixer is enabled and run on the file with the loaded ruleset. As described above, when the cache is enabled, the ruleset contains all rules, not just the one requested.

When a sniff calls addFixableError() (or warning), PHP_CodeSniffer returns true to indicate that the fixer should be activated / applied, and false when it should not. When phpcs runs, this is always false. When running phpcbf (or the "diff" report) with --sniffs=... this only returns true for the sniffs requested.

So far so good. Now for the bug: Generic.WhiteSpace.ScopeIndent does not call addFixableError() before making changes to the file. Instead it checks if the fixer is enabled/running, and then makes changes to the file. If it had called addFixableError(), it would be told to not modify the file. However, as the fixer happens to be active, it applies its changes to the file.

https://github.com/squizlabs/PHP_CodeSniffer/blob/c6c65ca0dc8608ba87631523b97b2f8d5351a854/src/Standards/Generic/Sniffs/WhiteSpace/ScopeIndentSniff.php#L595-L601