chore(quality): make composer check:strict an honest gate - #14
Merged
Conversation
The strict gate did not work. Measured on nextcloud:32-apache (PHP 8.3.32, imagick), matching the CI php-version 8.3 pin, against a clean composer install from composer.lock: lint exit 0 phpcs exit 3 NOT swallowed - the gate has been RED, not green phpmd exit 127 'phpmd: not found' (bare binary), swallowed by || echo psalm exit 1 swallowed by || echo phpstan exit 1 swallowed by || echo (no config at all) test:all exit 1 swallowed by || echo Every failure was a configuration error, not a code finding: this is a Python ExApp sidecar wrapper and lib/ does not exist, yet phpcs.xml, psalm.xml and the phpmd script all pointed at it. No CI workflow invokes check:strict, so nobody saw the red. The only PHP this repo authors is the 408-line NamedParametersSniff, which had therefore never been linted. Point the tools at it: - phpcs.xml / psalm.xml / phpmd + phpstan paths -> phpcs-custom-sniffs - add phpstan.neon (level 5) - phpstan is applicable, not N/A: there is real PHP here and the config costs nothing - add analysis-bootstrap.php supplying OCP/NCU and PHP_CodeSniffer symbols (neither package declares a composer autoload section). This removed a 52 -> 19 -> 0 phpstan cascade and a 1 -> 19 -> 0 psalm cascade by supplying the missing symbol source, not by suppressing diagnostics. - drop the || echo fallbacks from phpmd/psalm/phpstan; use ./vendor/bin/X Findings on the real target, and what was done with each: phpcs 5 (CRLF line endings, @package/@author order, @author format, missing @license, @license needs a URL) - ALL FIXED phpmd 4 - 2 fixed ($j -> $ptr), 2 baselined (class complexity 65>50, hasUnnamedArguments 20>15); both are complexity/style inherent to token-stream parsing, tracked in an issue, not defects psalm 0 phpstan 0 Positive control: a probe method with var_dump, a short variable and a call to an undefined method makes all four analyzers fail, and the phpmd baseline does not mask it - so the green result is not vacuous. test:all is left dead on purpose (no phpunit.xml, no tests/), but its skip message no longer claims a Nextcloud environment is the reason.
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
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.
Make
composer check:strictan honest gateThis repo's
check:strictwas not a working gate. This PR repairs it and reports exactly what the repaired gate surfaces.Analysis-path verification (before believing any number)
All analyzers were run inside
nextcloud:32-apache, which matches the CIphp-version: 8.3pin. Host PHP 8.2 is too old; PHP 8.5 (nextcloud:34) crashes Psalm 5.26.1 in its ownMethodCallAnalyzer.php -v→ PHP 8.3.32php -m→ imagick present (a missing imagick alone has shifted psalm counts elsewhere in the fleet)vendor/was built by a cleancomposer installfromcomposer.lock(49 packages), not copied from a shared checkout.vendor/nextcloud/ocp/OCPis a real directory, 132 entries / 960 PHP files, not a symlink and with noOCP.bakbeside it. Verifiedrequire 'vendor/nextcloud/ocp/OCP/IRequest.php'; interface_exists('OCP\IRequest')→true.interface_exists('OCP\IRequest')via the Composer autoloader isfalse, and that is correct —nextcloud/ocpdeclares noautoloadsection; it is a stubs-only package consumed through phpstanscanDirectories/ psalmextraFiles. This is not the "empty stub dir" failure mode../vendor/bin/phpmdexists. The shipped script invoked barephpmd, which does not resolve (127 command not found).Measured BEFORE any change
|| echo?check:strictlintfind … -print0 | xargs -n1 php -lphpcs./vendor/bin/phpcs --standard=phpcs.xmlphpmdphpmd lib text phpmd.xmlphpmd: not found)psalm./vendor/bin/psalm --threads=1 --no-cachephpstan./vendor/bin/phpstan analyse --memory-limit=1Gtest:all./vendor/bin/phpunit --colors=alwaysCorrection to the premise this work started from. The gate was not "green but dead" — it was red.
phpcscarries no|| echo, socomposer check:stricthas been exiting 1. Nobody noticed because no CI workflow in this repo invokescheck:strict,phpcs,psalm,phpstanorphpmdat all; the onlycomposercall anywhere in.github/workflows/iscomposer install --no-devin the three release workflows.Not one of those failures was a code finding. Every one was a configuration error with the same root cause.
Root cause: the tools were aimed at a directory that does not exist
This is a Python ExApp sidecar wrapper.
ex_app/lib/main.py+requirements.txt+Dockerfileare the app. There is nolib/directory, yetphpcs.xml(<file>lib</file>),psalm.xml(<directory name="lib"/>) and thephpmdscript (phpmd lib …) all pointed at it, andphpstanhad no config at all.The entire PHP toolchain was copied from the PHP-app template. The only PHP this repo authors is one file:
phpcs-custom-sniffs/CustomSniffs/Sniffs/Functions/NamedParametersSniff.php(408 lines) — the sniff enforcing our named-parameter rule. Becausephpcswas aimed atlib, that sniff has never once been linted, and the sniff itself has never run on anything in this repo.PHPStan decision: APPLICABLE — config added
lib/is absent, but PHP is present (the 408-line sniff), and it is real logic worth analysing. Addingphpstan.neoncosts nothing at runtime and gives the sniff genuine level-5 coverage, so phpstan is not marked N/A here. The config is deliberately minimal:level: 5,paths: [phpcs-custom-sniffs],scanDirectories: vendor/nextcloud/ocp. None of openregister's app-specificignoreErrorswere copied — none of them apply, and importing them would pre-suppress findings this repo has not earned.Both configs carry a comment: add
libback the moment PHP app code lands here.Counts the repaired gate surfaces, and how each was resolved
Symbol-source cascades were eliminated before counting — the raw numbers were badly inflated:
52→19→ 0.52was mostly "unknown classPHP_CodeSniffer\…";19was PHPCS'sT_*token constants, whichsrc/Util/Tokens.phpcreates at runtime viadefine().squizlabs/php_codesnifferalso declares no Composerautoloadsection, so neither symbol set is visible without help.1(UndefinedClass … Sniff) →19(UndefinedConstant T_*, exposed once the class resolved) → 0.analysis-bootstrap.php(new) supplies those symbols for both tools — PHPStanbootstrapFiles, Psalmautoloader. This adds the missing symbol source; it does not suppress the diagnostics. Suppressing them by rule name would have hidden real findings behind the same names.phpcsphpmdpsalmphpstanphpcs — all fixed, nothing suppressed:
phpcbf).@packagemust precede@author.@authormust beDisplay Name <email>.@license— surfaced only after (2) and (3) were fixed and PEAR could parse the block.@licensemust carry a URL and a name.phpmd — 2 fixed:
ShortVariable×2 ($j→$ptr, two local token cursors). Plain renames, no behaviour change.Baselines and their reasons
phpmd.baseline.xmlcontains exactly two entries, both complexity/style, neither a defect:WeightedMethodCount—NamedParametersSniffclass complexity 65 vs threshold 50.CyclomaticComplexity—hasUnnamedArguments()complexity 20 vs threshold 15.Reason: both are inherent to hand-written PHP token-stream parsing, which is unavoidably branchy. Reducing them means restructuring the control flow of the sniff that enforces a fleet-wide lint rule — a behavioural risk that does not belong in a gate-repair PR whose whole point is that this file has never been linted. Tracked in the issue below rather than silently suppressed. Both entries are pinned to a specific rule + file (+ method), so any new complexity violation elsewhere still fails the gate.
No psalm baseline and no phpstan baseline were created — neither tool needed one.
Issue filed: #13 — tracks removing both baseline entries (and deleting
phpmd.baseline.xmlonce empty).test:alldeliberately left dead — and its message correctedtest:allkeeps its|| echofallback. But the cost was measured, and the shipped message was false: phpunit exits 1 here because there is nophpunit.xmland notests/directory — not because "tests require a Nextcloud environment". The message now says so. Behaviour is unchanged; turning this into a real gate needs a test suite to exist first.composer.json shape chosen: plain
./vendor/bin/X(shape b)Matching hermiq / hrmq / portaliq / petstore / nldesign / nc-app-template rather than the
if [ -f … ]guard used by shillinq and openregister.Why: all four binaries are hard
require-devdependencies and are present aftercomposer install. A tolerantif [ -f … ]guard would reintroduce exactly the failure this PR fixes — an absent tool passing silently — and the original defect here was a barephpmdthat was never onPATH, which a file-existence guard would have kept invisible. If the tool is missing, the correct outcome is a loud failure.phpmd's bare binary is now./vendor/bin/phpmdregardless of shape. The same bare-binary / missing-lib/bug was also fixed in the non-gate helpersquality:phpcs-score,quality:phpmd-score,phpmetrics,phpmetrics:violations.Result
composer check:strict→ ALL CHECKS PASSED, exit 0, withlint,phpcs,phpmd,psalmandphpstanall genuinely executing against real code and passing on their merits.Known gaps, stated plainly
check:strictcorrect; it does not wire it into a workflow. Until that happens the repair is only enforced locally. This is the single highest-value follow-up.NamedParametersSniff.phpis byte-identical across openklant, opentalk, openzaak and valtimo — so is the debt in it. It should live in one shared package instead of four copies.autoload.psr-4map still pointsOCA\Valtimo\at the non-existentlib/. Left as the forward-looking declaration for when PHP app code lands.phpqa/phpqa:full/phpqa:ciscripts reference aphpqabinary that is not inrequire-devand cannot run. Not touched — out of scope for this gate.