feat(quality-config): adopt Nextcloud's coding standard — stricter, never different - #375
Merged
Merged
Conversation
…ion, and a CI/CD page that states where we diverge from Nextcloud Measured across all 18 core apps on canonical <app>@development, 2026-08-12: psalm.xml, phpstan.neon, playwright.config.ts and code-quality.yml each have EIGHTEEN distinct variants; eslint.config.js has 17; phpmd.xml 15. Even NamedParametersSniff.php — a custom RULE, not a setting — has six versions, so the same named rule enforces six different things. launchpad's is a superset that knows about Entity magic accessors; opencatalogi's is a 231-line stub with none of the scoping logic that calls addWarning() where the others call addError(); doriath ships two of the three sniffs not at all, including the one that blocks \OC::$server, removed in NC 34. quality-config/ is now the single source for phpcs, phpmd, phpstan and the three custom sniffs, shipped inside the existing conduction/hydra-gates package so `composer install` is the pull and a laptop behaves like CI. Two mechanics were MEASURED against phpcs 3.13.6 and 4.0.4 before anything was written, and the first result was a failing positive control: - Rules and custom sniffs DO inherit through <rule ref>, including sniff files at paths relative to the central ruleset. The first fixture registered 1 sniff instead of 2 and looked like proof this cannot work; the fixture was wrong — a custom sniff must sit at <Standard>/Sniffs/<Category>/<Name>Sniff.php. - <file> must stay in the app stub. phpcs resolves it relative to the ruleset that DECLARES it and it propagates through <rule ref>, so a <file>lib</file> here resolves to quality-config/lib inside vendor/ and every consumer dies with exit 3. psalm.xml is deliberately NOT stubbed: Psalm has no config inheritance. The frontend configs are templates, not live imports: ESLint flat config resolves plugins relative to the config file, so they need an npm channel (@conduction/nextcloud-vue, which all 18 apps already depend on). Both are written up rather than half-wired. docs/WayOfWork/ci-cd.md is new and states the divergences plainly — 4 spaces vs Nextcloud's tabs, next-line vs same-line braces, `(int) $x` vs `(int)$x` — plus the ones that are accidents rather than choices: - cs:check / cs:fix are nextcloud/coding-standard's script names and are aliases for PHPCS here, while 17 apps carry nextcloud/coding-standard in require-dev with no .php-cs-fixer config and no invocation anywhere. - No app ships an .editorconfig, so an editor configured from Nextcloud work defaults to tabs, which PHPCS then rejects. - info.xml declares NC 32-34 while nextcloud/ocp is pinned ^31 in 15 apps and nothing is tested on 34 — which is why the NC 34 removal of \OC::$server needed a hand-written sniff: the analyser was looking at NC 31. - 13 apps run stylelint with an UNQUOTED glob, so the shell expands src/**/ one level deep and nested components are silently unlinted. - No occ integrity:sign-app step, so published packages carry no appinfo/signature.json. - .prettierrc in 14 apps, prettier a dependency in none. Four claims in the existing pages were wrong and are corrected: PHPCS is not PSR-12, the pipeline is 18 job groups not four, Hydra is not "coming soon", and `channel: dev` development builds do exist (verified GitHub-prerelease-only — that path never POSTs to apps.nextcloud.com).
…entral ruleset unparseable `--warning-severity=0` in a <!-- --> block is illegal XML. phpcs exits 3 with "Ruleset is not valid ... Double hyphen within comment" BEFORE registering a single sniff, so every consuming app would have gone red on a comment. Caught by running the real config through a real stub from a real vendor/ tree in php:8.3-cli, which is also now the evidence that the mechanism works: the custom sniffs load from vendor/, and on a tab-indented fixture calling \OC::$server->getConfig() with positional arguments all three fire — NoLegacyServerAccessors, NamedParameters, and the 4-space ScopeIndent rule that is this fleet's deliberate divergence from Nextcloud core.
…emantics Policy change: Conduction code must PASS nextcloud/coding-standard unchanged. We may be stricter, never different. The previous version of this directory argued the opposite — that 4 spaces was a deliberate divergence worth documenting — and that framing is now wrong. Measured: php-cs-fixer was run over openregister's 1,427 files, then the old PEAR ruleset over the RESULT. 111,932 findings, 111,747 of them auto-fixable formatting — this file arguing with the formatter. Every whitespace, brace, indent and alignment sniff is removed; the same measurement now yields 185, all semantic (181 @SPEC warnings, 2 line-length, 2 SPDX end-char). The named-parameter and legacy-accessor sniffs fire ZERO times, which is what stricter-but-compatible looks like when it is true rather than assumed. The sharpest case: Squiz.ControlStructures.ElseIfDeclaration FORBIDS the elseif keyword that Nextcloud's elseif fixer REQUIRES. Two tools cannot both be right about one token, so only one gets an opinion about tokens. Docblock PRESENCE is semantic and stays; docblock LAYOUT is phpdoc_align's. The alignment codes are excluded individually rather than by dropping the sniff — losing the presence requirement would be a real regression. Each excluded code was observed firing, not guessed. tests/compatibility.sh makes this permanent: format a fixture with php-cs-fixer, run PHPCS over the result, fail on any formatting sniff. Its positive control asserts the fixer actually changed the fixture first — otherwise a clean PHPCS pass proves nothing. Also adds Nextcloud's .editorconfig verbatim (no fleet app had one, so editors defaulted to whatever the developer last worked in) and a .php-cs-fixer.dist.php stub.
…e-divergence framing The page previously argued that 4 spaces was a considered divergence worth documenting. The policy is the opposite: we may be stricter than Nextcloud, never different, so a difference is a defect rather than a dialect. Rewrites the formatting section around the measurement that settled it — all 1,427 openregister files failing nextcloud/coding-standard, and the 111,932 -> 185 drop once PHPCS stops arguing with php-cs-fixer — and around the tool split that makes the policy structural: conduction/coding-standard EXTENDS Nextcloud's Config and cannot override it, PHPCS keeps only semantics. Adds the two traps an adopting app hits first: cs:check/cs:fix used to be PHPCS aliases under Nextcloud's names, and a .php-cs-fixer.dist.php missing its autoloader reports a fatal as zero files needing changes.
rubenvdlinde
changed the title
…livery is not a publish Both composer packages are now on Packagist, which deletes a 'repositories' block from all 18 apps — a per-app file, and per-app files are what drift. The block also carried no-api:true, so composer cloned the whole .github repo, docs site included, on every install. The section exists mostly to record a verification trap found by walking into it. Packagist creates a push webhook on submission, so it LOOKS wired. Measured: a real push to conduction/coding-standard delivered 202 OK and ten minutes later dev-main still pointed at the previous commit. Three ways to misread that as success — a ping delivery is not a push delivery, 202 means queued not published, and package.time is the CREATION timestamp and never moves. The only honest check is whether the source reference matches git HEAD. The actual missing piece is the Packagist GitHub App, not installed on the org. It cannot be installed through the API — GitHub Apps require a browser consent flow — so there is no gh command for it.
rubenvdlinde
added a commit
to ConductionNL/nextcloud-app-template
that referenced
this pull request
Aug 12, 2026
This repo pinned hydra-gates-ref: v1.3.0. gate-65, added in ConductionNL/.github#375, flags it, and it is right to: a pin is a silent expiry date. 22 repos once sat on v1.0.1 while 16 gates were dead fleet-wide and every one reported PASS (.github#159); a default later flipped at @main then reached those same old runners and turned them red on gates they had no subject matter for (#173). Both directions come from the two halves of one system moving independently. The shared workflow is consumed at @main, so the gate package must be too — then a gate fix reaches this repo with no commit in this repo. conduction/hydra-gates is still constrained to dev-feat/centralised-quality-config in composer.json, which gate-65 also flags, correctly. That one becomes ^1.0 when #375 merges.
Centralising configuration does not stop it drifting. The 18 mutually different
psalm.xml files measured on 2026-08-12 were all copies of something that had been
shared once. What stops recurrence is a gate that fails when an app walks away
from the centre, or freezes itself against it.
Eleven rules, each one a defect that was LIVE in the fleet rather than a
preference. Measured against all 18 core apps at development:
cs-script-wired-to-phpcs 36 (2 scripts x 18 apps)
no-php-cs-fixer-config 18
phpcs-not-centralised 18
phpcs-declares-formatting-sniffs 18
no-editorconfig 18
coding-standard-not-required 18
nextcloud-coding-standard-declared-directly 17
local-custom-sniffs 16
stylelint-glob-unquoted 13
ocp-below-declared-minimum 13
The last figure cross-checks an independent count made three hours earlier: 15
apps pin nextcloud/ocp ^31 while declaring NC 32-34, minus docudesk and launchpad
on dev-master, which track the tip and are correctly not flagged.
NO PINNING is rule 10 and applies to all three shared things — the gate package,
the coding standard, and the shared workflow. floats and is fine; an exact
version, a dev-branch constraint, or a hydra-gates-ref that is not main, is not.
The rule found its first real target immediately: this fleet's own app template,
pinned at hydra-gates-ref v1.3.0.
23-assertion self-test, discovered automatically by tests/run-helper-suites.sh.
Every positive control is paired with a negative one, and three of them exist to
stop the rule over-firing: ^1.0 must not read as a pin, an explicit
hydra-gates-ref: main must not read as a pin, and a COMMENTED-OUT rule or pin
must not read as live — the gate-64 defect, where grepping for a string matches
it inside every comment.
The checker's terminal 'checked N rule(s)' line is required by the runner. A
crashed checker and a clean repo are otherwise the same silence.
…e way Nextcloud does Two changes, both to follow Nextcloud's own release template rather than run beside it. 1. THE VERSION NOW LIVES IN THE REPOSITORY. It used to exist only inside the built package: sed rewrote appinfo/info.xml in the copy under package/, and the committed file kept whatever stale number was last hand-edited. That is incompatible with Nextcloud's appstore-build-publish template, which reads the version FROM info.xml and asserts the tag matches it. Following Nextcloud means the repository is the source of truth, so the bump is committed first and the tag is cut from that commit — the tag then names a tree whose info.xml says what the tag says. Retires "Version Is Not In The Repository" from release-process.md. Guarded both ways: no commit when the number has not moved (a re-run must not create an empty commit and re-tag it), and a failed push is a HARD STOP with a diagnosable message. Releasing past it would put the old version number inside a package labelled with the new one. 2. occ integrity:sign-app — THE GAP. Every release this fleet has ever published went out WITHOUT appinfo/signature.json. We signed the tarball, which is what the App Store API authenticates the upload with, and stopped there. Nextcloud's integrity checker verifies something else entirely: a per-file manifest INSIDE the app directory. Without it, `occ integrity:check-app` reports the app as unverifiable, and an administrator running an integrity check sees our apps as the ones that cannot be vouched for. The server that provides occ is fetched at the version the app DECLARES it needs, read from info.xml rather than hardcoded — the same principle as icewind1991/nextcloud-version-matrix. The release cannot drift from the app's stated support range because it is computed from it. POSITIVE CONTROL, because integrity:sign-app exits 0 on several paths that produce no manifest: the step asserts signature.json exists AND that it hashes more than zero files. An empty manifest verifies nothing and would ship looking exactly like a signed package.
Found by running the new signing step against larpingapp rather than reading it.
It resolved min-version 8 and died on a null download URL, because info.xml
carries min-version on <php> too and <php> comes first.
Measured across all 18 apps, the unanchored form returned:
13 apps -> 8 from <php min-version="8.3"/>
portaliq -> 28 from a <nextcloud> element, but the WRONG one
The first is loud: updates.nextcloud.com has no NC 8, so the release stops. The
second is not. It resolves a real server and signs the app against a Nextcloud
four majors below the one it supports — a plausible wrong answer, which is the
worse kind.
Verified end to end afterwards on larpingapp's actual tree: min-version 32 ->
nextcloud-32.0.13 -> `occ integrity:sign-app` exit 0, appinfo/signature.json
121 KB hashing 619 files with both signature and certificate present. That is the
artefact every release this fleet has published so far has been missing.
rubenvdlinde
added a commit
to ConductionNL/nextcloud-app-template
that referenced
this pull request
Aug 12, 2026
ConductionNL/.github#375 is merged and tagged v1.7.0, so the temporary dev-feat/centralised-quality-config constraint can go — along with the VCS repositories block, since both packages are on Packagist now. gate-65 reported two findings on this branch and now reports none: 15 rules checked, zero deviations. The gate found its own author's pins first, which is the behaviour you want from it.
rubenvdlinde
added a commit
that referenced
this pull request
Aug 12, 2026
…check Nextcloud ships phpunit-mariadb, phpunit-mysql, phpunit-oci and phpunit-sqlite as separate workflow templates, because database-portability bugs are invisible on a single backend. This fleet ran PostgreSQL only. The `database` input already accepted sqlite, pgsql and mysql, but the phpunit job wired exactly one of them: the postgres service was keyed off `inputs.database`, and the install block had a pgsql branch with everything else falling through to a bare `--database <x>` that works for sqlite and cannot work for mysql — no host, no port, no credentials, and no service container to connect to. So "mysql" was a value the input accepted and the job could not honour. WHAT CHANGED - `database-test-matrix`, a JSON array, joins the phpunit matrix beside php-version and nextcloud-ref. - A mysql:8.4 service sits beside postgres:16. Both are keyed off matrix.database rather than inputs.database, and a service whose image evaluates to an empty string is not started — which is how one job definition serves several backends. - The install block is a case statement with real host/port/credentials per backend, and an explicit failure on an unknown one. - pdo_mysql and mysqli join the extension list unconditionally. Conditional extensions would mean a mysql leg silently falling through and testing sqlite while its name said mysql. INERT UNTIL AN APP OPTS IN. The default is an empty array, which collapses to `[inputs.database]` — today's dimensions exactly. Both sides of that comparison go through fromJSON on purpose: a string test against '[]' accepts '[ ]' and '[""]' and then resolves to an EMPTY MATRIX VECTOR, and an empty vector does not skip a job, it deletes it from the run with no row, no annotation and no trace. Measured on this repo's own self-test for frontend-checks: 19 jobs instead of 21. Verified here against all three spellings of empty plus a real three-backend array. POSITIVE CONTROL IN THE JOB. `maintenance:install` can exit 0 having fallen back to another backend, and a suite passing on sqlite while its leg is labelled mysql is worse than no leg at all — it is a green cell asserting something about a database it never touched. The step reads `occ config:system:get dbtype` back and fails if it does not match the leg's own name. That completes the five checks Nextcloud runs and we did not: info.xml XSD (#383), app:check-code and REUSE (this branch), integrity:sign-app (#375), and multi-database PHPUnit.
rubenvdlinde
added a commit
that referenced
this pull request
Aug 12, 2026
…, multi-DB PHPUnit (#385) * feat(quality): adopt occ app:check-code and REUSE, the last two Nextcloud checks Nextcloud runs five checks this fleet did not. #383 added the first (info.xml against the App Store XSD). This adds two more; multi-database PHPUnit is the remaining one and is a larger change to the phpunit job, tracked separately. occ app:check-code ------------------ Nextcloud's own tool for private and deprecated API usage. Our closest equivalent was ONE hand-written PHPCS sniff covering ONE symbol — \OC::$server, removed in NC 34 — written by hand precisely because static analysis could not see the removal while nextcloud/ocp sat a major below the declared min-version. app:check-code is the general form of that sniff: it knows the whole private surface of the server it ships with, needs no per-symbol maintenance, and moves forward when Nextcloud does. The server is fetched at the version the app DECLARES, anchored to the <nextcloud> element. Unanchored, the pattern returns 8 for thirteen fleet apps (from <php min-version="8.3"/>) and a plausible-but-wrong 28 for a fourteenth. REUSE ----- Nextcloud runs reuse.yml on every app. We had a PHPCS sniff checking @license and @copyright in PHP docblocks — a proxy for one file type that says nothing about images, JSON, YAML, or the LICENSES/ directory REUSE requires. It also closes a loop the coding-standard migration opened: the SPDX InvalidEndChar exception in the shared ruleset exists because a trailing full stop turns a machine-parsed SPDX expression into an invalid one. That exception is only worth having if something actually parses those expressions. BOTH ARE NON-BLOCKING BY DEFAULT, and that is a decision rather than timidity. Every app here reaches into server internals somewhere — OCA\DAV, OC_App and Doctrine\DBAL are all ignored by name in phpstan-base.neon — and only 2 of the 4 apps sampled ship a REUSE.toml at all. A gate that is red on arrival is a gate nobody turns on: that is how openregister ended up with an E2E job that had never once succeeded. Each carries a `*-blocking` input to flip per app once its findings are worked down. app:check-code carries a positive control. occ exits 0 on paths that inspect nothing — an app directory it cannot see, a command that bailed on the environment check — so the step asserts the log contains a recognisable verdict before treating any exit code as evidence. A silent exit 0 must not read as "no private API usage". * feat(quality): multi-database PHPUnit — the fifth and last Nextcloud check Nextcloud ships phpunit-mariadb, phpunit-mysql, phpunit-oci and phpunit-sqlite as separate workflow templates, because database-portability bugs are invisible on a single backend. This fleet ran PostgreSQL only. The `database` input already accepted sqlite, pgsql and mysql, but the phpunit job wired exactly one of them: the postgres service was keyed off `inputs.database`, and the install block had a pgsql branch with everything else falling through to a bare `--database <x>` that works for sqlite and cannot work for mysql — no host, no port, no credentials, and no service container to connect to. So "mysql" was a value the input accepted and the job could not honour. WHAT CHANGED - `database-test-matrix`, a JSON array, joins the phpunit matrix beside php-version and nextcloud-ref. - A mysql:8.4 service sits beside postgres:16. Both are keyed off matrix.database rather than inputs.database, and a service whose image evaluates to an empty string is not started — which is how one job definition serves several backends. - The install block is a case statement with real host/port/credentials per backend, and an explicit failure on an unknown one. - pdo_mysql and mysqli join the extension list unconditionally. Conditional extensions would mean a mysql leg silently falling through and testing sqlite while its name said mysql. INERT UNTIL AN APP OPTS IN. The default is an empty array, which collapses to `[inputs.database]` — today's dimensions exactly. Both sides of that comparison go through fromJSON on purpose: a string test against '[]' accepts '[ ]' and '[""]' and then resolves to an EMPTY MATRIX VECTOR, and an empty vector does not skip a job, it deletes it from the run with no row, no annotation and no trace. Measured on this repo's own self-test for frontend-checks: 19 jobs instead of 21. Verified here against all three spellings of empty plus a real three-backend array. POSITIVE CONTROL IN THE JOB. `maintenance:install` can exit 0 having fallen back to another backend, and a suite passing on sqlite while its leg is labelled mysql is worse than no leg at all — it is a green cell asserting something about a database it never touched. The step reads `occ config:system:get dbtype` back and fails if it does not match the leg's own name. That completes the five checks Nextcloud runs and we did not: info.xml XSD (#383), app:check-code and REUSE (this branch), integrity:sign-app (#375), and multi-database PHPUnit.
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.
The policy
"Stricter" means adding a rule Nextcloud has no opinion on. It never means giving one of their rules a different value.
We failed it completely
Measured 2026-08-12,
nextcloud/coding-standard(php-cs-fixer 3.95) over openregister'slib/: 1,427 of 1,427 files fail.curly_braces_positionindentation_typephpdoc_alignbinary_operator_spacescast_spacesconcat_spaceNot "stricter" — a different dialect.
The split
conduction/coding-standard— new repo,v1.0.0quality-config/— this PRquality-config/Conduction\CodingStandard\Configextends Nextcloud's and merges a privateADDITIONSontoparent::getRules(). Its invariant suite fails the build ifADDITIONSshares one key with the parent set, if a parent rule is dropped, or if a parent rule's value changed. The policy is enforced by inheritance, not by review. 13/13 pass, each with a positive control.ADDITIONSis empty — a result, not an omission. Every rule we want beyond Nextcloud's is semantic, and php-cs-fixer cannot express any of them.Why PHPCS had to be cut back
Two formatters with overlapping jurisdiction make an app unfixable:
cs:fixandphpcsdemand opposite things. Running the old ruleset over php-cs-fixer-formatted code gave 111,932 findings, 111,747 auto-fixable formatting:That last line is the whole problem in miniature: Squiz forbids the
elseifkeyword Nextcloud'selseiffixer requires.After stripping every formatting sniff, the same measurement yields 185 findings, all semantic — 181
@spec, 2 line-length, 2 SPDX. The named-parameter and legacy-accessor sniffs fire zero times: stricter-but-compatible, demonstrated rather than assumed.Docblock presence stays, docblock layout goes to
phpdoc_align— alignment codes excluded individually rather than by dropping the sniff, since losing the presence requirement would be a real regression. Each excluded code was observed firing, not guessed.Proven end-to-end
nextcloud-app-template#141, in
php:8.3-cli, installing fromvendor/:Three custom sniffs register and fire from the vendored package. That PR is the recipe the other 18 apps follow.
What's in this PR
quality-config/phpcs.xml— semantic-only rulesetquality-config/phpcs-custom-sniffs/— three sniffs, canonicallylaunchpad's NamedParameters (a 525-line superset;opencatalogi's 231-line copy callsaddWarning()where the rest calladdError()) +openregister's SpecTag and NoLegacyServerAccessorsquality-config/phpmd.xml,phpmd-unusedparams.xml,phpstan-base.neonquality-config/editorconfig— Nextcloud's, verbatim. No fleet app had one, so editors defaulted to whatever the developer last worked inquality-config/tests/compatibility.sh— format a fixture, run PHPCS over the result, fail on any formatting sniff. Its positive control asserts the fixer actually changed the fixture first, because otherwise a clean PHPCS pass proves nothingquality-config/stubs/— what an app's files becomedocs/WayOfWork/ci-cd.md+ four corrections to existing pagesMechanics measured, not assumed
Against phpcs 3.13.6 and 4.0.4:
<rule ref>, including sniff paths relative to the central ruleset. The first fixture registered 1 sniff instead of 2 and looked like proof this cannot work — the fixture was wrong (a sniff must sit at<Standard>/Sniffs/<Category>/<Name>Sniff.php).<file>must stay in the app stub — phpcs resolves it relative to the declaring ruleset, so a central one becomesquality-config/libinsidevendor/and every consumer exits 3.--inside an XML comment makes a ruleset unparseable, before any sniff registers. Caught by running it; it would have turned 18 apps red on a comment..php-cs-fixer.dist.phpwithoutrequire vendor/autoload.phpfatals, and in--format=jsonthat fatal is reported as zero files needing changes. It reads exactly like a clean tree.Blast radius
Zero until an app adopts it.
quality.ymlis unchanged; no app's files change in this PR.Deliberately not here
psalm.xml— Psalm has no config inheritance. Needs a generator or a drift gate.quality-config/frontend/— templates only. ESLint flat config resolves plugins relative to the config file, so a config invendor/cannot reachnode_modules. Frontend is being homed in@conduction/nextcloud-vue, which all 18 apps already depend on.Follow-ups
Roll to 18 apps ·
quality-config-driftgate · frontend homing · NC 34 forocp+ PHPUnit +psalm.xml phpVersion· Psalm to strictest level · quote the stylelint globs (13 apps lint one directory deep) · wire Prettier · align frontend versions · adopt the five Nextcloud checks we lack (info.xml XSD,app:check-code,integrity:sign-app, REUSE, multi-DB PHPUnit) · chain Nextcloud's release template · register both packages on Packagist.