This repository was archived by the owner on May 29, 2026. It is now read-only.
fix(quality): clean development to 0 failing CI checks#1565
Merged
Conversation
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)
rubenvdlinde
requested review from
SudoThijn,
WilcoLouwerse,
bbrands02,
remko48 and
rjzondervan
as code owners
Contributor
Quality Report — ConductionNL/openregister @
|
| 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 18:40 UTC
Download the full PDF report from the workflow artifacts.
2 tasks
rubenvdlinde
added a commit
that referenced
this pull request
May 18, 2026
Closes #1564. The static `self::\$constructCount` counter + `> 2` guard in `MagicMapper::__construct` was a March-2026 holdover from a refactor's debugging session — added in ef8ed0d, kept across the `file_put_contents('/tmp/or-debug.log', ...)` debug logging that PR #1565 just removed. The two circular-DI paths the guard was protecting against have both been broken via lazy container resolution since: - `CacheHandler → RegisterMapper → MagicMapper` — `CacheHandler` and `ICacheFactory` are now resolved lazily inside `initializeHandlers()` (commented in-line). - `SettingsService → MagicMapper` — `SettingsService` removed `MagicMapper` from its constructor signature (private docblock at SettingsService.php:243 marks "REMOVED: Object mapper"). The full PHPUnit suite (12,226 tests / 25,949 assertions) is green after removing the guard — proving the cycle no longer trips in any covered path. ## What changed - `lib/Db/MagicMapper.php` — dropped `\$constructCount` static + the guard branch from `__construct()`. Constructor now goes straight to `\$this->initializeHandlers()`. - `tests/Unit/Service/MagicMapperTest.php` — removed the reflection block that reset the static counter between tests (it referenced a property that no longer exists). ## Verification - `composer phpcs` on `MagicMapper.php` → 0 errors - `phpunit --filter MagicMapperTest` → 29/29 pass - `phpunit -c phpunit-unit.xml` (full) → 12,226 tests / 25,949 assertions / 0 errors / 0 failures
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.
Summary
Brings
developmentfrom 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.What changed
src/main.js— import ordering (5 errors)Imports were interleaved with side-effect code. Moved all
importstatements above the xwiki-registry setup block. No behavior change.package.json— nc-vue vueuse override (SBOM)@conduction/nextcloud-vue@1.0.0-beta.46declares@vueuse/core:^10.0.0but its own internal@nextcloud/dialogs@7.3.0transitively requires^14.0.0, so npm installs 14.2.1 (violating nc-vue's declaration →npm lsexit 1).Added an
overridesentry to re-declare nc-vue's constraint as^14.0.0. The upstream nc-vue package.json should be updated too — flagged as a separate follow-up against the nc-vue repo.13 test files — constructor-signature drift (142 errors)
Same pattern as the LeafProvidersMetadataTest fix in #1562: production constructors evolved, tests didn't. Files fixed:
tests/bootstrap-unit.php— ZipStream v2/v3 dispatch (22 errors)If the `forms` app is installed in test env, it ships ZipStream v2; PhpSpreadsheet's writer dispatcher picks v2 based on `class_exists`, then calls v2 methods against OR's v3 → TypeError. Bootstrap now strips forms-app ZipStream v2 vendor entries from all Composer autoloaders before tests run. No-op when forms isn't installed.
lib/Db/MagicMapper.php— debug `file_put_contents` removalTwo stray `file_put_contents('/tmp/or-debug.log', ...)` calls were silently writing to disk and triggering "Permission denied" warnings during tests. Replaced with a logger->warning call. The `$constructCount > 2` guard itself stays — circular construction is under investigation in #1564.
No production behavior changes outside MagicMapper
The MagicMapper change is the only lib edit. All other changes are tests / docs / configs / a single import reorder.
Verification
Test plan
Follow-ups