This repository was archived by the owner on May 29, 2026. It is now read-only.
fix(notifier): correct OCP\IL10N namespace + setParsedMessage param name#78
Merged
Merged
Conversation
Contributor
Quality Report — ConductionNL/mydash @
|
| Check | PHP | Vue | Security | License | Tests |
|---|---|---|---|---|---|
| lint | ✅ | ||||
| phpcs | ❌ | ||||
| phpmd | ✅ | ||||
| psalm | ✅ | ||||
| phpstan | ✅ | ||||
| phpmetrics | ✅ | ||||
| eslint | ❌ | ||||
| stylelint | ✅ | ||||
| composer | ✅ | ✅ 100/100 | |||
| npm | ✅ | ✅ 342/342 | |||
| PHPUnit | ⏭️ | ||||
| Newman | ⏭️ | ||||
| Playwright | ⏭️ |
Quality workflow — 2026-05-01 07:34 UTC
Download the full PDF report from the workflow artifacts.
This was referenced May 1, 2026
rubenvdlinde
force-pushed
the
fix/notifier-il10n-namespace
branch
2 times, most recently
from
e3ecaf3 to
2ee1526
Compare
PHPStan was silently passing on CI (the composer phpstan script swallows
its exit code via `|| echo 'PHPStan not installed, skipping...'`), so two
real bugs in Notifier.php had been sitting unflagged:
1. The L10N type-hints / docblocks referenced `\OCP\L10N\IL10N`, but
that class doesn't exist — the actual interface lives at `\OCP\IL10N`
(lib/public/IL10N.php in the server). Wrong type-hint meant DI would
have failed at runtime as soon as a `dashboard_shared` notification
was rendered. 6 occurrences fixed (2 sites × {docblock, signature}
plus permissionLabel).
2. Two `setParsedMessage(subject: $x)` calls used the wrong named
argument — INotification::setParsedMessage's parameter is
`$message`, not `$subject`. Same wrong-named-arg pattern PHP's
strict mode rejects with "Unknown named parameter".
After the fix, `phpstan analyse lib/Notification/Notifier.php` is clean
(was 4 errors), and the full-repo phpstan count drops 47 → 26 because
the IL10N typo was poisoning every `$l->t(...)` callsite downstream.
phpcbf-autofixed the docblock alignment that shifted as a side effect
of shortening `\OCP\L10N\IL10N` → `\OCP\IL10N` (5 chars narrower).
rubenvdlinde
force-pushed
the
fix/notifier-il10n-namespace
branch
from
2ee1526 to
bd809fc
Compare
Contributor
Quality Report — ConductionNL/mydash @
|
| Check | PHP | Vue | Security | License | Tests |
|---|---|---|---|---|---|
| lint | ✅ | ||||
| phpcs | ❌ | ||||
| phpmd | ✅ | ||||
| psalm | ✅ | ||||
| phpstan | ✅ | ||||
| phpmetrics | ✅ | ||||
| eslint | ❌ | ||||
| stylelint | ✅ | ||||
| composer | ✅ | ✅ 100/100 | |||
| npm | ✅ | ❌ | |||
| PHPUnit | ❌ | ||||
| Newman | ❌ | ||||
| Playwright | ❌ |
Quality workflow — 2026-05-01 08:01 UTC
Download the full PDF report from the workflow artifacts.
Contributor
Quality Report — ConductionNL/mydash @
|
| Check | PHP | Vue | Security | License | Tests |
|---|---|---|---|---|---|
| lint | ✅ | ||||
| phpcs | ✅ | ||||
| phpmd | ✅ | ||||
| psalm | ✅ | ||||
| phpstan | ✅ | ||||
| phpmetrics | ✅ | ||||
| eslint | ❌ | ||||
| stylelint | ✅ | ||||
| composer | ✅ | ✅ 100/100 | |||
| npm | ✅ | ✅ 342/342 | |||
| PHPUnit | ⏭️ | ||||
| Newman | ⏭️ | ||||
| Playwright | ⏭️ |
Quality workflow — 2026-05-01 08:03 UTC
Download the full PDF report from the workflow artifacts.
rubenvdlinde
added a commit
that referenced
this pull request
May 1, 2026
PR #78 dropped phpstan from 47 → 26 by fixing the IL10N namespace typo. This batch clears the rest in four contained groups and removes the `|| echo` swallow from the composer script so future regressions actually fail CI. DashboardShareApiController (21): swap `DataResponse` → `JSONResponse` throughout the controller, matching the pattern used by every other passing controller in the app (DashboardApiController, ResponseHelper). The two classes are wire-compatible (both extend Response, same constructor signature, both serialise to JSON), but phpstan can resolve the JSONResponse generics from the existing docblocks while the bare DataResponse type was forcing it to infer T = `array<string, string>` and reject the call. AdminSettingMapper (2): the entity's setUpdatedAt() expects `?string`, but setSetting() was passing a `DateTime` object. Format to `Y-m-d H:i:s` to match the convention from DashboardFactory::create(). AdminTemplateService (1): replace `Ramsey\Uuid\Uuid::uuid4()` with the same `random_bytes`-based generator used by DashboardFactory. Ramsey UUID is not a declared composer dep, so the previous code would have crashed at runtime — this surfaced because phpstan correctly couldn't resolve the class. FileService (2): two `Node::nodeExists()` / `Node::newFile()` / `Node::putContent()` calls were made on results of `Folder::get()`, which returns the `Node` interface. Type-narrow with explicit `instanceof Folder` / `instanceof File` checks (throwing `RuntimeException` on the impossible-but-typed branch) so phpstan can verify the methods exist. Side benefit: removes a stale `@phpstan-ignore-next-line` that was silencing a real downstream error. composer.json: drop the `|| echo 'PHPStan not installed, skipping...'` from the `phpstan` script. The `||` made every phpstan run exit 0, so the CI quality job was reporting green even when phpstan failed — exactly how the IL10N typo and the Ramsey-UUID call slipped through review. Now `composer phpstan` propagates the real exit code. Verified locally: phpstan `[OK] No errors`, phpcs clean across 49 files, phpunit 354/354 (the one pre-existing DashboardFactoryTest::testCreateRespectsExplicitPermissionLevel failure is fixed by PR #77 and out of scope here).
rubenvdlinde
added a commit
that referenced
this pull request
May 1, 2026
PR #78 dropped phpstan from 47 → 26 by fixing the IL10N namespace typo. This batch clears the rest in four contained groups and removes the `|| echo` swallow from the composer script so future regressions actually fail CI. DashboardShareApiController (21): swap `DataResponse` → `JSONResponse` throughout the controller, matching the pattern used by every other passing controller in the app (DashboardApiController, ResponseHelper). The two classes are wire-compatible (both extend Response, same constructor signature, both serialise to JSON), but phpstan can resolve the JSONResponse generics from the existing docblocks while the bare DataResponse type was forcing it to infer T = `array<string, string>` and reject the call. AdminSettingMapper (2): the entity's setUpdatedAt() expects `?string`, but setSetting() was passing a `DateTime` object. Format to `Y-m-d H:i:s` to match the convention from DashboardFactory::create(). AdminTemplateService (1): replace `Ramsey\Uuid\Uuid::uuid4()` with the same `random_bytes`-based generator used by DashboardFactory. Ramsey UUID is not a declared composer dep, so the previous code would have crashed at runtime — this surfaced because phpstan correctly couldn't resolve the class. FileService (2): two `Node::nodeExists()` / `Node::newFile()` / `Node::putContent()` calls were made on results of `Folder::get()`, which returns the `Node` interface. Type-narrow with explicit `instanceof Folder` / `instanceof File` checks (throwing `RuntimeException` on the impossible-but-typed branch) so phpstan can verify the methods exist. Side benefit: removes a stale `@phpstan-ignore-next-line` that was silencing a real downstream error. composer.json: drop the `|| echo 'PHPStan not installed, skipping...'` from the `phpstan` script. The `||` made every phpstan run exit 0, so the CI quality job was reporting green even when phpstan failed — exactly how the IL10N typo and the Ramsey-UUID call slipped through review. Now `composer phpstan` propagates the real exit code. Verified locally: phpstan `[OK] No errors`, phpcs clean across 49 files, phpunit 354/354 (the one pre-existing DashboardFactoryTest::testCreateRespectsExplicitPermissionLevel failure is fixed by PR #77 and out of scope here).
3 tasks
rubenvdlinde
added a commit
that referenced
this pull request
May 3, 2026
…ame (#78) PHPStan was silently passing on CI (the composer phpstan script swallows its exit code via `|| echo 'PHPStan not installed, skipping...'`), so two real bugs in Notifier.php had been sitting unflagged: 1. The L10N type-hints / docblocks referenced `\OCP\L10N\IL10N`, but that class doesn't exist — the actual interface lives at `\OCP\IL10N` (lib/public/IL10N.php in the server). Wrong type-hint meant DI would have failed at runtime as soon as a `dashboard_shared` notification was rendered. 6 occurrences fixed (2 sites × {docblock, signature} plus permissionLabel). 2. Two `setParsedMessage(subject: $x)` calls used the wrong named argument — INotification::setParsedMessage's parameter is `$message`, not `$subject`. Same wrong-named-arg pattern PHP's strict mode rejects with "Unknown named parameter". After the fix, `phpstan analyse lib/Notification/Notifier.php` is clean (was 4 errors), and the full-repo phpstan count drops 47 → 26 because the IL10N typo was poisoning every `$l->t(...)` callsite downstream. phpcbf-autofixed the docblock alignment that shifted as a side effect of shortening `\OCP\L10N\IL10N` → `\OCP\IL10N` (5 chars narrower).
rubenvdlinde
added a commit
that referenced
this pull request
May 3, 2026
PR #78 dropped phpstan from 47 → 26 by fixing the IL10N namespace typo. This batch clears the rest in four contained groups and removes the `|| echo` swallow from the composer script so future regressions actually fail CI. DashboardShareApiController (21): swap `DataResponse` → `JSONResponse` throughout the controller, matching the pattern used by every other passing controller in the app (DashboardApiController, ResponseHelper). The two classes are wire-compatible (both extend Response, same constructor signature, both serialise to JSON), but phpstan can resolve the JSONResponse generics from the existing docblocks while the bare DataResponse type was forcing it to infer T = `array<string, string>` and reject the call. AdminSettingMapper (2): the entity's setUpdatedAt() expects `?string`, but setSetting() was passing a `DateTime` object. Format to `Y-m-d H:i:s` to match the convention from DashboardFactory::create(). AdminTemplateService (1): replace `Ramsey\Uuid\Uuid::uuid4()` with the same `random_bytes`-based generator used by DashboardFactory. Ramsey UUID is not a declared composer dep, so the previous code would have crashed at runtime — this surfaced because phpstan correctly couldn't resolve the class. FileService (2): two `Node::nodeExists()` / `Node::newFile()` / `Node::putContent()` calls were made on results of `Folder::get()`, which returns the `Node` interface. Type-narrow with explicit `instanceof Folder` / `instanceof File` checks (throwing `RuntimeException` on the impossible-but-typed branch) so phpstan can verify the methods exist. Side benefit: removes a stale `@phpstan-ignore-next-line` that was silencing a real downstream error. composer.json: drop the `|| echo 'PHPStan not installed, skipping...'` from the `phpstan` script. The `||` made every phpstan run exit 0, so the CI quality job was reporting green even when phpstan failed — exactly how the IL10N typo and the Ramsey-UUID call slipped through review. Now `composer phpstan` propagates the real exit code. Verified locally: phpstan `[OK] No errors`, phpcs clean across 49 files, phpunit 354/354 (the one pre-existing DashboardFactoryTest::testCreateRespectsExplicitPermissionLevel failure is fixed by PR #77 and out of scope here).
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 subscribe to this conversation on GitHub.
Already have an account?
Sign in.
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.
Why
`composer phpstan` ends with `|| echo 'PHPStan not installed, skipping...'` so its exit code is always 0 — failures are silently dropped on CI. Two real bugs in `Notifier.php` were therefore sitting unflagged.
What
1. `\OCP\L10N\IL10N` does not exist
The interface lives at `\OCP\IL10N` (`lib/public/IL10N.php` in the server). The deprecated `OCP\L10N\IL10N` was removed several Nextcloud versions ago, but 6 occurrences in `Notifier.php` still referenced it (3 `@param` docblocks + 3 type-hinted method signatures). At runtime, dependency injection would have failed the moment the first `dashboard_shared` or `dashboard_ownership_transferred` notification was rendered.
2. `setParsedMessage(subject: …)` is the wrong named argument
`OCP\Notification\INotification::setParsedMessage(string $message)` takes `$message`, not `$subject`. PHP strict mode would reject the call with `Unknown named parameter $subject`. Two callsites fixed (lines 178, 220).
Impact
The 17 `Call to method t() on an unknown class OCP\L10N\IL10N` errors that propagated through the file disappear because phpstan can now resolve `$l` to the real interface.
Test plan
Followups (out of scope, called out for visibility)