Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Cache makes phpcbf fix unexpected sniff #3781

Open
takaram opened this issue Mar 27, 2023 · 1 comment
Open

Cache makes phpcbf fix unexpected sniff #3781

takaram opened this issue Mar 27, 2023 · 1 comment

Comments

@takaram
Copy link

takaram commented Mar 27, 2023

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
Copy link
Contributor

fredden commented May 20, 2024

I can confirm this bug exists in the latest version of PHP_CodeSniffer, and also in the master branch at PHPCSStandards/PHP_CodeSniffer@b0bb133 (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).

// Ignore sniff restrictions if caching is on.
if ($config->cache === true) {
$restrictions = [];
$exclusions = [];
}

// During caching, we don't filter out errors in any way, so
// we need to do that manually now by replaying them.
if ($this->configCache['recordErrors'] === true) {
$this->configCache['cache'] = false;
$this->replayErrors($this->errors, $this->warnings);
$this->configCache['cache'] = true;
}

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.

// When fixing, we're going to adjust the indent of this line
// here automatically, so use this new padding value when
// comparing the expected padding to the actual padding.
if ($phpcsFile->fixer->enabled === true) {
$tokenIndent = $length;
$this->adjustIndent($phpcsFile, $checkToken, $length, $adjustments[$first]);
}

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants