feat(ZMSKVR): implement php code scanning with psalm#2203
Merged
ThomasAFink merged 38 commits intoMay 31, 2026
Conversation
Ensure the Psalm workflow uploads SARIF findings even when Psalm returns a non-zero exit code, then fail the job explicitly afterward so CI enforcement remains intact.
Set upload-sarif checkout_path to the matrix project directory so GitHub can resolve Psalm paths and show code previews for alerts.
Switch upload-sarif checkout_path to the matrix project relative path so GitHub can resolve Psalm file locations correctly.
Rewrite SARIF artifact URIs to repository-root-relative paths so GitHub code scanning can resolve files and render alert previews.
Add Psalm as a dev dependency in each module, create per-module psalm.xml configs, and expand the Psalm CI matrix to scan all modules with module-aware SARIF path normalization.
Upload SARIF only when the module report exists and fail with a clear message when Psalm or SARIF generation fails.
Prevent matrix runs from canceling sibling jobs and commit regenerated composer.lock files after adding Psalm to all modules.
Disable dead-code checks in per-module scans to reduce alert noise and add an opt-in monorepo dead-code job that analyzes cross-module usage.
Execute the psalm-dead-code job alongside the matrix scan for push, pull request, scheduled, and manual workflow runs.
Add config/bootstrap/routing files to module and monorepo Psalm project files so global App symbols are available during analysis.
Match CodeQL Advanced on/pull_request and push to main, add pull-requests read permission, and cancel in-progress runs per ref.
…rror Handle BO\Zmsclient\Psr7\RequestException (e.g. SSL/cURL failures) so responses use zmsClientCommunicationError instead of unknownError. Align ErrorMessages::get call with single-parameter signature.
PHP cURL to https://*.ddev.site from the web container often fails TLS verification; same Apache is reachable over http://127.0.0.1 without SSL.
Remove root vendor ignore from psalm.monorepo.xml; with resolveFromConfigFile it pointed at missing ./vendor and broke CI.
Satisfy Psalm ClassMustBeFinal; class is not extended and only exposes static helpers.
github-actions
Bot
added
Infrastructure: actions
Infrastructure: ddev
Module: zmsadmin
Module: zmsapi
Module: zmsdb
Module: zmsdldb
Module: zmsentities
Module: zmsstatistic
Module: zmsticketprinter
Module: zmscalldisplay
Module: zmscitizenapi
Module: zmsclient
Module: zmsmessaging
Module: zmsslim
Module: mellon
labels
Contributor
|
Warning Review limit reached
More reviews will be available in 17 minutes and 10 seconds. Learn how PR review limits work. Your organization has run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans include higher PR review limits than trial, open-source, and free plans. In all cases, reviews become available again over time. During sustained high-volume PR review activity, CodeRabbit may temporarily slow when the next review becomes available. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: ⛔ Files ignored due to path filters (13)
📒 Files selected for processing (15)
📝 WalkthroughWalkthroughUpdates local DDEV settings (API URL, MariaDB version), amends CodeQL workflow display name, and adds a per-module ChangesMonorepo infra & dev config
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Nitpick comments (3)
zmsapi/psalm.xml (1)
1-20: Consider reducing duplicated Psalm config across modules.These per-module files are nearly identical; a shared template/base generation approach would reduce config drift and maintenance overhead.
As per coding guidelines "Be consistent. If you do something a certain way, do all similar things in the same way."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@zmsapi/psalm.xml` around lines 1 - 20, The per-module Psalm XML is duplicated; extract the common settings (the <psalm> root attributes like errorLevel, resolveFromConfigFile, xmlns/xsi, findUnusedBaselineEntry/findUnusedCode and the <projectFiles> structure) into a single shared base template and have module-level configs include or override only module-specific entries (e.g., additional <file> or <directory> entries). Implement this by creating a shared psalm base (template or generated file) and update module psalm.xml to reference or merge the base, leaving only unique <projectFiles> additions in each module; ensure attributes such as findUnusedBaselineEntry="true" and the <ignoreFiles> vendor entry are defined centrally so maintenance is centralized..github/workflows/psalm.yml (1)
170-181: Monorepo scan only installs dependencies for zmsapi.The dead-code scan analyzes all 13+ projects but only installs composer dependencies in
zmsapi. Psalm won't be able to resolve classes that depend on other projects' vendor packages, potentially causing false positives or analysis failures.Consider installing dependencies for all projects, or at minimum documenting this limitation if intentional.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/workflows/psalm.yml around lines 170 - 181, The workflow currently only installs Composer deps for the zmsapi folder before running the "Run Psalm dead-code scan (monorepo)" step (id: psalm_dead_code), which can cause unresolved symbols when Psalm analyzes other packages; update the job to install dependencies for every PHP project Psalm will scan (e.g., run composer install in each package directory or run a root-level install that bootstraps all workspaces) or explicitly document this limitation in the step description so reviewers know the scan is intentionally limited.zmscitizenapi/src/Zmscitizenapi/Services/Core/ExceptionService.php (1)
29-31: Consider tracking the TODO as a proper issue.The TODO comment suggests refactoring to a strategy pattern but lacks a tracking reference. As per coding guidelines, TODO comments should be converted to proper issues:
// `@see` Issue `#XXX`: Refactor ExceptionService to use strategy patternThis would help ensure the technical debt is tracked and prioritized appropriately.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@zmscitizenapi/src/Zmscitizenapi/Services/Core/ExceptionService.php` around lines 29 - 31, The TODO in ExceptionService (class ExceptionService) should be converted into a tracked issue reference: replace or augment the comment near the class/method declaration (where `@SuppressWarnings` and the TODO appear) with a link to a project issue (e.g., “@see Issue `#XXX`: Refactor ExceptionService to use strategy pattern”) so the suggested refactor is tracked; update the comment to reference the issue ID and keep the existing note about considering a strategy pattern or error handler chain for the method(s) with high complexity.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.ddev/.env.template:
- Line 2: The .ddev/.env.template change to ZMS_API_URL from
https://zms.ddev.site/... to http://127.0.0.1/... requires confirming team
agreement and updating all references; update the ZMS_API_URL value
documentation and tests that still reference zms.ddev.site by editing the
fixtures and docs listed
(zmscitizenapi/tests/Zmscitizenapi/fixtures/GET_process_ics_template.json,
zmsapi/public/doc/README.md, zmscitizenapi/public/doc/README.md,
zmsdb/tests/Zmsdb/fixtures/mysql_zmsbo.sql) to use the new localhost URL, add a
note in the project README or deployment docs about using 127.0.0.1 for local
dev, and notify the team so everyone configures their local environment
accordingly.
In @.ddev/config.yaml:
- Line 12: Update the MariaDB version declaration and verify config
compatibility: inspect the "version: \"10.11\"" entry and any MariaDB
configuration files referenced by DDEV for deprecated or changed options (e.g.,
explicit_defaults_for_timestamp, InnoDB compression, slow query log variable
names), adjust settings to match 10.11 defaults where needed, run your Flyway
migration tests against a MariaDB 10.11 instance to confirm SQL compatibility,
and after upgrading run mariadb-upgrade to update system tables; ensure any
adjusted settings are committed back into the ddev config or custom my.cnf used
by the project.
In @.github/workflows/psalm.yml:
- Around line 88-115: The inline Python SARIF normalization script should skip
processing when the SARIF file is missing to avoid FileNotFoundError: check
existence of the computed sarif_path (variable sarif_path derived from PROJECT)
using os.path.exists(sarif_path) before the first open(sarif_path, ...) call and
exit or return early if the file is absent; ensure the script logs or prints a
short message and does not raise if the file doesn't exist so the workflow can
continue to the "Check SARIF file exists" step.
- Around line 126-131: The Upload SARIF results step using
github/codeql-action/upload-sarif@v3 is passing an incorrect checkout_path;
remove the checkout_path: ${{ matrix.project }} line from that step so the
action uses the default github.workspace and the SARIF file paths (which your
Python normalization already prefixes) are interpreted correctly.
In `@psalm.monorepo.xml`:
- Around line 11-48: Add an <ignoreFiles> section to psalm.monorepo.xml to
exclude each project's vendor directory from analysis; inside the root
(alongside <projectFiles>) add ignore entries referencing the vendor paths for
the projects listed (e.g., mellon/vendor, zmsadmin/vendor, zmsapi/vendor,
zmscalldisplay/vendor, zmscitizenapi/vendor, zmsclient/vendor, zmsdb/vendor,
zmsdldb/vendor, zmsentities/vendor, zmsmessaging/vendor, zmsslim/vendor,
zmsstatistic/vendor, zmsticketprinter/vendor) so Psalm won’t scan third-party
code and generate false positives or slow runs.
In `@zmscitizenapi/src/Zmscitizenapi/Services/Core/ExceptionService.php`:
- Around line 150-156: The switch contains duplicate assignments for the same
error causing an unused assignment and misleading fall-through; replace the two
separate case labels 'BO\\Zmsapi\\Exception\\Request\\RequestNotFound' and
'BO\\Zmsdb\\Exception\\Request\\RequestNotFound' with a single combined case
that calls self::getError('requestNotFound') once (i.e., group both case labels
together before one $error = self::getError('requestNotFound') and the break),
so update the switch in ExceptionService where getError('requestNotFound') is
invoked to remove the redundant assignment.
---
Nitpick comments:
In @.github/workflows/psalm.yml:
- Around line 170-181: The workflow currently only installs Composer deps for
the zmsapi folder before running the "Run Psalm dead-code scan (monorepo)" step
(id: psalm_dead_code), which can cause unresolved symbols when Psalm analyzes
other packages; update the job to install dependencies for every PHP project
Psalm will scan (e.g., run composer install in each package directory or run a
root-level install that bootstraps all workspaces) or explicitly document this
limitation in the step description so reviewers know the scan is intentionally
limited.
In `@zmsapi/psalm.xml`:
- Around line 1-20: The per-module Psalm XML is duplicated; extract the common
settings (the <psalm> root attributes like errorLevel, resolveFromConfigFile,
xmlns/xsi, findUnusedBaselineEntry/findUnusedCode and the <projectFiles>
structure) into a single shared base template and have module-level configs
include or override only module-specific entries (e.g., additional <file> or
<directory> entries). Implement this by creating a shared psalm base (template
or generated file) and update module psalm.xml to reference or merge the base,
leaving only unique <projectFiles> additions in each module; ensure attributes
such as findUnusedBaselineEntry="true" and the <ignoreFiles> vendor entry are
defined centrally so maintenance is centralized.
In `@zmscitizenapi/src/Zmscitizenapi/Services/Core/ExceptionService.php`:
- Around line 29-31: The TODO in ExceptionService (class ExceptionService)
should be converted into a tracked issue reference: replace or augment the
comment near the class/method declaration (where `@SuppressWarnings` and the TODO
appear) with a link to a project issue (e.g., “@see Issue `#XXX`: Refactor
ExceptionService to use strategy pattern”) so the suggested refactor is tracked;
update the comment to reference the issue ID and keep the existing note about
considering a strategy pattern or error handler chain for the method(s) with
high complexity.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 83862cf4-3bbc-4e72-94b3-c55249245b4f
⛔ Files ignored due to path filters (13)
mellon/composer.lockis excluded by!**/*.lockzmsadmin/composer.lockis excluded by!**/*.lockzmsapi/composer.lockis excluded by!**/*.lockzmscalldisplay/composer.lockis excluded by!**/*.lockzmscitizenapi/composer.lockis excluded by!**/*.lockzmsclient/composer.lockis excluded by!**/*.lockzmsdb/composer.lockis excluded by!**/*.lockzmsdldb/composer.lockis excluded by!**/*.lockzmsentities/composer.lockis excluded by!**/*.lockzmsmessaging/composer.lockis excluded by!**/*.lockzmsslim/composer.lockis excluded by!**/*.lockzmsstatistic/composer.lockis excluded by!**/*.lockzmsticketprinter/composer.lockis excluded by!**/*.lock
📒 Files selected for processing (34)
.ddev/.env.template.ddev/config.yaml.github/workflows/codeql.yml.github/workflows/psalm.ymlmellon/composer.jsonmellon/psalm.xmlpsalm.monorepo.xmlzmsadmin/composer.jsonzmsadmin/psalm.xmlzmsapi/composer.jsonzmsapi/psalm.xmlzmscalldisplay/composer.jsonzmscalldisplay/psalm.xmlzmscitizenapi/composer.jsonzmscitizenapi/psalm.xmlzmscitizenapi/src/Zmscitizenapi/Services/Core/ExceptionService.phpzmsclient/composer.jsonzmsclient/psalm.xmlzmsdb/composer.jsonzmsdb/psalm.xmlzmsdldb/composer.jsonzmsdldb/psalm.xmlzmsdldb/src/Zmsdldb/Importer/test_sqlite.phpzmsentities/composer.jsonzmsentities/psalm.xmlzmsmessaging/composer.jsonzmsmessaging/psalm.xmlzmsslim/composer.jsonzmsslim/psalm.xmlzmsslim/src/Slim/Helper/Sanitizer.phpzmsstatistic/composer.jsonzmsstatistic/psalm.xmlzmsticketprinter/composer.jsonzmsticketprinter/psalm.xml
…ith-psalm # Conflicts: # zmsadmin/composer.lock # zmsapi/composer.lock # zmscalldisplay/composer.lock # zmscitizenapi/composer.lock # zmscitizenapi/src/Zmscitizenapi/Services/Core/ExceptionService.php # zmsclient/composer.lock # zmsdb/composer.lock # zmsdldb/composer.lock # zmsentities/composer.lock # zmsmessaging/composer.lock # zmsslim/composer.lock # zmsstatistic/composer.lock # zmsticketprinter/composer.lock
Psalm exits non-zero when it reports issues but still writes SARIF for CodeQL upload. Treat missing SARIF as the job failure condition instead.
Use continue-on-error on the scan step so SARIF normalize/upload run even when Psalm exits non-zero, then fail the job if SARIF is missing or the recorded Psalm exit code is not zero.
Drop pull_request and push triggers so branch CI is not blocked by existing findings; keep daily 05:00 UTC uploads on main with fail-on-error unchanged.
Psalm 6.16+ requires patched PHP 8.3.16+, which breaks reference-libraries on GitHub runners; 6.5.0 is the last release accepting PHP 8.3.6.
Prevent the dead-code job from analyzing third-party dependencies across all 13 modules, matching per-module psalm.xml ignoreFiles behavior.
5 tasks
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
2 participants
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.
Pull Request Checklist (Feature Branch to
next):nextBranch in meinen Feature-Branch gemergt.Summary by CodeRabbit
Bug Fixes
Infrastructure
Other Updates