Allow phpcbf to use the cache

[fredden]

Description

As part of the development process, I often find myself using phpcbf followed by phpcs in order to check that the code I've written complies with the configured coding standard for the codebase in question. I have recently also started using the --cache parameter on the command-line to speed up this process. This has a significant positive impact on the time taken for phpcs, however phpcbf seemed to take the same time as usual.

This pull request enables phpcbf to use the cache, just like phpcs does. In my use-case where I run phpcbf immediately followed by phpcs (with the same flags), the phpcs run is always very fast as it can use the cache and doesn't need to re-scan every file.

Suggested changelog entry

• Allow phpcbf to use the cache

Types of changes

• Bug fix (non-breaking change which fixes an issue)
• [x] New feature (non-breaking change which adds functionality)
• Breaking change (fix or feature that would cause existing functionality to change)
• 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.

Fixes #485

[jrfnl]

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

[fredden]

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

[jrfnl]

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.

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