Skip to content

chore(quality): migrate to PHPStan 2 — 35 findings to zero, plus a missing-manager bug - #697

Merged
rubenvdlinde merged 2 commits into
developmentfrom
chore/phpstan-2
Aug 22, 2026
Merged

chore(quality): migrate to PHPStan 2 — 35 findings to zero, plus a missing-manager bug#697
rubenvdlinde merged 2 commits into
developmentfrom
chore/phpstan-2

Conversation

@rubenvdlinde

Copy link
Copy Markdown
Contributor

Bumps phpstan/phpstan to ^2.0 and conduction/hydra-gates to ^1.8.2, and clears every finding the new major surfaces: 35 → 0.

A missing manager was never detected

ContactPersonHandler::setUserManager():

$user    = $this->_userManager->get($username);
$manager = $this->_userManager->get($managerUsername);

if ($user === null || $manager === false) { /* warn and return */ }

IUserManager::get() returns ?IUser — it signals "no such user" with null, never false. So the second half of that guard could never fire: calling setUserManager() with a manager username that does not exist skipped the warning entirely and carried on as if the manager were real. Fixed to === null.

PHPStan found it from the other end: inside the branch, $user !== null was reported as always false, because the only reachable way in was the first clause.

Dead guards (28)

Mostly in the ArchiMate import/export pair, which are near-copies of each other — so nearly every finding came in twos:

Pattern Why it could never fire
PERFORMANCE_OPTIMIZATIONS['memory_cleanup'] !== false, ['parallel_processing'] === true both are class constants set to true
$identifier !== false, $versionId !== false, $refCompId !== false, $amefKey !== false all strings; none can be false
is_array($sectionData) === false the parameter is declared array, so PHP rejects anything else at the call boundary
isset($statistics[$sectionKey]) === false the branch above pins $sectionKey to a key $statistics always has
if ($section !== 'omschrijving')"Skip summary section itself" omschrijving is assigned to $statistics on the line after the loop, so the loop never sees it
method_exists($this->archiMateService, '...Optimized') the method is declared on the class

Plus several isset() && !== null pairs and non-nullable-entity null tests.

One scoped ignore

OrganizationSyncService's if ($contactObject !== null) is provably true — the code a few lines above already dereferences $contactObject unconditionally, which is the giveaway. Left in place because the block it wraps is 243 lines: removing the if is a pure re-indentation of a quarter of the method, a large review-hostile diff for zero behaviour change. The ignore comment says so and marks it for the next real edit to that method.

Verification

phpstan 0 · phpcs clean · phpmd clean

PHPUnit is deliberately not part of this evidence. The bootstrap requires a booted Nextcloud (Class "OC_App" not found) and cannot run standalone. I confirmed this is pre-existing by stashing every change in this branch and re-running — byte-identical failure. CI runs the suite inside the container.

Conduction Release Bot added 2 commits August 22, 2026 02:09
…al bug

Bumps `phpstan/phpstan` to ^2.0 and `conduction/hydra-gates` to ^1.8.2,
and clears every finding the new major surfaces.

## A missing manager was never detected

ContactPersonHandler::setUserManager():

    $user    = $this->_userManager->get($username);
    $manager = $this->_userManager->get($managerUsername);

    if ($user === null || $manager === false) { ...warn and return... }

IUserManager::get() returns `?IUser` — it signals "no such user" with
NULL and never returns false. So the second half of that guard could
never fire: calling setUserManager() with a manager username that does
not exist skipped the warning entirely and carried on as if the manager
were real. Fixed to `=== null`.

PHPStan found it from the other end: inside that branch `$user !== null`
was reported as always FALSE, because the only reachable way in was the
first clause.

## Dead guards (28)

Mostly in the ArchiMate import/export pair, which are near-copies of each
other, so nearly every finding came in twos:

- `self::PERFORMANCE_OPTIMIZATIONS['memory_cleanup'] !== false` and
  `['parallel_processing'] === true` — both are class constants set to
  true, so neither was ever conditional.
- `$identifier !== false`, `$versionId !== false`, `$refCompId !== false`,
  `$amefKey !== false` — all strings; none can be false.
- `is_array($sectionData) === false` — the parameter is declared `array`,
  so PHP rejects anything else at the call boundary first.
- `isset($statistics[$sectionKey]) === false` — the branch above pins
  $sectionKey to a key $statistics always has.
- `if ($section !== 'omschrijving')` with the comment "Skip summary
  section itself" — `omschrijving` is assigned to $statistics on the line
  AFTER the loop, so the loop can never see it.
- `method_exists($this->archiMateService, '...Optimized')` — the method
  is declared on the class.
- Several `isset() && !== null` pairs and non-nullable-entity null tests.

## One scoped ignore

OrganizationSyncService's `if ($contactObject !== null)` is provably true
— the code a few lines above already dereferences $contactObject
unconditionally. It is left in place because the block it wraps is 243
lines: removing the `if` is a pure re-indentation of a quarter of the
method, a large review-hostile diff for zero behaviour change. The
comment says so and marks it for the next real edit to that method.

## Verification

phpstan 0, phpcs clean, phpmd clean.

PHPUnit is NOT part of this evidence: the bootstrap requires a booted
Nextcloud (`Class "OC_App" not found`) and cannot run standalone. Checked
that this is pre-existing by stashing every change in this commit and
re-running — byte-identical failure. CI runs the suite inside the
container.
CI's phpcs step runs `--warning-severity=0`, and it failed on every one
of the three PRat in this series for the same reason: comments I wrote.

Two sniffs:

- Squiz.Commenting.InlineComment.NotCapital — many of my new comments
  open with a lowercase function name ("// find() throws rather than
  ..."). Rephrased so the first word is a real capitalised word.
- Generic.Commenting.DocComment.TagsNotGrouped — the `@param-out` tags I
  added were interleaved between `@param` tags, splitting the group.
  Moved below the last `@param`. One of those inserts had also orphaned a
  continuation line off the `@param` above it; that is rejoined.

Where PHPStan genuinely needs a `/** @var */` inline doc-block (which
Squiz.Commenting.InlineComment.DocBlock rejects), the line now carries a
targeted `phpcs:ignore` naming that sniff and saying why, rather than
dropping the annotation and leaving the type wrong.

I should have caught this locally. I did run phpcs, but with
`--report=summary | tail -3`, which prints only the timing line — so I
read an empty tail as "clean" when the error count was two lines above
the cut. Re-verified here with CI's exact invocation.
@github-actions

Copy link
Copy Markdown
Contributor

Quality Report — ConductionNL/stackiq @ 0a78052

Check PHP Vue Security License Tests
lint
phpcs
phpmd
psalm
phpstan
phpmetrics
eslint
stylelint
build
check-manifest
check-vue-demi
test-l10n
format
composer ✅ 130/130
npm ✅ 703/703
app:check-code ⏭️
info.xml
REUSE
PHPUnit
Newman ⏭️
Playwright
Hydra gates

Quality workflow — 2026-08-22 00:22 UTC

Download the full PDF report from the workflow artifacts.

@github-actions

Copy link
Copy Markdown
Contributor

Quality Report — ConductionNL/stackiq @ 915bc7a

Check PHP Vue Security License Tests
lint
phpcs
phpmd
psalm
phpstan
phpmetrics
eslint
stylelint
build
check-manifest
check-vue-demi
test-l10n
format
composer ✅ 130/130
npm ✅ 703/703
app:check-code ⏭️
info.xml
REUSE
PHPUnit
Newman ⏭️
Playwright
Hydra gates

Quality workflow — 2026-08-22 00:39 UTC

Download the full PDF report from the workflow artifacts.

@rubenvdlinde
rubenvdlinde merged commit 53692a1 into development Aug 22, 2026
81 checks passed
@rubenvdlinde
rubenvdlinde deleted the chore/phpstan-2 branch August 22, 2026 00:52
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