docs(hydra-gates): the contract opt-in must cover the analysers too - #551
Merged
Conversation
The opt-in section told adopters how to make the interface resolvable AT RUNTIME and stopped there. That is how I missed it: psalm and phpstan never run the test bootstrap — they resolve types through the composer autoload map — so the guarded require does nothing for them. If the interface appears in PRODUCTION code rather than only in tests, removing the psr-4 prefix turns every mention into an undefined class. Measured on adoption of v1.9.0: 204 errors on decidiq, 213 on stackiq, all the same class, and both went red in CI. Adds the two seams that fix it — psalm `<stubs>`, phpstan `scanDirectories` — with the reason they are stubs rather than autoload entries: the class exists at runtime because OpenRegister supplies it, and putting it back into the autoloader is exactly the defect #531 removed. Also records why phpstan's entry belongs in the APP's config and not the shared base: openregister owns the real lib/Contract/, so scanning the vendored copy there would declare the same class twice. The migration was verified against PHPUnit in both directions and the analysers were never run. Check lib/, not just tests/.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
The opt-in section added with #547 explained how to make the interface resolvable at runtime and stopped there. That omission is how the gap reached the fleet.
Psalm and PHPStan never run the test bootstrap. They resolve types through the composer autoload map, so the guarded
requireis invisible to them. If the interface appears in production code — constructor promotion, a parameter type, a::classfetch inlib/— every mention becomes an undefined class.Measured on adoption of v1.9.0:
OCA\OpenRegister\Contract\ObjectServiceInterfaceBoth went red in CI, and both are fixed by a stub (ConductionNL/decidiq#843, ConductionNL/stackiq#706) — verified 204 → 0 and 213 → 0 locally on the real v1.9.0.
Why a stub and not an autoload entry
The class exists at runtime because OpenRegister supplies it. Putting it back into the autoloader is precisely the defect #531 removed. A stub teaches the analyser the shape without re-creating the shadowing.
Why PHPStan's entry is per-app
openregister owns the real
lib/Contract/. Scanning the vendored copy in the shared base would declare the same class twice, in the one repo that must not.The migration was verified against PHPUnit in both directions and the analysers were never run. The doc now says: check
lib/, not justtests/.