feat(GH-1634): move LoggerService from zmscitizenapi to zmsslim#2467
Open
ThomasAFink wants to merge 21 commits into
Open
feat(GH-1634): move LoggerService from zmscitizenapi to zmsslim#2467ThomasAFink wants to merge 21 commits into
ThomasAFink wants to merge 21 commits into
Conversation
Relocate HTTP logging, rate limiting, and header filtering to BO\Slim\LoggerService with configurable enrichers for citizen-api process context and error codes.
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughRefactors logging into BO\Slim\LoggerService with cache-backed rate limiting and callback hooks, adds a ClientIp helper, introduces ModuleLoggerInitializer for per-module env-driven logger configuration and middleware registration, moves/adapts Slim middleware and tests, and updates module bootstraps, env templates, and docs. ChangesLogger Service Refactoring and Core Infrastructure
Module Logger Initialization and Wiring
Per-Module Deployment and UI tweaks
Estimated code review effort 🎯 4 (Complex) | ⏱️ ~60 minutes Suggested labels
Suggested reviewers
🚥 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 |
![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: 2
🧹 Nitpick comments (5)
zmsslim/tests/Slim/Helper/ClientIpTest.php (2)
24-33: ⚡ Quick winAdd a malformed-header regression case.
The suite only covers valid forwarded headers and the no-header fallback. Please add a case where a forwarded header is present but invalid and
REMOTE_ADDRis valid, so the helper is forced to skip the bad value instead of returning it.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@zmsslim/tests/Slim/Helper/ClientIpTest.php` around lines 24 - 33, Add a new test method (e.g. testSkipsMalformedForwardedHeader) in Slim/Helper/ClientIpTest that sets $_SERVER['HTTP_X_FORWARDED_FOR'] to a malformed value (for example 'not-an-ip' or 'unknown') and $_SERVER['REMOTE_ADDR'] to a valid IP like '203.0.113.5', then assert that ClientIp::getClientIp() returns the REMOTE_ADDR value; ensure the test cleans or overrides $_SERVER appropriately so the malformed forwarded header is skipped and the helper falls back to using REMOTE_ADDR.
12-15: ⚡ Quick winReset the full
$_SERVERsurface this helper reads.
ClientIp::getClientIp()inspects more headers than the two unset here. If another test setsHTTP_CLIENT_IP,HTTP_FORWARDED, etc., that value can leak into later cases and make this class order-dependent. Snapshot/restore$_SERVERor unset the full header list. As per coding guidelines, "Tests 4. Independent. 5. Repeatable."🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@zmsslim/tests/Slim/Helper/ClientIpTest.php` around lines 12 - 15, The tearDown currently only unsets HTTP_X_FORWARDED_FOR and REMOTE_ADDR but ClientIp::getClientIp() reads many more server headers; fix by snapshotting $_SERVER in setUp and restoring it in tearDown (or explicitly unset the full header list ClientIp reads such as HTTP_CLIENT_IP, HTTP_X_FORWARDED_FOR, HTTP_FORWARDED, HTTP_FORWARDED_FOR, HTTP_X_REAL_IP, REMOTE_ADDR, etc.) so tests are independent and repeatable; modify the tearDown method and add a corresponding setUp to save/restore $_SERVER or add the complete unset list referenced above, keeping ClientIp::getClientIp() header names in sync.zmsslim/tests/Slim/LoggerServiceTest.php (1)
159-190: ⚡ Quick winAssert the header redaction result here.
This test now seeds
X-Api-Key, but it still only checks thatlogRequest()does not throw. If header filtering regresses and logs the raw secret, this test will still pass. Please assert the emitted context or logger interaction so the redaction behavior is actually covered.As per coding guidelines,
**/*: Tests should be "Readable" and "Independent".🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@zmsslim/tests/Slim/LoggerServiceTest.php` around lines 159 - 190, The test currently only ensures LoggerService::logRequest doesn't throw but doesn't verify header redaction; update testSensitiveHeaderFiltering to assert the logger was called with redacted header values by mocking/instrumenting the logger used by LoggerService and adding an expectation (or capturing the logged context) that the headers in the emitted log do not contain the raw secret 'secret-key' (and do not expose the Authorization token) — e.g., set a mock on the logger method (info/debug) and use a PHPUnit callback to assert the headers array in the logged context replaces sensitive values (not equal to the original secret) or includes the known redaction placeholder used by LoggerService; keep references to LoggerService::logRequest and testSensitiveHeaderFiltering when locating where to add the mock and assertions.zmscitizenapi/src/Zmscitizenapi/Application.php (1)
14-15: 💤 Low valueTODO comments should reference tracked issues.
Lines 14, 79, and 120 contain TODO comments indicating planned refactors but lack issue references. As per coding guidelines, these should be converted to proper issues or reference existing ones.
Example format
// `@see` Issue `#XXX`: Refactor Application into smaller focused classes🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@zmscitizenapi/src/Zmscitizenapi/Application.php` around lines 14 - 15, Replace the bare TODO comments in the Application class with referenced issue tags: create or link tracked issues and update each TODO to the format shown in the example (e.g. "// `@see` Issue `#XXX`: Refactor Application into smaller focused classes") so every TODO in class Application (top-of-class TODO and the other TODOs near the logger/middleware initializers) points to a specific issue number; ensure the comment text briefly describes the intended refactor and uses the `@see` Issue `#NNN` pattern for traceability.zmsslim/composer.json (1)
26-27: 💤 Low valueDuplicate
roave/security-advisoriesdependency.
roave/security-advisoriesis listed in bothrequire(line 26) andrequire-dev(line 33). This meta-package only needs to be inrequire-devsince it prevents installing vulnerable packages during development/CI without affecting production runtime.♻️ Suggested fix
"tracy/tracy": "^2.9", - "roave/security-advisories": "dev-latest", "psr/simple-cache": "^3.0" },🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@zmsslim/composer.json` around lines 26 - 27, Duplicate dependency "roave/security-advisories" appears in composer.json under both "require" and "require-dev"; remove the entry from "require" so the package remains only in "require-dev" (leave the "psr/simple-cache" and other production deps untouched) — locate the "require" block that contains "roave/security-advisories" and delete that line, ensuring the "require-dev" block retains the "roave/security-advisories" entry.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@zmsslim/src/Slim/Helper/ClientIp.php`:
- Around line 26-32: The code currently returns the first comma-split header
value even if invalid; update the logic in ClientIp.php so instead of returning
$ip unconditionally, iterate through $ips and validate each candidate using
filter_var(..., FILTER_VALIDATE_IP) and return the first valid one, otherwise
continue scanning other headers and only fall back to REMOTE_ADDR (or null) at
the end; ensure you change the block that uses $_SERVER[$header], $ips and $ip
so no unvalidated header value can be returned (this prevents bad HTTP_* headers
from overriding REMOTE_ADDR and affecting RateLimitingMiddleware).
In `@zmsslim/src/Slim/LoggerService.php`:
- Around line 213-215: The array_filter call that builds $queryParams in
LoggerService:: (use the $queryParams variable and the array_filter call with
ARRAY_FILTER_USE_BOTH) uses a callback whose parameters are named ($key, $value)
but ARRAY_FILTER_USE_BOTH passes (value, key), which is misleading; rename the
callback parameters to ($value, $key) and update any references inside the
closure accordingly (or swap their usage) so the parameter names match the
actual semantics of array_filter's callback with ARRAY_FILTER_USE_BOTH.
---
Nitpick comments:
In `@zmscitizenapi/src/Zmscitizenapi/Application.php`:
- Around line 14-15: Replace the bare TODO comments in the Application class
with referenced issue tags: create or link tracked issues and update each TODO
to the format shown in the example (e.g. "// `@see` Issue `#XXX`: Refactor
Application into smaller focused classes") so every TODO in class Application
(top-of-class TODO and the other TODOs near the logger/middleware initializers)
points to a specific issue number; ensure the comment text briefly describes the
intended refactor and uses the `@see` Issue `#NNN` pattern for traceability.
In `@zmsslim/composer.json`:
- Around line 26-27: Duplicate dependency "roave/security-advisories" appears in
composer.json under both "require" and "require-dev"; remove the entry from
"require" so the package remains only in "require-dev" (leave the
"psr/simple-cache" and other production deps untouched) — locate the "require"
block that contains "roave/security-advisories" and delete that line, ensuring
the "require-dev" block retains the "roave/security-advisories" entry.
In `@zmsslim/tests/Slim/Helper/ClientIpTest.php`:
- Around line 24-33: Add a new test method (e.g.
testSkipsMalformedForwardedHeader) in Slim/Helper/ClientIpTest that sets
$_SERVER['HTTP_X_FORWARDED_FOR'] to a malformed value (for example 'not-an-ip'
or 'unknown') and $_SERVER['REMOTE_ADDR'] to a valid IP like '203.0.113.5', then
assert that ClientIp::getClientIp() returns the REMOTE_ADDR value; ensure the
test cleans or overrides $_SERVER appropriately so the malformed forwarded
header is skipped and the helper falls back to using REMOTE_ADDR.
- Around line 12-15: The tearDown currently only unsets HTTP_X_FORWARDED_FOR and
REMOTE_ADDR but ClientIp::getClientIp() reads many more server headers; fix by
snapshotting $_SERVER in setUp and restoring it in tearDown (or explicitly unset
the full header list ClientIp reads such as HTTP_CLIENT_IP,
HTTP_X_FORWARDED_FOR, HTTP_FORWARDED, HTTP_FORWARDED_FOR, HTTP_X_REAL_IP,
REMOTE_ADDR, etc.) so tests are independent and repeatable; modify the tearDown
method and add a corresponding setUp to save/restore $_SERVER or add the
complete unset list referenced above, keeping ClientIp::getClientIp() header
names in sync.
In `@zmsslim/tests/Slim/LoggerServiceTest.php`:
- Around line 159-190: The test currently only ensures LoggerService::logRequest
doesn't throw but doesn't verify header redaction; update
testSensitiveHeaderFiltering to assert the logger was called with redacted
header values by mocking/instrumenting the logger used by LoggerService and
adding an expectation (or capturing the logged context) that the headers in the
emitted log do not contain the raw secret 'secret-key' (and do not expose the
Authorization token) — e.g., set a mock on the logger method (info/debug) and
use a PHPUnit callback to assert the headers array in the logged context
replaces sensitive values (not equal to the original secret) or includes the
known redaction placeholder used by LoggerService; keep references to
LoggerService::logRequest and testSensitiveHeaderFiltering when locating where
to add the mock and assertions.
🪄 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: deee8e53-299e-4b97-a000-10c5b305ae0c
⛔ Files ignored due to path filters (1)
zmsslim/composer.lockis excluded by!**/*.lock
📒 Files selected for processing (18)
zmscitizenapi/bootstrap.phpzmscitizenapi/src/Zmscitizenapi/Application.phpzmscitizenapi/src/Zmscitizenapi/Middleware/IpFilterMiddleware.phpzmscitizenapi/src/Zmscitizenapi/Middleware/RateLimitingMiddleware.phpzmscitizenapi/src/Zmscitizenapi/Middleware/RequestLoggingMiddleware.phpzmscitizenapi/src/Zmscitizenapi/Middleware/RequestSanitizerMiddleware.phpzmscitizenapi/src/Zmscitizenapi/Middleware/RequestSizeLimitMiddleware.phpzmscitizenapi/src/Zmscitizenapi/Middleware/SecurityHeadersMiddleware.phpzmscitizenapi/src/Zmscitizenapi/Services/Core/ZmsApiClientService.phpzmscitizenapi/src/Zmscitizenapi/Services/Core/ZmsApiFacadeService.phpzmscitizenapi/src/Zmscitizenapi/Utils/ClientIpHelper.phpzmscitizenapi/src/Zmscitizenapi/Utils/ErrorHandler.phpzmscitizenapi/tests/Zmscitizenapi/TestLogger.phpzmsslim/composer.jsonzmsslim/src/Slim/Helper/ClientIp.phpzmsslim/src/Slim/LoggerService.phpzmsslim/tests/Slim/Helper/ClientIpTest.phpzmsslim/tests/Slim/LoggerServiceTest.php
… logging Extract request logging, sanitization, and security headers middleware into zmsslim so zmsapi can log every HTTP request via LoggerService alongside zmscitizenapi.
Match zmscitizenapi by annotating initializeLogger() until logger config is extracted into a dedicated initializer.
…rvice Rename the ARRAY_FILTER_USE_BOTH callback parameters to ($value, $key) so they match PHP semantics.
Skip malformed comma-separated values and continue scanning headers so bad HTTP_* values cannot override REMOTE_ADDR.
…citizenapi Split shared LOGGER_* settings into ZMS_CITIZENAPI_LOGGER_* and ZMS_API_LOGGER_* so each module can tune request logging independently, matching the per-module Twig cache pattern.
…and ticketprinter Introduce ModuleLoggerInitializer in zmsslim and wire per-module ZMS_*_LOGGER_* settings with shared HTTP logging middleware across the remaining Slim web modules.
Explain ZMS_*_LOGGER_* env vars and recommend lower LOGGER_MAX_REQUESTS for zmscalldisplay and zmsticketprinter due to frequent monitor and printer polling.
…th ClientIp ModuleLoggerInitializer needs symfony/cache as a direct zmsslim dependency so admin, calldisplay, statistic, and ticketprinter can bootstrap in CI. Update IpFilterMiddlewareTest after ClientIp skips invalid REMOTE_ADDR and falls back to 127.0.0.1.
SecurityHeadersMiddleware sets style-src 'self', which prevents browsers from applying the inline background-color on #page-header that Twig used for ZMS_ENV dev (green) and stage (blue). Move those colors to body.zms-env-* CSS classes and skip strict CSP on Twig UI modules that still rely on inline styles elsewhere; APIs keep SecurityHeadersMiddleware unchanged.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
zmsslim/src/Slim/Helper/ClientIp.php (1)
21-30:⚠️ Potential issue | 🟠 Major | 🏗️ Heavy liftDo not trust forwarded headers unless the sender is a trusted proxy.
Any direct client can currently spoof
HTTP_X_FORWARDED_FOR/HTTP_CLIENT_IPand choose the IP that flows into logging and per-IP controls. This helper should only read forwarded headers whenREMOTE_ADDRbelongs to a configured trusted-proxy list; otherwise it should return the validatedREMOTE_ADDR.As per coding guidelines, "Always find root cause. Always look for the root cause of a problem."
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@zmsslim/src/Slim/Helper/ClientIp.php` around lines 21 - 30, The code currently trusts forwarded headers in the foreach ($headers as $header) loop (e.g., HTTP_X_FORWARDED_FOR, HTTP_CLIENT_IP) without verifying the sender; change the logic in the ClientIp helper so it first checks that $_SERVER['REMOTE_ADDR'] is in a configured trusted-proxy list (or use a getTrustedProxies() helper/config) and only then parses and returns forwarded IPs, otherwise validate and return $_SERVER['REMOTE_ADDR'] via filter_var; ensure the trusted-proxy check happens before the foreach and keep the existing IP validation (filter_var with FILTER_VALIDATE_IP) when parsing header values.
🧹 Nitpick comments (5)
zmsslim/src/Slim/Helper/ModuleLoggerInitializer.php (2)
17-19: 💤 Low valueUnjustified
@SuppressWarnings(PHPMD.NPathComplexity)on a simple config builder.
configure()only assembles an array of env-backed defaults. The NPath inflation comes from the repeated?:chain; suppressing the warning hides that rather than addressing it. Consider a tinyintFromEnv($key, $default)helper to drop both the complexity and the suppression.As per coding guidelines: "Suppressing warnings without proper justification" / "Consider refactoring instead of suppressing".
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@zmsslim/src/Slim/Helper/ModuleLoggerInitializer.php` around lines 17 - 19, The `@SuppressWarnings` on NPathComplexity should be removed and the repeated ?: chains in configure() refactored into a small helper; add a private static intFromEnv(string $key, int $default): int (or a similarly named helper) that reads getenv()/$_ENV, casts/validates to int and returns the default when missing/invalid, then replace the multiple inline ?: expressions in configure() with calls to intFromEnv() (and similarly factor any repeated string/default patterns into a simple envFromDefault helper) to reduce NPath complexity and eliminate the suppression.
51-57: 💤 Low valueRequest limits read global env vars while logger config is per-module.
configure()keys off{$envPrefix}_LOGGER_*, butgetRequestLimits()reads the unprefixedMAX_STRING_LENGTH/MAX_RECURSION_DEPTH, so all modules sharing an environment get identical sanitizer limits. If per-module tuning is the goal of this PR, consider prefixing these too; otherwise this is fine but worth a clarifying note.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@zmsslim/src/Slim/Helper/ModuleLoggerInitializer.php` around lines 51 - 57, getRequestLimits() currently reads global env vars MAX_STRING_LENGTH and MAX_RECURSION_DEPTH, which conflicts with the per-module logger configuration in configure() that uses a module-specific $envPrefix and keys like {$envPrefix}_LOGGER_*. Update getRequestLimits() to read prefixed variables using the same $envPrefix pattern (e.g., "{$envPrefix}_MAX_STRING_LENGTH" and "{$envPrefix}_MAX_RECURSION_DEPTH") or accept $envPrefix as a parameter so each module can supply its prefix; ensure fallbacks to the existing defaults (32768 and 10) remain when the prefixed vars are unset.zmsapi/src/Zmsapi/Application.php (1)
146-162: 💤 Low valueNew
@SuppressWarnings/@TODOindicates carried-over technical debt.
initializeLogger()is a flat sequence of env reads; the NPath suppression masks the?:chain rather than fixing it, and the@TODOflags an unaddressed extraction. Note thatModuleLoggerInitializer::configure()already encapsulates exactly this logic —zmsapicould call it with prefixZMS_APIinstead of duplicating the per-key reads and the suppression.As per coding guidelines: flag "
@SuppressWarnings... without proper justification" and "TODO comments that indicate technical debt".🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@zmsapi/src/Zmsapi/Application.php` around lines 146 - 162, The initializeLogger() method duplicates environment-key parsing and suppresses NPath complexity; replace this duplication by calling the existing ModuleLoggerInitializer::configure() with the prefix "ZMS_API" (or equivalent) and then call LoggerService::configure(self::getLoggerConfig()) if still needed, removing the `@SuppressWarnings` and `@TODO`; specifically remove the getenv(...) casts and the static property assignments in initializeLogger(), invoke ModuleLoggerInitializer::configure('ZMS_API') to centralize per-key reads, and delete the TODO/NPath suppression comment so the duplication is resolved and logging initialization is delegated.zmsslim/tests/Slim/TestLogger.php (1)
60-72: ⚡ Quick win
logErroronly asserts the exception class, not the message; also missing the null-testCaseguard.
assertInstanceOf(get_class($expected[1]), $exception)ignores the message, so a callsite likeexpectLogError(new \RuntimeException('Handler failed'))passes regardless of the actual error message — weakening the very tests relying on it. Additionally, unlikelogInfo(Line 40), this method dereferencesself::$testCase->fail(...)without a null check.♻️ Strengthen assertion and align null handling
public static function logError( \Throwable $exception, ?RequestInterface $request = null, ?ResponseInterface $response = null, array $context = [] ): void { + if (self::$testCase === null) { + throw new \RuntimeException('Test case not initialized. Call initTest() first.'); + } + $expected = array_shift(self::$expectedLogs); if (!$expected) { self::$testCase->fail('Unexpected log error: ' . $exception->getMessage()); } self::$testCase->assertEquals('error', $expected[0]); self::$testCase->assertInstanceOf(get_class($expected[1]), $exception); + self::$testCase->assertSame($expected[1]->getMessage(), $exception->getMessage()); }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@zmsslim/tests/Slim/TestLogger.php` around lines 60 - 72, The logError method currently only checks exception class and calls self::$testCase->fail() without guarding for null; update logError to first shift $expected from self::$expectedLogs, then if !$expected, check if self::$testCase is null and if so throw or return (match behavior in logInfo) otherwise call self::$testCase->fail(...); keep the existing class assertion (assertInstanceOf(get_class($expected[1]), $exception)) but add an assertion that the exception message equals the expected message (e.g. self::$testCase->assertEquals($expected[1]->getMessage(), $exception->getMessage())); ensure you reference the same symbols: logError, self::$expectedLogs, self::$testCase, assertInstanceOf, and assertEquals when implementing these checks.zmscalldisplay/src/Zmscalldisplay/Application.php (1)
88-89: ⚖️ Poor tradeoffLoad-time
initialize()couples class autoloading to filesystem cache setup.
ModuleLoggerInitializer::initializeCache()validates and creates a filesystem cache directory and can throwRuntimeException. Triggering it as a side effect when the class file is included means any cache-dir problem aborts class loading itself, which is harder to diagnose than a failure during explicit bootstrap. Consider invokingApplication::initialize()frombootstrap.php(alongside the existing middleware registration) rather than at file load.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@zmscalldisplay/src/Zmscalldisplay/Application.php` around lines 88 - 89, Application::initialize() is being called at file-load time which causes ModuleLoggerInitializer::initializeCache() (which can throw RuntimeException) to run during class inclusion; remove the immediate call to Application::initialize() from the class file and instead invoke Application::initialize() explicitly from bootstrap.php alongside the existing middleware registration so cache setup happens during bootstrap rather than at include time, ensuring failures surface during startup rather than class autoloading.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@zmsadmin/src/Zmsadmin/Application.php`:
- Around line 116-123: Application::initialize currently calls
ModuleLoggerInitializer::initializeCache() unconditionally which lets
ModuleLoggerInitializer::validateCacheDirectory() throw and abort startup;
change Application::initialize to call
ModuleLoggerInitializer::initializeCache() inside a try/catch, preserve the call
to ModuleLoggerInitializer::configure('ZMS_ADMIN'), and on exception set
self::$cache = null (and optionally log a warning) so bootstrap continues and
LoggerService::checkRateLimit() can handle a null cache fallback.
In `@zmsslim/src/Slim/Middleware/RequestSanitizerMiddleware.php`:
- Around line 19-23: The constructor RequestSanitizerMiddleware::__construct
must validate that $maxRecursionDepth and $maxStringLength are positive; add
checks at the start of the constructor to throw an InvalidArgumentException
(with a clear message mentioning which parameter is invalid) if either value is
<= 0 so the middleware fails fast during bootstrap; keep assignments to
$this->maxRecursionDepth and $this->maxStringLength unchanged after the
validation.
In `@zmsslim/src/Slim/Middleware/SecurityHeadersMiddleware.php`:
- Around line 31-32: The callback stored in $errorResponseBuilder may return
null but process() (and the other spot at the second usage) currently returns
that value directly from the process() method which must return a
ResponseInterface; update both call sites (the uses of
$this->errorResponseBuilder in process()) to check the result and handle a null
return: if the builder returns null, either construct and return a default
ResponseInterface (e.g. a 500 error response) or rethrow/wrap the original
Throwable as a Http-friendly error so a ResponseInterface is always returned;
ensure you reference the property $errorResponseBuilder and the process() method
when making the change.
In `@zmsslim/tests/Slim/Helper/ModuleLoggerInitializerTest.php`:
- Around line 17-25: The tearDown in ModuleLoggerInitializerTest resets
LoggerService with an incomplete/incorrect config; update
ModuleLoggerInitializerTest::tearDown to call LoggerService::configure with the
full set of default keys including messageSize and backoffMax and ensure
stackLines is 20 and lockTimeout is 5 (matching the real defaults) so the static
LoggerService state is fully restored and tests don't leak configuration into
other suites.
In `@zmsslim/tests/Slim/TestLogger.php`:
- Around line 38-72: TestLogger::logError is missing the same self::$testCase
initialization guard as logInfo and only asserts the exception class; add the
init check (throw RuntimeException('Test case not initialized. Call initTest()
first.') if self::$testCase === null), ensure you obtain the expected entry from
self::$expectedLogs (use array_shift as in logInfo), assert expected[0] ===
'error', assert the exception class (assertInstanceOf) and also assert the
exception message contains the expected message (use assertStringContainsString
on $exception->getMessage() vs expected[1] when expected[1] is a string), and if
expected[2] exists iterate its keys to assertArrayHasKey/assertEquals against
the $context just like logInfo uses self::$expectedLogs, self::$testCase,
logInfo and logError to locate the changes.
---
Outside diff comments:
In `@zmsslim/src/Slim/Helper/ClientIp.php`:
- Around line 21-30: The code currently trusts forwarded headers in the foreach
($headers as $header) loop (e.g., HTTP_X_FORWARDED_FOR, HTTP_CLIENT_IP) without
verifying the sender; change the logic in the ClientIp helper so it first checks
that $_SERVER['REMOTE_ADDR'] is in a configured trusted-proxy list (or use a
getTrustedProxies() helper/config) and only then parses and returns forwarded
IPs, otherwise validate and return $_SERVER['REMOTE_ADDR'] via filter_var;
ensure the trusted-proxy check happens before the foreach and keep the existing
IP validation (filter_var with FILTER_VALIDATE_IP) when parsing header values.
---
Nitpick comments:
In `@zmsapi/src/Zmsapi/Application.php`:
- Around line 146-162: The initializeLogger() method duplicates environment-key
parsing and suppresses NPath complexity; replace this duplication by calling the
existing ModuleLoggerInitializer::configure() with the prefix "ZMS_API" (or
equivalent) and then call LoggerService::configure(self::getLoggerConfig()) if
still needed, removing the `@SuppressWarnings` and `@TODO`; specifically remove the
getenv(...) casts and the static property assignments in initializeLogger(),
invoke ModuleLoggerInitializer::configure('ZMS_API') to centralize per-key
reads, and delete the TODO/NPath suppression comment so the duplication is
resolved and logging initialization is delegated.
In `@zmscalldisplay/src/Zmscalldisplay/Application.php`:
- Around line 88-89: Application::initialize() is being called at file-load time
which causes ModuleLoggerInitializer::initializeCache() (which can throw
RuntimeException) to run during class inclusion; remove the immediate call to
Application::initialize() from the class file and instead invoke
Application::initialize() explicitly from bootstrap.php alongside the existing
middleware registration so cache setup happens during bootstrap rather than at
include time, ensuring failures surface during startup rather than class
autoloading.
In `@zmsslim/src/Slim/Helper/ModuleLoggerInitializer.php`:
- Around line 17-19: The `@SuppressWarnings` on NPathComplexity should be removed
and the repeated ?: chains in configure() refactored into a small helper; add a
private static intFromEnv(string $key, int $default): int (or a similarly named
helper) that reads getenv()/$_ENV, casts/validates to int and returns the
default when missing/invalid, then replace the multiple inline ?: expressions in
configure() with calls to intFromEnv() (and similarly factor any repeated
string/default patterns into a simple envFromDefault helper) to reduce NPath
complexity and eliminate the suppression.
- Around line 51-57: getRequestLimits() currently reads global env vars
MAX_STRING_LENGTH and MAX_RECURSION_DEPTH, which conflicts with the per-module
logger configuration in configure() that uses a module-specific $envPrefix and
keys like {$envPrefix}_LOGGER_*. Update getRequestLimits() to read prefixed
variables using the same $envPrefix pattern (e.g.,
"{$envPrefix}_MAX_STRING_LENGTH" and "{$envPrefix}_MAX_RECURSION_DEPTH") or
accept $envPrefix as a parameter so each module can supply its prefix; ensure
fallbacks to the existing defaults (32768 and 10) remain when the prefixed vars
are unset.
In `@zmsslim/tests/Slim/TestLogger.php`:
- Around line 60-72: The logError method currently only checks exception class
and calls self::$testCase->fail() without guarding for null; update logError to
first shift $expected from self::$expectedLogs, then if !$expected, check if
self::$testCase is null and if so throw or return (match behavior in logInfo)
otherwise call self::$testCase->fail(...); keep the existing class assertion
(assertInstanceOf(get_class($expected[1]), $exception)) but add an assertion
that the exception message equals the expected message (e.g.
self::$testCase->assertEquals($expected[1]->getMessage(),
$exception->getMessage())); ensure you reference the same symbols: logError,
self::$expectedLogs, self::$testCase, assertInstanceOf, and assertEquals when
implementing these checks.
🪄 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: 007d0d27-22e7-453d-b1b8-6d4b3d57f5ab
⛔ Files ignored due to path filters (5)
zmsadmin/composer.lockis excluded by!**/*.lockzmscalldisplay/composer.lockis excluded by!**/*.lockzmsslim/composer.lockis excluded by!**/*.lockzmsstatistic/composer.lockis excluded by!**/*.lockzmsticketprinter/composer.lockis excluded by!**/*.lock
📒 Files selected for processing (42)
.ddev/.env.template.devcontainer/.env.templatedocs/de/operations/monolog-logging.mddocs/en/operations/monolog-logging.mdzmsadmin/bootstrap.phpzmsadmin/custom_scss/admin.scsszmsadmin/scss/admin.scsszmsadmin/src/Zmsadmin/Application.phpzmsadmin/templates/block/layout/main.twigzmsadmin/templates/layout/fullwidth.twigzmsadmin/templates/layout/main.twigzmsapi/bootstrap.phpzmsapi/src/Zmsapi/Application.phpzmscalldisplay/bootstrap.phpzmscalldisplay/src/Zmscalldisplay/Application.phpzmscitizenapi/README.mdzmscitizenapi/bootstrap.phpzmscitizenapi/src/Zmscitizenapi/Application.phpzmscitizenapi/tests/Zmscitizenapi/ApplicationTest.phpzmscitizenapi/tests/Zmscitizenapi/Middleware/IpFilterMiddlewareTest.phpzmsslim/composer.jsonzmsslim/src/Slim/Helper/ClientIp.phpzmsslim/src/Slim/Helper/ModuleLoggerInitializer.phpzmsslim/src/Slim/LoggerService.phpzmsslim/src/Slim/Middleware/RequestLoggingMiddleware.phpzmsslim/src/Slim/Middleware/RequestSanitizerMiddleware.phpzmsslim/src/Slim/Middleware/SecurityHeadersMiddleware.phpzmsslim/tests/Slim/Helper/ClientIpTest.phpzmsslim/tests/Slim/Helper/ModuleLoggerInitializerTest.phpzmsslim/tests/Slim/Middleware/MiddlewareTestCase.phpzmsslim/tests/Slim/Middleware/RequestLoggingMiddlewareTest.phpzmsslim/tests/Slim/Middleware/RequestSanitizerMiddlewareTest.phpzmsslim/tests/Slim/Middleware/SecurityHeadersMiddlewareTest.phpzmsslim/tests/Slim/TestLogger.phpzmsstatistic/bootstrap.phpzmsstatistic/custom_scss/statistic.scsszmsstatistic/scss/statistic.scsszmsstatistic/src/Zmsstatistic/Application.phpzmsstatistic/templates/layout/fullwidth.twigzmsstatistic/templates/layout/main.twigzmsticketprinter/bootstrap.phpzmsticketprinter/src/Zmsticketprinter/Application.php
✅ Files skipped from review due to trivial changes (5)
- zmscitizenapi/README.md
- zmsadmin/scss/admin.scss
- zmsadmin/bootstrap.php
- docs/en/operations/monolog-logging.md
- docs/de/operations/monolog-logging.md
🚧 Files skipped from review as they are similar to previous changes (2)
- zmsslim/composer.json
- zmsslim/src/Slim/LoggerService.php
Use tryInitializeCache() so Twig modules keep running when CACHE_DIR is missing or unwritable; validate sanitizer limits at construction; rethrow when SecurityHeadersMiddleware error builder returns null; tighten TestLogger::logError assertions and test teardown.
Register PhpErrorHandler from Bootstrap::configureLogger() so engine-level warnings no longer appear as unstructured "NOTICE: PHP message: PHP Warning" lines on stderr; they are logged through App::$log with file, line, and errno.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
zmsslim/tests/Slim/Helper/PhpErrorHandlerTest.php (1)
52-56: ⚡ Quick winRestore
error_reporting()in afinallyblock.If
PhpErrorHandler::handle()ever throws here, the process stays inerror_reporting(0)and later tests become misleading.As per coding guidelines, tests should be “Independent” and “Repeatable”.♻️ Proposed fix
- $previousReporting = error_reporting(0); - - $handled = PhpErrorHandler::handle(E_USER_WARNING, 'suppressed warning', __FILE__, __LINE__); - - error_reporting($previousReporting); + $previousReporting = error_reporting(0); + try { + $handled = PhpErrorHandler::handle(E_USER_WARNING, 'suppressed warning', __FILE__, __LINE__); + } finally { + error_reporting($previousReporting); + }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@zmsslim/tests/Slim/Helper/PhpErrorHandlerTest.php` around lines 52 - 56, Wrap the call to PhpErrorHandler::handle(...) with a try/finally so error_reporting($previousReporting) always runs; specifically, capture $previousReporting = error_reporting(0), then call PhpErrorHandler::handle(E_USER_WARNING, 'suppressed warning', __FILE__, __LINE__) inside a try block and restore error_reporting($previousReporting) in the finally block to guarantee restoration even if handle() throws.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@zmsslim/src/Slim/Helper/PhpErrorHandler.php`:
- Around line 17-33: The handler in PhpErrorHandler::handle currently always
returns true after logging, which suppresses PHP's built-in handling for
E_USER_ERROR; modify handle(int $severity, string $message, string $file, int
$line) so that after logging it returns false when $severity === E_USER_ERROR
(or E_ERROR-equivalent user-level constant) to allow PHP's default error handler
to run, otherwise keep the existing behavior (return true for non-fatal errors);
locate the severityToLogLevel call and logging block to add the conditional
return based on the $severity.
---
Nitpick comments:
In `@zmsslim/tests/Slim/Helper/PhpErrorHandlerTest.php`:
- Around line 52-56: Wrap the call to PhpErrorHandler::handle(...) with a
try/finally so error_reporting($previousReporting) always runs; specifically,
capture $previousReporting = error_reporting(0), then call
PhpErrorHandler::handle(E_USER_WARNING, 'suppressed warning', __FILE__,
__LINE__) inside a try block and restore error_reporting($previousReporting) in
the finally block to guarantee restoration even if handle() throws.
🪄 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: a89ab936-c429-4b73-9db1-7701f1b6f8ff
📒 Files selected for processing (14)
zmsadmin/src/Zmsadmin/Application.phpzmscalldisplay/src/Zmscalldisplay/Application.phpzmsslim/src/Slim/Bootstrap.phpzmsslim/src/Slim/Helper/ModuleLoggerInitializer.phpzmsslim/src/Slim/Helper/PhpErrorHandler.phpzmsslim/src/Slim/Middleware/RequestSanitizerMiddleware.phpzmsslim/src/Slim/Middleware/SecurityHeadersMiddleware.phpzmsslim/tests/Slim/Helper/ModuleLoggerInitializerTest.phpzmsslim/tests/Slim/Helper/PhpErrorHandlerTest.phpzmsslim/tests/Slim/Middleware/RequestSanitizerMiddlewareTest.phpzmsslim/tests/Slim/Middleware/SecurityHeadersMiddlewareTest.phpzmsslim/tests/Slim/TestLogger.phpzmsstatistic/src/Zmsstatistic/Application.phpzmsticketprinter/src/Zmsticketprinter/Application.php
🚧 Files skipped from review as they are similar to previous changes (5)
- zmsticketprinter/src/Zmsticketprinter/Application.php
- zmsstatistic/src/Zmsstatistic/Application.php
- zmsslim/src/Slim/Helper/ModuleLoggerInitializer.php
- zmsslim/tests/Slim/TestLogger.php
- zmsadmin/src/Zmsadmin/Application.php
# Please enter a commit message to explain why this merge is necessary, # especially if it merges an updated upstream into a topic branch. # # Lines starting with '#' will be ignored, and an empty message aborts # the commit.
PhpErrorHandler must preserve the previous error-to-exception behavior so missing route parameters still fail fast instead of reaching domain logic.
RequestLoggingMiddleware consumed the PSR-7 response stream while logging, so internal HTTP clients such as zmscalldisplay received empty API bodies.
Throwing after logging turned every warning into a fatal request failure (empty API bodies, calldisplay 500s). Log warnings as structured JSON and return true so execution continues as before PHP error handler registration.
…havior Module test bootstraps convert PHP notices into ErrorException before asserting domain exceptions. Do not replace that handler when PHPUnit runs; production still logs warnings as JSON without aborting requests.
Ticketprinter and other modules pass nested query params (e.g. statusList[0]). Casting those values to string triggered "Array to string conversion", which PhpErrorHandler logged and (on older builds) escalated to a fatal ErrorException.
Extract path building and error parsing from logRequest so PHPMD stays under the complexity threshold without changing logging behavior.
Regenerate composer.lock for zmsadmin, zmscalldisplay, zmsslim, zmsstatistic, and zmsticketprinter from merged composer.json files.
Regenerate locks so Symfony 8.1 and doctrine/instantiator 2.1 (PHP 8.4+) are replaced with PHP 8.3-compatible versions.
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.
Relocate HTTP logging, rate limiting, and header filtering to BO\Slim\LoggerService with configurable enrichers for citizen-api process context and error codes.
Pull Request Checklist (Feature Branch to
next):nextBranch in meinen Feature-Branch gemergt.docshinzugefügt.Summary by CodeRabbit
New Features
Configuration
Tests