feat(ZMSKVR-1122): Allow more sources dynamically and handle null source cases#1896
feat(ZMSKVR-1122): Allow more sources dynamically and handle null source cases#1896ThomasAFink wants to merge 6 commits intonextfrom
Conversation
… getSourceNames to handle null values
📝 WalkthroughWalkthroughApp::$source_name default was cleared. ZmsApiClientService#getSourceNames now returns normalized local source names when configured; otherwise it fetches, deduplicates, caches, and returns source names from the API /source/ endpoint. Changes
Sequence DiagramsequenceDiagram
actor Client
participant Service as ZmsApiClientService
participant Config as App::$source_name
participant Cache as Cache Layer
participant API as ZMS API
Client->>Service: getSourceNames()
Service->>Config: read App::$source_name
alt config has non-empty names
Config-->>Service: configured names (string|array)
Service->>Service: normalize, split, trim, dedupe
Service-->>Client: return local source names
else
Service->>Cache: lookup "sources_list"
alt cache hit
Cache-->>Service: cached list
Service-->>Client: return cached names
else
Service->>API: GET /source/
API-->>Service: source items
Service->>Service: extract/normalize/dedupe names
Service->>Cache: store "sources_list" (TTL)
Cache-->>Service: ack
Service-->>Client: return fetched names
end
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~22 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 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 |
|
@MoDaae moved to this new branch: |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@zmscitizenapi/src/Zmscitizenapi/Services/Core/ZmsApiClientService.php`:
- Around line 497-499: Cache hit may return non-array data; update the
cache-check in ZmsApiClientService (around the block using \App::$cache,
$cacheKey and $data) to validate the type before returning: if
\App::$cache->get($cacheKey) returns a value, ensure is_array($data) and only
return it when true, otherwise optionally delete/invalidate the corrupted entry
(e.g. \App::$cache->delete($cacheKey) if available) and proceed to fetch fresh
data so the method always returns an array.
- Around line 470-472: The code assigns $raw = \App::$source_name ?? 'null'
which sets the literal string "null" instead of a true null and then checks
($raw !== null && $raw !== '') causing a false-positive "null" source; update
the assignment in ZmsApiClientService so $raw is either directly
\App::$source_name (since the property is typed string) or uses a real null
fallback (?? null) instead of the string, then leave the existing conditional
that checks $raw !== null && $raw !== '' unchanged; locate the assignment to
$raw and replace the 'null' string fallback with null or remove the
null-coalescing entirely.
🧹 Nitpick comments (3)
zmscitizenapi/src/Zmscitizenapi/Services/Core/ZmsApiClientService.php (2)
463-488: Dead code:is_array()branch and array docblock are unreachable.
\App::$source_nameis declared aspublic static string $source_name, so$rawcan never be an array. Theis_arraybranch (line 473-474) is dead code, and the docblock advertising array acceptance is misleading. Consider removing the array path and simplifying:Simplified version
- /** - * Akzeptiert sowohl: - * - String: "dldb", "dldb,zms", "dldb; zms", "dldb zms", "dldb|zms" - * - Array: ["dldb","zms"] - */ private static function getSourceNames(): array { - $raw = \App::$source_name ?? 'null'; - - if ($raw !== null && $raw !== '') { - if (is_array($raw)) { - $names = array_values(array_filter(array_map('strval', $raw))); - } else { - $s = (string)$raw; - $names = preg_split('/[,\;\|\s]+/', $s, -1, PREG_SPLIT_NO_EMPTY) ?: []; - } - - $out = []; - foreach ($names as $n) { - $n = trim($n); - if ($n !== '' && !in_array($n, $out, true)) { - $out[] = $n; - } - } - - return $out; - } + $raw = trim(\App::$source_name ?? ''); + + if ($raw !== '') { + $names = preg_split('/[,;\|\s]+/', $raw, -1, PREG_SPLIT_NO_EMPTY) ?: []; + return array_values(array_unique(array_map('trim', $names))); + } + return self::fetchSourceNamesFromApi(); }As per coding guidelines, "Keep it simple stupid. Simpler is always better. Reduce complexity as much as possible."
501-511: Consider handling API failure gracefully infetchSourceNamesFromApi.If
\App::$http->readGetResult('/source/')throws an exception, it will propagate uncaught throughgetSourceNames()to the calling public methods (e.g.,getOffices,getServices), which do catch exceptions. This works, but it means a single/source/endpoint failure will cause all source-dependent methods to fail even if cached source data exists.Consider wrapping the API call with a try/catch that returns an empty array (or logs and falls back), so that callers don't silently break when only the source-listing endpoint is down.
zmscitizenapi/src/Zmscitizenapi/Application.php (1)
20-20: Add environment-based initialization for$source_namefor consistency with other config properties.
$source_nameis the only configurable property in this class not initialized from an environment variable. All other config properties (CACHE_DIR, LOGGER_, CAPTCHA_, RATE_LIMIT_, MAX_, IP_BLACKLIST, etc.) usegetenv()with fallback values during initialization. Addself::$source_name = getenv('SOURCE_NAME') ?: '';in one of theinitialize*()methods to follow the established pattern and enable runtime configuration.
zmscitizenapi/src/Zmscitizenapi/Services/Core/ZmsApiClientService.php
Outdated
Show resolved
Hide resolved
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@zmscitizenapi/src/Zmscitizenapi/Services/Core/ZmsApiClientService.php`:
- Around line 473-478: The is_array branch is dead because \App::$source_name is
typed as string; either widen that property to accept arrays or remove the
branch — prefer widening to match the docblock: change the property declaration
for \App::$source_name in the Application class to public static string|array
$source_name = ""; so the array handling in ZmsApiClientService remains valid
(leave the array branch in the method), and ensure the default value and any
usages handle both string and array types consistently.
🧹 Nitpick comments (3)
zmscitizenapi/src/Zmscitizenapi/Services/Core/ZmsApiClientService.php (3)
493-525:fetchSourceNamesFromApilacks its own error handling — relies on callers.All current callers (
getOffices,getServices, etc.) wrapgetSourceNames()in try/catch, so exceptions from the API call on line 501 will be caught. However, if this method is ever called from a new path without a catch, the exception will propagate unhandled. Consider adding a try/catch with a fallback to[]for resilience, consistent with the defensive style used elsewhere in this class.
436-461: ExistingfetchSourceDataForcache hit doesn't validate type — same pattern flagged forfetchSourceNamesFromApi.Line 440 returns the cached
$datawithout checking$data instanceof Source. If the cache entry is corrupted, callers will receive an unexpected type. This mirrors the concern previously raised (and now fixed) infetchSourceNamesFromApiat line 498.Proposed fix
if (\App::$cache && ($data = \App::$cache->get($cacheKey))) { - return $data; + if ($data instanceof Source) { + return $data; + } }
480-488: Minor style inconsistency: manual dedup vsarray_unique.
fetchSourceNamesFromApi(line 513) usesarray_values(array_unique(...))while this block uses a manual loop. Both work, but using the same approach would improve consistency.
| if (is_array($raw)) { | ||
| $names = array_values(array_filter(array_map('strval', $raw))); | ||
| } else { | ||
| $s = (string)$raw; | ||
| $names = preg_split('/[,\;\|\s]+/', $s, -1, PREG_SPLIT_NO_EMPTY) ?: []; | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Find the Application class and look for the $source_name property
find . -name "Application.php" -o -name "App.php" | head -20Repository: it-at-m/eappointment
Length of output: 462
🏁 Script executed:
#!/bin/bash
# Search for $source_name property declaration
rg '\$source_name' --type=php -B2 -A2Repository: it-at-m/eappointment
Length of output: 16658
🏁 Script executed:
#!/bin/bash
# Also search for the ZmsApiClientService file to understand the context better
find . -path "*ZmsApiClientService.php" -type fRepository: it-at-m/eappointment
Length of output: 135
🏁 Script executed:
#!/bin/bash
# Check the docblock and context for the getSourceNames method
sed -n '460,485p' zmscitizenapi/src/Zmscitizenapi/Services/Core/ZmsApiClientService.phpRepository: it-at-m/eappointment
Length of output: 845
🏁 Script executed:
#!/bin/bash
# Get more context including docblock
sed -n '450,490p' zmscitizenapi/src/Zmscitizenapi/Services/Core/ZmsApiClientService.phpRepository: it-at-m/eappointment
Length of output: 1263
Remove the dead is_array() branch or widen $source_name to accept array.
The docblock correctly documents that this method should accept both string and array inputs. However, \App::$source_name is typed as string in zmscitizenapi/src/Zmscitizenapi/Application.php, so $raw = \App::$source_name ?? '' will always be a string. The is_array($raw) check on line 473 is unreachable dead code.
Either change the property type to public static string|array $source_name = ""; to match the documented interface, or remove the unused array handling branch.
🤖 Prompt for AI Agents
In `@zmscitizenapi/src/Zmscitizenapi/Services/Core/ZmsApiClientService.php` around
lines 473 - 478, The is_array branch is dead because \App::$source_name is typed
as string; either widen that property to accept arrays or remove the branch —
prefer widening to match the docblock: change the property declaration for
\App::$source_name in the Application class to public static string|array
$source_name = ""; so the array handling in ZmsApiClientService remains valid
(leave the array branch in the method), and ensure the default value and any
usages handle both string and array types consistently.
1 participant
Pull Request Checklist (Feature Branch to
next):nextBranch in meinen Feature-Branch gemergt.Summary by CodeRabbit