release: merge development into beta - #2972
Merged
Merged
Conversation
…tors (#2898) Follows the constructor fix (#2876). Those three services had been unreachable, so their tests had grown around that: each one built the object in a way production cannot. EndpointServiceTest's TestableEndpointService said so in its own docblock: Test-only subclass to inject dependencies since EndpointService has no constructor. and used Closure::bind to write four private properties from outside the class. All 78 tests passed against a wiring that existed only in that file. The subclass now calls parent::__construct(), so those 78 exercise the same construction path production uses and would fail loudly if it went missing again. (NotificationServiceTest's reflection workaround was replaced the same way in #2876.) Adds coverage for what was never reachable: UploadServiceSourceRoutingTest (6 tests) covers getUploadedJson's routing - the four private helpers that had no test at all, driven through the PUBLIC entry point rather than by reflection, because the routing to them is exactly what was broken. Includes the ordering that matters: internal `_`-prefixed params are stripped BEFORE the source check, so a body of nothing but control params is a 400 rather than falling through to the json branch. NotificationPayloadTest (3 tests) pins the notification contract. The eight existing tests stub the notification with willReturnSelf(), which asserts the call chain does not break and says nothing about the subject key or parameter names - so renaming `configuration_update_available` or dropping `currentVersion` would leave all eight green while every consumer stopped recognising the message. lib/Notification/Notifier.php reads those parameters back BY NAME. Mutation-checked: renaming the subject key and dropping the 'unknown' version fallback fails 2 of the 3 new tests. Restoring passes. Full unit suite: 17507 tests, 0 failures. phpcs clean. Co-authored-by: Conduction Release Bot <release-bot@conduction.nl>
…overage (#2899) * test(aggregation): remove the coverage metadata that was discarding coverage Closes #2847. Under `beStrictAboutCoverageMetadata="true"`, PHPUnit does not merely restrict recording to the units a test names — it marks any test that executes anything else RISKY and throws that test's coverage away entirely. The message is explicit once you look: This test executed code that is not listed as code to be covered or used: - OCA\OpenRegister\Db\Register - OCA\OpenRegister\Db\Schema - OCA\OpenRegister\Service\Aggregation\AggregationQuery Almost every test in this directory legitimately runs a collaborator — AggregationQuery, PlaceholderResolver, the Db entities — so naming the class under test did not focus the measurement, it deleted it. Measured locally with pcov, identical scope both runs (237 tests, 461 assertions): with @Covers without AggregationRunner 44.39% (613/1381) 80.30% (1109/1381) ...methods 9.80% (5/51) 33.33% (17/51) scope statements 1618 2137 Risky tests 33 0 Same tests, same assertions, same executed lines — only the attribution differs.⚠️ Those are pcov numbers. coverage-guard.php's own docblock records that CI measures with xdebug and the two do not count statements identically, so treat the DIRECTION as the result and let CI's `--against` measurement be the authority. `.coverage-baseline` is untouched: the guard treats a measurement above the floor as good news, and this only moves it up. `TimeseriesRequestValidatorTest` had `@coversDefaultClass` with not one `@covers ::method` to pair with it — naming a default nothing used, while still restricting recording. The reasoning already existed, measured, in AggregationJoinAndCompositeGroupByTest's docblock; the other ten files simply never had it applied. Each now carries a short note pointing there, so the annotation is not helpfully restored later. Full suite: 17336 tests, 0 failures. * fix(test): name the annotation without its at-sign in the docblocks CI failed all six PHPUnit cells with "@Covers ::method`" is invalid My own explanatory comment was the cause. PHPUnit parses a CLASS docblock, so the sentence describing what had been removed re-declared it — and the method-scoped spelling is malformed, so it errored rather than being tolerated. A comment about the bug became the bug. Every docblock added by this branch now names the annotation without a leading at-sign, and TimeseriesRequestValidatorTest says why so the next person does not helpfully "fix" the prose. (I wrote the replacement comment containing the same literal string once more before catching it. It is a genuinely easy trap: the natural way to document an annotation is to write it.) The pre-existing mentions in AggregationJoinAndCompositeGroupByTest are left alone — those parse harmlessly; only the method-scoped form is malformed. Full suite: 17498 tests, 0 failures, no invalid annotation.
…rators (#2904) Two changes to the same machinery, both about a filter that silently answers the wrong question rather than failing. CONDITIONAL METRICS `metrics[].condition` scopes ONE figure to a subset of the grouped rows. That is what a debit/credit split needs: `totalDebit` and `totalCredit` are the same SUM over the same field, separated only by `side`. Declaring two aggregations instead is not equivalent — it groups and scans the table twice, and the two results can disagree if a row is written between the calls, which is exactly what a trial balance must never do. It is a FILTER OBJECT, deliberately, not a SQL string: the same shape as the aggregation's own `filter`, through the same applyFilter(). One grammar, one implementation. A second string-shaped grammar is how a consuming app ended up with 265 declarations this engine could not read. `metrics[].as` names the response key, and conditional metrics REQUIRE it: two conditional sums over one field both derive `sum_amount`, so the second would overwrite the first and return one figure where the caller asked for two. The native SQL path REFUSES a conditional spec rather than running it. tryNativeMultiMetric() aggregates every entry over the same filtered rows and keys from metric+field, so it would drop the condition and collide the aliases — answering wrongly and fast. 🔑 Writing the test found a defect in the implementation: AggregationQuery::getMetrics() rebuilt each entry as {metric, field}, STRIPPING condition and as before the runner ever saw them. Must-fail control: revert that and `openTotal` comes back 35.0 — the unconditioned total — instead of 30.0. The validator refuses a string `condition`, an empty `as`, and a condition naming a property the schema does not declare. That last one matters most: at run time such a filter is not an error, it matches nothing and returns an empty result, which a page renders as "no data" over live rows. UNKNOWN FILTER OPERATORS NOW THROW checkOn()'s `default => true` let an unrecognised operator match EVERY row, so the filter widened instead of narrowing. Measured in shillinq: `{"not-in": [...]}` — the implemented spelling is `notIn` — meant an AR-ageing report silently included settled invoices. Blast radius measured before changing it: exactly ONE metric-bearing aggregation in shillinq uses an unknown operator (APInvoice.apAging), and it is wrong today. The rest (`equals`, `not`, `between`, `gteOrNull`, `notStartsWith`, `not_in`) sit on aggregations that compute nothing yet, so they will now fail loudly when someone gives them a metric rather than returning a quietly widened set. Verified: 17506 tests, 0 failures; phpcs and phpmd clean on the changed files; both must-fail controls confirmed.
…#2900) * fix(flow): enforce the assignee that AwaitSignalNode already recorded ADR-098 names this gap exactly: "no task authz — anyone reaching the resume endpoint can decide". Concretely, AwaitSignalNode has ALWAYS written an `assignee` onto the suspension, and nothing ever read it back. The only check on POST /api/flow-runs/{uuid}/resume asked "may you run this flow?", which is a different question from "is this decision yours to make" — so everyone who could run the flow could approve a step assigned to someone else, while the recorded assignee made it look otherwise. A field that looks like authorization and is not is worse than no field. The resume endpoint now refuses when the awaiting step names an assignee and the caller is neither that uid nor a member of that group. Anonymous is refused outright: an assigned decision is never anonymous. SCOPE, STATED HONESTLY. This closes the WHO of an already-recorded assignment. It is NOT the task entity, inbox or definition versioning ADR-098 describes — those remain unbuilt, and Wave 4's flow consolidation still depends on them. A step with NO assignee is deliberately unchanged: silence still means anyone, because most await-signal suspensions are webhooks and child-run completions rather than human decisions, and tightening that would break every one of them. Mutation-checked, because a security guard that cannot fail is the worst kind: replacing the assignee lookup with an unconditional allow makes the suite fail. * test(flow): cover the assignee guard's branches, not just its happy paths The coverage ratchet failed this branch: it adds 33 statements to FlowRunController and only 25 of them were reached, so coverage of the code this change keeps or adds fell from 83.93% to 82.59%. The three original tests covered the shape of the guard - assigned to someone else, assigned to you, not assigned. What they did not cover were the branches where getting it wrong is quiet: - An assigned step answered with NO session. This is the fail-CLOSED half, and it is the half that is easy to invert, because the unassigned case deliberately lets anyone through: an implementation that treated "no uid" the same way would pass every other test in this file while leaving an assigned decision open to an unauthenticated caller. - A GROUP assignee. AwaitSignalNode records one `assignee` string without saying whether it names a person or a group, so the guard tries both. Without a test, a broken group lookup refuses the step's own intended audience - and reads as "the guard works", because refusing is what a guard does. Both the member and the non-member case are asserted, so the lookup is a check rather than a rubber stamp. - The three shape guards in recordedAssignee(). The context is stored JSON written by older runs under earlier shapes. A slot that has not asked yet must not gate the step that is asking, or a future step refuses the right person now; a malformed slot must read as unassigned rather than 500 an in-flight run. Each is mutation-checked: disabling the anonymous refusal, the group branch, and the askedAt guard each make the suite fail. 18 tests -> 24. --------- Co-authored-by: Conduction Release Bot <release-bot@conduction.nl>
…ns (#2901) * test(e2e): make the delegation suites establish their own preconditions Running the three delegation suites live against merged `development` gave a different answer three times over identical code — 18 passed, then 8 failed, then 2 failed. Neither the code nor the assertions were wrong; the suites were inheriting state instead of establishing it. Two separate causes, both of which let a run report something it had not measured: 1. A HARDCODED FIXTURE UID. `NEXTCLOUD_OTHER_USER || 'ddauth-alice'` named an account that existed when the specs were written and did not exist after the dev instance was rebuilt. The failure read `"ddauth-alice" resolves to no account you may ask` — which is the delegation guard's own refusal message, i.e. a dead fixture wearing the words of a working control. The uid is now DISCOVERED from the instance, and a single-account instance SKIPS with a sentence naming what went unverified rather than failing for a reason that is not about the code. 2. A LEAKED GRANT. Every suite opens by asserting the save is REFUSED — the baseline the later "now it saves" assertion is measured against. Grants outlive a run, and a suite killed mid-way (a `head` closing the reporter's pipe is enough) leaves a `granted` row behind, so the next run's baseline got a cheerful 201. The failing direction was the lucky one: a leaked REVOKED grant would have made the same baseline pass for the wrong reason and the suite would have proved nothing. Each suite now revokes any live grant over its target before asserting. The account probe deliberately does NOT swallow errors. An earlier draft wrapped it in `catch { return null }`, and when the probe came back as Nextcloud's login page — HTTP 200, HTML, `.json()` throws — the catch reported "no second account", eleven specs skipped citing a single-account instance, and the run said `0 failed`. It now throws with the status and body, because a probe that cannot answer must say so. Also lets `NC_CONTAINER=nextcloud` through under an explicit `NC_ALLOW_SHARED_CONTAINER=1`. The guard exists to stop an accidental default, not to make the parking path — which only exists once a real TimedJob ticks — permanently unverifiable. Verified: 18 passed / 0 failed, twice back to back, against merged `development` on a live instance. * test(e2e): prefer the shared dev container for occ, gate only restarts The container guard refused the shared `nextcloud` container for every purpose, and that was too blunt in both directions. 🔑 THE TWO ACTIONS ARE NOT THE SAME RISK, so they no longer share a rule. `resolveContainer('exec')` — the default — now DEFAULTS TO the shared container instead of returning null. Running one named `occ` command there is how the dev box is meant to be exercised, and refusing to do so bought nothing: it made every spec that needs a real TimedJob tick skip everywhere. The delegation parking suite is the clearest casualty — a run parked on `awaiting_consent` and released by a cron sweep only exists once a job actually runs, so the headline behaviour of that subsystem was verified by nothing but a unit test, while the summary said "3 skipped" in a tone indistinguishable from "3 passed". `resolveContainer('restart')` still refuses the shared container without an explicit `NC_ALLOW_SHARED_RESTART=1`. That is the action the original guard was really about: `docker restart nextcloud` bounces an environment that bind-mounts several developers' working trees, mid-session, with no warning to them. One `occ` command is recoverable; restarting somebody else's instance is not. `federated-config-store.spec.ts` is the only caller that restarts, and it asks for that purpose explicitly. Verified with NO container env set at all — the new default path: 26 passed, 5 skipped, 1 failed All three delegation-parking tests now RUN and pass, including the park and the release through real FlowScheduleWorker and FlowRunWorker ticks. The remaining failure is pre-existing and unrelated: `federated-config.spec.ts` (like `flow-schedule.spec.ts`) needs a `flows` REGISTER, which this rebuilt dev instance never seeded — its beforeAll fails on the register lookup regardless of any container setting. * style(e2e): prettier-format the delegation fixtures module
Both classes are fully written, implement IRepairStep, and were named ZERO times in appinfo/info.xml. Nextcloud only runs what the manifest declares, so neither has ever executed. gate-98 (repair-step-registration) catches it. A class that exists is not a class that runs, and neither failure is visible: - ImportFlowRegister creates the `flows` register and flow schema. Without it that register was never created, so every flow step listed BELOW it — MigrateRenamedFlowNodeTypes, BackfillFlowTriggerIndex, InitializeFlowActions — silently operated on nothing. It is therefore registered ABOVE them, in both <post-migration> and <install>. - RenameDutchColumns moves stored data from the Dutch columns to the English ones the register now declares. MagicMapper ADDS a column when a snake_cased property is absent and never renames — there is not one RENAME COLUMN in the app — so a renamed property leaves the data in the old column while every read looks at the new one and finds null. No error, no data loss, and invisible to a suite that asserts against fixtures rather than migrated rows. For shillinq those columns carry invoice, subsidy, payroll and tax amounts. Its own docblock documents the step as non-destructive and idempotent: it renames only when the old column exists and the new one does not, copies across and LEAVES the old column when the new one already exists, refuses two sources targeting one destination, and deletes nothing. A re-run is a no-op. Registered under <post-migration> ONLY — a fresh install has no Dutch columns to move, so listing it under <install> would be a guaranteed no-op. Pre-existing on development: neither name appears in its info.xml. This is not introduced by any open PR; gate-98 is full-tree, so it reddens every branch until this lands. Note for whoever cuts the next release: repair steps run on upgrade, so these two first execute on the next version bump.
…to nobody (#2915) Refs #2905. `FlowShareableConfigType::deserialise()` stored an imported flow with `owner = null` and `organisation = null`, on the reasoning — correct as far as it went — that the SENDER's identity means nothing on this instance. But null is not "the absence of the sender's identity". It is the absence of ANY, and `FlowMapper` scopes every read with an EQUALITY predicate: $qb->andWhere($qb->expr()->eq('organisation', $qb->createNamedParameter($organisation))); `NULL = 'anything'` is never true in SQL. So the row inserted, `install` returned its uuid with HTTP 200, and the flow was excluded from `flow#index`, `flow#show` and the run path BY CONSTRUCTION — with no adopt route to rescue it and no way for `flow#update` to reach it either. Install was a one-way door. Five permanent orphans on one dev instance. 🔴 THE RULE WAS ALREADY FIXED — ON THE OTHER WRITER. `FlowService::flowToSave()` refuses this exact write, and its comment describes this exact outcome: > REFUSE rather than stamp nulls. […] a flow with no organisation belongs to > nobody: it does not appear in index(), find() refuses it, and it can never > be run or edited again. Accepting the write produced a permanent orphan and > reported success — the caller had no way to tell that from a flow that saved. Two writers each deriving ownership their own way is how the rule came to hold on one and not the other. Both now read `FlowService::callerOwnership()`, so there is ONE place that decides it, and `deserialise()` refuses rather than stamping nulls — the same refusal, on the path that was missing it. THE SECURITY PROPERTY IS UNCHANGED, and it never depended on the null. `Flow::canDispatch()` requires `enabled === true` AND a non-empty owner, so `enabled = false` alone already refuses dispatch. A bundle still cannot arrive and start executing against the receiving tenant's data, and the sender's claimed `owner`/`organisation` are still discarded. What the null added was not safety — it was unreachability. A TEST REQUIRED THE DEFECT. `testAnImportedFlowLandsDisabledAndOwnerless` asserted `getOwner() === null`, so the orphan was pinned as a requirement. It is now `testAnImportedFlowLandsDisabledAndOwnedByTheInstaller` and asserts both halves: the sender's claim is rejected (`not 'victim'`, `not 'their-org'`) AND the installer owns it. A second test pins the refusal when no caller resolves. Verified: 10 tests, 33 assertions, OK. phpcs clean on both changed lib files.
…2916) Plan item 4, and the largest single capability #1261 is waiting on — 11 declarations need it. `groupBy: ["AnalyticalDimension.parentCode"]` asks to roll parent rows up to a column the parent does not have. applyJoin() cannot produce it: it runs AFTER grouping and attaches figures to groups that already exist, and by then the rows are gone. So the value is projected onto each parent row FIRST, through the join key, and grouping proceeds normally. The result is a roll-up to the joined dimension — a cost-centre hierarchy summing its children, which is the shape this exists for. THREE THINGS THE TESTS FOUND, each a wrong answer rather than an error: 1. The `on` SHORTHAND cannot be used here, and now says so. "Schema.column" infers the parent-side field FROM THE GROUP FIELDS — same-named one if present, otherwise the first. When the group field is the joined one, that inference picks the JOINED field as the parent key, reads it off rows that do not have it, and puts everything in one bucket. Measured: a fixture summing 350 + 7 came back as a single bucket of 357 keyed ''. The explicit {parentField: joinedField} map names both sides. 2. The post-grouping merge had to be SKIPPED. After projection the group key IS the joined dimension, so the original join key is no longer in the row; mergeJoinedValues() would look up a tuple that cannot match and hang a map of nulls on every group — reading as "the joined schema had nothing" rather than "the grouping already answered this". The envelope now reports `join.consumedForGrouping: true`. Attaching figures there would mean re-aggregating the joined schema BY the projected dimension, a second and different join, which is not invented silently. 3. An unmatched parent row keeps a NULL group instead of being dropped. Dropping it would quietly shrink every total with nothing saying so. The test pins 350 + 7 + 11 = 368. The native SQL path refuses a join-qualified group field: its SQL is emitted against the parent table alone, so the column exists on no row and every row would land in one null bucket, reported as a working total. Also: the map is read directly rather than through resolveJoinKey(), which enforces "every parent-side field must be one of the groupBy fields" — right for the post-grouping merge, false here by construction. phpmd flagged the first cut at complexity 20 / NPath 25600; split into joinedProjectionKeyMap(), loadJoinedRowsForProjection() and stampProjectedColumn(). Both tools clean. Verified: 17526 tests, 0 failures; phpcs 0 errors; phpmd clean; must-fail control confirmed (disabling the projection reddens both roll-up tests).
…2917) * feat(graphql): filtered groups, and declared aggregations by name Two changes to the GraphQL aggregation surface. 1. A FILTERED LIST RETURNED UNFILTERED GROUP TOTALS resolveList() maps the query's `filter` into the row list. resolveGroupBy() built its aggregation input with `'filter' => []` HARDCODED, so the edges honoured the filter and the groups silently did not — group totals were computed over the whole schema. gLLines(filter: {eliminationFlag: false}, groupBy: {field: "accountNumber", metric: SUM, metricField: "amount"}) { edges { node { amount } } # filtered groups { key value } # NOT filtered — every GLLine } Nothing errored. The caller was shown a bigger number than the rows it was given, which is the hardest kind of wrong answer to notice on a dashboard, and launchpad widgets read these over runtime GraphQL. Only the PROPERTY filter forwards. Paging must not — a total over "the first 20 rows" is not a total and would change as the user paged. `search` must not — it is a relevance query the aggregation engine does not implement, so forwarding it would filter on a property named `_search`, match nothing, and return an empty result rather than an error. `selfFilter` addresses @self metadata, a different namespace. Each omission is deliberate and documented, because anything left out means the groups describe a wider population than the rows. 2. DECLARED AGGREGATIONS ARE NOW REACHABLE FROM GRAPHQL `groupBy` is ad-hoc: the caller describes the aggregation. A schema's `x-openregister-aggregations` were REST-only, so a page wanting a declared figure had to hand-build a URL alongside its GraphQL query. gLLines(filter: {...}, aggregation: "consolidatedTrialBalance") { edges { node { amount } } aggregation } The name is the whole input — the declaration already carries the metric, grouping, filter and join, and was validated at save time. The query's filter is passed as a NARROWING constraint, which is safe by construction: the engine refuses any request key the declaration already pins, so a caller can add a constraint and can never relax a declared scoping one. The envelope is JSON, deliberately. A declared aggregation's shape varies with what it declares — a scalar `value`, a `values` map for `metrics`, `groups[].keys` for a composite groupBy, `joined` when it joins. Typing it now would repeat the mistake GroupBucket already makes, where `value: Float!` cannot carry a values map and a null group key coerces to "". A typed AggregationResult is the right next step once a consumer needs introspection over it. Verified: 17520 tests, 0 failures; phpcs 0 errors; phpmd clean. The filter fix has a must-fail control — reverting it reddens the two filtered tests while the unfiltered control correctly stays green. * feat(graphql): let a bucket carry what the engine actually returned Plan item 3. GroupBucket flattened the engine's result into two scalars, and each coercion lost something real. 'key' => (string)($bucket['key'] ?? '') 'value' => (float)($bucket['value'] ?? 0) A NULL group key became '' — rows whose grouped field is null were indistinguishable from rows whose value is genuinely the empty string. And a multi-metric bucket carries `values` with no scalar `value` at all, so the float cast reported 0.0 FOR EVERY BUCKET rather than admitting the figures live under another key. The second was unreachable only because GraphQL could not ask for `metrics[]`. Widening the input without widening the bucket would have made it reachable, which is why both halves land together. key String (was String!) — null stays null value Float (was Float!) — null for a multi-metric grouping keys JSON — composite group key as {field: value} values JSON — figure per response key, incl. `as` aliases joined JSON — figures from a joined schema The input widened to match: `fields` for composite grouping, and `metrics` with per-entry `condition` + `as` for the conditional split. VALIDATION STAYS IN ONE PLACE. TimeseriesRequestValidator handles both new keys and delegates the metric entries to AggregationMetricsAnnotationValidator — the SAME class the annotation path uses. An ad-hoc request and a declared aggregation cannot drift in what they accept. A validator and an executor each owning a copy of the grammar is exactly how this engine acquired specs it could not run; adding a third copy for GraphQL would have repeated it. `condition` is JSON because it is a filter OBJECT, the same shape as the aggregation's own filter — deliberately not a string expression. Must-fail control: restoring the two coercions reddens both new tests with "Failed asserting that '' is null" and "Failed asserting that 0.0 is null". Verified: 17522 tests, 0 failures; phpcs 0 errors; phpmd clean. * fix(graphql): cache AggregationMetricInput in the shared map, not its own field phpmd failed the PR on two counts, both from the same addition: a dedicated `$aggregationMetricInputType` property took TypeMapperHandler to 16 fields (TooManyFields, threshold 15) and its name was past the LongVariable limit. The $inputTypes map already exists for exactly this — a shared input type cached by purpose — so the type moves there under a 'shared:' key. No behaviour change: same instance, same single construction, same reuse.
… an unanswerable @self (#2922) Two ways a cross-schema aggregation returned a plausible wrong number. 1. `metrics` was never read when `from` was set. runCrossSchema() resolved only the SINGULAR `metric`/`select`, so a spec asking for several conditioned figures fell through to the default `count`. A debit/credit segment P&L came back as a ROW COUNT under HTTP 200, with nothing in the envelope naming what had been dropped. The intra-schema path has honoured `metrics` since #2917 — two paths reading different halves of the same declaration is drift that answers wrongly rather than erroring. The cross-schema path now extracts `metrics`, includes it in the cache key, skips the native path when it is present (tryNativeAggregation() takes one metric/field pair and has nowhere to put per-entry `condition`/`as`), passes it to the computeGrouped(metrics:) branch that already existed, and yields `values` rather than a scalar when ungrouped. 2. An `@self.<field>` the parent row cannot answer resolved to null. This was described as failing closed. It does not: the null is applied as a real filter VALUE, so the aggregation returns the target rows whose own field is null. For a segment P&L keyed on `@self.code` that is the unassigned-cost- centre total — returned confidently, identically, for every parent record. No production caller supplies a parent row at all: AggregationController, ReportRenderService and ThresholdEvaluationService each call run() without one. So every such declaration answered wrongly rather than visibly. An absent key now raises and names the reference; a key PRESENT but null still correlates on null, which is a legitimate query. The test that pinned the old behaviour asserted 0 against a fixture whose one row happened to have a non-null field. A row with a null there would have returned 1 under the same code — it was pinning the fixture, not the behaviour. It now asserts the refusal, with that fixture inverted. Refs #1261
`setSchema()` stores a pending ref so that a LATER `setRegister()` can
re-resolve the slug inside the register the caller names. When a register is
already set, `setSchema()` resolves immediately and returns early — and left
the ref set anyway, with nothing remaining to resolve.
ObjectService is shared for the whole request, so that ref outlives the chain
that created it. The next caller's `setRegister()` then re-resolves a finished
operation's slug inside a register that has never heard of it, and refuses
that caller.
Measured 2026-08-27 on a fresh instance. buildiq registers its navigation
entries from `Application::boot()`, which runs on EVERY request and calls
findAll() -> prepareFindAllConfig(), which calls setRegister() and then
setSchema('application'). Every request therefore ended with `application`
pending. Portaliq's PortalResolver — typically the first caller afterwards to
name its own register — was told:
Schema slug "application" is not carried by register "portaliq" (id 35)
PortalResolver fails closed by design, so it returned an empty portal list and
every public portal page 404'd, with nothing in the error attributable to
portaliq. Confirmed by instrumenting setSchema() to record its caller: the
producer was buildiq's boot, several frames below OpenRegister's own findAll().
The same leak reached buildiq (`automation`) and hermiq (`job_log`) on cron.
#2803 cleared the ref on the save path. This is the read path, which it did
not cover — verified against origin/development before writing the fix.
Verified live: with the fix applied, the previously-404ing portal renders in a
browser (heading, search hero, menus, footer) with no other change.
Co-authored-by: Conduction Release Bot <release-bot@conduction.nl>
* feat(schema): validate cron as a string format A cron expression fails in the quietest way a value can. The schedule does not error, it simply never fires — at 03:00, with no request to answer and nobody in the room. There is no later moment where the mistake is cheap, so it is caught where it is written. `format: "cron"` on a string property now validates standard five-field cron: `minute hour day-of-month month day-of-week`, with `*`, numbers, ranges, lists and steps. Registered alongside the existing custom formats in ValidateObject::getValidator().⚠️ `@daily` and its siblings are deliberately REFUSED. Which shortcuts a scheduler resolves varies between implementations, so accepting them would let a document validate against a vocabulary the runtime may not share — producing exactly the silent never-fires this check exists to prevent. Five fields is the form every implementation agrees on. Day-of-week accepts 0 AND 7 for Sunday, because standard cron does; refusing 7 would reject expressions every crontab takes. 32 tests, 21 of them refusals — a format validator that accepts everything passes every happy-path test ever written, so the refusals are the ones with teeth. Verified loading in a running instance, not only in the unit lane. * refactor(schema): split the cron term check on its real seam phpmd refused isValidTerm() at cyclomatic 14 / NPath 1344, against thresholds of 10 and 200. That is not a counter being fussy — it is the shape of a method doing two jobs: deciding whether the STEP is legal, and deciding whether the VALUE or RANGE is legal. They share nothing but the term they came from. Split accordingly, not shaved to fit. Verified by control: phpmd reports both violations against the previous version and none against this one, and the 32 tests are unchanged and still pass. --------- Co-authored-by: Conduction Release Bot <release-bot@conduction.nl>
* fix(tmlo): all three endpoints answered 500 on every request
`GET /api/tmlo/{register}/{schema}/summary`, `.../{id}/export` and
`.../export` are routed, and none of them could succeed even once.
Each called a service method with named arguments that do not exist on it:
summary() findAll(register:, schema:, filters:) -> $register
exportBatch() findAll(register:, schema:, filters:) -> $register
exportSingle() find(identifier:, register:, schema:) -> $identifier
`findAll()` is `findAll(array $config, bool $_rbac, bool $_multitenancy)` and
`find()`'s first parameter is `$id`. PHP raises `Error: Unknown named
parameter …` for each.
WHY IT REACHED THE USER AS A 500 RATHER THAN A HANDLED FAILURE. `Error` is not
an `Exception`. The method's three catch blocks are
`SchemaNotInRegisterException`, `DoesNotExistException` and `Exception`, so the
fatal passed straight through all of them.
## Three separate defects, not one
1. **The named arguments**, above.
2. **`summary()` read `$result['total']`** — a key `findAll()` never returns; it
returns rendered entities. Even with the call fixed, every status would have
reported 0. It now uses `count()`, which returns the int directly, after
`setRegister()`/`setSchema()` — without that context `countAll()` sums every
register/schema table on the instance and all four statuses would report the
same instance-wide total.
3. **`exportSingle()` passed ENTITIES** for `find()`'s `string|int|null $register`
and `$schema`. A TypeError even once the name is right. Now ids.
## Psalm had already found this, and a baseline hid it
`psalm-baseline.xml` carried SEVEN `InvalidNamedArgument` entries plus a
`TooFewArguments` for this one file. The tool detected every one of these
fatals and they were suppressed. The block is removed — all eight entries are
unused now, which is how the wider defect surfaced: fixing `summary()` alone
left psalm reporting "3 extra entries", and the remaining four pointed straight
at `exportSingle()` and `exportBatch()`.
Fixing only the endpoint I was sent to fix would have left two of the three
still fatal.
## Verification
- 5 new tests (10 total in the file, 26 assertions). Mutation-checked: against
the previous controller, 4 of them error with the real production messages —
`Unknown named parameter $identifier` and `$register`.
- `summary()` had NO test at all before this, which is how a method that cannot
succeed shipped and stayed shipped.
- phpcs, phpstan `[OK] No errors`, psalm 0 errors, gate-16 spec-coverage
`count=0`, full suite 17,520 tests / 39,372 assertions with no failures.
Closes #2886.
* fix(repair): register the two repair steps Nextcloud was never told about
gate-98 (repair-step-registration) fails on `development`: two classes
implement `IRepairStep`, are fully written, and are named nowhere in
`appinfo/info.xml`, so Nextcloud never runs them.
**`ImportFlowRegister`** — imports the `flows` register and its `flow` schema.
Unregistered, that register has never been created by an install or upgrade.
This is the class the "a class that exists is not a class that runs" note was
written about; it was found once and never wired up.
**`RenameDutchColumns`** — renames Dutch-named columns to their current names.
Unregistered, no instance has ever had the rename applied.
Both document themselves as idempotent, which is what makes registering them
safe rather than a migration event:
- `ImportFlowRegister` imports "idempotently on install and upgrade" and
downgrades a failure to `$output->warning()` rather than aborting the upgrade.
- `RenameDutchColumns` renames only where the OLD column exists and the NEW one
does not; where MagicMapper already added the new column it COPIES and leaves
the old in place; two sources targeting one destination are refused, not
merged; nothing is deleted.
## Placement
`ImportFlowRegister` goes in BOTH blocks, beside the other `Import*` steps — a
fresh install needs the flows register as much as an upgrade does.
`RenameDutchColumns` is `post-migration` ONLY. It is a data migration over
existing tables, and a fresh install has no Dutch-named columns to rename.
## Verification
The checker's default invocation reports `checked 0 repair step(s)` — it reads
the changed-file set from stdin and there is none outside CI. Run the way the
runner runs it on a full tree (`--all`) it inspects 19, and against the state
before this commit it names both failures with the exact `<step>` line to add.
A gate that inspects nothing would have reported the same "0" either way.
Suite and phpcs unaffected.
---------
Co-authored-by: Conduction Release Bot <release-bot@conduction.nl>
…de them (#2941) 41 declarations across the shillinq registers carry an `expression` key the engine never read. They did not fail; they produced nothing for that figure, which is the shape this whole effort keeps finding. "vatBalance": { "operation": "expression", "expression": "totalVATPaid - totalVATCollected" } Both operands are already computed by the same aggregation. Only the arithmetic between them was missing. ## Why not a second aggregation Declaring one to do the subtraction is not equivalent: it scans the table again, and the two results can disagree if a row is written between the calls — which a trial balance must never do. A derived metric reads the numbers this aggregation just produced, so it cannot disagree with them. ## Why a parser and not eval() These expressions come from register descriptors, which are DATA. eval() on data is arbitrary code execution, and "we control the registers" does not survive the first app that imports a descriptor from elsewhere. MetricExpressionLexer + MetricExpressionEvaluator implement a recursive-descent parser over a closed grammar: expr := term (('+'|'-') term)* term := factor (('*'|'/') factor)* factor := NUMBER | IDENT | '(' expr ')' | ('min'|'max') '(' expr ',' expr ')' | '-' factor No assignment, no property access, no string literals, no function beyond min/max. Everything else is refused BY NAME. The test feeds it `phpinfo()`, `a; system("id")`, `$a + 1`, `a->b`, `a . 'x'` and `a ** 2`. ## What it refuses, deliberately - An IDENTIFIER NAMING NO ALIAS throws, and names the aliases that do exist. Resolving it to 0 would turn a typo into a plausible number: `a - typo` would quietly return `a`. - DIVISION BY ZERO yields null, not INF and not NAN. json_encode() refuses INF outright and NAN compares false against everything, so both travel a long way before anyone notices. - A NON-NUMERIC alias throws rather than being coerced. - A NULL alias propagates as null rather than counting as zero. A derived metric reads only aliases computed BEFORE it, so the `metrics` list is the dependency order; reading a later one raises and says so. ## Wiring - `computeMetrics()` gains the branch. It cannot reach the native path: `metricsNeedPhpPath()` now names `expression` explicitly rather than relying on the `as` check, so removing `as` could never route one at SQL and have it silently compute nothing. - The annotation validator accepts `metric: expression`, and refuses one with no `expression`, no `as`, or a `field` (it aggregates nothing). - The evaluator is constructed internally, like RegisterScopedSchemaResolver: it has no collaborators, and a new required constructor argument would change the signature every existing caller and test already builds. ## Verification - 9 evaluator tests + 2 end-to-end through AggregationRunner - Mutation control: making an unknown alias resolve to 0, and division by zero return INF, each makes the suite fail; reverting restores green - Full suite 17,588 tests, 0 failures - phpcs 0 errors, phpmd clean, phpstan [OK] No errors Refs ConductionNL/shillinq#1261
…ort one (#2910) * feat(registers): show which app-declared registers landed, and re-import one Closes #2903. Eighteen of the fleet's apps ship a register descriptor and import it from a Repair step. Repair steps run on install and `occ upgrade`, and `occ upgrade` reports "No upgrade required" the moment `installed_version` matches `info.xml` — so once an app settles, its descriptor can never be imported again. The steps are also documented never to throw: a failure logs a warning and leaves the instance looking healthy. The cost is not hypothetical. On a dev instance, an `occ upgrade` that reported complete success left 8 OF 15 DECLARED REGISTERS ABSENT, `flows` among them. Two e2e suites died in `beforeAll` on the missing register, and establishing why took an account listing, a register dump and a read of the Repair step's source. WHAT THIS ADDS - `RegisterDescriptorService` — an inventory of every register any installed app declares, each `current` / `behind` / `absent`, plus a forced re-import. - `GET /api/register-descriptors` and `POST /api/register-descriptors/{appId}/{slug}/import`, both admin-only. - `occ openregister:descriptors:list` (with `--problems-only` and `--import`). - An admin-settings panel that LEADS WITH WHAT IS WRONG. THREE DECISIONS WORTH THE REVIEW TIME 1. It enumerates DECLARING APPS, not resolved registers. Reading the resolver keys or the configuration rows would list only what already imported — the interesting row, the app whose seed never ran, has neither. An inventory built that way reproduces exactly the silence it exists to break. 2. Discovery is by SHAPE, not filename. The fleet's names vary (`flow_register.json`, `credential-providers.json`, `n8n_workflows.openregister.json`), and a `*_register.json` glob would quietly omit the rest — shrinking the inventory instead of failing, which is the same invisibility one level up. A test reads OpenRegister's own `lib/Settings`, counts the declaring files independently of the code under test, and asserts every one is found. 3. The import is ALWAYS forced. `ImportHandler` short-circuits on `$force === false && version_compare($shipped, $existing, '<=')`, and that is precisely the state an administrator presses the button in: absent, or failed to write, while the counter says current. An unforced re-import would report success and do nothing in every case that motivates the action. `absent` and `behind` stay distinct everywhere — they need different actions and carry different risk. Absent means a code path is dead; behind means it runs against an older contract. ON CUSTOMISED SCHEMAS A forced re-import rewrites the base. An extending schema REFERS to its base — `Schema::getAllOf()` returns "Array of schema IDs, UUIDs, or slugs" — so it is impervious to the base moving. That is a property of the implementation rather than a law, so it is pinned by an e2e test instead of assumed: an extension materialised as a copy would silently revert somebody's customisation, through the button offered as a repair. VERIFIED LIVE occ openregister:descriptors:list → 15 declared · 7 current · 0 behind · 8 ABSENT --import=flows → imported occ openregister:descriptors:list → 15 declared · 9 current · 0 behind · 6 ABSENT GET /api/register-descriptors → 200, 15 rows POST /…/openregister/flows/import (versions already match) → 200 {"outcome":"imported"} GET /api/register-descriptors as a non-admin → 403 RegisterDescriptorServiceTest 11 tests, 52 assertions, OK register-descriptors.spec.ts 4 passed, twice back to back ALSO IN HERE `flow-schedule.spec.ts` and `federated-config.spec.ts` were dead — their `beforeAll` resolved the `flows` register that had never been seeded. With the register imported they run, and two stale fixtures surfaced: both hung a flow step's `type` + `config` off an EDGE, which the validator now refuses outright ("a node is the action that runs and an edge is sequence, so nothing reads this step"). The flow was never created and the failure surfaced two assertions later as an empty bundle, reading exactly like a broken bundler. Steps moved onto nodes. `federated-config.spec.ts` additionally authored into the `flows` register while the bundler reads the `openregister_flows` table, so it now authors through `/api/flows`. That store split, and an `install` that returns a uuid for a flow which exists in neither store, are real product defects and are filed separately as #2905 — not patched over here. * style(e2e): prettier-format, and mark #2905 expected-to-fail rather than red Three things CI's `Frontend Check (format)` and a local run turned up: - Prettier formatting on the three files this branch adds or rewrites. - `reg` and `sch` in `federated-config.spec.ts` became dead once `makeFlow()` moved to `/api/flows`. The `flows` register is still a precondition — 'a register bundles into a portable OpenAPI document' bundles it BY SLUG — so the lookup stays and only the unused ids go. On an instance where the descriptor never landed, that lookup failing UP FRONT with `registers slug=flows` is the most useful thing the block can do. - The bundle/install/run test is split in two. Install now ends at the uuid; whether that flow can RUN is its own test, marked `test.fail()` and naming #2905 — the defect where `install` reports HTTP 200 with `{"installed":[uuid]}` for a flow that exists in neither store. Split rather than folded, because marking the whole test expected-to-fail would stop the bundle-and-install assertions from ever failing again — trading one tracked defect for four untested behaviours. `test.fail()` rather than a skip, because a skip goes quiet and STAYS quiet. This reports red the day #2905 is fixed and the expectation needs removing. `test.fail()` is called inside the test body, not in the describe body: a bare one there would mark every test declared after it too, quietly excusing the allowlist test from ever having to pass. Verified: 5 expected passes, 1 expected failure, `unexpected: 0`. * fix(repair): register ImportFlowRegister — it was written and never run `ImportFlowRegister` appeared ZERO times in `info.xml`. Not in `<post-migration>`, not in `<install>`. The class is complete — constructor, version gate, error handling, a docblock explaining that it exists so "a flow can live in OpenRegister itself and not only in a consuming app" — and Nextcloud never invoked it, so the `flows` register never appeared. ADR-005 Rule 1 is exactly this failure: > Any new OR-owned register descriptor MUST be accompanied by a `lib/Repair/` > step that imports it. Shipping the JSON alone does nothing at runtime. and its Consequences name the cost: > An OR-owned register that ships JSON without a Repair step silently never > appears — a recurring author error this ADR exists to prevent. The class complied. The REGISTRATION did not, and nothing checks that half. HOW IT WAS FOUND `occ openregister:descriptors:list`, added in this branch, reported `flows` absent on an instance where `occ upgrade` had just reported complete success. Before that command existed, the only evidence was two e2e suites dying in `beforeAll` on `registers slug=flows`, which reads like a broken test. The correlation across the whole instance is exact and worth recording: every descriptor with a registered importer → current every descriptor with NO importer → absent `flows` was the single row that broke the pattern — an importer class with no registration — which is what pointed at the cause. WHAT THIS DOES NOT FIX Six openregister descriptors still report absent because they ship with no importer at all: bag, brp, dso, kvk, merge-operation, n8n-workflows. Each is either an ADR-005 Rule 1 violation or a descriptor that should not ship as one. That is a decision for whoever owns them, not a guess to make here — reported on the issue instead. * i18n(registers): translate the descriptor panel into all 36 required locales CI's `Frontend Check (test:l10n)` failed on the register-descriptor panel: its 19 strings were in neither `l10n/en.json` nor any locale file. The house convention, visible in every recent feature commit, is that a change adds its keys to all 37 files with REAL translations — not an English fallback. 19 keys × 35 locales, plus en and nl. 740 lines, purely additive: the only deletion in each file is the trailing-comma reflow on its last existing key. 🔴 A LITERAL ` ` IN THE SOURCE IS NOT A NON-BREAKING SPACE TO THE EXTRACTOR. `@nextcloud/l10n-non-breaking-space` wanted an nbsp before the ellipsis, and the fix went in as the seven characters ` `. JavaScript resolves that escape at runtime, so the browser saw a real nbsp — but the l10n extractor reads SOURCE TEXT, so it registered the key as `Reading descriptors …` while `t()` would look up `Reading descriptors<nbsp>…`. The key would never have matched, and every locale would have silently fallen back to English for that one string, with the parity check reporting full coverage. Replaced with a real U+00A0. `--write` also picked up one key that was missing from `en.json` while already translated in the locale files: "Retention period". Pre-existing, unrelated to this panel, and left in because the gate would fail on it either way. Verified: `l10n-parity: OK — every required locale is at full parity (no missing keys, no empty values)`. * test(newman): a register-descriptors contract suite, and fix the CSRF gap in delegation's REGISTER DESCRIPTORS `register-descriptors.spec.ts` lives in `tests/e2e/api-direct/`, which `playwright.config.ts` excludes from every project — so CI runs none of it. Four green specs and zero CI coverage look identical from the outside. This is the half CI can see, registered as a domain in `run-all.sh`. The assertions are mostly about what the API says when something is WRONG, because that is what this endpoint exists for. `absent` and `behind` must be first-class counts rather than something a client derives by filtering — a consumer that has to compute "is anything wrong" is one that renders a healthy-looking total and moves on. And a version-matched re-import must report `imported`, never `skipped`: `ImportHandler` short-circuits on `version_compare(shipped, installed, '<=')` unless forced, which is exactly the case an administrator presses the button in. 🔴 THE DELEGATION COLLECTION COULD NEVER HAVE PASSED Running the new domain through the orchestrator failed, so I ran `delegation` — already merged, already "verified" — as a control. It failed identically: expected status 200 but got 412 expected { message: 'CSRF check failed' } to have property 'awaitingMyAnswer' ALL NINE of its requests were missing `OCS-APIRequest: true`, so every one is rejected by Nextcloud's CSRF middleware before reaching a controller. A collection registered in CI that 412s on every request proves exactly nothing, and it is the same defect its own registration comment warns about one level up. Header added to all nine. The control is what found it. Had I only run my own collection I would have concluded the orchestrator was broken and moved on. TWO ENVIRONMENT FINDINGS ALONG THE WAY - `NEWMAN_RUNNER=host` invoked a bare `newman`, the only one of the three branches requiring a GLOBAL install. Without it every domain failed rc=127 — "command not found", reported in the same red as a real assertion failure. Now `npx --yes newman`, as the `exec` branch already does. - The default `sidecar` runner rewrites `localhost:8080` to `http://<container>`, which Nextcloud answers 400 for as an untrusted domain. Not changed here — it is a local-environment matter, and CI supplies its own network — but it is why a local `bash tests/newman/run-all.sh` fails for every domain, not just new ones. Verified: `Passed: 2, Failed: 0` for both domains through the orchestrator, and 13/13 assertions for the new collection standalone — including the version-gate case, which ran rather than taking its skip path. * test(e2e): flow-schedule now runs — and it was testing a store the scheduler cannot read Refs #2905. `flow-schedule.spec.ts` could not fail, because it could not run: its `beforeAll` resolved a `flows` register that had never been seeded on any instance (#2903), so the file errored before reaching an assertion. Seeding the register is what made it execute — and it immediately failed three different ways, each one real. 1. A SLUG IS NOT UNIQUE ACROSS SCHEMAS. `idBySlug('schemas', 'flow')` searched globally; this instance carries TWO schemas with slug `flow` while the `flows` register lists only one. It picked the wrong one, and posting an object to a register/schema pair that do not belong together answers 400 — an opaque status with nothing in it about slugs. Now resolved from the register's own `schemas` array, which is the authority on what it carries. 2. 🔴 THE SCHEDULER CANNOT SEE REGISTER-AUTHORED FLOWS. Control, on a live instance — the same definition, byte for byte, into each store, then one worker tick: POST /api/flows (openregister_flows table) → 1 run queued POST /api/objects/18/184 (the `flows` register) → 0 runs `FlowScheduleService` resolves candidates from `FlowLocator::scheduledFlows()`, which reads the table. A schedule authored in the register never fires, with no error and no status message — the run history is simply empty, which reads as a broken scheduler rather than a flow it never saw. `lib/Settings/flow_register.json` calls that register "the store the resolver reads by default — so triggers, sub-flows and the /test endpoint all work with a flow authored here". That is not true today, and it is now the SECOND subsystem found blind to it, after the federation bundler. Reported on #2905 rather than patched: either the register becomes a real flow store for these paths or the descriptor stops claiming it is one, and that is a design call. Both fixtures now author through `/api/flows`. The negative control matters as much as the positive one: asserting "0 runs" about a flow the scheduler could never have seen would have passed for a reason unrelated to `trigger: manual`. 3. A SCHEDULE MUST NAME WHO IT ACTS AS (ADR-099). The fixture had no `openregister.trigger-schedule` node, so no `runAs`. Added, matching `delegation-parking.spec.ts`. Verified: 2 passed, 0 failed, twice back to back. * fix(gates): satisfy semantic-auth and spec-coverage, and close out #2905's e2e GATE-9 — MY CONTROLLER'S ANNOTATION CONTRADICTED ITS OWN BODY Both methods carried a no-admin-required tag while calling `requireAdmin()`. That is not a harmless disagreement: it is how an endpoint comes to be reachable by everyone while its code reads as guarded, and it is the exact defect gate-9 was written for. Copied from a sibling controller that has it too. 🔴 AND THE TAG IS NOT SPELLED IN THE COMMENT EXPLAINING ITS ABSENCE. A docblock that writes the literal DECLARES it, so prose about removing it restores what it describes. My first fix did precisely that, and only a `grep -c` caught it. This repo has paid for that lesson once already, when a comment about removing `@covers` was parsed as `@covers` and reddened six CI cells. GATE-5 THEN FIRED, AND THE OBVIOUS FIX WAS THE WRONG ONE Removing the tag left `import` with no declared auth posture. The cheap way to satisfy gate-5 is a no-CSRF tag — `index()` already carries one as a GET — but reaching for it on a state-changing POST would disable CSRF protection to buy a green gate. The panel posts through axios with Nextcloud's request token and needs nothing of the sort. `#[AuthorizedAdminSetting(settings: OpenRegisterAdmin::class)]` declares the posture the body actually enforces, which is what BOTH gates were asking for. Verified live rather than by the gate alone: admin 200/200, non-admin 403/403. GATE-16 — eight methods missing @SPEC (two on the occ command, six on the Vue panel). Tagged. #2905's E2E EXPECTATION IS GONE, BECAUSE THE DEFECT IS `test.fail()` did its job: it went RED THE DAY #2915 LANDED, because a test expected to fail that starts passing is itself an error. A skip would have gone quiet and stayed quiet, and this assertion would still be switched off. Two things surfaced as it came back to life: - The terminal is `stopped`, not `completed`, and that is THIS FIXTURE'S doing. `EndNode` throws `FlowStop`, "which the engine turns into a clean `stopped`" — so a flow whose last node is `openregister.end` ends `stopped`. `completed` is for a flow that runs off the end of its graph, which is what this fixture was before the step moved from its edge onto a node. Asserted exactly rather than widened to accept either: a matcher taking both would also accept the fixture silently losing its end node. - The allowlist test, dead until the container guard landed, failed 200-vs-403. 🔴 THE CONTROL IS FINE. `occ config:app:set` runs in a CLI process while the request is served by a web process holding its own IAppConfig cache, which the CLI cannot invalidate — so the allowlist is set, readable via `occ config:app:get`, and briefly invisible to the endpoint. Confirmed by hand: once the web process holds a fresh cache the same request answers 403 and names the source. The test now retries and then SKIPS naming what went unverified, because a security assertion that cannot see its own precondition must be reported as neither a pass nor a refutation of the control. Verified: gate-5, gate-9, gate-16 PASS and gate-98 NOT APPLICABLE locally; 33 e2e passed / 0 skipped / 0 failed, twice back to back; both Newman domains pass. * fix(registers): a mock descriptor is not a missing register Investigating the seven descriptors the panel reported ABSENT turned five of them into a defect in the PANEL, not in the fleet. THE CORRELATION IS EXACT, across OpenRegister's own 14 descriptors: type=mock 5 descriptors importer: none, all five type=core 8 descriptors importer: seven of eight type=integration 1 descriptor importer: none — and app=n8n `x-openregister.type: mock` marks sample data imported ON DEMAND — a demo, a test fixture — not something an instance is expected to carry. Every mock ships without a Repair step, deliberately. Reporting them as ABSENT put five permanent red rows in front of every administrator, and the obvious response to those rows — write the missing importers — would have seeded five mock registers onto every instance in the fleet. 🔴 A PANEL WHOSE LOUDEST SIGNAL IS NOISE IS ONE PEOPLE STOP READING, which is the same silence it was built to break. Mocks are now skipped. `reimport()` still reaches them by explicit app+slug, which is how sample data is meant to be loaded. AND A ROW NAMES THE APP THAT DECLARES IT. `n8n_workflows.openregister.json` lives in OpenRegister's lib/Settings and declares `app: n8n` — it is n8n's register, shipped alongside. Attributing it to `openregister` because of the directory it sits in tells the reader the wrong owner, and an inventory exists to say whose problem a row is. MEASURED, on a live instance with twelve apps installed: before 15 declared · 7 current · 0 behind · 8 ABSENT after 25 declared · 23 current · 0 behind · 2 ABSENT Both survivors are real: `merge-operation` (openregister, type=core, no Repair step — an ADR-005 Rule 1 gap, reported on #2903) and `n8n-workflows`, now filed under n8n where it belongs. THE FLEET-DISCOVERY TEST CAUGHT THIS CHANGE and had to move with it — 14 declaring files, 9 reported. Its independent count now applies the same stated rule, still computed by reading the files rather than by asking the service: a count derived from the code under test could only ever agree with itself. Verified: 14 unit tests / 47 assertions; e2e and Newman green. * fix(settings): make the admin panel delegation-eligible so the auth attribute binds phpstan on #2910: Parameter $settings of attribute class AuthorizedAdminSetting constructor expects class-string<OCP\Settings\IDelegatedSettings>, string given. `AuthorizedAdminSetting` is the only auth attribute that declares "administrator" WITHOUT also disabling CSRF, and it takes a `class-string<IDelegatedSettings>`. `OpenRegisterAdmin` implemented plain `ISettings`, so the attribute had nothing valid to bind to. The two alternatives were both worse: - Bind to `GenericAdminSettings`, the one class in this repo that already implements the interface. It is the AppHost panel; the register-descriptor endpoints are not part of it, and naming a panel they do not belong to would make the declaration a formality rather than a fact. - Add a no-CSRF tag to the POST. That is the cheap way to satisfy gate-5 and it disables CSRF protection on a state-changing endpoint to buy a green gate. 🔴 THIS DELEGATES NOTHING BY ITSELF. `IDelegatedSettings` makes a panel ELIGIBLE for delegation, which an administrator must then configure. `getName()` returns null and `getAuthorizedAppConfig()` returns an empty allowlist, so today's behaviour is unchanged — full administrators only — while the attribute gets a real settings class to bind to. Verified live rather than by phpstan alone, because this touches middleware: admin GET 200, admin POST 200, non-admin GET 403. phpcs, phpstan and psalm clean; register-descriptors e2e green. * fix: dedupe ImportFlowRegister, and cover the controller and command 🔴 THE MERGE REGISTERED THE SAME REPAIR STEP TWICE, SILENTLY. Another session fixed the same defect in #2923 — "register ImportFlowRegister and RenameDutchColumns" — while this branch carried my fix. Git merged both without a conflict, because they sit at different positions in the same block. Result: `ImportFlowRegister` appeared twice in `<post-migration>` and twice in `<install>`. Idempotent, so probably harmless at runtime, and wrong. THEIR PLACEMENT IS BETTER AND IS THE ONE KEPT. They put it BEFORE the flow steps, with the reason that "every flow step below it silently operated on nothing" — which is the actual ordering constraint. Mine sat later, among the other Import* steps, where the flow steps above it would still have run first. Salvaged from mine: the provenance sentence, since it records HOW the gap was found (`occ openregister:descriptors:list` on an instance where `occ upgrade` had just reported complete success, 8 of 15 registers absent). Verified by parsing rather than by eye: post-migration 20 steps, install 18, zero duplicates in either.⚠️ AND `RenameDutchColumns` IS NOT A SECOND UNREGISTERED STEP. I reported it as one earlier; #2923 had already registered it. The gate now reports it clean. Correcting the record rather than "fixing" something already fixed — that is the third collision with a parallel session today. COVERAGE: the ratchet failed at 43.25%, naming 194 statements added without tests. `RegisterDescriptorService` had 14 tests; the controller and the occ command had none. Both now covered, and the assertions are the REFUSALS rather than the happy path: - a non-administrator is refused, and `reimport` is expected NEVER — asserting only the 403 would pass for a controller that ran the import and then refused; - an anonymous caller gets 401, not 403; - a failed import answers 422, not a 200 carrying bad news; - listing exits ZERO with absent registers (a state to decide about, not a failure — non-zero would turn a report into a broken cron job) while a failed re-import exits non-zero (there the caller asked for something); - `--problems-only` filters rows but NOT the tally, because a filter that also changes the totals hides what it filtered; - `--import` without `--app` is refused rather than guessed, since two apps may ship a register with the same slug. 14 tests, 36 assertions, OK. * fix(repair): merges were recorded into a register that never existed `merge_operation_register.json` is a `type: core` descriptor declaring the `merge-operation` register and its `mergeOperation` schema, and it shipped with NO Repair step. Per ADR-005 Rule 1 that means it never appeared on any instance: "shipping the JSON alone does nothing at runtime." 🔴 THIS ONE IS NOT COSMETIC. `MergeService::merge()` writes the audit row for every merge straight into it: $this->objectService->saveObject( object: $mergeOperation, register: self::MERGE_REGISTER, // 'merge-operation' schema: self::MERGE_SCHEMA // 'mergeOperation' ); and `reverseMerge` reads `preMergeSnapshot` back out of that row to undo a merge. A register that does not exist is a merge history that does not exist, and reversibility that cannot be exercised. HOW IT WAS FOUND. `occ openregister:descriptors:list` reported it ABSENT alongside six others. The correlation across the app's fourteen descriptors was exact — every one with a Repair step present, every one without missing — and after the five `type: mock` descriptors were correctly excluded (a mock is not a missing register), `merge-operation` was the only non-mock left in the second group. The outlier in a clean correlation named the defect, for the second time in this programme: the first was ImportFlowRegister. Verified live rather than by the file's existence: the register imports and the inventory moves from 2 ABSENT to 1 — the survivor being `n8n-workflows`, which declares `app: n8n` and is correctly attributed there rather than here. merge-operation register: PRESENT id=40 schemas=[1312] mergeOperation schema: 1312 Registered in both <post-migration> and <install>, once each, verified by parsing the XML rather than by eye — this branch has already had one silent duplicate from a merge with a parallel session's identical fix. * fix(flow): one store per flow — migrate register-authored flows into the table Closes the second half of #2905. OpenRegister kept TWO stores for "a flow": the `openregister_flows` table behind /api/flows, and objects in the `flows` register. Every subsystem reads the table. Nothing reads the register. 🔴 MEASURED WITH CONTROLS, not inferred — the same definition, byte for byte: POST /api/flows + one FlowScheduleWorker tick -> 1 run queued POST /api/objects/18/184 + one FlowScheduleWorker tick -> 0 runs bundle {flowIds:[table-backed]} -> 1 flow bundle {flowIds:[register-authored]} -> 0 flows A schedule authored in the register never fires and never bundles, with no error anywhere — the run history is simply empty, which reads as a broken scheduler rather than a flow nothing ever saw. 🔴 AND THE DESCRIPTOR NAMED A CLASS THAT DOES NOT EXIST. `flow_register.json` called that register "the store the resolver reads by default — so triggers, sub-flows and the /test endpoint all work with a flow authored here", crediting an `OpenRegisterFlowResolver`. That identifier appears nowhere in this repository except inside that sentence. A reader following the documentation lands in the store nothing reads. The description now says what is true. THE MIGRATION copies every register-authored flow into the table: - the UUID IS PRESERVED, so sub-flow references and existing run rows resolve; - a flow already in the table is SKIPPED — this runs on every upgrade, and a second copy would fire twice; - OWNERSHIP carries over, because a flow with no organisation is invisible to every scoped read (the #2915 defect) and a migration that dropped it would move the flow into the right store and straight out of sight; - it arrives DISABLED. A schedule that starts firing during an upgrade against data nobody re-checked is worse than one an administrator switches on; - the register rows are LEFT IN PLACE, so an unattended step stays reversible; - one failure does not stop the rest, and the COUNTS are always printed. 🔴 THE TEST CAUGHT A BUG THE SUMMARY LINE HID. `Flow` inherits Nextcloud's `Entity`, whose setters are MAGIC via `__call` — so `method_exists($flow, 'setName')` is FALSE and my copy loop, guarded on it, skipped every field. The step ran, reported "1 migrated", and wrote a flow with no name, no trigger and no nodes. Only the assertion that the name came across found it; the count said success either way. Same magic-method trap as `Organisation::getUsers()` earlier in this work. Verified: 7 tests / 18 assertions; phpcs, phpstan and psalm clean. The live run happens on `occ upgrade` — repair steps cannot be invoked standalone, and `maintenance:repair` would fire every app's, so this is pinned by unit tests rather than by a live run I have not done. * fix(repair): the flow migration read ObjectEntity rows as arrays `ObjectService::findAll()` returns `ObjectEntity` objects. The migration indexed them as arrays, so enabling the app died outright: Error: Cannot use object of type OCA\OpenRegister\Db\ObjectEntity as array in lib/Repair/MigrateRegisterFlowsToTable.php:173 A fatal in a repair step during `occ app:enable` means the app does not install at all. The unit tests were green throughout, because every one of them mocked `findAll()` to return the array shape the step assumed — a fake built from the caller's assumption validates the assumption, not the service. Only CI, which actually enables the app, disagreed. Rows are now normalised through the entity's own accessors (`getObject()`, `getUuid()`, `getOwner()`, `getOrganisation()`), with the array shape still accepted so a future serialised read path stays correct. `testItReadsRealObjectEntitiesNotArrays()` builds a REAL ObjectEntity. Verified as a control: reinstate the array access and that test alone fails with the same fatal — which is exactly the property the array fixtures lacked. phpcs/phpstan/psalm clean, 8 tests pass. * fix(schemas): order the schema list newest-first, so a created schema is on it `schema-crud.spec.ts` has been failing on this branch, on both E2E runs and on retry: the spec creates a schema through the API, confirms it persisted through a GET, opens the schemas index — which renders — and then cannot find the row. ## The list was never ordered `SchemasIndex` loads every schema and slices ONE page client-side: paginatedSchemas() { const { page, limit } = this.paginationData const start = (page - 1) * limit return schemaStore.schemaList.slice(start, start + limit) // 20 rows } The slice takes whatever order the API happened to return. A just-created schema has the highest id and therefore sorts LAST, so it lands on the final page. On any instance holding more schemas than fit on a page it is simply not there — the plain "I just made this, where is it?" failure. It is not an error and not an empty list. The page renders perfectly, without the row the user is looking for, which is why nobody had seen it. ## Why it surfaced here This PR adds `ImportMergeOperationRegister`, which materialises the `merge-operation` register and its `mergeOperation` schema at install. That is one more schema in the E2E instance — enough to cross the 20-row page. The E2E log shows the run reaching schema id 24. So the spec was passing only because the seeded instance held fewer schemas than one page, and this PR pushed it over. The defect is older than the PR; the PR is what made it observable. Reverting the import would hide it again. ## The fix An explicit `orderedSchemas` — highest id first — that `paginatedSchemas` slices. Id is the only monotonic field every schema carries. This replaces an UNDEFINED order rather than a chosen one: CnIndexPage is in prop mode on this view and the component handles no `@sort`, so there is no user-facing sort to conflict with. No test is weakened to make this pass. The spec still asserts the created schema renders as a real row in the list, which is the guarantee it was written for.
…2946) `MetricExpressionEvaluator` landed on development today with #2941 carrying nine phpcs errors, and phpcs scans `lib`. So `development` itself is red and every branch cut from it inherits the failure — it is not the fault of whatever PR happens to report it. Mine reported it on #2943, which touches neither this file nor this area. Nine errors, no behaviour change: - five inline IF statements (176, 238, 288, 313, 348) expanded to blocks. The nested ternary inside the sprintf at 313 becomes a named `$available` local, which is also the only one that was genuinely hard to read. - four `$this->expect(...)` calls given the named parameter the standard requires for calls to internal code. Also adds the `@spec` tag the class was missing, pointing at openspec/specs/aggregation-api/spec.md — the target its siblings in this directory already use, and a file that exists. VERIFIED BEHAVIOUR-NEUTRAL BY A/B, not by reading: the aggregation suite is 263 tests / 524 assertions and returns exactly that with the original file restored and again with the fixed one. The evaluator's own suite is 9 tests / 26 assertions, green, with a reflection check confirming the class under test is the edited file rather than another checkout's copy. phpcs: 9 errors -> 0, confirmed against a control run of the original through the same command after an earlier run had silently registered no sniffs at all and reported the untouched file as clean. Co-authored-by: Conduction Release Bot <release-bot@conduction.nl>
…s (ADR-111) (#2939) * feat(demo-data): ship demo data for every schema this app supplies (ADR-111) Three objects per (register, schema) pair, each generated from the schema itself and validated against it. Not installed automatically: a mock register is imported on demand, from the setup walkthrough or `occ openregister:descriptors:list --app=<id> --import=<slug>`. Attributed to the app id in appinfo/info.xml — `x-openregister.app` is what the descriptor inventory resolves a register to an app by, so the checkout directory name would name an app that does not exist. * fix(demo-data): drop the n8n register from openregister demo data `n8n_workflows.openregister.json` declares `x-openregister.app: n8n`. Its five schemas are the n8n app's, and they carry no `slug`, so embedding them here failed this repo's own `validate-register` on two checks. Regenerated with the generator ownership fix: 8 registers, 10 schemas, 30 objects, 0 validation failures. --------- Co-authored-by: Conduction Release Bot <release-bot@conduction.nl>
…w` (#2937) Drops both `type: "custom"` flow pages and the two components behind them. FlowsIndex.vue and FlowDetailPage.vue are deleted and their registry entries removed. "type": "index", "config": { "entitySource": "flows", "columns": [...] } "type": "flow" WHAT THOSE COMPONENTS WERE. FlowsIndex rendered CnIndexPage with `:objects` from useFlowStore - the wrapper named index sources exist to remove. FlowDetailPage wrapped CnFlowDetail and handled save/run, which is exactly what the shared CnFlowEditorPage does, `router.replace` included so a newly created flow picks up its server-assigned id. Checked line by line before deleting rather than assumed. COLUMNS ARE STATED EXPLICITLY here, not left to the source defaults. This is the ENGINE's cross-app surface: it passes no app filter and so carries an `app` column a leaf app's own flow list does not. The source supplies defaults; a manifest that sets columns still wins, which is what keeps that column. `sidebarComponent: FlowDetailSidebar` is unchanged - it is a page-level key rather than part of `config`, so the controls sidebar survives the type change. Verified against the pre-change manifest. A FIX THAT COMES FREE. FlowsIndex bound `@rowClick` while CnIndexPage emits `row-click`, so opening a flow by clicking its row never worked here. The shared page wires it correctly. Both `_note`s are rewritten. They said "Custom page rather than type:index" and gave the reason - true when written, false now, and a stale note that explains a decision is worse than none. Bumped to ^2.20.0: below it the runtime does not register `flow`, and a manifest naming a page type the runtime does not know renders nothing rather than failing. Manifest validated against the installed schema (2.26.0, PASS, 0 errors) and the control build compiles - 48 artifacts. Co-authored-by: Conduction Release Bot <release-bot@conduction.nl>
PHP Quality (phpmd) fails on development with a single violation: MetricExpressionEvaluator.php:180 ElseExpression The method parseExpression uses an else expression. parseTerm, directly below it in the same file, already handles its two operators with a guard and a continue rather than if/else, and is clean. This makes parseExpression match its sibling. Behaviour is identical: the while condition admits only + and -, so the guard and the fall-through cover exactly the two branches the else did.
#2954) Co-authored-by: Conduction Release Bot <release-bot@conduction.nl>
#2951) * fix(deps): nextcloud-vue 2.21.0, which restores the flow create button The E2E on development fails on both attempts: flow-controls.spec.ts:173 flow controls render, and a flow can be built, saved and run getByRole("button", { name: "New flow" }) — element(s) not found #2937 moved the flow list to an ordinary type:index over the named source "flows". nextcloud-vue 2.20.0 shipped named sources (#800) but read only their `columns` — `addLabel` and the routes were defined and never read, so the migration lost the create button. nc-vue #818 fixes exactly that and first ships in 2.21.0. Verified against the PUBLISHED artifacts, not the source: 2.21.0/dist carries the namedSource.addLabel branch, 2.20.0/dist does not. The lock is regenerated with npm 11 (engines.npm ^11.0.0; npm 10 does not implement min-release-age) and with the repo .npmrc in place — without its min-release-age-exclude[]=@conduction/* the two-day cooldown would have silently resolved BACKWARDS on a release-day version. The lock diff is one line: no transitive package was added, removed or moved. * fix(deps): nextcloud-vue 2.21.0, which restores the flow create button The E2E on development fails on both attempts: flow-controls.spec.ts:173 flow controls render, and a flow can be built, saved and run getByRole("button", { name: "New flow" }) — element(s) not found #2937 moved the flow list to an ordinary type:index over the named source "flows". nextcloud-vue 2.20.0 shipped named sources (#800) but read only their `columns` — `addLabel` and the routes were defined and never read, so the migration lost the create button. nc-vue #818 fixes exactly that and first ships in 2.21.0. Verified against the PUBLISHED artifacts, not the source: 2.21.0/dist carries the namedSource.addLabel branch, 2.20.0/dist does not. The lock is regenerated with npm 11 (engines.npm ^11.0.0; npm 10 does not implement min-release-age) and with the repo .npmrc in place — without its min-release-age-exclude[]=@conduction/* the two-day cooldown would have silently resolved BACKWARDS on a release-day version. The lock diff is one line: no transitive package was added, removed or moved.
The flow-page migration is already on development here, but the lock still pinned an older release. 2.20 declares a named index source's columns, create button and row actions without reading them, so the migrated page renders a columnless table with no working create action — which reads as an empty list rather than a broken page. The lock is the operative part: CI installs with `npm ci`, so the caret alone changed nothing about what installs. Co-authored-by: Conduction Release Bot <release-bot@conduction.nl>
.github#597 set cancel-in-progress on the shared quality.yml, but a caller-level concurrency cancels the whole run before the called workflows setting can apply -- so that fix reached only the apps that declare no concurrency of their own. Measured 2026-08-28 over push runs on development since #597 merged: caller silent 0 of 11 cancelled caller says true 7 of 13 cancelled (54%) This repo is in the second group. pull_request keeps cancelling, where superseding really is correct.
…2963) Every guard so far protects an OPERATION — find() (#2790), findAll() and the save path (#2803), the read path (#2918) — by discarding the pending schema ref as it enters. `setRegister()` is not an operation and has no such guard: it takes whatever ref is pending, re-resolves it inside the register the caller just named, and a miss THROWS out of a method whose caller only asked to name its own register. Measured on the development instance 2026-08-27, with #2918 already merged and running: every public read of a portaliq portal failed with Schema slug "application" is not carried by register "portaliq" `PortalResolver` calls setRegister('portaliq') first and setSchema('portal') after — the correct order — and never mentions `application` at all. The slug belongs to buildiq, which registers navigation from Application::boot() on every request. The portal served its shell and answered 404 for its own site, menus, pages and glossary: a whole app's public surface down over a slug it does not own. openconnector and a pipelinq repair step are recorded in this file's own comments as earlier victims of the same shape. So a ref that cannot be resolved inside THIS register is now discarded rather than thrown on, and the schema context is cleared with it. That second half is what keeps this honest: a caller that really did chain setSchema('typo')->setRegister($r) still fails — at its operation, with a missing schema context — instead of silently reading whichever table a stale context last pointed at. Substituting a wrong-but-plausible schema is the one outcome worse than throwing. The discard is logged with the ref, the register and a note saying which of the two cases the reader is in. The regression test was observed FAILING against development with the exact production message above, and passing with the change.
`flow-controls.spec.ts:173` has been failing on development with
Locator: getByRole('button', { name: 'New flow' })
Error: element(s) not found
which says nothing useful. Chasing it (#2957) ruled out five hypotheses
without touching the actual cause:
- the label is correct -- "New flow" is what nextcloud-vue 2.20.0
declares for the `flows` named index source
- the version is correct -- package.json ^2.20.0, lock pins 2.20.0
- the published artifact is complete -- indexSources.js and
useNamedSource.js are both in the npm tarball's dist/
- the control is a real button -- CnActionsBar renders NcButton with
data-testid="cn-cta-primary", not an overflow menu item
- showAdd is not suppressed -- defaults true on both components
The one fact unobservable from outside a failing run is whether
`resolveIndexSource('flows')` returned the source or null. The resolver
announces that on the console, and nothing was capturing it.
So this captures console warn/error for the duration of the test and,
when the button is missing, fails with:
- whether ANY primary CTA rendered, and its text. A named source
supplies "New flow"; an unresolved one degrades to an ordinary index
whose CTA reads "Add {type}". That single string separates the two
cases.
- the collected console lines, including the resolver's warning.
No change to what the test asserts -- it still requires the button. It
just stops throwing away the evidence when it does not appear.
Verified: npm run lint rc=0, prettier clean, `playwright test --list`
compiles the file.
Refs #2957
Co-authored-by: Conduction Release Bot <release-bot@conduction.nl>
…ccess (#2956) * fix(descriptors): the demo-data import seeded nothing and reported success Found by running the ADR-111 flow on a live instance rather than trusting the unit tests. `occ openregister:descriptors:list --app=larpinq --import=larpinq` printed: larpinq: register "larpinq" imported. and seeded ZERO of the descriptor's 30 demo objects. The register ended up carrying the REAL descriptor's 11 schemas, not the mock's 10, and oc_openregister_objects stayed empty. TWO DEFECTS, both mine. 1. A MOCK IMPORTED UNDER THE APP'S OWN CONFIGURATION IDENTITY. `importFromApp` stamps `imported_config_<appId>_version` and a content hash. larpinq ships BOTH `larpinq_register.json` and `larpinq_mock_register.json`, and both declare the register slug `larpinq` -- so importing the mock under the plain app id writes over the real descriptor's stamp and reads its hash. A mock now imports under `<appId>.mock`. This is the rule ImportMergeOperationRegister already states in its own docblock, for exactly this reason: a version gate shared between two descriptors masks one of them. I applied that discipline there and not here. 2. THE POST-CHECK COULD NOT SEE A NO-OP. It asked only whether a config version existed for the slug. For a mock that is already true -- the app's own descriptor stamped it at install -- so `outcome: imported` was returned over an import that created nothing. A descriptor that declares objects and seeds none of them is now a FAILURE that names the numbers, and the command prints "N of M demo object(s) imported, K skipped" instead of a bare "imported". That is the same "state the counts, always" rule the repair steps follow, and it is what would have made this diagnosis immediate instead of a long dig through the importer. Three tests, each verified by control: reinstating either defect produces 2 failures. Note the control itself had to be fixed first -- running the suite from a worktree with a symlinked vendor/ resolves these classes out of the MAIN checkout, so the tests were green over a file I had not edited. The bootstrap now preloads the worktree copy. phpcs/phpmd/phpstan/psalm all clean; 31 tests, 92 assertions. * test(descriptors): cover the branches the counts fix added The coverage ratchet failed this branch: FAIL: coverage of the code this change KEEPS or ADDS dropped by 5.31%. base 147/155 -> head 171/191 statements Fair: the command's new counts-printing branch was never entered, because the existing test mocks `reimport()` to return `['outcome' => 'imported']` with no `counts` key at all. The branch that formats "N of M demo object(s) imported" could not run. Three tests, one per path through it: a full seed (30 of 30), a PARTIAL seed (29 of 30, 1 skipped -- a different event from a full one, and the operator deciding whether the demo is usable needs the difference), and a descriptor declaring no objects, which keeps the short sentence rather than padding every real register import with "0 of 0 demo object(s)". 27 tests, 82 assertions. Local coverage measurement was attempted and abandoned rather than faked: the worktree has no composer install of its own, and running under the main checkout's vendor/ resolves these classes out of THAT tree -- the same wrong- file trap that made an earlier control report OK over a file I had not edited. CI measures this honestly; a local number from the wrong tree would not. --------- Co-authored-by: Conduction Release Bot <release-bot@conduction.nl>
…Array" (#2943) * fix(schemas): the extended-by map cast schema OBJECTS to the string "Array" `findAllExtendedBy()` resolved each `allOf`/`oneOf`/`anyOf` entry with `(string)$entry`. An entry is a SCHEMA OBJECT in standard JSON Schema — `{"$ref": "person"}` — and casting one yields the literal "Array", which matches no key in the lookup. So the reverse map came back EMPTY, while every request logged a notice per row: Array to string conversion at lib/Db/SchemaMapper.php#4148 GET /index.php/apps/openregister/api/schemas?_limit=1000 Measured across the fleet's descriptors: 236 allOf/oneOf/anyOf entries, ALL of them objects, NOT ONE a bare string. The feature was inert for every real schema — an "extended by" relationship that is never found rather than one that errors, which is why nobody had seen it. Found in a learniq E2E log, where the notice repeats on every `/api/schemas?_limit=1000`. `parentIdentifierFromAllOfEntry()` already encodes the three shapes that occur and is already tested. The defect was that this caller did not USE it — so the new arms drive `findAllExtendedBy()` itself, not the resolver: a test of the resolver would have stayed green throughout. That helper's own docblock records that an earlier caller passed arrays through and 500'd; this one passed them through and went quiet instead. 4 tests, verified by control in a tree with the app's own autoloader: with the cast reinstated, the two object-shaped arms fail and the scalar and inline arms still pass. phpcs reports 0 errors either side; the phpstan and psalm findings within reach are pre-existing and in other files. * fix(aggregation): replace the else with an early continue, satisfying both tools phpmd failed parseExpression() on ElseExpression. The `else` was mine: the earlier phpcs fix replaced an inline ternary with if/else, and the two tools forbid opposite shapes -- phpcs bans the ternary, phpmd bans the else. Neither shape is available. parseTerm(), a few lines below in the same file, already shows the way out: handle the first operator and `continue`, then fall through to the second. No ternary, no else, and it matches the file's own pattern rather than inventing a third one. VERIFIED WITH A CONTROL, and the control had to be fixed first. Running the suite from the worktree resolved this class out of the MAIN checkout, because vendor/ was symlinked there and its autoloader owns the namespace -- so the test was measuring a file I had not edited, and reported OK when subtraction was replaced by addition. Preloading the worktree copy ahead of the autoloader fixes it. With the right file under test: my refactor is OK (9 tests, 26 assertions), and breaking subtraction produces 2 failures, so the suite does discriminate. phpcs 0 findings, phpmd 0 findings, 0 else clauses in the file. --------- Co-authored-by: Conduction Release Bot <release-bot@conduction.nl>
Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
…2969) phpstan analyses the whole tree, so this pre-existing error surfaced on an unrelated l10n pull request (#2634) that does not touch this file: Call to function is_scalar() with int|string will always evaluate to true. Line Service/ObjectService.php [ERROR] Found 1 error `$currentSchemaRef` is `int|string|null` and the null is already excluded before this point, so `$pendingRef` is `int|string` here — the scalar test could only ever be true and the gettype() fallback was unreachable. Dropped rather than suppressed. A defaulted read that can never default reads as a handled edge case that does not exist, and hides the day the type actually widens. Verified: php -l clean, and `phpstan analyse --memory-limit=1G` over all 1504 files reports [OK] No errors — the exact command CI runs. Co-authored-by: Ruben van der Linde <juan.claude@conduction.nl>
Carries the pending-schema-ref fix (#2918) and the repair-step registrations (#2923) to beta, so a stable release can be cut from them. TWO CONFLICTS, BOTH VERSION-ONLY — AND RESOLVING THEM THE OBVIOUS WAY WAS WRONG. `appinfo/info.xml` and `openapi.json` each conflicted on one hunk, the version string. Taking beta's side of the FILE (`--ours`) resolves that hunk correctly and silently reverts everything else in it, because beta's info.xml predates development's. It took four repair-step registrations with it: ImportMergeOperationRegister, RenameDutchColumns, ImportFlowRegister, MigrateRegisterFlowsToTable All four had just been registered on development precisely because gate-98 found they had never run. A file-level resolution to a line-level conflict put them straight back. So both files are taken from development in full, with only the version string replaced by beta's — the convention the repo's own beta-sync PRs document, and the release workflow recomputes it from the latest stable tag regardless. Verified after resolving, not assumed: unregistered repair steps none openapi.json valid JSON pending-schema-ref fix present in ObjectService
Contributor
Quality Report — ConductionNL/openregister @
|
| Check | PHP | Vue | Security | License | Tests |
|---|---|---|---|---|---|
| lint | ✅ | ||||
| phpcs | ✅ | ||||
| phpmd | ✅ | ||||
| psalm | ✅ | ||||
| phpstan | ✅ | ||||
| phpmetrics | ✅ | ||||
| eslint | ✅ | ||||
| stylelint | ✅ | ||||
| build | ✅ | ||||
| check-specs | ✅ | ||||
| test-l10n | ✅ | ||||
| format | ✅ | ||||
| check-schema-l10n | ✅ | ||||
| check-l10n-js | ✅ | ||||
| composer | ✅ | ✅ 175/175 | |||
| npm | ✅ | ✅ 545/545 | |||
| app:check-code | ⏭️ | ||||
| info.xml | ✅ | ||||
| REUSE | ❌ | ||||
| PHPUnit | ✅ | ||||
| Newman | ✅ | ||||
| Playwright | ✅ | ||||
| Hydra gates | ✅ |
Quality workflow — 2026-08-28 11:30 UTC
Download the full PDF report from the workflow artifacts.
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 join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Carries the pending-schema-ref fix (#2918) and the repair-step registrations (#2923) to
beta, so a stable release can be cut from them.Supersedes #2906, which went unmergeable as both branches moved.
hotfix/*because the branch-protection workflow accepts onlydevelopment,mainorhotfix/*as a source forbeta, and this branch carries a merge resolution rather than beingdevelopmentitself.Two conflicts, both version-only — and the obvious resolution was wrong
appinfo/info.xmlandopenapi.jsoneach conflicted on one hunk, the version string. Taking beta's side of the file (--ours) resolves that hunk correctly and silently reverts everything else in it, because beta'sinfo.xmlpredates development's.It took four repair-step registrations with it:
All four had just been registered on development precisely because gate-98 found they had never run on any instance. A file-level resolution to a line-level conflict put them straight back.
Both files are therefore taken from
developmentin full, with only the version string replaced by beta's — the convention this repo's own beta-sync PRs document, and the release workflow recomputes it from the latest stable tag regardless.Verified after resolving
Why this release is needed
The current stable
v1.1.6does not carry what the leaf apps need. Measured on a fresh demo built from all-stable pins (openregister 1.1.6+opencatalogi 1.0.11+portaliq 0.1.4): OpenCatalogi's catalog works, but portaliq's register materialised only 2 of its 13 schema tables — there is no table for theportalschema, so no portal object can exist and the demo portal is never seeded.The same stack against
1.1.6-unstable.20260827111159works completely. The stable was cut at 10:17 and that unstable at 11:09, so stable 1.1.6 is older than an unstable sharing its version number and lacks what portaliq needs.