Skip to content

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

Conversation

Kenneth-Sills
Copy link
Contributor

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.

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. :|
… hashing

Left a comment detailing not to use `::$configuration` like I had attempted to,
so future maintainers don't make the same mistake.
@TomasVotruba
Copy link
Contributor

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.

@Kenneth-Sills
Copy link
Contributor Author

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

@TomasVotruba
Copy link
Contributor

I'd prefer PR first, to avoid piling up issues with idea -solutions.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Cache invalidation on configuration change
2 participants