@fredden Thanks for suggesting this, but I'm not terribly keen on this change (quite apart from having serious doubts about the proposed change working the way you seem to expect it would).

Let's think this through:

• The cache stores a file hash + the seen errors for each file (and some more things).
• In a PHPCS run, if caching is on and there is no cache initially, the run will be (sometimes significantly) slower as the cache will need to be created and will need to write the results to the temporary file.
• In a PHPCS run, if caching is on and there is a cache, the run will be (significantly) faster as only changed files will be scanned and for all unchanged files, the previously seen errors will be replayed.

Now what about PHPCBF ?

• PHPCBF could benefit from the cache in its initial state to determine if the fixer should run for a file.
• However, as far as I can see, that's where all benefits stop.
  • For any file which is unchanged, the cache causes the errors to be replayed, the sniff code isn't being run, in other words, the fixers would not run.
  • For any file which was changed, in the first loop, the sniffs would run, but the errors seen (and cached) will be wrong as the fixer will fix the fixable errors, so now the cache will contain errors which have already been fixed.
  • The fixer runs up to 50 loops as one fixer may fix a certain issue, but then the fix may need "clean up" by other sniffs to comply with all the rules.
    At the end of each loop, the cache will be updated with the outdated (already fixed errors) instead of with the new errors.
  • As soon as we hit the second loop, the file will have been "changed" (via the fixer), so the cache is now useless, quite apart from it containing the wrong errors, but with the caching feature on, the cache will keep getting updated for each of these loops.

All in all, this change does not fill me with confidence.

As per the CONTRIBUTING guide, maybe this topic (and all potential angles and potential problems this can cause) should be discussed in an issue instead ?

I will test this more locally. The lack of test coverage means that this sort of change is risky.

I will test this more locally. The lack of test coverage means that this sort of change is risky.

Exactly, which means I would not accept this change without significant test coverage being added.

@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.