-
-
Notifications
You must be signed in to change notification settings - Fork 86
Improve Reliability of Configuration Caching #226
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
Improve Reliability of Configuration Caching #226
Conversation
The output of `getBindings()` is largely just a list of the class names registered in the container, but it doesn't contain information on how those classes are actually configured. As such, changing `ecs.php` in a way that updates sniff / fixer rules would not invalidate the cache, meaning those new settings would not be checked against any unchanged files. My approach here still isn't perfectly reliable, but I think it's a usable middle ground. We still report all the registered classes, but now I explicitly pull out the configuration data for sniffers and fixers to add to the serialized data. Then I also hash out the settings used for our sniffers, to support the upcoming PHPCS rulesets / standards support. The ECSConfig previously constructed as-needed, is now provided via DI. This is required for the upcoming feature, but has the added benefits of making sure the cache hash uses the hash for the exact same config that will be used for sniffing *and* improving the performance of sniffing (since every file is currently creating a config, triggering filesystem ops). Closes easy-coding-standard#30
First, the class has been relocated to a more idiomatic directory location, matching it's namespace. Then many new tests were added to cover different types of cache invalidation that can occur. I also added a positive control test to make sure caches *aren't* invalidated when we don't want them to be. You will want to read the associated class comment for information on why it's structured so strangely. The existing cache test was removed, since it didn't work correctly Again, see the class comment for why.
In the course of testing, I discovered the previous approach didn't work for third-party sniffs. The two needed changes: 1. "Real" fixers following the PhpCsFixer intended implementation with `AbstractFixer` have the `$configuration` property pulled explicitly instead with reflection-based visiblity overwriting. The serialized leading `\0\0` approach seemed too non-obvious for future maintainers on a second viewing. 2. The condition was wrong. I don't know what I was thinking at the time. :|
…tandard into kesills-cache-invalidation
… hashing Left a comment detailing not to use `::$configuration` like I had attempted to, so future maintainers don't make the same mistake.
I'm a bit reluctant to use more internal php-cs-fixer or code sniffer classes. The ECS vision is to keep as slim dependency on internals as possible, to make it easier to maintain and evolve in the future. There are used many classes that can change and break our workflow, rendering "easy" obsolete. Still, it could be good improvement to introduce simple "withConfiguredRule()" 1st + 2nd param hash key to invalidate if change. I'd be open to such standalone feature. |
@TomasVotruba Sure, I can try that instead. Would you mind if we re-open #30 in the interim, since that tracks the undesirable behavior the cache functionality has now? |
I'd prefer PR first, to avoid piling up issues with idea -solutions. |
Closes #30
Each commit message body includes details on the changes made, but as a whole I've refactored the
FileHashComputer
so that instead of just using the class names of services and rules for hashing material, it also uses the internal state in order to capture configuration.Since we did experience this regression, and caching issues are often hard to notice, I made sure to update the related testing suite to include many more test cases. Given the nature of global / static state, it's a bit annoying. But I added a comment for maintainers to understand why it must be structured the way it is.