Skip to content

fix(psalm): fix the real type problems, narrow the suppressions, replace the deprecated OCP calls - #1283

Merged
rubenvdlinde merged 4 commits into
developmentfrom
chore/w1a-psalm-types
Sep 11, 2026
Merged

fix(psalm): fix the real type problems, narrow the suppressions, replace the deprecated OCP calls#1283
rubenvdlinde merged 4 commits into
developmentfrom
chore/w1a-psalm-types

Conversation

@rubenvdlinde

Copy link
Copy Markdown
Contributor

Second W1a static-analysis PR for #1277, on top of #1278.

Psalm

Lifting the 33 globally suppressed issue types produced 389 errors: about 296 unused-code reports caused by DI wiring, the rest real. After this PR the same measurement is 327, and everything in it is either DI noise or a documented false positive. Psalm exits 0 and reports no deprecations at all.

Fixed:

  • ($a ?? null) ?? $default written on an expression, where ?? cannot do its isset job: TranscriptRetentionJob (3), MigrateConsultationsToOneSchema, MigrateQuestionsToAgendaItems. In the last one the guard was load-bearing: a question row naming neither a question nor a request number raised Undefined array key "requestNumber" on every upgrade. testAnUntitledRowSavesWithoutAWarning fails on the old code with exactly that string.
  • Return types that did not describe the return: DecisionIntegrationService::registerOutcomeCallback (returns decisionId and callbackUrl too), SeedProfileService::listProfiles (always returns icon), DecidiqDashboardWidget::getWidgetButtons, DecidiqSearchProvider::getOrder.
  • ParticipantNotifier::resolveUid now checks the container-resolved user manager is an IUserManager before calling it, so psalm can type the two calls it could not.
  • 7 docblocks whose {...} key lists psalm parsed as part of the type, which made it drop the whole docblock.
  • 35 casts and one array_values() that psalm proves redundant from native types, plus one always-true condition in MinutesDraftComposer.
  • The three "reserved for future" parameters: LiveDecisionService::recordDecision($actorId), MinutesService::notifyApproversOnSubmit($actorId) (and MinutesWorkflowService::submitForApproval, whose only use of it was passing it on), OpenCatalogiPublisher::retract($catalogId). Callers and tests follow.

psalm.xml now suppresses 2 issue types globally instead of 33: UnusedClass and PossiblyUnusedMethod, which are what DI wiring looks like to a static analyser (73 and 223 reports). Everything else is scoped to the file that needs it with the reason written next to it, and each scope was probed by removing it and re-running. UndefinedClass went from 74 listed classes to the 19 psalm actually reports.

Deprecated OCP calls

All 20 from the audit, plus IUserManager::search, which only became visible once the user manager was typed. Each replacement is the one the OCP deprecation note names, and each was checked against the server implementation:

was now evidence
IAppManager::isInstalled (6) isEnabledForAnyone AppManager::isInstalled() is return $this->isEnabledForAnyone($appId)
IConfig::get/set/deleteUserValue, getUserKeys IUserConfig AllConfig delegates to exactly these
IConfig::getAppValue IAppConfig::getValueString same store
IPreparedStatement::fetch/fetchAll (4) the IResult that execute() returns what the deprecation note says
IDBConnection::getDatabasePlatform (2) new SqlIdentifierQuoter on getDatabaseProvider() compared with Doctrine's own quoteSingleIdentifier() on the five platforms Nextcloud ships: 30 of 30 identical
IServerContainer in boot() IBootContext::injectFn() IAppContainer is deprecated too, so the container is not named at all
IUserManager::search searchDisplayName that step is documented as a display-name search

No app id string was changed. The fleet rename makes those duck-typed lookups, so only the API moved.

New tests: PreferencesControllerTest (6), SqlIdentifierQuoterTest (8), the Health degraded-version test, and the untitled-row warning test. Every one was mutation-checked: broken, watched the right assertion redden, restored, diff against a backup.

Left deliberately: IAppManager::getInstalledApps in DemoDataService and SeedProfileService. The replacement is getEnabledApps(), but hydra gate 66 (openregister-dependency-shape) recognises only isInstalled('openregister'), isEnabledForUser, getInstalledApps( and class_exists as an availability guard, so the non-deprecated spelling turns a guarded lookup into 2 FAILs. That is a gate gap for the whole fleet rather than something to fix per app: gate 66's _AVAILABILITY_RE needs isEnabledForAnyone('openregister') and getEnabledApps(.

Verification (local, on this branch, after merging development)

  • composer check:strict exit 0: lint, phpcs (0 warnings), phpmd, psalm, phpstan, 1507 tests.
  • ./vendor/bin/hydra-gates exit 0: 87 of 87 applicable gates ran and passed.
  • Psalm with every suppression lifted: 389 errors before, 327 after, 0 of them a type or docblock problem.
  • Psalm with --show-info: 0 deprecation reports.

Refs #1277

🤖 Generated with Claude Code

Lifting the 33 globally suppressed psalm issue types produced 389 errors.
About 296 are unused-code reports from DI wiring; the rest were real.

Fixed:
- `($a ?? null) ?? $d` on expressions (TranscriptRetentionJob x3,
  MigrateConsultationsToOneSchema, MigrateQuestionsToAgendaItems). In the
  last one the outer `??` guarded nothing, so a row with neither a question
  nor a request number raised "Undefined array key" on every upgrade. New
  test testAnUntitledRowSavesWithoutAWarning fails on the old code.
- Wrong return docblocks: DecisionIntegrationService::registerOutcomeCallback
  (decisionId, callbackUrl), SeedProfileService::listProfiles (icon),
  DecidiqDashboardWidget, DecidiqSearchProvider.
- ParticipantNotifier: guard the container-resolved IUserManager.
- 7 docblocks whose `{...}` descriptions psalm read as types.
- 32 casts and 1 array_values() psalm proves redundant from native types,
  and one always-true condition in MinutesDraftComposer.
- Removed the three "reserved for future" parameters: recordDecision
  $actorId, notifyApproversOnSubmit $actorId (and submitForApproval, which
  only passed it on), OpenCatalogiPublisher::retract $catalogId.

psalm.xml: 33 global suppressions down to 2 (UnusedClass,
PossiblyUnusedMethod, both DI). Everything else is scoped to the file or
class that needs it, each with its reason. UndefinedClass narrowed from 74
referenced classes to the 19 psalm reports.

Refs #1277
All 20 deprecated calls from the #1277 audit, plus IUserManager::search,
which psalm could only see once ParticipantNotifier typed its user manager.
Each replacement is the one the OCP deprecation note names, and each was
checked against the server implementation for the same behaviour.

- IAppManager::isInstalled (6): isEnabledForAnyone. AppManager::isInstalled
  is literally `return $this->isEnabledForAnyone($appId)`. App id strings
  unchanged.
- IConfig user values (PreferencesController, MigrateUserPreferences):
  IUserConfig getValueString / setValueString / deleteUserConfig / getKeys.
  New PreferencesControllerTest (6 tests).
- IConfig::getAppValue (HealthController degraded body): IAppConfig
  getValueString. New test for the degraded version.
- IPreparedStatement::fetch/fetchAll (4): read from the IResult execute()
  returns, then close the cursor.
- IDBConnection::getDatabasePlatform (2): new pure SqlIdentifierQuoter keyed
  on getDatabaseProvider(). Compared against the Doctrine platforms Nextcloud
  ships (MySQL80, MariaDB, PostgreSQL, SQLite, Oracle): 30 of 30 identical.
- IServerContainer in Application::boot: IBootContext::injectFn().
- IUserManager::search: searchDisplayName, which is what the docblock says
  that step does.

Left: IAppManager::getInstalledApps in DemoDataService and SeedProfileService.
Its replacement getEnabledApps() is not recognised by hydra gate 66 as an
OpenRegister availability guard, so switching fails the gate.

Refs #1277
@github-actions

Copy link
Copy Markdown
Contributor

Quality Report — ConductionNL/decidiq @ 4efabb7

Check PHP Vue Security License Tests
lint
phpcs
phpmd
psalm
phpstan
phpmetrics
eslint
stylelint
build
check-manifest
check-nav-ceiling
test-l10n
format
check-l10n-js
check-schema-l10n
composer ✅ 104/104
npm ✅ 545/545
app:check-code ⏭️
info.xml
REUSE
PHPUnit
Newman
Playwright
Hydra gates

Quality workflow — 2026-09-11 07:46 UTC

Download the full PDF report from the workflow artifacts.

@rubenvdlinde
rubenvdlinde merged commit 9c87c9c into development Sep 11, 2026
49 checks passed
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