Ignore errors/warnings which are ignored

[fredden]

Description

While working on #481, I noticed that the fixableCount property had an incorrect value. Upon investigation, I found that issues with severity zero are still being included in the count for how many issues can be fixed. This is misleading, as running phpcbf does not fix these.

Unfortunately there don't appear to be any tests which cover this functionality, so getting suitable test coverage may be considered a blocker to this pull request.

Reproduction details

In order to reproduce this bug, run the following commands:

• php bin/phpcbf autoload.php to fix any fixable errors. Notice that there are no errors reported as fixable.
• php bin/phpcs --cache=local-test-file.json autoload.php to create a cache file. Notice that there are no errors reported (fixable or otherwise).
• jq < local-test-file.json '.["'$PWD'/autoload.php"] | "\(.errorCount) errors, \(.warningCount) warning, \(.fixableCount) fixable issues"' to see the number of errors/warnings/fixable recorded in the cache. I expect these to match the output of phpcs, but they are all non-zero.

Suggested changelog entry

Fix bug where internal property fixableCount was incorrectly including issues with severity zero.

Related issues/external references

This is related #481 - without this change, that pull request is somewhat less effective.

Types of changes

• [x] Bug fix (non-breaking change which fixes an issue)
• New feature (non-breaking change which adds functionality)
• Breaking change (fix or feature that would cause existing functionality to change)
  • [x] This change is only breaking for integrators, not for external standards or end-users.
• Documentation improvement

PR checklist

• [x] I have checked there is no other PR open for the same change.
• [x] I have read the Contribution Guidelines.
• [x] I grant the project the right to include and distribute the code under the BSD-3-Clause license (and I have the right to grant these rights).
• [ ] I have added tests to cover my changes.
• [x] I have verified that the code complies with the projects coding standards.
• [Required for new sniffs] I have added XML documentation for the sniff.

[jrfnl]

@fredden While I understand this is related to the other PR, I can't actually seem to create a (test) situation where I can reproduce an incorrect fixable count, let alone see a difference with your PR.

If the fixable count was incorrect, it should also show an incorrect number in the reports, but it doesn't.

So, yes, without a reproduction case proving there is actually a bug (which I haven't been able to find), I don't think this change should be accepted. Oh and if a reproduction case can be designed, then it can also be translated into a test, which should then be added to the PR.

[fredden]

@jrfnl I have written some steps to reproduce the problem and added these to the pull request description. When writing these, I realised that the problem is much larger than I'd initially thought. I'll mark this as a draft while I work on a solution.

[jrfnl]

@jrfnl I have written some steps to reproduce the problem and added these to the pull request description. When writing these, I realised that the problem is much larger than I'd initially thought. I'll mark this as a draft while I work on a solution.

I still don't see the problem. The cache records the severity and takes that into account correctly when replaying the errors. The fact that a number, which is only for internal use and is not exposed to the outside world, doesn't look right to you, doesn't make it wrong.

P.S.: not sure what jq is supposed to do, but not a command which works.

[jrfnl]

@fredden and me discussed this PR in a call. Basically, this PR is blocked until the cache feature has proper (full) test coverage, so we can be sure nothing breaks with this change.

Closing this PR until those preliminaries are fulfilled. Once they are fulfilled, I'm perfectly happy to revisit this PR and re-open it.