fix(GIT-1370): ADR cleanup pass for coverage-report Bucket 4#1432
fix(GIT-1370): ADR cleanup pass for coverage-report Bucket 4#1432WilcoLouwerse merged 3 commits intodevelopmentfrom
Conversation
Address Bucket 4 findings from openspec/coverage-report.md: - SchemaMapper.php:892 — replace print_r() debug helper with json_encode() (ADR-003 / forbidden-debug-calls). - ReferentialIntegrityService.php:372 — fix MariaDB incompatibility: current_schema() is PostgreSQL-only. Now branches on database platform using DATABASE() for MySQL/MariaDB and current_schema() for PostgreSQL, mirroring MagicMapper.php:1697-1707. - RegisterService.php:418 — fix MariaDB incompatibility: CAST AS VARCHAR is non-standard on MariaDB. Now branches on platform using CAST AS CHAR for MariaDB/MySQL and CAST AS VARCHAR for PostgreSQL, mirroring MagicMapper.php:1346-1349. - All 10 db->prepare() raw-SQL sites now carry an explanatory comment describing why QueryBuilder cannot express the query (UNION ALL across dynamic magic tables, information_schema metadata access, foreign cross-app tables, platform-specific JSON-array operators, runtime- resolved table/column names) and noting MariaDB compatibility status. Refs: #1370, ConductionNL/.github#27
Quality Report — ConductionNL/openregister @
|
| Check | PHP | Vue | Security | License | Tests |
|---|---|---|---|---|---|
| lint | ✅ | ||||
| phpcs | ✅ | ||||
| phpmd | ✅ | ||||
| psalm | ✅ | ||||
| phpstan | ✅ | ||||
| phpmetrics | ✅ | ||||
| eslint | ✅ | ||||
| stylelint | ✅ | ||||
| composer | ✅ | ✅ 147/147 | |||
| npm | ✅ | ✅ 598/598 | |||
| PHPUnit | ❌ | ||||
| Newman | ❌ | ||||
| Playwright | ⏭️ |
Quality workflow — 2026-05-05 22:11 UTC
Download the full PDF report from the workflow artifacts.
MWest2020
left a comment
There was a problem hiding this comment.
Review — getest op MariaDB 11.2
Lokaal getest tegen MariaDB 11.2 + Nextcloud 33.0.3 (vendor via composer install --no-dev):
Setup
app:enable openregister→ schoon, geen SQL fouten in migraties- Register
testreg+ Schematestschemaaangemaakt; magic tableoc_openregister_table_1_1correct gegenereerd
Pad 1 — RegisterService::getSchemaObjectCounts() (CAST AS CHAR fix)
GET /api/registers?_extend[]=schemas&_extend[]=@self.statsna het aanmaken van een object- Response:
{"objects":{"total":1,"deleted":0,"invalid":0,"locked":0,"size":0}} - → de UNION ALL met
CAST({schemaId} AS CHAR)heeft daadwerkelijk gedraaid op MariaDB. Vóór de fix faalde dit opCAST AS VARCHARzonder lengte. ✅
Pad 2 — ReferentialIntegrityService::buildSchemaRegisterMap() (DATABASE() fix)
DELETE /api/objects/testreg/testschema/{uuid}→ soft-delete OK,_deletedingevuld in magic table- Path:
DeleteObject::canDelete()→ensureRelationIndex()→buildSchemaRegisterMap()(information_schema.tablesmetDATABASE()) - Geen SQL errors in
nextcloud.logvoor dit pad. ✅
Logscan
- 0 hits op
sql syntax|sqlstate|near "varchar"|near "current_schema"|RegisterService] Error|buildSchemaRegisterMap.*errorover de hele sessie
Twee kleine nits (geen blocker)
Nit 1 — consistentie met precedent
RegisterService.php:398 kiest CAST AS VARCHAR voor de Postgres-tak, maar het precedent in lib/Db/MagicMapper.php:1346-1349 gebruikt {$col}::text voor Postgres. Beide werken; mirror-voor-mirror zou ::text literal-consistenter zijn. Geen functioneel verschil.
Nit 2 — pad in PR-body
De PR-beschrijving verwijst naar MagicMapper.php:1697-1707 en :1346-1349 zonder pad. Het bestand is recent verplaatst van lib/Service/MagicMapper.php → lib/Db/MagicMapper.php. Voor toekomstige lezers handig om het volle pad te noemen.
LGTM verder, fix is correct, klein en gefocust. ✅
- RegisterService: switch Postgres CAST branch to ::text to mirror lib/Db/MagicMapper.php:1346-1349 precedent (no functional change). - Comment paths now reference lib/Db/MagicMapper.php (full path) after the recent move from lib/Service/MagicMapper.php. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
The multi-line ternary in getSchemaObjectCounts triggered two phpcs errors on the project's coding standard (expected single space before ? and :). Refactor to if/else matches the existing platform-branching idiom already used in MagicMapper.php:1345-1349. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Summary
Closes #1370. Addresses the Bucket 4 findings from
openspec/coverage-report.mdgenerated 2026-04-30 (parent: ConductionNL/.github#27).SchemaMapper.php:892— replacedprint_r($value, true)withjson_encode($value)for the error-message string conversion (ADR-003 / forbidden-debug-calls).db->prepare()sites — each now carries an inline comment explaining why QueryBuilder cannot express the query and noting MariaDB compatibility status.The audit also surfaced two real MariaDB-incompat bugs that were fixed in the same pass since the codebase already contained portable counterparts to mirror:
ReferentialIntegrityService.php:372usedcurrent_schema()unconditionally — that function is PostgreSQL-only and would fail on MariaDB. Now branches on the database platform (DATABASE()for MySQL/MariaDB,current_schema()for PostgreSQL), mirroring the established pattern inMagicMapper.php:1697-1707.RegisterService.php:418usedCAST(... AS VARCHAR)unconditionally —VARCHAR(without length) is non-standard on MariaDB. Now branches on platform (CAST AS CHARfor MariaDB/MySQL,CAST AS VARCHARfor PostgreSQL), mirroringMagicMapper.php:1346-1349.Audit findings (per site)
RegisterService.phpReferentialIntegrityService.phpinformation_schema.tablesmetadata querycurrent_schema()fixReferentialIntegrityService.phpJSON_CONTAINS/jsonb @>operatorsLinkedEntityEnricher.phpoc_mail_messages)LinkedEntityEnricher.phpoc_cards)LinkedEntityEnricher.phpoc_calendarobjects)LinkedEntityEnricher.phpoc_calendarobjects)LinkedEntityEnricher.phpoc_talk_rooms)LinkedEntityEnricher.phpCacheHandler.phpThe issue body summary said "8" but the table actually lists 10 occurrences — all 10 are covered.
Test plan
php -lclean on all five edited files (verified locally)phpcsproduces no new warnings beyond pre-existing long-line warnings (verified locally)RegisterService::getSchemaObjectCounts()andReferentialIntegrityService::buildSchemaRegisterMap()no longer raise SQL syntax errors🤖 Generated with Claude Code