Skip to content

ci: make the QA gates and the test suite pass again on 4.1.x - #676

Merged
Spomky merged 5 commits into
4.1.xfrom
fix/qa-tooling-4-1-x
Aug 26, 2026
Merged

ci: make the QA gates and the test suite pass again on 4.1.x#676
Spomky merged 5 commits into
4.1.xfrom
fix/qa-tooling-4-1-x

Conversation

@Spomky

@Spomky Spomky commented Aug 26, 2026

Copy link
Copy Markdown
Member

Why

All three QA gates are red on 4.1.x, and two of them never ran at all — the job died on a config constant that upstream had removed, before analysing a single file. That is the same breakage #669 repaired on 4.2.x; it was never backported, and Rector has since broken the same way on both branches.

This blocks every PR against 4.1.x, including #675.

What was wrong

ECS imported SetList::PHPUNIT and SetList::STRICT, both removed from Easy Coding Standard. Dropped — STRICT's three fixers were already registered explicitly. The failure was well hidden: ECS crashed inside its own error printer (str_repeat(): Argument #2 must be >= 0) while trying to report the undefined constant. Run the docker command without -it to see the real message.

OrderedImportsFixer was also registered bare, which silently discarded the class, function, const order that CLEAN_CODE sets. It is now configured explicitly, which is why 35 files are reformatted here — that is the config contradicting itself, not drift.

Rector imported PHPUnitSetList::PHPUNIT_120, folded upstream into the version-aware sets. Dropped; withComposerBased(phpunit: true) was already selecting the right rules, so nothing is lost. The 89 files Rector then wanted to rewrite are applied in this PR:

rule files
RenameMethodRector (PHPUnit 12 renames) 46
RemoveReadonlyPropertyVisibilityOnReadonlyClassRector 16
CommandConfigureToAttributeRector / CommandHelpToAttributeRector 18
SimplifyBoolIdenticalTrueRector 7
dead-code and useless-tag removal 12

PHPStan ran, but against ergebnis/phpstan-rules and phpstan-beberlei-assert. Both extensions are dropped to match 4.2.x. ergebnis alone accounted for 777 of the 1694 baseline entries — the code never followed it, and beberlei/assert is unused in src/.

The PHPStan errors are fixed, not baselined

Stripping ergebnis exposed 34 real errors, mostly types tightened by brick/math and Symfony. They are fixed in code, following the rule that a new baseline entry should mean a new problem:

  • non-empty-string for BigInteger::fromBase() — guarded in BigInteger, RSAKey, Math, AbstractECDH, KeyConverter, ESKeyAnalyzer
  • positive lengths for random_bytes() / randomBits()Curve, RSACrypt, JWKFactory
  • crit and key_ops validated as lists of strings — HeaderCheckerManager, JWKSet, UsageAnalyzer
  • json_decode() no longer passed JSON_UNESCAPED_SLASHES | JSON_UNESCAPED_UNICODE, which are encode-only flags and did nothing

Most of these are ports of the equivalent fix already on 4.2.x. Where a 4.2.x file also carried features that do not belong on 4.1.x — Brainpool curves in AbstractECDH, the RFC 7516 §7.2.1 header-disjointness change in JWEBuilder — only the type hunk was taken.

Two Symfony denormalizer covariance errors stay baselined, as they are on 4.2.x: fixing them would mean changing a public return type.

The baseline goes from 1694 to 910 entries. 777 removed with ergebnis, 9 more because the underlying code is now fixed.

One behaviour change

JWKFactory::createOctKey() now rejects sizes below 8 bits. Previously createOctKey(0) returned a key built from an empty secret; sizes 1–7 already threw. This matches 4.2.x.

Verification

  • ECS, PHPStan, Rector: all exit 0 locally against ghcr.io/spomky-labs/phpqa:8.5
  • Test suite unchanged: the same 38 pre-existing failures, all in the Symfony bundle configuration tests, before and after. Verified by diffing the failing-test list against a stashed baseline.

Watch out when reviewing

php-cs-fixer's LongToShorthandOperatorFixer rewrites $s[0] = $s[0] ^ "\xff" into $s[0] ^= "\xff", which PHP rejects at runtime with Cannot use assign-op operators with string offsets. It would have broken two Chacha20 tests. They now use substr_replace(), as on 4.2.x. Worth remembering for any byte-flip test.

@Spomky
Spomky force-pushed the fix/qa-tooling-4-1-x branch from e96c16c to 94de666 Compare August 26, 2026 09:06
Spomky added 2 commits August 26, 2026 11:12
All three QA gates were red on this branch, two of them because a tool
config referenced a constant that upstream had removed, so the job died
before analysing anything.

- ECS imported `SetList::PHPUNIT` and `SetList::STRICT`, both gone from
  Easy Coding Standard. Dropped; STRICT's three fixers were already
  registered explicitly. `OrderedImportsFixer` was registered bare, which
  silently discarded the `class, function, const` order that CLEAN_CODE
  sets, so it is now configured explicitly. 35 files reformatted.
- Rector imported `PHPUnitSetList::PHPUNIT_120`, folded upstream into the
  version-aware sets. Dropped; `withComposerBased(phpunit: true)` was
  already selecting the right rules. The 89 files it then wanted to
  rewrite are applied here: PHPUnit 12 method renames, Symfony console
  attributes, and dead-code removal.
- PHPStan dropped the `ergebnis/phpstan-rules` and
  `phpstan-beberlei-assert` extensions, matching 4.2.x. ergebnis alone
  accounted for 777 of the 1694 baseline entries, which is to say the
  code never followed it.

The 34 errors PHPStan then reported are fixed rather than baselined,
mostly types tightened by brick/math and Symfony: `non-empty-string` for
`BigInteger::fromBase`, positive lengths for `random_bytes` and
`randomBits`, `crit` and `key_ops` validated as lists of strings.
Two Symfony denormalizer covariance errors remain baselined, as on 4.2.x.

`JWKFactory::createOctKey()` now rejects sizes below 8 bits, which only
turns an already-broken zero-length secret into an explicit error.

Test results are unchanged: the same 38 pre-existing failures, all in the
bundle configuration tests.
The 38 bundle configuration tests all failed with `exception message '' contains ...`,
whatever the configuration actually reported.

`ConfigurationValuesAreInvalidConstraint` hands the caught exception object to PHPUnit's
`ExceptionMessageIsOrContains`, but since PHPUnit 10.0.15 that constraint matches against
the message string instead. `is_string($other)` is false for an exception, so every
expectation failed and the reported message was always empty. v6.2.0 of
matthiasnoback/symfony-config-test is the latest release and still does this, so there is
nothing to upgrade to.

`ConfigurationAssertionsTrait` keeps the upstream assertions and replaces only
`assertConfigurationIsInvalid()`, comparing the message itself. The 38 call sites are
unchanged; the twelve test classes swap which trait they use.

Also removes the `Ergebnis\PHPUnit\SlowTestDetector\Extension` bootstrap. The package is
not a dependency, so PHPUnit reported a runner warning and exited 1 on every run, even
with no failing test.

The suite now passes: 777 tests, 0 failures.
@Spomky
Spomky force-pushed the fix/qa-tooling-4-1-x branch from 94de666 to 9a97e9b Compare August 26, 2026 09:14
@Spomky Spomky changed the title ci: make ECS, PHPStan and Rector run again on 4.1.x ci: make the QA gates and the test suite pass again on 4.1.x Aug 26, 2026
@Spomky

Spomky commented Aug 26, 2026

Copy link
Copy Markdown
Member Author

Update: the test job as well

Once the three QA gates went green, the test job actually ran for the first time — and exposed 38 failing bundle configuration tests, previously invisible because the job was skipped whenever a gate failed. They are fixed here too.

Cause. Every one of them failed with Failed asserting that exception message '' contains ..., whatever the configuration actually reported. ConfigurationValuesAreInvalidConstraint hands the caught exception object to PHPUnit's ExceptionMessageIsOrContains, but since PHPUnit 10.0.15 that constraint matches against the message string:

protected function matches(mixed $other): bool
{
    if (!is_string($other)) {
        return false;          // an exception object lands here
    }
    return str_contains($other, $this->expectedMessage);
}

So the assertion could never pass, and the failure description always printed an empty message. matthiasnoback/symfony-config-test v6.2.0 is the latest release and still does this — there is nothing to upgrade to.

Fix. ConfigurationAssertionsTrait keeps the upstream assertions and replaces only assertConfigurationIsInvalid(), comparing the message itself. The 38 call sites are untouched; the twelve test classes just swap which trait they use.

Also removed the Ergebnis\PHPUnit\SlowTestDetector\Extension bootstrap from the PHPUnit config. The package is not a dependency, so PHPUnit emitted a runner warning and exited 1 on every run even with no failing test — the job could not have gone green while it was there.

Result: 777 tests, 0 failures, exit 0.

Note on brick/math

The Point.php:109 PHPStan error that failed the first run here was version-specific: brick/math ≤ 0.17 declares fromBase(non-empty-string $number) in a way that PHPStan cannot satisfy at that call site. The range on 4.1.x now includes 0.19, which resolves it. Worth knowing that running PHPStan against an older brick/math in the allowed range will still report it.

Spomky added 3 commits August 26, 2026 11:23
Three problems, all of which made the pipeline's result depend on
something other than the code under test.

**The dependency cache was frozen.** The cache key was
`composer-${{ runner.os }}-${{ hashFiles('composer.lock') }}`, but
composer.lock is in .gitignore, so hashFiles() returned the empty string
and the key was the constant `composer-Linux-` on every branch and every
run. GitHub cache entries are immutable per key, so the first run ever to
save one froze `vendor` for good, and the analysis jobs — which only
restore the cache and never install — have been analysing those versions
ever since. The key is now hashed from the composer.json files, so
widening a constraint invalidates it.

That is why PHPStan reported `Point.php:109` on one PR and not on the
next commit of the same branch: the two runs restored different vendors.
The call site is guarded now, so the analysis no longer depends on which
brick/math is installed. The mask is as wide as the longer operand and
can never be empty; the guard is there because that cannot be proven
statically.

**Rector renamed `expectExceptionMessage()` to
`expectExceptionMessageIsOrContains()`** in 125 places, because it
resolves PHPUnit from composer and gets 13. But `castor phpunit` runs
`phpunit-11`, pinned because the PHP 8.2 image ships nothing newer, and
that method only exists from PHPUnit 12. Every one of those call sites
died with `Call to undefined method`. The rename is reverted and
`RenameMethodRector` is skipped under tests/; it made no other change
there. The pin has to stay while the matrix tests PHP 8.2.

Verified with `phpunit-11`, the binary CI runs, rather than the newer one
in vendor/bin: 777 tests, 0 failures.
The PHP 8.2 and 8.3 jobs died before PHPUnit started:

    Castor requires PHP >= 8.4.0, but you are running PHP 8.2.31.

The matrix is the only place where the container image varies; every
other castor call runs on phpqa:8.4 and is fine. The job already installs
its own dependencies, so it now invokes the same PHPUnit binary castor
would have, without the wrapper that cannot run there.

Like the rest of this branch, the failure was invisible until now: the
test job is gated behind the analysis jobs and was skipped whenever one
of them failed.
The lowest-deps job failed 179 times with `Unknown named parameter $help`.
Rector's CommandHelpToAttributeRector had moved `->setHelp()` into
`#[AsCommand(help: ...)]`, but that parameter only exists from Symfony
7.3 and composer.json requires `^7.0|^8.0`. The rule is skipped and the
help text is back in `configure()`.

Also drops the trailing comma the attributes carried before the argument
list was reordered — `description: '...', )]` — which had been there,
unspaced, since the attributes were introduced.
@Spomky
Spomky merged commit 84b04d7 into 4.1.x Aug 26, 2026
17 checks passed
@Spomky
Spomky deleted the fix/qa-tooling-4-1-x branch August 26, 2026 09:47
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.

1 participant