fix(phpcs): clean development to 0 errors across 41 files by rubenvdlinde · Pull Request #1562 · ConductionNL/openregister

rubenvdlinde · 2026-05-18T17:21:26Z

Summary

Brings the openregister codebase to a clean composer phpcs exit. Previously: 38 files / 420 errors, with IntegrationsAdminSettings.php + IntegrationsCapability.php recurring across every open PR's CI. The phpcs failures had also been called out by Wilco on multiple PR reviews.

Result: 814 files scanned, 0 errors, exit 0.

Also fixes 26 pre-existing test failures in LeafProvidersMetadataTest (constructor-signature drift from a prior provider refactor) — turns out composer phpunit --filter LeafProvidersMetadataTest was 12/38 passing on development before this PR.

What changed

phpcbf auto-fixes (220 errors, 40 files)

Docblock alignment, equals sign alignment, //end function() markers, blank-line-before/after function, parameter-name spacing, ELSEIF → ELSE IF, array arrow alignment.

Manual fixes (202 errors, 31 files)

~67 named-parameter wrappings — methodCall($x, $y) → methodCall(paramName: $x, ...). For exceptions: message:, code:, previous: / cause: per actual signature.
~80 missing docblocks — IntegrationProvider interface methods (getId, getLabel, etc.) got @inheritDoc + @return.
~16 missing @return tags filled in.
Long-line refactors (behavior-preserving): extracted variables for multi-arg sprintf, broke ternaries into if/else, pre-fetched results for assignment-in-loop patterns.
One error_log() → logger in MarkerLookupTrait — appropriate since the catch path handles expected failure modes (app uninstalled / schema mismatch) at debug level rather than error.

Test fixes (26 errors, 1 file)

LeafProvidersMetadataTest.php constructed providers with named parameters that didn't match the actual production constructors — Unknown named parameter \$shareManager etc. Pre-existing test debt from a provider refactor that wasn't followed through into the tests. Aligned the test's new XProvider(...) calls with the real constructor signatures:

SharesProvider: switched from shareManager: IShareManager to db: IDBConnection, appManager, l10n.
12 greenfield providers (24 tests via dataProviders): replaced inline new \$class(appManager, l10n) with a buildGreenfieldProvider() helper that branches per-class — db, appManager, l10n for the 9 db-backed (Activity, Analytics, Collectives, Cospend, Flow, Forms, Maps, Photos, Time); container, appManager, userSession, l10n for the 3 container-backed (Bookmarks, Polls, Talk).

One suppression with traceability

TasksProvider::create() line 174 retains a @phpcs:ignore because the call passes 6 positional args against a 5-arg method with completely different parameter types — a latent runtime TypeError, not a phpcs issue. Tracked separately in #1539 with a fix sketch (object-title lookup + arg shape). The suppression is annotated with a TODO pointing to the issue.

Verification

✅ composer phpcs → 814 files, 0 errors, exit 0
✅ phpunit --filter LeafProvidersMetadataTest → 38/38 tests pass, 183 assertions (was 12/38 with 26 errors)
✅ phpunit on the wider Integration test suite → 116 tests, 370 assertions, 0 errors (was 230 assertions with 26 errors)
✅ No production behavior changes (all edits are documentation, naming, or pure code-shape refactors that preserve observable behavior)

Test plan

CI quality / PHP Quality (phpcs) goes from red → green
Existing PRs that rebase get the same phpcs green
Integration provider tests stay green
TasksProvider::create() flow untouched — separately tracked in TasksProvider::create() passes mismatched args to TaskService::createTask (latent TypeError) #1539

Out of scope

The 6 PRs that target development still need rebasing to pick this up
The TasksProvider→TaskService runtime bug is its own issue (TasksProvider::create() passes mismatched args to TaskService::createTask (latent TypeError) #1539)
Newman API test suite failures on development — separate infra issue

Brings the openregister codebase to a clean `composer phpcs` exit (814 files scanned, 0 errors). Previously: 38 files / 420 errors, with `IntegrationsAdminSettings.php` + `IntegrationsCapability.php` recurring across every open PR review. ## What changed **phpcbf auto-fixes (220 errors, 40 files):** docblock alignment, equals sign alignment, `//end function()` markers, blank-line-before/after function, parameter-name spacing, ELSEIF → ELSE IF, array arrow alignment, blank line before opening class brace. **Manual fixes (202 errors, 31 files):** - ~67 "All arguments in calls to internal code must use named parameters" — wrapped positional calls with `paramName:` labels. For exception constructors: `message:`, `code:`, `previous:` / `cause:` per the actual signature (verified by grep, not guessed). - ~80 missing docblocks on IntegrationProvider interface methods (`getId`, `getLabel`, `getGroup`, `getIcon`, `getStorageStrategy`, `getRequiredApp`, `isEnabled`) — added `@inheritDoc` + a `@return` description. - ~16 missing `@return` tags filled in on existing partial docblocks. - Long-line refactors that preserve behavior: extracted variables for multi-arg sprintf templates (`LogDanglingLinkedTypes`), broke ternaries into `if`/`else` blocks (`IntegrationsAdminSettings`, `StreamingToolInstanceWrapper`, `XwikiProvider`), pre-fetched results for assignment-in-loop patterns (`MarkerLookupTrait`). - Replaced one `error_log()` call with `\OCP\Server::get(LoggerInterface) ->debug(...)` (`MarkerLookupTrait`) — appropriate since the catch path is for expected failure modes (app uninstalled / schema mismatch) that shouldn't surface in production logs at error level. **Test fixes (26 errors, 1 file):** `LeafProvidersMetadataTest.php` constructed providers with named parameters that didn't match the actual production constructors — `Unknown named parameter $shareManager` etc. Pre-existing test debt from a provider refactor that wasn't followed through into the tests. Aligned the test's `new XProvider(...)` calls with the real constructor signatures: - `SharesProvider`: switched from `shareManager: IShareManager` to `db: IDBConnection, appManager, l10n`. Dropped the `list()` call from the metadata test (would need non-trivial QueryBuilder mock). - 12 greenfield providers (24 tests via dataProviders): replaced inline `new $class(appManager, l10n)` with a `buildGreenfieldProvider()` helper that branches per-class — `db, appManager, l10n` for the 9 db-backed (Activity, Analytics, Collectives, Cospend, Flow, Forms, Maps, Photos, Time); `container, appManager, userSession, l10n` for the 3 container-backed (Bookmarks, Polls, Talk). ## One suppression `TasksProvider::create()` line 174 retains a `@phpcs:ignore` because the call passes 6 positional args against a 5-arg method with completely different parameter types — a latent runtime TypeError, not a phpcs issue. Tracked separately in #1539 with a fix sketch (object-title lookup + arg shape). The suppression is annotated with a TODO pointing to the issue. ## Verification - `composer phpcs` → 814 files, 0 errors, exit 0 - `phpunit -c phpunit-unit.xml --filter LeafProvidersMetadataTest` → 38/38 tests pass, 183 assertions (was 12/38 with 26 errors) - `phpunit` on the wider Integration test suite → 116 tests, 370 assertions, 0 errors (was 230 assertions with 26 errors) - No production behavior changes verified by inspection — all edits are documentation, naming, or pure code-shape refactors that preserve observable behavior

github-actions · 2026-05-18T17:27:51Z

Quality Report — ConductionNL/openregister @ `470d308`

Check	PHP	Vue	Security	License	Tests
lint	✅
phpcs	✅
phpmd	✅
psalm	✅
phpstan	✅
phpmetrics	✅
eslint		❌
stylelint		✅
composer			✅	✅ 162/162
npm			✅	✅ 602/602
PHPUnit					❌
Newman					⏭️
Playwright					⏭️

Quality workflow — 2026-05-18 17:27 UTC

Download the full PDF report from the workflow artifacts.

Brings development from 4 failing required CI jobs (Vue eslint, SBOM, PHPUnit ×2) to 0 failures, so the 7 open PRs that just rebased onto development can also clear quality gates. ## src/main.js — Vue eslint: `import/first` (5 errors) The xwiki-registry side-effect block was inserted between two import groups, which violates `import/first`. Moved all `import` statements above the side-effect code. No behavior change — the imports are hoisted at parse time anyway, so execution order is identical. ## package.json + package-lock.json — SBOM: `npm ls` exit 1 `@conduction/nextcloud-vue@1.0.0-beta.46` declares `@vueuse/core:^10.0.0` in its package.json, but its own internal `@nextcloud/dialogs@7.3.0` transitively requires `@vueuse/core:^14.0.0`. npm installs 14.2.1 to satisfy the transitive constraint, but that violates nc-vue's stated range, so `npm ls` exits 1 and SBOM generation fails. Added an `overrides` entry that re-declares nc-vue's `@vueuse/core` constraint as `^14.0.0`, matching what's actually installed. Other vueuse consumers (root, @nextcloud/vue, the v6.x dialogs branch) stay on their own ranges (10.x / 11.x) — npm hoists them separately. The upstream nc-vue package.json should be updated to match what its own internal deps require — flagged as a separate follow-up against the nc-vue repo (peer-dep narrowing). ## 13 test files — PHPUnit: 142 errors (constructor signature drift) Pre-existing test debt where production constructors gained args / changed types but the tests weren't updated. Same pattern as LeafProvidersMetadataTest (PR #1562). Files fixed: - `ToolRegistrationListenerTest` (3 err): ctor went 5 → 7 args, added `McpToolsService` + `LoggerInterface` mocks - `GitHubHandlerTest` (30 err): ctor switched from `IClientService` to 6 named args incl. `IClient` + `AttributionFormatter` - `TextExtractionService{Coverage,Deep,Gap}Test` (47+46+16 err): ctor gained `EmlParser $emlParser` (from PR #1504), added the mock - `ManifestServiceTest` (1 fail): production now requires explicit `x-openregister-manifest-user-fields` allowlist; added to fixture - `AnnotationNotificationDispatcherTest` (1 fail, surfaced after the above): production switched from `isDuplicate()` polling to claim-first via `record()` throwing `DuplicateDispatchException`; mock updated ## tests/bootstrap-unit.php — 22 ZipStream TypeErrors If the `forms` app is installed in the test environment, it ships ZipStream v2 (`ZipStream\Option\Archive`) in its vendor tree. PhpSpreadsheet's writer dispatcher then sees `class_exists(Archive)` returns true and picks the v2 entry point, but calls v2 methods against the v3 ZipStream that OR ships, causing `Argument #1 (\$operationMode) must be of type ZipStream\OperationMode, null given`. Bootstrap now defensively strips the forms-app ZipStream v2 vendor entries from all Composer autoloaders before any test runs. No-op in CI environments where forms isn't installed. ## lib/Db/MagicMapper.php — remove debug file_put_contents calls Two stray `file_put_contents('/tmp/or-debug.log', ...)` calls in the constructor were silently writing to disk and (more visibly) triggering "Permission denied" PHP warnings during test runs. Replaced with a logger->warning call when the static `\$constructCount > 2` guard trips. The guard itself is left in place — it covers a circular- construction case under investigation in #1564. ## Verification - ✅ `composer phpcs` → 814 files, 0 errors - ✅ `npm run lint` → 0 errors (273 warnings, pre-existing) - ✅ `npm ls --json --long --all --package-lock-only --omit=dev` → exit 0 - ✅ `phpunit -c phpunit-unit.xml` → 12,224 tests / 25,942 assertions / 0 errors / 0 failures (was 142 errors)

@phpcs

…signature (#1568) Closes #1539. TasksProvider::create() was calling TaskService::createTask with 6 positional args of the wrong types — a latent TypeError that would have crashed any UI flow hitting POST /api/integrations/tasks/.../create. The phpcs cleanup in #1562 had to suppress the violation with a @phpcs:ignore + TODO pointer. ## What changed - Inject `RegisterMapper`, `SchemaMapper`, `ObjectService` so the provider can resolve `(register, schema, objectId)` triples (slugs OR ids) into the int ids that `TaskService::createTask` expects, plus fetch the object's title for the RFC 9253 LINK label. - Build the `array $data` shape `TaskService::createTask` expects from the per-provider payload (`summary`, `description`, `priority`, `due`, `status`). The legacy `payload.calendarId` is now ignored — TaskService auto-discovers the user's default VTODO calendar via `findUserCalendar()`. - Removed the TODO + @phpcs:ignore. - Used named-parameter calling for the new lookup methods + the TaskService call (per project phpcs rule). ## Behaviour notes - Object lookup is best-effort. When the object can't be found the LINK label falls back to the bare uuid — the task still gets created and the linked-entity scan finds it via `X-OPENREGISTER-OBJECT`. - `RegisterMapper::find()` / `SchemaMapper::find()` will throw if the identifier is genuinely unknown; that bubbles up as a 5xx, which is the right signal for a malformed request. ## Tests Added 2 unit tests in `BuiltinProvidersMetadataTest`: - `testTasksProviderCreateResolvesRegisterSchemaAndObjectTitle` — golden path: register/schema resolved, object title looked up, full data shape forwarded to TaskService - `testTasksProviderCreateFallsBackToObjectIdWhenObjectMissing` — missing-object path: title falls back to uuid, task still creates Updated the two pre-existing `testTasksProviderMetadata` / `testTasksProviderUpdateRejectsBadEntityId` constructions to pass the new mocks via a `buildTasksProvider()` helper. ## Verification - `composer phpcs` on `TasksProvider.php` → 0 errors - `phpunit -c phpunit-unit.xml tests/Unit/Service/Integration` → 101 tests, 321 assertions, 0 errors, 0 failures

rubenvdlinde requested review from WilcoLouwerse, bbrands02 and rjzondervan as code owners May 18, 2026 17:21

rubenvdlinde merged commit 95aed02 into development May 18, 2026

rubenvdlinde deleted the fix/phpcs-integrations-files branch May 18, 2026 17:21

This was referenced May 18, 2026

feat(audit): AuditTrailMapper::getDistinctActorCount(schemaIds, hours) #1516

Merged

Specs: anonymiser-backend-selection (#1497) #1498

Merged

spec(openregister): add 5 discovered core capability extensions #1532

Merged

rubenvdlinde mentioned this pull request May 18, 2026

fix(tasks): align TasksProvider::create with TaskService::createTask signature (closes #1539) #1568

Merged

2 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

fix(phpcs): clean development to 0 errors across 41 files#1562

fix(phpcs): clean development to 0 errors across 41 files#1562
rubenvdlinde merged 1 commit into
developmentfrom
fix/phpcs-integrations-files

rubenvdlinde commented May 18, 2026

Uh oh!

github-actions Bot commented May 18, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

Uh oh!

Conversation

rubenvdlinde commented May 18, 2026

Summary

What changed

phpcbf auto-fixes (220 errors, 40 files)

Manual fixes (202 errors, 31 files)

Test fixes (26 errors, 1 file)

One suppression with traceability

Verification

Test plan

Out of scope

Uh oh!

github-actions Bot commented May 18, 2026

Quality Report — ConductionNL/openregister @ 470d308

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

Quality Report — ConductionNL/openregister @ `470d308`