`Uuf6429/block_string` rule breaks php-cs-fixer cache (every run is a full re-run)

[darthf1]

Hi!

When the Uuf6429/block_string rule is enabled with a formatter object (e.g. CliPipeFormatter) in its configuration, php-cs-fixer's signature check fails on every subsequent run, so the cache file is rewritten but never produces a hit. Removing the rule restores normal cache behaviour.

Environment

• uuf6429/php-cs-fixer-blockstring: 1.0.5
• friendsofphp/php-cs-fixer: 3.95.1
• PHP: 8.5.4
• OS: Linux (Docker)

Reproduction

.php-cs-fixer.dist.php (minimised):

use PhpCsFixer\Config;
use PhpCsFixer\Finder;
use uuf6429\PhpCsFixerBlockstring\Fixer\BlockStringFixer;
use uuf6429\PhpCsFixerBlockstring\Formatter\CliPipeFormatter;
use uuf6429\PhpCsFixerBlockstring\InterpolationCodec\GeneratedTokenCodec;

return new Config()
    ->registerCustomFixers([new BlockStringFixer()])
    ->setCacheFile(__DIR__ . '/var/cache/php-cs-fixer/.php-cs-fixer.cache')
    ->setFinder(new Finder()->in([__DIR__]))
    ->setRiskyAllowed(true)
    ->setRules([
        'Uuf6429/block_string' => [
            'formatters' => [
                'SQL' => new CliPipeFormatter(
                    ['cmd' => ['sqlfluff', '--version']],
                    ['cmd' => ['sqlfluff', 'fix', '-']],
                    new GeneratedTokenCodec(),
                    true,
                ),
            ],
        ],
    ]);

Steps:

1. vendor/bin/php-cs-fixer fix — runs against the whole tree, writes cache file.
2. vendor/bin/php-cs-fixer fix — should re-use the cache and skip unchanged files.

Expected: second run skips files (cache hit).
Actual: second run re-processes every file. Cache file is rewritten but never consulted.

Root cause

php-cs-fixer stores the cache as JSON (FileHandler → json_encode($signature)), and compares the stored signature to the freshly computed one via Signature::equals(), which does an == on the rules array.

The Uuf6429/block_string rule's value contains object instances (CliPipeFormatter, GeneratedTokenCodec, deprecated bool that the constructor replaces with new DefaultNormalizer(...)). When the signature is JSON-encoded:

• These instances don't implement JsonSerializable and don't define __serialize / jsonSerialize.
• json_encode therefore emits {} (or the public-property projection), and json_decode returns it as a plain stdClass / array.
• After deserialization, the rule's value is no longer the original object graph, so the == comparison against the freshly constructed CliPipeFormatter always fails.

In our cache file, the Uuf6429/block_string entry is dropped entirely — cache.json lists 144 rules but the block_string key is absent. Either way, the stored signature can never equal the runtime one.

Quick proof (after a run):

$ jq '.rules | keys | map(select(test("block_string|Uuf6429"; "i")))' var/cache/php-cs-fixer/.php-cs-fixer.cache
[]

The rule is configured, runs against every file, yet leaves no trace in the persisted signature.

Impact

• Every CI / pre-commit run pays the full php-cs-fixer cost, even on a clean tree.
• Disabling the rule (or stripping its formatters config) restores cache hits.

Suggested fix

The formatters embedded in the rule configuration need to participate in the signature in a way that survives json_encode → json_decode → ==. A few options:

1. Implement JsonSerializable on AbstractStringFormatter / CliPipeFormatter / GeneratedTokenCodec, emitting a deterministic associative array (e.g. ['class' => static::class, 'version' => $this->version, 'formatter' => $this->formatter]). Provide a matching restoration path, or document that the rule's signature contribution is a scalar fingerprint rather than the object itself.
2. Have BlockStringFixer expose a custom configurationDescription / signature hook that reduces the formatter graph to a stable scalar (e.g. sha1 of a normalized array) before the signature is computed.
3. Document the limitation and refuse to register objects in the rule configuration; require users to pass an identifier/factory key that the fixer resolves internally.

Option (1) seems the smallest change and keeps the user-facing API unchanged.

[uuf6429]

Hey, thanks for such a detailed report!

Option 1 seems reasonable easy and logical - I'll apply the change later on today.

[uuf6429]

As you can see in the PR, my attempt fell short on two fronts:

1. The newly created test didn't fail. For some reason, without the fix, the 2nd run of the test still uses the cache (the 3rd and 4th runs of the test are unrelated - see next point).
2. After the fix (code in the test description), I expected run 3 and 4 to be successful, but it still failed on the 3rd run - meaning that despite changing the rule config, (by changing the formatter version), php-cs-fixer still used the old cache and ignored the version change.

I'll need to dig a bit deeper - I wasn't expecting such complexity.

[uuf6429]

I think I might have figured out why there is this strange behaviour (and why my test was passing unexpectedly): The json_encode() part only happens when saving the cache.

When checking if the cache is stale, I suspect it relies on simple equality matching. Now that I remember, I guess that's why $version is readonly and required to be passed at construction time.

This reveals another issue: version is definitely not enough as cache identity - more info is needed. On the other hand, the json encoding part becomes easier; I just need to use get_object_vars() or simply the cache identity key.

[uuf6429]

@darthf1 I think I have this issue covered now. Unfortunately, it's a drastic change so I'll have to draft a v2 release for it.

Could you maybe review/try the fix in PR #19? You can install that in composer by specifying dev-fix-caching as the package version. I'll release it as soon as you give the go-ahead.

[darthf1]

Hi! Thanks for the effort!

I don't have much time today; i quickly updated the package and my config to match the expected format; when I run phpcs twice I still see it running the full suite a second time.

If you need more info I can provide it later this weekend.

[uuf6429]

Sure, thanks for trying!

When you have time later, could you maybe check the cache file? At the very least the rule settings should be dumped there now.

[darthf1]

so, i updated the version to dev-fix-caching and my config from

'Uuf6429/block_string' => [
    'formatters' => [
        'SQL' => new CliPipeFormatter(
            ['cmd' => ['sqlfluff', '--version']],
            ['cmd' => ['sqlfluff', 'fix', '-']],
            new GeneratedTokenCodec(),
            true,
        ),
    ],
],

to

'Uuf6429/block_string' => [
    'SQL' => new CliPipeFormatter(
        ['cmd' => ['sqlfluff', '--version']],
        ['cmd' => ['sqlfluff', 'fix', '-']],
        new GeneratedTokenCodec(),
        true,
    ),
],

I ran the command twice and twice its running over all files, nothing is cached.

Loaded config default from "/home/www/app/.php-cs-fixer.dist.php".
Running analysis on 4 cores with 10 files per process.
Using cache file "/home/www/app/var/cache/php-cs-fixer/.php-cs-fixer.cache".
 4229/4229 [▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓] 100%


Fixed 0 of 4229 files in 80.422 seconds, 172.00 MB memory used

Inside the cache file, i see

"Uuf6429\/block_string":{"SQL":{}},

I think the empty object is the fault here?

edit:

I just reverted everything, and looked inside the cache file, there i also see

"Uuf6429\/block_string":{"SQL":{}},

So maybe theres something in my config that you havent taken into account yet?

[uuf6429]

I changed the config a bit.

First of, there's a constant for the rule name (BlockStringFixer::NAME), but it's just syntax sugar.

Secondly, (and more importantly), the whole rule config needs to be passed through BlockStringFixer::config(), which ensures proper serialization. I thought I made sure it would fail if it didn't - I need to check why it was still allowed to run.

Please refer to the CacheTest fixture here:

php-cs-fixer-blockstring/tests/Fixtures/Scenarios/caching/config-v1.php

Line 10 in 19341b3

BlockStringFixer::NAME => BlockStringFixer::config(

If that still doesn't work, try running at least the CacheTest - it should pinpoint better what's going wrong.

[darthf1]

Ah yes, that works!

What I noticed in the cache file is that other rules are json style serialized, where yours is PHP style serialized. I think it means implementing \JsonSerialize?

Other then that, caching seems to be working right now :)

Edit:
About the config; if I remember correctly I got an error when I kept the formatters key, and that's why I removed it. But I would have to test again when behind a computer

[uuf6429]

Ah yes, that works!

Glad to hear that!

What I noticed in the cache file is that other rules are json style serialized, where yours is PHP style serialized. I think it means implementing \JsonSerialize?

Yes, they're using JSON. json_encode() automatically interacts with the method from \JsonSerialize, at least in newer PHP versions.

There are a couple of reasons why I didn't keep it like that:

1. Serializing typed objects as JSON is not easily reversible. Coming up with some sort of scheme to do that:
   • is a lot of work (e.g. having it serialize as {class, state}, then figuring out how to hydrate the newly created object later and to be flexible, providing some sort of onDeserialized/__wakeup hook etc).
   • increases maintenance cost, higher risk of bugs and requires decent documentation.
   • even with documentation, it would surprise users.
   • is going to be a performance bottleneck.
2. PHP already provides this sort of functionality natively. :)

---

I do wonder what @keradus thinks about it though (if he has some time to spare).

---

One final thing, to consider: the cacheFingerprint is currently stared "raw"/"as-is". I could instead store a hash of a serialized value, to make the config smaller... but then I thought keeping it like that improves debuggability (for one thing, the CacheTest also depends on it). I could be convinced to change that if there are good reasons.

---

I'll draft a new release as soon as those final points are cleared up.
I'm drafting and releasing a release candidate now, I could always change the internal storage mechanism later without BC breaks, if needs be.

[darthf1]

Thanks for fixing! I upgraded to rc1