fix(quality): clear the psalm and phpstan gates and delete the phpstan baseline - #263
Merged
Conversation
…n baseline phpstan: 15 errors -> 0. psalm: 16 errors -> 0. Both without a baseline and without widening any suppression — `phpstan-baseline.neon` is deleted and its `includes:` entry removed, and the psalm `UndefinedMethod` suppression block for ActionAuthService is deleted too. Root cause of 22 of the 31 findings: Scholiq's engine-backed classes extend OpenRegister AppHost base classes that are absent from the analysis path, so Repair\InitializeActions, Sections\SettingsSection and Settings\AdminSettings all read as "extends unknown class" — which PHPStan explicitly refuses to let ignoreErrors suppress — and every #[AuthorizedAdminSetting(AdminSettings::class)] attribute additionally failed its class-string<IDelegatedSettings> check. Fixed by adding an analysis-only stub (tests/stubs pattern already shipping in doriath) declaring GenericSettingsSection, GenericAdminSettings, GenericInitializeActions and Bootstrap with signatures mirroring the real openregister classes, and by registering the existing TenantKeyService, TalkLinkService and GenericActionAuthService stubs in psalm.xml <stubs>. That gives Psalm real type information instead of suppressing the symptom, which is why the ActionAuthService UndefinedMethod block could be removed. Remaining findings were dead code, now removed rather than baselined: - 4 redundant `instanceof DOMElement === false` guards that both analysers agree can never be true (getElementsByTagName only yields element nodes) - a redundant `is_string()` guard on end() of a non-empty explode() result - a redundant `?? []` on a non-nullable getContext() - an unused $tenantId parameter on resolveProgrammeIds() - an unused return value on createLessonForMaterial() QtiImportService gains a firstElement() helper so the DOMNode -> DOMElement narrowing lives behind a declared return type; this satisfies both analysers without an inline annotation, which the house style forbids anyway. test:unit/test:all now pass --no-coverage, matching doriath. Coverage still has its own test:coverage script; without this the suite exits 1 on any runner with no xdebug/pcov driver even though all 887 tests pass. phpcs 0, psalm 0, phpstan 0, 887 tests green.
Contributor
Quality Report — ConductionNL/scholiq @
|
| Check | PHP | Vue | Security | License | Tests |
|---|---|---|---|---|---|
| lint | ✅ | ||||
| phpcs | ✅ | ||||
| phpmd | ❌ | ||||
| psalm | ✅ | ||||
| phpstan | ✅ | ||||
| phpmetrics | ✅ | ||||
| eslint | ✅ | ||||
| stylelint | ✅ | ||||
| build | ✅ | ||||
| composer | ✅ | ✅ 100/100 | |||
| npm | ✅ | ✅ 750/750 | |||
| PHPUnit | ⏭️ | ||||
| Newman | ⏭️ | ||||
| Playwright | ⏭️ |
Quality workflow — 2026-08-03 22:42 UTC
Download the full PDF report from the workflow artifacts.
rubenvdlinde
added a commit
that referenced
this pull request
Aug 4, 2026
…ot cause, three production bugs (#264) * fix(tests): make the OpenRegister stubs a faithful mirror, not a fiction scholiq's PHPUnit legs had never run. The job was gated behind the static-analysis jobs, so on `development` it sat `skipped` with an unexpanded matrix name — literally quality / PHPUnit (PHP ${{ matrix.php-version }}, NC ${{ matrix.nextcloud-ref }}) which reads like a configured-but-idle leg rather than a gate that has never once produced a verdict. #263 cleared psalm and phpstan, the leg expanded into two real jobs, and both went red: 887 tests, 231 errors, 12 failures. That is a discovery, not a regression. The root cause is a single one: `OCA\OpenRegister\*` resolves to two different implementations depending on where the suite runs. Standalone it resolves to `tests/Stubs/` via the PSR-4 mapping in tests/bootstrap.php. In CI the app is checked out into a real Nextcloud server tree next to openregister@development, the app is enabled, and Nextcloud's autoloader wins — so every `createMock()` is built from the REAL class. The stubs did not match the real class. `ObjectService::find()` was declared `find(string $id, string $register, string $schema): mixed`; the real one is `find($id, $_extend, $files, $register, $schema, ...) : ?ObjectEntity`. `saveObject()` was declared `saveObject(string $register, string $schema, array $object)`; the real one takes the payload FIRST. `ObjectEntity` declared `getRegister()`, `getSchema()` and `getUuid()` as concrete methods; on the real entity those are `__call` magic from `OCP\AppFramework\Db\Entity`, which PHPUnit cannot configure on a mock. The stub's own docblock asserted that OpenRegister "accepts two call signatures". It does not, and never did — the prose was written to match the stub, and the phpstan baseline hid the contradiction. So 887 tests passed standalone against an API that does not exist, and 231 of them errored the moment the real class was on the autoloader. This commit fixes the harness rather than the symptom: - tests/Stubs/Service/ObjectService.php and tests/Stubs/Db/ObjectEntity.php are now signature-for-signature mirrors of the real classes, including the `@method` block that makes the magic accessors visible to PHPStan. ObjectEntity extends the same OCP base class, so it has the same magic-accessor behaviour, and is concrete so tests can build instances. - tests/Stubs/Db/{Register,Schema}.php added so the mirrored signatures can name the same union types the real ones name. - tests/Support/OrEntityFactory builds real ObjectEntity instances, which is the only way to give a test an entity with a register/schema/payload now that the magic getters correctly cannot be mocked. Required from the bootstraps rather than composer autoload-dev, because a dev-built vendor/ bakes autoload-dev into the runtime classmap and can shadow real app classes instance-wide (openregister#2036). - tests/Unit/Contract/OpenRegisterContractTest asserts the contract against whichever class actually resolved. It passes against the stub mirror AND against openregister@development, so the next drift is one named red test instead of a silently dead suite. Positive control: declaring getRegister() concretely on the mirror turns it red naming that method. Two production defects fall out of the corrected stub, both of which would have fatalled against the real service: - CoursePackageImportService called saveObject() positionally at 12 sites and QtiImportService at one, passing the register slug where the payload belongs. Now named arguments. - QtiImportService then null-checked and is_array()-checked a return value that is a non-nullable ObjectEntity. Correcting the stub also turns PHPStan into a real detector for this class of drift: it went from 0 errors to 108, of which these 29 were the call-shape bugs. The phpstan gate had been green because it was resolving lib/ against a stub that lib/ was written to match — circular, and green for the same reason the suite was. Harness parity, measured: before, standalone reported 0 errors while CI reported 231. After, standalone reports 227 and a local run against openregister@development reports 229. The suite now fails in the same place it fails on the runners. * fix(tests): repair the 231 PHPUnit errors and the three production bugs behind them Second half of the scholiq PHPUnit repair. The first commit made the OpenRegister stubs a faithful mirror; this one fixes the 59 test files that had been written against the fiction, and the production defects that fixing the stubs made visible. Result: 887 tests / 231 errors / 12 failures -> 895 tests / 0 / 0. Standalone and a local run against openregister@development now report byte-identical results, so the suite fails in the same place it fails on the runners. Three production defects, all of which the old stubs hid: 1. `method_exists()` cannot see a `__call` accessor. ObjectEntity's getUuid()/getRegister()/getSchema() come from OCP\AppFramework\Db\Entity::__call, so `method_exists($entity, 'getSchema')` is FALSE on a real entity — measured against openregister@development, not assumed. `ListenerSchemaResolver::schemaSlug()` and `registerSlug()` and `isOwnRegister()` all guarded on exactly that. They returned '' / false for every genuine ObjectEntity, and as their own docblock says, "every caller treats '' as 'not my object' and returns early" — so every scholiq OpenRegister listener silently did nothing in production. It failed closed and silently, which is why nothing ever reported it. `CoursePackageImportService::extractUuid()` had the same probe, so it returned null for every save: createCourse() produced no course id and the QTI item-bank import was skipped entirely. The old tests/Stubs entity declared those accessors concretely, so `method_exists()` was true there and only there. The unit suite could not have caught this. Fixed to `is_callable()`, which is true for a magic accessor. jsonSerialize() IS declared, so `method_exists()` remains correct for that one — the sweep distinguishes the two. 2. Two more stub fictions, found by fixing the first two. `TransitionEngine::transition()` was declared `: void`; the real one returns ObjectEntity. Tests whose transition callback returned nothing were green standalone and threw in CI — and RejectionMappingHandler and SupportRequestSubmitHandler wrap the call in `catch (\Throwable)`, so their tests passed in CI while swallowing that TypeError. Green for the wrong reason is worse than red. `ObjectCreatedEvent` declared getRegister()/getSchema(); the real event carries only getObject(). Its sibling ObjectTransitionedEvent genuinely does expose them — the two differ, which is precisely why the surface has to be mirrored rather than assumed. 3. PHPStan was measuring nothing for this API. `scanDirectories` includes tests/Stubs, so it checked lib/ against a stub lib/ had been written to match. With the mirror corrected it went 0 -> 108 errors: 29 were the positional saveObject() call shapes (fixed in the previous commit) and 63 were branches handling an array or a null that the real return types make unreachable. Those branches are now removed, so the gate measures the real contract. PHPStan and Psalm are both clean. OpenRegisterContractTest is extended to cover all of the above — signatures, the event-surface difference, and an explicit assertion that `method_exists()` does NOT see the magic accessors while `is_callable()` does. Positive control re-run: declaring getRegister() concretely on the mirror turns it red naming that method; reverting restores green. Test-side changes are re-plumbing, not weakening: no test was skipped, deleted or had an assertion loosened. Four expectations were changed on purpose, each because they encoded a state the real API cannot reach: - RejectionResubmitGuardTest and SupportRequestSubmitHandlerTest simulated a save failure by returning null from saveObject(), which is non-nullable. Both now exercise the reachable failure — a saved entity with no usable id — which is what the guards actually check. - ExemptionGrantHandlerTest's helper dropped its `null` branch for the same reason. - SessionConflictListenerTest compared against a bare payload array; real jsonSerialize() returns the payload plus an `@self` block, so exact equality could never hold. Now asserts the same intent via a callback. One genuine flake fixed: AssessmentDrawResolverTest's shuffle test used 15 trials over a two-choice fixture, so P(false green) is 2*(1/2)^15 — measured at 7 in 200,000 simulated runs, and it fired once during verification. Raised to 40 trials, which is strictly harder (every added trial must also satisfy the per-trial invariant), not looser. Known remaining, deliberately not fixed here: RejectionResubmitGuard has no try/catch around createResubmissionJob(), so a real save failure now propagates out of check() rather than blocking the transition. That is a behaviour question, not a test-harness one. * fix(import): parse XML with loadXML(), not DOMDocument::load() Third and last root cause behind the red PHPUnit legs — and the one with the widest production blast radius. Nextcloud's `OC::init()` installs, in server `lib/base.php`: libxml_set_external_entity_loader(static fn () => null); to block XXE. libxml routes the PRIMARY document of `DOMDocument::load($path)` through that same loader, so with it installed `load()` always returns false. Measured, not assumed: baseline (no entity loader) load()=true loadXML(file_get_contents)=true after NC-style entity loader load()=false loadXML(file_get_contents)=true Every scholiq XML parser used `load($path)` — 8 call sites across CommonCartridgeParser, MoodleBackupParser, MoodleQuizQuestionMapper, QtiImportService and CoursePackageImportService. Inside a real Nextcloud they all fail, which means Common Cartridge import, Moodle backup import, QTI import and course-package import have never worked on an actual instance. The unit suite could not see it because it ran without Nextcloud bootstrapped. This accounted for the last 10 red tests: 4 errors reporting "Could not parse imsmanifest.xml as XML" plus 6 downstream failures in the importers that consume those parsers. All 10 were already failing on `development` with the identical message — they are pre-existing, and they only became visible when the PHPUnit job first ran. All 8 sites now read the file and call `loadXML()`, which takes a string and is unaffected by the entity loader while still resolving no external entities — so the XXE hardening is preserved, not worked around. tests/Unit/Contract/NextcloudXmlLoaderContractTest installs the same loader and asserts both parsers still work, plus the premise itself (that `load($path)` fails and `loadXML()` does not) so the workaround can be retired if Nextcloud or libxml ever changes. Positive control: reverting CommonCartridgeParser to `load($path)` reproduces the exact CI error locally — `RuntimeException: Could not parse imsmanifest.xml ... as XML` — and restoring it (verified byte-identical) restores green. The local reproduction harness now installs the entity loader too, so it models both halves of the CI environment: the real OpenRegister classes AND Nextcloud's libxml hardening. Suite: 898 tests, 4121 assertions, 0 errors, 0 failures — byte-identical between the standalone and openregister@development harnesses. PHPStan 0, Psalm clean, PHPCS 0 errors. * test: TEMPORARY positive-control probe — proves the PHPUnit leg can fail Deliberately broken assertion in LeaderboardControllerTest. A green suite is only evidence about the suite once you have shown it can go red, and this leg had never produced a verdict at all before this branch. Reverted in the next commit. * test: revert the positive-control probe The probe did its job: both PHPUnit legs went red naming the test — 1) OCA\Scholiq\Tests\Unit\Controller\LeaderboardControllerTest::testNoLeaderboardRowRefused CI positive control probe Failed asserting that 404 is identical to 418. so the green on this branch is a measured green, not an absent check. Tree is byte-identical to dc2ab9c (verified with git diff), which is the commit whose legs reported 898 tests / 4121 assertions / OK.
This was referenced Aug 4, 2026
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
2 participants
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.
What
Drives two of scholiq's four red
composer check:strictgates to zero without a baseline and without adding any suppression — in fact two existing suppressions are removed.Root cause
22 of the 31 psalm/phpstan findings came from one cause: scholiq's engine-backed classes extend OpenRegister AppHost base classes that are absent from the analysis path.
Repair\InitializeActions,Sections\SettingsSectionandSettings\AdminSettingsall read as "extends unknown class" — which PHPStan explicitly refuses to letignoreErrorssuppress — and every#[AuthorizedAdminSetting(AdminSettings::class)]attribute additionally failed itsclass-string<IDelegatedSettings>check.Fixed by adding an analysis-only stub (the
tests/stubspattern already shipping in doriath, whose psalm/phpstan gates are green for exactly this reason) declaringGenericSettingsSection,GenericAdminSettings,GenericInitializeActionsandBootstrapwith signatures mirroring the real openregister classes, and by registering the already-presentTenantKeyService,TalkLinkServiceandGenericActionAuthServicestubs inpsalm.xml <stubs>.That gives Psalm real type information instead of suppressing the symptom — which is why the
UndefinedMethodsuppression block forActionAuthService::requireAction/getMatrix/setMatrixcould be deleted.Suppressions removed, not added
phpstan-baseline.neondeleted along with itsincludes:entry. Its 5 tracked-debt findings are fixed, not re-muted.UndefinedMethodblock forActionAuthServicedeleted.--generate-baseline, no--set-baseline, notreatPhpDocTypesAsCertain: false, no newignoreErrorspatterns, noexcludePathswidening.Dead code removed rather than baselined
instanceof DOMElement === falseguards both analysers agree can never be true (getElementsByTagName()only ever yields element nodes)is_string()guard onend()of a non-emptyexplode()result?? []on a non-nullablegetContext()$tenantIdparameter onresolveProgrammeIds()createLessonForMaterial()QtiImportServicegains afirstElement()helper so theDOMNode -> DOMElementnarrowing lives behind a declared return type. This satisfies both analysers without an inline annotation — which the house style forbids anyway (Squiz.Commentingrejects inline doc blocks, and there are zeroif (!inlib/).test:all
test:unit/test:allnow pass--no-coverage, matching doriath. Coverage keeps its owntest:coveragescript. Without this the suite exits 1 on any runner with no xdebug/pcov driver even though all 887 tests pass — a failure that has nothing to do with test health.Verification
PHP 8.3 container, fresh
composer install. Positive control run before declaring green: an injected$node->thisMethodDoesNotExistPositiveControl()produced psalm exit 2 and phpstan exit 1 both naming the injected line; an injected$x=1;plus a lowercase comment produced phpcs exit 2 naming lines 443-444. Reverting restored exit 0 on all three, verified bygit status --porcelainbeing empty.