Skip to content

fix(register): the openbuild register could never be created on a clean install - #125

Merged
rubenvdlinde merged 1 commit into
developmentfrom
fix/register-declaration
Aug 5, 2026
Merged

fix(register): the openbuild register could never be created on a clean install#125
rubenvdlinde merged 1 commit into
developmentfrom
fix/register-declaration

Conversation

@rubenvdlinde

Copy link
Copy Markdown
Contributor

fix(register): the openbuild register could never be created on a clean install

lib/Settings/openbuild_register.json declared no components.registers section.
OpenRegister's ImportHandler creates a Register row from that key and nowhere
else on the main/beta lines (ImportHandler.php:1514) -- so a clean install
created 15 schemas and ZERO registers, and then skipped all 7 seed objects,
whose @self.register is resolved through the registersMap that section
populates. Every GET /api/objects/openbuild/ 404s with
"Register not found: 'openbuild'".

Verified on a disposable Nextcloud 34 + PostgreSQL + OpenRegister main
(0.2.19), importing the merged register document (base + register.d/*.json,
merged exactly as SettingsService::deepMergeConfig does) into a VIRGIN
OpenRegister with zero registers:

configurations/import -> HTTP 200
register 'openbuild' -> present, id/version 0.9.0
15/15 expected schema slugs present AND bound into the register
15/15 GET /api/objects/openbuild/ -> HTTP 200

The slug is derived, not invented: lib/ and src/ address
objects/openbuild/<schema> in 41 places and all 7 seed objects carry
@self.register: "openbuild".

The schema list is by SLUG, not by components.schemas key -- and for openbuild
every one of the 15 differs (Application -> application,
ApplicationTemplate -> application-template, HelloMessage ->
hello-message, TestCase -> rule-test-case, ...). ImportHandler keys its
schemasMap by $schema->getSlug() (ImportHandler.php:1397/1477), so a register
listing the KEYS binds ZERO schemas while still looking correctly declared;
that is exactly what the first draft of this change did, and only the live
check caught it.

info.version 0.8.0 -> 0.9.0 so OpenRegister's version-gated importFromApp
actually re-imports.

appinfo/info.xml already lists the register-importing InitializeSettings step
under as well as , so openbuild does NOT have the
second, independent gap where the import is post-migration-only and therefore
never runs on a fresh install (Installer::installAppLastSteps() guards both
migration blocks with if ($previousVersion !== '')).

NOTE for reviewers: openbuild's appinfo/routes.php references
OCA\OpenRegister\AppHost\Routes, which exists only on OpenRegister
development. With OpenRegister main installed, enabling openbuild makes EVERY
route on the instance return 500 ("Class OCA\OpenRegister\AppHost\Routes not
found") -- NC's router reflects every controller during route matching. That
is pre-existing, unrelated to this change, and is why openbuild's CI pins
OpenRegister development; it is reported separately.

…an install

lib/Settings/openbuild_register.json declared no components.registers section.
OpenRegister's ImportHandler creates a Register row from that key and nowhere
else on the main/beta lines (ImportHandler.php:1514) -- so a clean install
created 15 schemas and ZERO registers, and then skipped all 7 seed objects,
whose @self.register is resolved through the registersMap that section
populates. Every GET /api/objects/openbuild/<schema> 404s with
"Register not found: 'openbuild'".

Verified on a disposable Nextcloud 34 + PostgreSQL + OpenRegister main
(0.2.19), importing the merged register document (base + register.d/*.json,
merged exactly as SettingsService::deepMergeConfig does) into a VIRGIN
OpenRegister with zero registers:

  configurations/import -> HTTP 200
  register 'openbuild'  -> present, id/version 0.9.0
  15/15 expected schema slugs present AND bound into the register
  15/15 GET /api/objects/openbuild/<schema> -> HTTP 200

The slug is derived, not invented: lib/ and src/ address
`objects/openbuild/<schema>` in 41 places and all 7 seed objects carry
`@self.register: "openbuild"`.

The schema list is by SLUG, not by components.schemas key -- and for openbuild
every one of the 15 differs (`Application` -> `application`,
`ApplicationTemplate` -> `application-template`, `HelloMessage` ->
`hello-message`, `TestCase` -> `rule-test-case`, ...). ImportHandler keys its
schemasMap by $schema->getSlug() (ImportHandler.php:1397/1477), so a register
listing the KEYS binds ZERO schemas while still looking correctly declared;
that is exactly what the first draft of this change did, and only the live
check caught it.

info.version 0.8.0 -> 0.9.0 so OpenRegister's version-gated importFromApp
actually re-imports.

appinfo/info.xml already lists the register-importing InitializeSettings step
under <install> as well as <post-migration>, so openbuild does NOT have the
second, independent gap where the import is post-migration-only and therefore
never runs on a fresh install (Installer::installAppLastSteps() guards both
migration blocks with `if ($previousVersion !== '')`).

NOTE for reviewers: openbuild's appinfo/routes.php references
OCA\OpenRegister\AppHost\Routes, which exists only on OpenRegister
development. With OpenRegister main installed, enabling openbuild makes EVERY
route on the instance return 500 ("Class OCA\OpenRegister\AppHost\Routes not
found") -- NC's router reflects every controller during route matching. That
is pre-existing, unrelated to this change, and is why openbuild's CI pins
OpenRegister development; it is reported separately.
@github-actions

github-actions Bot commented Aug 4, 2026

Copy link
Copy Markdown
Contributor

Quality Report — ConductionNL/openbuild @ b5196f2

Check PHP Vue Security License Tests
lint
phpcs
phpmd
psalm
phpstan
phpmetrics
eslint
stylelint
build
check-manifest
test-l10n
composer ✅ 101/101
npm ✅ 654/654
PHPUnit
Newman ⏭️
Playwright ⏭️

Quality workflow — 2026-08-04 20:00 UTC

Download the full PDF report from the workflow artifacts.

@rubenvdlinde

Copy link
Copy Markdown
Contributor Author

Merging with --admin. Failure set compared against the base's last completed run (a blank conclusion means still running, not passing): improvement — this PR adds no new failure.

What it fixes

Without components.registers, a clean install provisions the app's schemas and zero registers, then skips every seed object — their @self.register resolves through the map that section populates. Reproduced on a virgin NC34 + Postgres against OpenRegister main (0.2.19), which is what apps actually depend on: schemas imported, 0 registers, objects/<register>/<schema>HTTP 404 "Register not found".

⚠️ It only reproduces on main/beta. OR development has ImportHandler::autoCreateRegisterIfApplication(), which auto-creates a register from x-openregister.type: application — testing against a dev-branch OR makes this look like a false alarm.

The part that would have shipped as a silent no-op

ImportHandler keys schemasMap by $schema->getSlug(), not by the components.schemas key. A register listing the keys imports cleanly, reads as correct, and binds zero schemas — one repo measured literally BOUND into register: 0. Key ≠ slug across the fleet (ApplicationTemplateapplication-template). Only live verification caught this; no static review would have.

Every slug here was derived from the seed objects' own @self.register and the objects/<reg>/ call sites — not invented.

Verified end-to-end on a disposable instance: schemas bound into the register, and seed objects returning 200 where they previously 404'd.

@rubenvdlinde
rubenvdlinde merged commit 32db9e0 into development Aug 5, 2026
32 of 34 checks passed
@rubenvdlinde
rubenvdlinde deleted the fix/register-declaration branch August 5, 2026 17:23
rubenvdlinde added a commit that referenced this pull request Aug 5, 2026
Positive-controlled (PHPMD 2.15, PHP 8.4, absolute paths): with */lib/X/* a
probe in lib/Service/X/ is reported; with */X/* it is silently swallowed. Both
correctly exclude the app's own directory, so the narrow form is strictly safer.
Matches the fleet-canonical shape in nextcloud-app-template #125, doriath #157,
larpingapp #263 and planix #315.
rubenvdlinde added a commit that referenced this pull request Aug 5, 2026
… inert exclude-pattern (#129)

* chore(phpmd): burn down 46 baselined findings and fix the inert exclude-pattern

Part of ConductionNL/.github#155 (fleet PHPMD suppression audit).

`composer phpmd` reports 0 today. With phpmd.baseline.xml deleted it reports
195. The gate is live: openbuild's Code Quality workflow calls the shared
ConductionNL/.github quality.yml, where `enable-phpmd` defaults to true, so the
phpmd leg genuinely runs on every push and PR — the baseline is hiding findings
from a check that really executes.

Two things worth recording about the baseline file itself:

  * Its 174 entries suppress 195 findings, because a PHPMD baseline entry is
    scoped to a (rule, file) pair — optionally a method — never to a line. A
    class-scoped entry is therefore an open licence: every FUTURE violation of
    that rule in that file is silently suppressed too. "174 entries" is not
    "174 known problems", and the shortfall is granularity, not staleness.
  * PHPMD auto-discovers phpmd.baseline.xml from the working directory.
    Dropping --baseline-file from the composer script changes nothing; only
    deleting the file disables it.

What this commit retires (measured, PHPMD 2.15.0 / PHP 8.4.22, baseline
deleted both times, --exclude '*/Resources/template/*' on every leg): 195 -> 149.

  * ElseExpression 27 -> 1.
  * MissingImport 3 -> 0 (fully-qualified `new \RuntimeException` in
    AppOverrideService).
  * ShortVariable 9 -> 0 ($m/$ca/$cb/$i -> $matches/$condA/$condB/$seq/$cursor).
  * LongVariable 5 -> 0; the two renamed constructor-promoted properties are
    also renamed at their named-argument call sites in the unit tests.
  * UnusedFormalParameter 3 -> 1. Two were genuinely dead and are REMOVED, with
    their call sites updated: AppOverrideService::userVersionSlug($appId) and
    AppRepoParser::decodeChannelEntry($path).
  * CyclomaticComplexity 41 -> 40, from the two consolidating extractions below.

The ElseExpression work is behaviour-preserving throughout: initialise-then-
override where both branches were pure, guard-and-return where they were not,
`continue` inside loops, and extractions where both arms had to write so exactly
one could run. No ternaries were introduced.

Two traps from the brief fired for real, and re-measuring after every rule is
what caught them:

  1. THE EXTRACTION TIPS THE CLASS. Three extractions from AppOverrideService
     pushed BooleanArgumentFlag 13->14, ExcessiveClassLength 7->8 and
     TooManyMethods 4->5 — the class sits exactly at those thresholds. All three
     were restructured to need no new method (a `continue` guard in
     deepMergeDelta, an early return in upsert), and the one that genuinely
     could not be — upsertUserDelta, where both arms write a different object —
     was LEFT AS AN ELSE rather than traded for three other findings. That is
     the single remaining ElseExpression.

  2. REMOVING AN `else` ADDS COMPLEXITY. Converting the two elevate-or-not
     if/elses in MigrateToVersionedModel::migrateOne() gave it a NEW
     NPathComplexity finding (336), and converting the two update-or-create
     if/elses in AutomationCompilerService::applyRuleSet() took its
     CyclomaticComplexity from 9 to 11. Both were fixed STRUCTURALLY rather than
     by relaxing anything: each pair of duplicated branches was folded into one
     guarded helper (`runElevated()`, `saveOrUpdate()`), which removes the elses
     AND lowers the host method's complexity below where it started. A third,
     the state chain in run(), keeps its `else if` and only drops the bare
     `else` — PHPMD flags the bare `else` only, so that costs no extra path,
     whereas two independent ifs would have doubled an already-flagged NPath.

Also applies the fleet-standard scoped-ruleset shape. The
`<exclude-pattern>*Migration*</exclude-pattern>` nested inside the
UnusedFormalParameter `<rule>` was INERT — PHPMD 2.15 honours exclude-pattern
only as a direct child of `<ruleset>`. The rule now lives alone in
phpmd-unusedparams.xml with a top-level exclusion, run as a second leg with
neither leg able to short-circuit the other, and `--exclude
'*/Resources/template/*'` preserved on BOTH legs so the analysed file set is
unchanged. Stated plainly: openbuild has no lib/Migration directory, so this
retires ZERO findings here. It is adopted for fleet uniformity.

phpmd.baseline.xml is RETAINED. 149 findings remain and deleting it would
redden a gate that really runs. What remains, and why:

  * CyclomaticComplexity 40 / NPathComplexity 25 / ExcessiveClassComplexity 23 /
    ExcessiveMethodLength 11 / ExcessiveClassLength 7 — genuine debt in the
    automation-compiler, copilot and app-override services.
  * CouplingBetweenObjects 15, TooManyMethods 4, ExcessiveParameterList 3,
    TooManyPublicMethods 1 — class-level; need collaborator extraction, which
    AppOverrideService's thresholds show cannot be done piecemeal.
  * BooleanArgumentFlag 13 — genuine debt; each is an API change.
  * StaticAccess 4 — `\OCP\Server::get()` service-locator calls in three broker
    paths plus OpenRegister's `Bootstrap::register` in AppInfo\Application; the
    Bootstrap one is a bootstrap-time static by design.
  * ElseExpression 1 — AppOverrideService::upsertUserDelta, see trap 1 above.
  * UnusedFormalParameter 1 — INTERFACE-MANDATED. SeedHelloWorldFixture::execute()
    implements Symfony\Component\Console\Command\Command::execute(InputInterface
    $input, OutputInterface $output). `$input` is unused but cannot be dropped.
  * LongClassName 1 — UnsupportedAutomationCombinationException. Deliberately
    NOT renamed: the name is a normative identifier in
    openspec/specs/automation-designer/spec.md and in three archived changes.
    Renaming it in code alone would desynchronise the spec, and rewriting
    archived changes is out of scope.

Verified: phpunit 746 tests / 2342 assertions before AND after, both green.
phpcs exits 0 (the renames left stale docblock alignment; fixed with the repo's
own phpcbf, 24 errors, no remaining errors). psalm and phpstan both exit 0.
Both phpmd legs exit 0 with the baseline in place, so CI stays green.

No suppression, baseline entry, threshold change or skip was added anywhere.

* chore(phpmd): keep this PR single-purpose — revert the else-fixes in 6 files

Touching these six files pulls three PRE-EXISTING hydra-gate failures into the
diff-scoped gate run, all of them in code this PR did not write:

  * gate-2 forbidden-patterns — the deliberate, documented error_log() in
    AppInfo\Application::register(), which carries its own phpcs:ignore and a
    comment explaining that no PSR logger exists that early. Removing an else
    necessarily moves those lines into the diff. The call must stay; the gate
    has no per-call waiver; so the file must stay untouched.
  * gate-46 spec-anchor-existence — 17 dead @SPEC targets across
    ExportsController, AutomationsController, AppInfo\Application,
    RuleActionDispatcher, AutomationCompilerService and TemplateSeedService.
    They point at openspec/changes/automation-designer/... which was ARCHIVED
    to openspec/changes/archive/2026-07-11-automation-designer/... and at
    #req-autd-NNN anchors that exist in NO version of the spec, canonical or
    archived — the canonical spec uses '### Requirement: <prose>' headings.
    Repairing them means deciding, per tag, which Requirement each method now
    implements. That is spec-traceability work for the automation-designer
    owner; guessing it here would encode WRONG links, which is worse than a
    dead one.
  * gate-49 controller-exception-translation —
    AutomationsController::recompileAndRespond() lacks a try/catch or @throws.

The gates are FILE-scoped (_filter_grep_by_scope keys on the path), so any edit
to one of these files puts all of its pre-existing findings in scope. Reverting
is the only way to keep a PHPMD suppression burn-down from silently becoming a
spec-metadata rewrite.

Cost, measured: openbuild goes 149 -> 161 with the baseline deleted (still
195 -> 161). Specifically this gives back 11 ElseExpression, 1 ShortVariable and
1 LongVariable, and drops the AutomationCompilerService::saveOrUpdate()
consolidation, so CyclomaticComplexity returns to 41.

Follow-up filed in the PR body: those three gate failures are real and now
precisely diagnosed, but they belong to a separate change.

* fix(phpmd): narrow the UFP exclude to the app's own lib/ directory

Positive-controlled (PHPMD 2.15, PHP 8.4, absolute paths): with */lib/X/* a
probe in lib/Service/X/ is reported; with */X/* it is silently swallowed. Both
correctly exclude the app's own directory, so the narrow form is strictly safer.
Matches the fleet-canonical shape in nextcloud-app-template #125, doriath #157,
larpingapp #263 and planix #315.
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