Allow phpcbf to use the cache

[fredden]

Is your feature request related to a problem?

Yes and no. When using phpcs --cache, subsequent runs are fast. When using phpcbf --cache, every run takes the same amount of time as the first.

Describe the solution you'd like

When using phpcbf --cache, subsequent runs should be faster - as they already are with phpcs --cache. Also, when using both phpcs and phpcbf, the cache generated by one should be usable by the other.

phpcbf should be able to use the cache to determine if the sniffs need to be run or not on a particular file. When the cache is valid for a file, and there are no fixable errors reported, then the file can be skipped. When the cache is invalid, or there are fixable errors reported, then the file needs to be run through the sniffs as normal.

Internally when phpcbf runs, there is actually a run of (the equivalent of) phpcs first, and if there are any fixable errors reported then the fixer is enabled and run through the file. When the fixer is enabled, the cache should be temporarily disabled to avoid any negative performance overhead as it runs the sniffs up to 50 times.

At the end of a run of phpcbf, the cache does not need to be in a perfect state. If there were no changes made, then the cache should still be valid for the files it was valid for to start with. If any changes were made, then the state of the cache is undefined. The next time the cache is read, it will be validated as normal.

phpcs

This is the existing behaviour for phpcs --cache.

flowchart
    A[Load ruleset]
    A --> B{Caching enabled?}
    B -->|Yes| C[Load cache]
    B -->|No| D[Run sniffs]
    D --> F
    C --> E{Cache entry valid?}
    E -->|No| D
    E -->|Yes| F[Report results]

Loading

phpcbf

This is the proposed behaviour for phpcbf --cache.

flowchart
    A[Load ruleset] --> B{Caching enabled?}
    B -->|Yes| C[Load cache]
    B -->|No| D[Run sniffs]
    C --> E{Cache entry valid?}
    E -->|Yes| H{Fixable errors?}
    H -->|No| F[Report results]
    H -->|Yes| D
    E -->|No| D
    D --> G[Run fixer]
    G -->|1-50x| G
    G --> F

Loading

Additional context (optional)

Given the low test coverage of the areas in question here, making changes to this functionality is risky. Therefore this feature is likely blocked by #146 and/or #147

The proposed changes have been implemented in #481

• I intend to create a pull request to implement this feature.

[jrfnl]

Thanks for opening this issue @fredden. You already know my opinion: this is more complicated than you initially thought.

If I look at the table above, I still see logic logic errors.

If the caching feature is active, the cache at the end of a run should always be complete and correct, so in my opinion, your entries in the "Only fixable errors" and "Fixable and unfixable errors" rows for phpcbf are incorrect.

In the mean time, let's close #481.

[fredden]

@jrfnl let's discuss this on our next call. I am interested to learn about these logic errors that you hinted at. I think that the implementation in #481 matches the table above.

[mabar]

Wouldn't it be possible to start small? Just skip the files that are already cached and with no errors if unmodified since last phpcs run.
Even if it does not update the cache and re-runs for fixed files before phpcs is ran again, it would still make fixing few files in codebases with 1000+ files significantly faster and not break anything.
And updating the cache would be as simple as running phpcs again.
It would be far from perfect but from my (possible naive) point of view it would do 90% of the results for 10% of the work, with very little (possibly none?) risk involved.

[fredden]

In my call with @jrfnl, we determined that there are not actually any logic errors, but instead the language used was confusing. I will review the words/language to better communicate the intent.

[jrfnl]

Just came across old repo issue squizlabs/PHP_CodeSniffer#3781 and I think that needs to be looked into in relation to this ticket as it gives the impression that - at least in some cases - the cache is already being taken into account for PHPCBF....

[fredden]

In my call with @jrfnl, we determined that there are not actually any logic errors, but instead the language used was confusing. I will review the words/language to better communicate the intent.

I have replaced the table with a flow-chart, and written more words in the description.

Just came across old repo issue squizlabs/PHP_CodeSniffer#3781 and I think that needs to be looked into in relation to this ticket as it gives the impression that - at least in some cases - the cache is already being taken into account for PHPCBF....

From what I can tell, that bug seems independent of this issue / feature request. I have put a comment there with my findings.

[staabm]

would end-to-end tests be sufficient to get this moving forward or do we also need unit tests for the caching related stuff?

[jrfnl]

@staabm I have a feeling a combination of both will be needed. With end-to-end, we'd probably need too many tests to cover it properly, with a combination, the effort needed to get the tests in place will probably be lower.