Repository navigation
Conversation
The configuration hash covered the registered checkers' class names and configuration, so editing a project's own fixer or sniff kept serving results cached with its previous code until --clear-cache. Hash the source files of checkers living outside vendor/ (with their parents and traits) into it; vendor checkers still change only with their package. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_0187H51XtF5pkbEiJDmttSnZ
| { | ||
| $fileHashes = []; | ||
| foreach ($checkerClasses as $checkerClass) { | ||
| foreach ($this->resolveClassFileNames(new ReflectionClass($checkerClass)) as $fileName) { |
There was a problem hiding this comment.
At first glance, this seems quite costly to help with cache.
I'd accept some non-reflection approach, like package version.
There was a problem hiding this comment.
Thanks for looking. Cost is the right concern, so I measured it before answering.
Hashing the checker list takes 0.27 ms per call (PHP 8.4, average of 200 runs). That's with all 301 classes from PHP-CS-Fixer's built-in set plus one local sniff. It runs once per container boot, for the main process and each parallel worker. The reflection is cheap because ECS has already loaded every checker class by then: new ReflectionClass() only reads metadata already in memory. The only file reads are md5_file() on checkers outside vendor/. Most projects have none of those; projects with custom rules have a handful.
A package version can't cover this case, unfortunately. The checkers it targets live in the project itself, so there is no version to read. That's exactly why their changes are invisible to the cache today.
If you'd still rather avoid reflection, an explicit option would work too. For example, withCache(directory: ..., invalidateOnChangeOf: [__DIR__ . '/utils/ecs']) would hash the files the user lists. It's what we do in our own config in the meantime, through the cache namespace. Happy to switch the PR to that if you prefer it.
There was a problem hiding this comment.
It's quite hard for me to undestand, can you put it in simpler words?
If it only concerns local rules, the native ones starting with Symplify\, code sniffer and php-cs-fixer should be skipped using string compare, not reflection.
| $checkerConfiguration = $ecsConfig->getCheckerConfiguration(); | ||
| $fileHash = sha1( | ||
| Json::encode($checkerConfiguration) . $this->hashLocalCheckerFiles(array_keys($checkerConfiguration)) | ||
| ); |
There was a problem hiding this comment.
Can you split this onto more lines? Seems connecting 2 different cases here.
Problem
The cache's configuration hash covers the registered checkers' class names, their configuration and the ECS version, but not their code. A project that ships its own fixer or sniff (registered from
src/,tools/, …) keeps getting cached "no errors" for every unchanged file after editing that checker, until someone thinks of--clear-cache. CI caches restored from a previous run have the same problem, and nothing tells you the results are stale.Fix
FileHashComputer::computeConfig()now also hashes the source files of registered checkers that live outsidevendor/, together with their parent classes and traits. Checkers undervendor/, which covers every built-in rule in an installed ECS, are skipped: they only change with their package. In practice that means reading only the handful of custom checker files a project has, once per run.Only file contents are hashed, not their paths, so the same project checked out elsewhere (a git worktree, a CI runner) keeps its cache.
testInvalidateCacheOnLocalCheckerChangeregisters a local sniff, edits its file and expects a different configuration hash; it fails without the change.Context
PHP-CS-Fixer considered hashing every fixer's code in PHP-CS-Fixer/PHP-CS-Fixer#7003, which stalled partly on the cost of reading hundreds of vendor fixer files per run; skipping
vendor/avoids that cost. Until now we worked around it by feeding a hash of our fixers intowithCache(namespace: …), which only works because the namespace happens to be part of the hashed parameters.🤖 Generated with Claude Code
https://claude.ai/code/session_0187H51XtF5pkbEiJDmttSnZ