Feature zmskvr implement cache in zmsdb #1570
Conversation
WalkthroughAdds PSR-16 caching (Symfony FilesystemAdapter + Psr16Cache) to zmsapi and zmsdb. Application classes initialize cache on load. Many entity/read methods gain an optional Changes
Sequence Diagram(s)sequenceDiagram
participant Loader as Module Load
participant App as Application
participant FS as Filesystem
participant CacheFactory as Symfony FilesystemAdapter -> Psr16Cache
Loader->>App: Application::initialize()
activate App
App->>FS: ensure(CACHE_DIR) / is_writable
FS-->>App: ok / error
alt directory writable
App->>CacheFactory: create FilesystemAdapter($CACHE_DIR,$ttl)
CacheFactory-->>App: Psr16Cache instance
App->>App: App::$cache = Psr16Cache
end
deactivate App
sequenceDiagram
participant Caller
participant Entity as Repository/Entity
participant Cache as App::$cache
participant DB as Database
Caller->>Entity: readEntity(id, resolve, $disableCache=false)
activate Entity
alt cache available and not disabled
Entity->>Cache: get(cacheKey)
Cache-->>Entity: cachedValue
Entity-->>Caller: cachedValue
else
Entity->>DB: query...
DB-->>Entity: result
opt cache present
Entity->>Cache: set(cacheKey, result)
Cache-->>Entity: ok
end
Entity-->>Caller: result
end
deactivate Entity
Caller->>Entity: writeEntity(data)
activate Entity
Entity->>DB: insert/update/delete
DB-->>Entity: success
Entity->>Entity: removeCache(entity)
Entity->>Cache: delete(key1), delete(key2), ...
Entity->>Entity: readEntity(id, resolve, $disableCache=true)
Entity-->>Caller: freshResult
deactivate Entity
Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 minutes Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ 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}
There was a problem hiding this comment.
Actionable comments posted: 10
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
zmsapi/src/Zmsapi/UseraccountSearchByDepartment.php (1)
28-29: Remove duplicate checkRights() call.Line 29 is a duplicate of line 28 that serves no purpose and should be removed.
Apply this diff to remove the duplicate:
$workstation = (new Helper\User($request, 1))->checkRights('useraccount'); -(new Helper\User($request, 1))->checkRights('useraccount'); $resolveReferences = Validator::param('resolveReferences')->isNumber()->setDefault(1)->getValue();zmsdb/src/Zmsdb/Scope.php (2)
21-44: Clarify or remove dual caching layers to prevent stale data.The method maintains two cache layers:
- Static cache (
self::$cache): In-memory array cache (line 32, 41, 44)- PSR-16 cache (
App::$cache): Filesystem cache (line 26, 42)Issues:
- Incomplete cache bypass: Setting
$disableCache = truebypasses PSR-16 cache but still checks and populates the static cache (line 32), which is inconsistent.- Stale data risk: The static cache is never synchronized when the PSR-16 cache is updated elsewhere (e.g., by another process or explicit cache manipulation).
- Memory leaks: The static cache grows unbounded and is only cleared on write/update/delete operations (lines 411, 432, 487, 579), not on cache eviction.
Consider one of these approaches:
Option 1: Remove static cache (simplest):
- public static $cache = [ ]; - public function readEntity($scopeId, $resolveReferences = 0, $disableCache = false) { $cacheKey = "scope-$scopeId-$resolveReferences"; if (!$disableCache) { $data = App::$cache->get($cacheKey, false); if ($data !== false) { return $data; } } - if ($disableCache || ! array_key_exists($cacheKey, self::$cache)) { $query = new Query\Scope(Query\Base::SELECT); // ... query logic ... - self::$cache[$cacheKey] = $this->readResolvedReferences($scope, $resolveReferences); + $result = $this->readResolvedReferences($scope, $resolveReferences); - App::$cache->set($cacheKey, self::$cache[$cacheKey]); - } - return self::$cache[$cacheKey]; + if (!$disableCache) { + App::$cache->set($cacheKey, $result); + } + return $result; }Option 2: Honor
$disableCachefor both layers:- if ($disableCache || ! array_key_exists($cacheKey, self::$cache)) { + if ($disableCache || ! array_key_exists($cacheKey, self::$cache)) { // ... query logic ... self::$cache[$cacheKey] = $this->readResolvedReferences($scope, $resolveReferences); - App::$cache->set($cacheKey, self::$cache[$cacheKey]); + if (!$disableCache) { + App::$cache->set($cacheKey, self::$cache[$cacheKey]); + } } return self::$cache[$cacheKey];
409-439: Invalidate PSR-16 cache on write/update operations.When writing or updating scopes, the code clears the static cache (
self::$cache = [];) but does not invalidate the PSR-16 cache (App::$cache). This causes stale data to be served from the filesystem cache.Add cache invalidation for the PSR-16 layer. Since you cannot easily determine all affected keys (due to varying
$resolveReferencesvalues), consider either:Option 1: Clear all scope caches (if PSR-16 implementation supports namespaced clearing):
public function writeEntity(\BO\Zmsentities\Scope $entity, $parentId) { self::$cache = []; + // Clear all scope-related cache entries + // Note: This requires a cache implementation that supports pattern-based clearing + // or you need to track all keys that need invalidation // ... rest of method }Option 2: Invalidate specific known keys:
public function updateEntity($scopeId, \BO\Zmsentities\Scope $entity, $resolveReferences = 0) { self::$cache = []; + // Invalidate known cache keys for this scope + for ($i = 0; $i <= 2; $i++) { + App::$cache->delete("scope-$scopeId-$i"); + } // ... rest of method }Option 3: Implement a cache tagging strategy using Symfony Cache's tag-aware adapters (see learnings), allowing you to tag all scope-related cache entries and invalidate them by tag.
♻️ Duplicate comments (1)
zmsticketprinter/templates/page/process.twig (1)
22-22: Verify backend providesprocess.queue.number.Same concern as in print.twig: ensure the backend correctly populates
process.queue.numberbefore this change goes live.
🧹 Nitpick comments (18)
zmsapi/src/Zmsapi/UseraccountSearchByDepartment.php (1)
37-48: Consider consolidating access control logic.The code applies access filtering twice: once via
withAccessByWorkstation()(line 37) and again through manual iteration (lines 39-48). If both filters serve the same purpose, consider consolidating them into a single method to improve maintainability.zmsdb/src/Zmsdb/DayOff.php (2)
70-75: Simplify the cache check—App::$cacheis redundant.The condition
if (App::$cache && !empty($data))unnecessarily checks whether the cache object exists before checking the retrieved data. SinceApp::$cache->get()is always called (line 72), the cache must already be initialized. Simplify to:- $data = App::$cache->get($cacheKey); - if (App::$cache && !empty($data)) { - return $data; - } + $data = App::$cache->get($cacheKey); + if (!empty($data)) { + return $data; + }This pattern appears consistently across multiple files in the PR (Closure, Department, Config, etc.)—consider applying this simplification codebase-wide for consistency.
68-68: Consider adding a$disableCacheparameter for consistency.Other cached read methods in this PR (e.g.,
Department::readEntity,Scope::readEntity,Config::readEntity) include an optional$disableCacheparameter to allow callers to bypass the cache when needed. Adding this parameter here would align with the broader caching pattern and provide flexibility for scenarios requiring fresh data (e.g., after writes, in tests, or when cache invalidation is complex).zmsdb/src/Zmsdb/Department.php (1)
29-34: Simplify the cache check—App::$cacheis redundant.The condition
if (App::$cache && !empty($data))is redundant becauseApp::$cache->get()is called unconditionally on line 30. Simplify to:if (!$disableCache) { $data = App::$cache->get($cacheKey); - if (App::$cache && !empty($data)) { + if (!empty($data)) { return $data; } }This simplification applies across the codebase—see also DayOff.php, Closure.php, Config.php, Provider.php, Source.php, Request.php, and Cluster.php.
zmsdb/src/Zmsdb/Closure.php (2)
14-19: Simplify the cache check—App::$cacheis redundant.The condition
if (App::$cache && !empty($data))unnecessarily checks whether the cache object exists. SinceApp::$cache->get()is called unconditionally (line 16), simplify to:- $data = App::$cache->get($cacheKey); - if (App::$cache && !empty($data)) { - return $data; - } + $data = App::$cache->get($cacheKey); + if (!empty($data)) { + return $data; + }
12-12: Consider adding a$disableCacheparameter for consistency.Other cached read methods in this PR include an optional
$disableCacheparameter to allow bypassing the cache when needed. Adding this parameter would align with the caching pattern used inDepartment::readEntity,Scope::readEntity,Config::readEntity, and others.zmsdb/src/Zmsdb/Config.php (2)
17-22: Simplify the cache check—App::$cacheis redundant.The condition
if (App::$cache && !empty($data))is redundant. SinceApp::$cache->get()is always called (line 18), simplify to:if (!$disableCache) { $data = App::$cache->get($cacheKey); - if (App::$cache && !empty($data)) { + if (!empty($data)) { return $data; } }
32-36: Verify cache invalidation timing inupdateEntity.The cache is deleted on line 34, but
readEntity()is called on line 36 without$disableCache = true. Although the cache was just deleted, explicitly passing$disableCache = truemakes the intent clearer and avoids a redundant cache lookup:- App::$cache->delete('config'); - - $compareEntity = $this->readEntity(); + App::$cache->delete('config'); + + $compareEntity = $this->readEntity($disableCache = true);Then at line 60:
- return ($result) ? $this->readEntity() : null; + return ($result) ? $this->readEntity($disableCache = false) : null;This ensures the final return uses the fresh cached value after all updates are complete.
zmsdb/src/Zmsdb/Provider.php (2)
15-20: Simplify the cache check—App::$cacheis redundant.The condition
if (App::$cache && !empty($data))is redundant:if (!$disableCache) { $data = App::$cache->get($cacheKey); - if (App::$cache && !empty($data)) { + if (!empty($data)) { return $data; } }
31-31: Cache is written even when$disableCacheis true.Line 31 unconditionally caches the result, even when the caller passed
$disableCache = true. While this may be intentional (read bypasses cache, but write still populates it), it's inconsistent with the typical interpretation of$disableCache. Consider guarding the cache write:$provider = $this->fetchOne($query, new Entity()); - App::$cache->set($cacheKey, $provider); + if (!$disableCache) { + App::$cache->set($cacheKey, $provider); + } return $provider;This pattern appears in multiple files (Request, Source, Cluster)—apply consistently if you adopt this approach.
zmsdb/src/Zmsdb/Source.php (2)
29-34: Simplify the cache check—App::$cacheis redundant.The condition
if (App::$cache && !empty($data))is redundant:if (!$disableCache) { $data = App::$cache->get($cacheKey); - if (App::$cache && !empty($data)) { + if (!empty($data)) { return $data; } }
47-47: Cache is written even when$disableCacheis true.Line 47 unconditionally caches the result, even when
$disableCache = true. Consider guarding the cache write:$entity = $this->readResolvedReferences($entity, $resolveReferences); - - App::$cache->set($cacheKey, $entity); + + if (!$disableCache) { + App::$cache->set($cacheKey, $entity); + } return $entity;See also Provider.php, Request.php, and Cluster.php for the same pattern.
zmsdb/src/Zmsdb/Request.php (2)
19-24: Simplify the cache check—App::$cacheis redundant.The condition
if (App::$cache && !empty($data))is redundant:if (!$disableCache) { $data = App::$cache->get($cacheKey); - if (App::$cache && !empty($data)) { + if (!empty($data)) { return $data; } }
39-39: Cache is written even when$disableCacheis true.Line 39 unconditionally caches the result, even when
$disableCache = true. Consider guarding the cache write for consistency:if (! $request->hasId()) { throw new Exception\Request\RequestNotFound("Could not find request with ID $source/$requestId"); } - - App::$cache->set($cacheKey, $request); - + + if (!$disableCache) { + App::$cache->set($cacheKey, $request); + } + return $request;zmsdb/src/Zmsdb/Cluster.php (2)
30-35: Simplify the cache check—App::$cacheis redundant.The condition
if (App::$cache && !empty($data))is redundant:$cacheKey = "cluster-$itemId-$resolveReferences"; if (!$disableCache) { $data = App::$cache->get($cacheKey); - if (App::$cache && !empty($data)) { + if (!empty($data)) { return $data; } }
47-48: Cache is written even when$disableCacheis true.Line 48 unconditionally caches the result, even when
$disableCache = true. Consider guarding the cache write:$cluster = $this->readResolvedReferences($cluster, $resolveReferences); - App::$cache->set($cacheKey, $cluster); - + if (!$disableCache) { + App::$cache->set($cacheKey, $cluster); + } + return $cluster;zmsdb/src/Zmsdb/Scope.php (2)
23-23: Use consistent cache key naming conventions.The cache keys use different naming patterns:
- Line 23:
"scope-$scopeId-$resolveReferences"(short, entity-type prefix)- Line 95:
"scopeReadByProviderId-$providerId-$resolveReferences"(long, method-name prefix)Inconsistent naming makes cache key management harder and increases the risk of collisions.
Standardize to a consistent pattern. For example, use entity-operation format:
- $cacheKey = "scope-$scopeId-$resolveReferences"; + $cacheKey = "scope-byId-$scopeId-$resolveReferences";- $cacheKey = "scopeReadByProviderId-$providerId-$resolveReferences"; + $cacheKey = "scope-byProviderId-$providerId-$resolveReferences";Or adopt a structured format like
{entity}:{operation}:{params}:$cacheKey = "scope:byId:$scopeId:$resolveReferences"; $cacheKey = "scope:byProviderId:$providerId:$resolveReferences";Also applies to: 95-95
21-44: Add error handling for cache operations.Per PSR-16, cache implementations MUST throw
Psr\SimpleCache\InvalidArgumentExceptionfor invalid keys and MAY throwPsr\SimpleCache\CacheExceptionon operational failures. Currently, no error handling is present—exceptions will propagate and potentially break request processing.Wrap cache operations in try-catch blocks to gracefully degrade when caching fails:
if (!$disableCache) { + try { $data = App::$cache->get($cacheKey, false); if ($data !== false) { return $data; } + } catch (\Psr\SimpleCache\CacheException $e) { + // Log the error but continue without cache + // Assuming a logger is available via dependency injection or App::$log + } }Similarly for cache writes:
- App::$cache->set($cacheKey, $result); + try { + App::$cache->set($cacheKey, $result); + } catch (\Psr\SimpleCache\CacheException $e) { + // Log the error but continue + }Also applies to: 93-131
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (19)
zmsapi/composer.json(1 hunks)zmsapi/src/Zmsapi/Application.php(3 hunks)zmsapi/src/Zmsapi/UseraccountSearchByDepartment.php(1 hunks)zmsdb/composer.json(1 hunks)zmsdb/src/Zmsdb/Application.php(3 hunks)zmsdb/src/Zmsdb/Calldisplay.php(1 hunks)zmsdb/src/Zmsdb/Closure.php(3 hunks)zmsdb/src/Zmsdb/Cluster.php(3 hunks)zmsdb/src/Zmsdb/Config.php(2 hunks)zmsdb/src/Zmsdb/DayOff.php(3 hunks)zmsdb/src/Zmsdb/Department.php(3 hunks)zmsdb/src/Zmsdb/Provider.php(2 hunks)zmsdb/src/Zmsdb/Request.php(3 hunks)zmsdb/src/Zmsdb/Scope.php(5 hunks)zmsdb/src/Zmsdb/Source.php(3 hunks)zmsdb/src/Zmsdb/Useraccount.php(1 hunks)zmsentities/src/Zmsentities/Collection/DepartmentList.php(1 hunks)zmsticketprinter/templates/block/content/print.twig(1 hunks)zmsticketprinter/templates/page/process.twig(2 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
**/*
⚙️ CodeRabbit configuration file
**/*: Apply the following Clean Code guidelines to all files, as summarized by wojteklu's Clean Code gist:Code is clean if it can be understood easily – by everyone on the team. Clean code can be read and enhanced by a developer other than its original author. With understandability comes readability, changeability, extensibility and maintainability.
General rules
- Follow standard conventions.
- Keep it simple stupid. Simpler is always better. Reduce complexity as much as possible.
- Boy scout rule. Leave the campground cleaner than you found it.
- Always find root cause. Always look for the root cause of a problem.
Design rules
- Keep configurable data at high levels.
- Prefer polymorphism to if/else or switch/case.
- Separate multi-threading code.
- Prevent over-configurability.
- Use dependency injection.
- Follow Law of Demeter. A class should know only its direct dependencies.
Understandability tips
- Be consistent. If you do something a certain way, do all similar things in the same way.
- Use explanatory variables.
- Encapsulate boundary conditions. Boundary conditions are hard to keep track of. Put the processing for them in one place.
- Prefer dedicated value objects to primitive type.
- Avoid logical dependency. Don't write methods which works correctly depending on something else in the same class.
- Avoid negative conditionals.
Names rules
- Choose descriptive and unambiguous names.
- Make meaningful distinction.
- Use pronounceable names.
- Use searchable names.
- Replace magic numbers with named constants.
- Avoid encodings. Don't append prefixes or type information.
Functions rules
- Small.
- Do one thing.
- Use descriptive names.
- Prefer fewer arguments.
- Have no side effects.
- Don't use flag arguments. Split method into several independent methods that can b...
Files:
zmsapi/composer.jsonzmsdb/src/Zmsdb/Calldisplay.phpzmsdb/src/Zmsdb/Request.phpzmsdb/src/Zmsdb/Source.phpzmsdb/composer.jsonzmsdb/src/Zmsdb/Config.phpzmsdb/src/Zmsdb/Application.phpzmsdb/src/Zmsdb/Scope.phpzmsdb/src/Zmsdb/Provider.phpzmsdb/src/Zmsdb/Cluster.phpzmsdb/src/Zmsdb/Closure.phpzmsdb/src/Zmsdb/Department.phpzmsdb/src/Zmsdb/Useraccount.phpzmsticketprinter/templates/page/process.twigzmsticketprinter/templates/block/content/print.twigzmsapi/src/Zmsapi/UseraccountSearchByDepartment.phpzmsdb/src/Zmsdb/DayOff.phpzmsentities/src/Zmsentities/Collection/DepartmentList.phpzmsapi/src/Zmsapi/Application.php
**/*.php
⚙️ CodeRabbit configuration file
**/*.php: Flag any usage of error_log() as it should be replaced with proper Monolog logging mechanisms provided by zmsslim which should be setup in the App/Application of each module:
- For error handling: Use the proper Monolog logging framework with error levels
- For application info logs: Use the proper Monolog logging framework with info levels
- For debugging: Use a dedicated debug logger or remove debug statements
- For CLI output: Use a CLI output handler or symfony/console
- The application log levels are as follows DEBUG, INFO, NOTICE, WARNING, ERROR, CRITICAL, ALERT, and EMERGENCY
Example replacement:
// Instead of: error_log("Import failed - " . $e->getMessage()); // Use: $log->error("Import failed", ['error' => $e->getMessage()]);Flag specific logging violations:
- error_log(), var_dump(), print_r(), die(), exit() usage (except proper error logging in catch blocks)
- Any debug output that should use proper Monolog logging
- Debug constants like DEBUG = true
- Debug logging that should be removed in production
- Commented debug code that should be cleaned up
Example replacements:
// Instead of: error_log("Error occurred"); var_dump($data); die('debug point'); // Use: $log->error("Error occurred", ['context' => 'processing']); $log->debug('Data dump', ['data' => $data]); // Remove die() statements entirelyException handling should use proper logging:
// Instead of: try { $result = $this->process(); } catch (Exception $e) { error_log("Processing failed: " . $e->getMessage()); } // Use: try { $result = $this->process(); } catch (Exception $e) { $log->error("Processing failed", ['error' => $e->getMessage(), 'trace' => $e->getTraceAsString()]); } ```...
Files:
zmsdb/src/Zmsdb/Calldisplay.phpzmsdb/src/Zmsdb/Request.phpzmsdb/src/Zmsdb/Source.phpzmsdb/src/Zmsdb/Config.phpzmsdb/src/Zmsdb/Application.phpzmsdb/src/Zmsdb/Scope.phpzmsdb/src/Zmsdb/Provider.phpzmsdb/src/Zmsdb/Cluster.phpzmsdb/src/Zmsdb/Closure.phpzmsdb/src/Zmsdb/Department.phpzmsdb/src/Zmsdb/Useraccount.phpzmsapi/src/Zmsapi/UseraccountSearchByDepartment.phpzmsdb/src/Zmsdb/DayOff.phpzmsentities/src/Zmsentities/Collection/DepartmentList.phpzmsapi/src/Zmsapi/Application.php
🧬 Code graph analysis (14)
zmsdb/src/Zmsdb/Request.php (6)
zmsdb/src/Zmsdb/Application.php (1)
Application(19-100)zmsdb/src/Zmsdb/Cluster.php (1)
readEntity(27-51)zmsdb/src/Zmsdb/Department.php (1)
readEntity(22-49)zmsdb/src/Zmsdb/Provider.php (1)
readEntity(11-33)zmsdb/src/Zmsdb/Scope.php (1)
readEntity(21-45)zmsdb/src/Zmsdb/Source.php (1)
readEntity(25-49)
zmsdb/src/Zmsdb/Source.php (2)
zmsdb/src/Zmsdb/Application.php (1)
Application(19-100)zmsdb/src/Zmsdb/Cluster.php (1)
readEntity(27-51)
zmsdb/src/Zmsdb/Config.php (4)
zmsdb/src/Zmsdb/Application.php (1)
Application(19-100)zmsdb/src/Zmsdb/Cluster.php (2)
readEntity(27-51)updateEntity(378-389)zmsdb/src/Zmsdb/Department.php (2)
readEntity(22-49)updateEntity(193-217)zmsdb/src/Zmsdb/Provider.php (1)
readEntity(11-33)
zmsdb/src/Zmsdb/Application.php (2)
zmsapi/src/Zmsapi/Application.php (5)
initializeCache(124-130)validateCacheDirectory(132-141)setupCache(143-147)initialize(149-152)Application(25-153)zmscitizenapi/src/Zmscitizenapi/Application.php (5)
initializeCache(108-114)validateCacheDirectory(146-155)setupCache(157-161)initialize(63-70)Application(16-208)
zmsdb/src/Zmsdb/Scope.php (1)
zmsdb/src/Zmsdb/Application.php (1)
Application(19-100)
zmsdb/src/Zmsdb/Provider.php (8)
zmsdb/src/Zmsdb/Application.php (1)
Application(19-100)zmsentities/src/Zmsentities/Provider.php (1)
Provider(5-104)zmsdb/src/Zmsdb/Query/Provider.php (1)
Provider(5-86)zmsdb/src/Zmsdb/Cluster.php (1)
readEntity(27-51)zmsdb/src/Zmsdb/Department.php (1)
readEntity(22-49)zmsdb/src/Zmsdb/Request.php (1)
readEntity(15-42)zmsdb/src/Zmsdb/Scope.php (1)
readEntity(21-45)zmsdb/src/Zmsdb/Source.php (1)
readEntity(25-49)
zmsdb/src/Zmsdb/Cluster.php (7)
zmsdb/src/Zmsdb/Application.php (1)
Application(19-100)zmsdb/src/Zmsdb/Config.php (1)
readEntity(14-30)zmsdb/src/Zmsdb/Department.php (2)
readEntity(22-49)readResolvedReferences(51-62)zmsdb/src/Zmsdb/Provider.php (1)
readEntity(11-33)zmsdb/src/Zmsdb/Request.php (1)
readEntity(15-42)zmsdb/src/Zmsdb/Scope.php (2)
readEntity(21-45)readResolvedReferences(47-56)zmsdb/src/Zmsdb/Source.php (2)
readEntity(25-49)readResolvedReferences(79-94)
zmsdb/src/Zmsdb/Closure.php (1)
zmsdb/src/Zmsdb/Application.php (1)
Application(19-100)
zmsdb/src/Zmsdb/Department.php (1)
zmsdb/src/Zmsdb/Application.php (1)
Application(19-100)
zmsdb/src/Zmsdb/Useraccount.php (2)
zmsentities/src/Zmsentities/Schema/Entity.php (1)
toProperty(264-267)zmsentities/src/Zmsentities/Collection/Base.php (1)
addEntity(109-113)
zmsapi/src/Zmsapi/UseraccountSearchByDepartment.php (1)
zmsdb/src/Zmsdb/Useraccount.php (1)
readSearchByDepartmentId(276-296)
zmsdb/src/Zmsdb/DayOff.php (1)
zmsdb/src/Zmsdb/Application.php (1)
Application(19-100)
zmsentities/src/Zmsentities/Collection/DepartmentList.php (1)
zmsentities/src/Zmsentities/Collection/Base.php (1)
addEntity(109-113)
zmsapi/src/Zmsapi/Application.php (2)
zmsdb/src/Zmsdb/Application.php (5)
initializeCache(71-77)validateCacheDirectory(79-88)setupCache(90-94)initialize(96-99)Application(19-100)zmscitizenapi/src/Zmscitizenapi/Application.php (5)
initializeCache(108-114)validateCacheDirectory(146-155)setupCache(157-161)initialize(63-70)Application(16-208)
🪛 GitHub Actions: Combined Workflow
zmsapi/composer.json
[error] 1-1: Git detected dubious ownership for repository; add safe.directory for path '/__w/eappointment/eappointment'. Composer could not detect the root package (eappointment/zmsapi); defaulting to version 1.0.0.
zmsdb/src/Zmsdb/Calldisplay.php
[warning] 1-1: Composer could not detect the root package (eappointment/zmsdb) version, defaulting to '1.0.0'. See https://getcomposer.org/root-version
zmsdb/src/Zmsdb/Request.php
[warning] 1-1: Composer could not detect the root package (eappointment/zmsdb) version, defaulting to '1.0.0'. See https://getcomposer.org/root-version
zmsdb/src/Zmsdb/Source.php
[warning] 1-1: Composer could not detect the root package (eappointment/zmsdb) version, defaulting to '1.0.0'. See https://getcomposer.org/root-version
zmsdb/composer.json
[warning] 1-1: Composer could not detect the root package (eappointment/zmsdb) version, defaulting to '1.0.0'. See https://getcomposer.org/root-version
[warning] 1-1: Warning: Lock file is not up to date with composer.json. It is recommended to run 'composer update' to refresh dependencies.
zmsdb/src/Zmsdb/Config.php
[warning] 1-1: Composer could not detect the root package (eappointment/zmsdb) version, defaulting to '1.0.0'. See https://getcomposer.org/root-version
zmsdb/src/Zmsdb/Application.php
[warning] 1-1: Composer could not detect the root package (eappointment/zmsdb) version, defaulting to '1.0.0'. See https://getcomposer.org/root-version
zmsdb/src/Zmsdb/Scope.php
[warning] 1-1: Composer could not detect the root package (eappointment/zmsdb) version, defaulting to '1.0.0'. See https://getcomposer.org/root-version
zmsdb/src/Zmsdb/Provider.php
[warning] 1-1: Composer could not detect the root package (eappointment/zmsdb) version, defaulting to '1.0.0'. See https://getcomposer.org/root-version
zmsdb/src/Zmsdb/Cluster.php
[warning] 1-1: Composer could not detect the root package (eappointment/zmsdb) version, defaulting to '1.0.0'. See https://getcomposer.org/root-version
zmsdb/src/Zmsdb/Closure.php
[warning] 1-1: Composer could not detect the root package (eappointment/zmsdb) version, defaulting to '1.0.0'. See https://getcomposer.org/root-version
zmsdb/src/Zmsdb/Department.php
[warning] 1-1: Composer could not detect the root package (eappointment/zmsdb) version, defaulting to '1.0.0'. See https://getcomposer.org/root-version
zmsdb/src/Zmsdb/Useraccount.php
[warning] 1-1: Composer could not detect the root package (eappointment/zmsdb) version, defaulting to '1.0.0'. See https://getcomposer.org/root-version
zmsapi/src/Zmsapi/UseraccountSearchByDepartment.php
[error] 1-1: Git detected dubious ownership for repository; add safe.directory for path '/__w/eappointment/eappointment'. Composer could not detect the root package (eappointment/zmsapi); defaulting to version 1.0.0.
zmsdb/src/Zmsdb/DayOff.php
[warning] 1-1: Composer could not detect the root package (eappointment/zmsdb) version, defaulting to '1.0.0'. See https://getcomposer.org/root-version
zmsapi/src/Zmsapi/Application.php
[error] 1-1: Git detected dubious ownership for repository; add safe.directory for path '/__w/eappointment/eappointment'. Composer could not detect the root package (eappointment/zmsapi); defaulting to version 1.0.0.
🔇 Additional comments (3)
zmsticketprinter/templates/block/content/print.twig (1)
4-4: No action needed;process.queue.numberis populated in the backend and mirrors the previousdisplayNumberlogic.zmsticketprinter/templates/page/process.twig (1)
64-64: No action needed:waitingNumberalready maps to the queue indexThe Notification endpoint in zmsticketprinter/src/Zmsticketprinter/Notification.php reads
waitingNumberto fetch/scope/{id}/queue/{waitingNumber}. Tests (NotificationByScopeTest, NotificationFailedTest) post a numericwaitingNumbermatchingprocess.queue.number. This change aligns with existing endpoint logic.zmsapi/src/Zmsapi/UseraccountSearchByDepartment.php (1)
36-36: Verify removal of withLessData for department searchRemoving
withLessData()at UseraccountSearchByDepartment returns fullUseraccountobjects. Confirm that this aligns with the caching strategy, that downstream consumers and existing tests handle the larger payload without performance regressions or failures.
| "symfony/cache": "^6.0", | ||
| "psr/simple-cache": "^3.0", | ||
| "monolog/monolog": "^2.0", |
There was a problem hiding this comment.
Target a supported symfony/cache release.
symfony/cache 6.0 reached end of support in January 2023, so new security fixes no longer land on that branch. Please bump the constraint to a maintained line (e.g. ^6.4 LTS or ^7.0) before shipping the cache integration.
🤖 Prompt for AI Agents
In zmsapi/composer.json around lines 42 to 44, the dependency constraint for
"symfony/cache" is set to "^6.0" which is EOL and must be updated; change the
constraint to a supported release (for example "^6.4" for the LTS line or
"^7.0") and run composer update to refresh the lockfile and verify compatibility
with other packages (adjust any code if new major version BC breaks appear).
| $parameters = $request->getParams(); | ||
|
|
||
| /** @var Useraccount $useraccount */ | ||
| $useraccountList = new Collection(); |
There was a problem hiding this comment.
Remove unused initialization.
The collection created on line 35 is immediately overwritten on line 36, making this line dead code.
Apply this diff to remove the unused initialization:
/** @var Useraccount $useraccount */
-$useraccountList = new Collection();
$useraccountList = (new Useraccount())->readSearchByDepartmentId($department->id, $parameters, $resolveReferences);🤖 Prompt for AI Agents
In zmsapi/src/Zmsapi/UseraccountSearchByDepartment.php around line 35, the code
creates a new Collection assigned to $useraccountList which is immediately
overwritten on the next line; remove the unused initialization on line 35 so
$useraccountList is only assigned the actual value on line 36, eliminating the
dead code.
| "symfony/cache": "^6.0", | ||
| "psr/simple-cache": "^3.0", |
There was a problem hiding this comment.
Use a maintained symfony/cache version.
Version 6.0 is out of support; it no longer receives security updates. Please depend on a supported branch (^6.4 LTS or ^7.0) to keep the new cache layer within a maintained release window.
🤖 Prompt for AI Agents
In zmsdb/composer.json around lines 37 to 38, the dependency on "symfony/cache":
"^6.0" targets an unsupported release; update the constraint to a maintained
branch such as "^6.4" or "^7.0" (choose the one compatible with your codebase),
keep "psr/simple-cache": "^3.0" compatibility, then run composer update and run
your test suite or static checks to ensure no breaking changes.
| { | ||
| if (!is_dir(self::$CACHE_DIR) && !mkdir(self::$CACHE_DIR, 0750, true)) { | ||
| throw new \RuntimeException(sprintf('Cache directory "%s" could not be created', self::$CACHE_DIR)); | ||
| } | ||
|
|
||
| if (!is_writable(self::$CACHE_DIR)) { | ||
| throw new \RuntimeException(sprintf('Cache directory "%s" is not writable', self::$CACHE_DIR)); | ||
| } |
There was a problem hiding this comment.
Harden cache-directory creation against race conditions
Two workers hitting initialize() concurrently can both see the cache directory missing, one succeeds, the other hits mkdir() after the directory already exists, gets false, and we throw. This causes spurious runtime failures on first requests. Add a second is_dir guard after mkdir() (or otherwise handle the EEXIST case) before raising.
Apply this diff:
- if (!is_dir(self::$CACHE_DIR) && !mkdir(self::$CACHE_DIR, 0750, true)) {
- throw new \RuntimeException(sprintf('Cache directory "%s" could not be created', self::$CACHE_DIR));
- }
+ if (!is_dir(self::$CACHE_DIR) && !mkdir(self::$CACHE_DIR, 0750, true) && !is_dir(self::$CACHE_DIR)) {
+ throw new \RuntimeException(sprintf(
+ 'Cache directory "%s" could not be created',
+ self::$CACHE_DIR
+ ));
+ }🤖 Prompt for AI Agents
In zmsdb/src/Zmsdb/Application.php around lines 80 to 87, the cache-directory
creation is racy: two processes can both observe the directory missing and the
second mkdir() can fail when the first created it, causing a thrown
RuntimeException. Fix by re-checking is_dir() immediately after a failed mkdir()
and only throw if the directory still does not exist (or alternatively inspect
errno for EEXIST); then proceed to the is_writable() check as before. Ensure the
code preserves the original permissions attempt and only treats mkdir failure as
fatal if the directory truly does not exist.
| if (0 < $resolveReferences && $entity->toProperty()->id->get()) { | ||
| $entity->departments = $this->readAssignedDepartmentList( | ||
| $entity, | ||
| $resolveReferences - 1 | ||
| ); | ||
| } | ||
| $collection->addEntity($entity); |
There was a problem hiding this comment.
Remove code duplication and ensure consistent reference resolution.
This code duplicates the logic from readResolvedReferences (lines 39-41) and creates inconsistent behavior compared to similar methods like readCollectionByDepartmentId (line 139).
Issues:
- DRY violation: The department population logic is duplicated here instead of reusing
readResolvedReferences. - Inconsistent depth: Passing
$resolveReferences - 1toreadAssignedDepartmentListdiffers fromreadResolvedReferenceswhich passes$resolveReferences, causing departments to be resolved one level shallower than in other methods. - Maintenance burden: If
readResolvedReferencesis extended with additional logic, this method won't benefit from those changes.
Refactor to use the standard resolution path:
if (count($result)) {
foreach ($result as $entity) {
- if (0 < $resolveReferences && $entity->toProperty()->id->get()) {
- $entity->departments = $this->readAssignedDepartmentList(
- $entity,
- $resolveReferences - 1
- );
- }
- $collection->addEntity($entity);
+ $collection->addEntity($this->readResolvedReferences($entity, $resolveReferences));
}
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if (0 < $resolveReferences && $entity->toProperty()->id->get()) { | |
| $entity->departments = $this->readAssignedDepartmentList( | |
| $entity, | |
| $resolveReferences - 1 | |
| ); | |
| } | |
| $collection->addEntity($entity); | |
| if (count($result)) { | |
| foreach ($result as $entity) { | |
| $collection->addEntity($this->readResolvedReferences($entity, $resolveReferences)); | |
| } | |
| } |
🤖 Prompt for AI Agents
In zmsdb/src/Zmsdb/Useraccount.php around lines 155-161, remove the duplicated
department population block and instead invoke the existing resolution helper so
department references are handled consistently; replace the if-block that calls
readAssignedDepartmentList with a call to $this->readResolvedReferences($entity,
$resolveReferences) (matching the pattern used elsewhere) so you reuse the
shared logic and pass $resolveReferences (not $resolveReferences - 1) to
maintain consistent resolution depth.
| if ($department->hasAccess($useraccount)) { | ||
| if ($useraccount->rights['organisation']) { | ||
| return clone $this; | ||
| return $this; |
There was a problem hiding this comment.
Returning $this breaks immutability and allows unintended mutations.
Returning the original collection reference ($this) instead of a clone violates the immutability pattern followed by other methods in this class (e.g., withMatchingScopes at line 73 clones entities). This allows callers to modify the original collection, potentially causing side effects elsewhere in the codebase.
Additionally, the early return inside the loop is inefficient—if organisation rights grant full access, checking before the loop would be cleaner.
Apply this diff to check organisation rights before the loop and return a reference safely, or return a clone if immutability is required:
public function withAccess(\BO\Zmsentities\Useraccount $useraccount)
{
+ // Early check for organisation rights - return full list if granted
+ if ($useraccount->rights['organisation']) {
+ return $this; // or: return clone $this; if immutability is required
+ }
+
$list = new static();
foreach ($this as $department) {
if ($department->hasAccess($useraccount)) {
- if ($useraccount->rights['organisation']) {
- return $this;
- }
- $list->addEntity($department);
+ $list->addEntity(clone $department);
}
}
return $list;
}🤖 Prompt for AI Agents
In zmsentities/src/Zmsentities/Collection/DepartmentList.php around line 61, the
method currently returns $this which breaks the class's immutability contract
and performs an early return inside the loop; update the method to check
organisation-level rights once before looping (so if organisation grants full
access you can short-circuit) and return a clone of the collection (not $this)
to preserve immutability consistent with withMatchingScopes at line 73; ensure
any early-return branch returns a cloned instance and not the original object.
| return $this; | ||
| } | ||
| $list->addEntity(clone $department); | ||
| $list->addEntity($department); |
There was a problem hiding this comment.
Not cloning departments breaks the immutability pattern.
Other methods in this class (e.g., withMatchingScopes at line 73, withOutClusterDuplicates at line 15) create new instances or clones before adding to result collections. Adding the original department reference here means modifications to departments in the returned list will affect the source collection.
Apply this diff to clone departments before adding them:
- $list->addEntity($department);
+ $list->addEntity(clone $department);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| $list->addEntity($department); | |
| $list->addEntity(clone $department); |
🤖 Prompt for AI Agents
In zmsentities/src/Zmsentities/Collection/DepartmentList.php around line 63 the
code adds the original $department to the new list which breaks the class's
immutability pattern; change the call to add a cloned instance instead (use
PHP's clone operator) so the returned collection contains clones: replace adding
the original $department with adding clone $department to ensure modifications
to the returned list don't affect the source collection and stay consistent with
other methods.
There was a problem hiding this comment.
Actionable comments posted: 8
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (8)
zmsdb/src/Zmsdb/Closure.php (1)
14-35: Guard cache access before dereferencing.
App::$cacheis nullable. CallingApp::$cache->get()/->set()without first verifying the instance will trigger a fatal error whenever the cache isn’t initialized (e.g., CLI scripts, tests). Add an earlyif (App::$cache && !$disableCache) { ... }guard around both the read and write paths.zmsdb/src/Zmsdb/Source.php (1)
25-48: Add cache null-check and invalidate on writes.Two blockers here:
App::$cacheis nullable; calling->get()/->set()without checking will crash if the cache isn’t initialized (same scenario as in other repositories). Wrap both operations inif (App::$cache && !$disableCache) { … }.writeEntity()/writeDeleteBySource()never clear this cache key, so updates can return stale data straight from cache. Ensure youdelete()(or bypass caching) before re-reading.Please address both to keep the cache safe and coherent.
zmsdb/src/Zmsdb/Request.php (1)
15-40: Protect cache access and handle invalidation.
App::$cachemay benull; invoking->get()or->set()without checking will fatally error when caching hasn’t been bootstrapped. Please guard both calls. Also, mutations (writeEntity,writeDeleteEntity, etc.) don’t purge the cached request, so consumers can read stale entries. Make sure to delete the corresponding cache key whenever the request changes.zmsdb/src/Zmsdb/Department.php (1)
22-47: Guard cache usage and invalidate external cache.Like the other services,
App::$cachecan benull; the current code calls->get()/->set()without verifying it. Add a guard to avoid fatal errors when the cache isn’t initialised. In addition, write/update/delete paths reset onlyself::$departmentCache, leavingApp::$cachewith stale department instances. Make sure todelete()the PSR-16 cache entry (or skip cached reads) when mutating data.zmsdb/src/Zmsdb/Cluster.php (1)
357-368: Cache invalidation missing on write operations.The
writeEntityandupdateEntitymethods do not invalidate the cache before writing. This could lead to serving stale cached data after entities are modified. Consider adding cache invalidation at the start of these methods.For
writeEntity, apply this diff:public function writeEntity(\BO\Zmsentities\Cluster $entity) { + // Invalidate cache for this cluster (we don't know the ID yet, but after insert we should invalidate) $query = new Query\Cluster(Query\Base::INSERT); $values = $query->reverseEntityMapping($entity); $query->addValues($values); $this->writeItem($query); $lastInsertId = $this->getWriter()->lastInsertId(); if ($entity->toProperty()->scopes->isAvailable()) { $this->writeAssignedScopes($lastInsertId, $entity->scopes); } + // Note: readEntity will populate cache with fresh data return $this->readEntity($lastInsertId, 1); }For
updateEntity, apply this diff:public function updateEntity($clusterId, \BO\Zmsentities\Cluster $entity) { + // Invalidate cache for all resolve levels of this cluster + App::$cache->delete("cluster-$clusterId-0"); + App::$cache->delete("cluster-$clusterId-1"); + App::$cache->delete("cluster-$clusterId-2"); $query = new Query\Cluster(Query\Base::UPDATE); $query->addConditionClusterId($clusterId); $values = $query->reverseEntityMapping($entity); $query->addValues($values); $this->writeItem($query); if ($entity->toProperty()->scopes->isAvailable()) { $this->writeAssignedScopes($clusterId, $entity->scopes); } return $this->readEntity($clusterId, 1); }Alternatively, consider implementing a helper method to invalidate all cache entries for a given cluster across all resolution levels.
Also applies to: 378-389
zmsdb/src/Zmsdb/Provider.php (2)
22-33: Cache set should occur after validation.The cache is set on line 31 without checking if the provider entity is valid. If
fetchOnereturns an empty or null provider, this will cache invalid data.Apply this diff:
$this->testSource($source); $query = new Query\Provider(Query\Base::SELECT); $query ->setResolveLevel($resolveReferences) ->addEntityMapping() ->addResolvedReferences($resolveReferences) ->addConditionProviderSource($source) ->addConditionProviderId($providerId); $provider = $this->fetchOne($query, new Entity()); - App::$cache->set($cacheKey, $provider); + if ($provider && $provider->hasId()) { + App::$cache->set($cacheKey, $provider); + } return $provider; }
68-99: Cache invalidation missing on write and delete operations.The
writeEntityandwriteDeleteEntitymethods do not invalidate the cache. This will cause stale data to be served after modifications.For
writeEntity, apply this diff to invalidate cache for both the old and new provider:public function writeEntity(Entity $entity) { $this->writeDeleteEntity($entity->getId(), $entity->getSource()); + // Invalidate cache for all resolve levels + $source = $entity->getSource(); + $id = $entity->getId(); + App::$cache->delete("provider-$source-$id-0"); + App::$cache->delete("provider-$source-$id-1"); + App::$cache->delete("provider-$source-$id-2"); $contact = $entity->getContact(); // ... rest of method }For
writeDeleteEntity, apply this diff:public function writeDeleteEntity($providerId, $source) { + // Invalidate cache for all resolve levels + App::$cache->delete("provider-$source-$providerId-0"); + App::$cache->delete("provider-$source-$providerId-1"); + App::$cache->delete("provider-$source-$providerId-2"); $query = new Query\Provider(Query\Base::DELETE); $query->addConditionProviderId($providerId); $query->addConditionProviderSource($source); return $this->deleteItem($query); }Also applies to: 146-152
zmsdb/src/Zmsdb/Scope.php (1)
21-45: Inconsistent two-level cache implementation.This method uses both in-memory (
self::$cache) and persistent (App::$cache) caching, but only the in-memory cache is checked after the persistent cache miss (line 32). This creates an inconsistent caching hierarchy. More critically, when the cache is cleared in write operations (lines 411, 432, 487, 579), onlyself::$cacheis cleared butApp::$cacheis not invalidated, causing stale persistent cache to be served.Consider one of these approaches:
Option 1: Remove dual caching and use only App::$cache (recommended)
public function readEntity($scopeId, $resolveReferences = 0, $disableCache = false) { $cacheKey = "scope-$scopeId-$resolveReferences"; if (!$disableCache) { $data = App::$cache->get($cacheKey); - if (App::$cache && !empty($data)) { + if (!empty($data)) { return $data; } } - if ($disableCache || ! array_key_exists($cacheKey, self::$cache)) { - $query = new Query\Scope(Query\Base::SELECT); - $query->addEntityMapping() - ->addResolvedReferences($resolveReferences) - ->addConditionScopeId($scopeId); - $scope = $this->fetchOne($query, new Entity()); - if (! $scope->hasId()) { - return null; - } - self::$cache[$cacheKey] = $this->readResolvedReferences($scope, $resolveReferences); - App::$cache->set($cacheKey, self::$cache[$cacheKey]); - } - return self::$cache[$cacheKey]; + $query = new Query\Scope(Query\Base::SELECT); + $query->addEntityMapping() + ->addResolvedReferences($resolveReferences) + ->addConditionScopeId($scopeId); + $scope = $this->fetchOne($query, new Entity()); + if (! $scope->hasId()) { + return null; + } + $scope = $this->readResolvedReferences($scope, $resolveReferences); + App::$cache->set($cacheKey, $scope); + return $scope; }Option 2: Invalidate both caches on writes
In methods at lines 411, 432, 487, and 579, also clear
App::$cache:public function writeEntity(\BO\Zmsentities\Scope $entity, $parentId) { self::$cache = []; + // Invalidate persistent cache - note: we don't know the exact keys, so this is incomplete + // A better approach would be to use cache tags or a helper method $query = new Query\Scope(Query\Base::INSERT); // ... rest of method }Note: Option 2 is incomplete because we don't know all the cache keys to invalidate. Option 1 is cleaner and recommended.
♻️ Duplicate comments (1)
zmsapi/composer.json (1)
42-43: See prior note on symfony/cache version.The concern about locking to the unmaintained 6.0 branch applies here as well.
🧹 Nitpick comments (7)
zmsdb/src/Zmsdb/Cluster.php (1)
29-35: Simplify the cache existence check.The condition
if (App::$cache && !empty($data))is redundant. IfApp::$cacheis null, theget()call on line 31 would have already failed. Consider simplifying to just check if data is not empty.Apply this diff:
$cacheKey = "cluster-$itemId-$resolveReferences"; if (!$disableCache) { $data = App::$cache->get($cacheKey); - if (App::$cache && !empty($data)) { + if (!empty($data)) { return $data; } }zmsdb/src/Zmsdb/Provider.php (1)
15-20: Simplify the cache existence check.Similar to other files, the condition
if (App::$cache && !empty($data))is redundant. IfApp::$cacheis null, theget()call would fail.Apply this diff:
if (!$disableCache) { $data = App::$cache->get($cacheKey); - if (App::$cache && !empty($data)) { + if (!empty($data)) { return $data; } }zmsdb/src/Zmsdb/Config.php (1)
17-22: Simplify the cache existence check.The condition
if (App::$cache && !empty($data))is redundant across the codebase.Apply this diff:
$cacheKey = "config"; if (!$disableCache) { $data = App::$cache->get($cacheKey); - if (App::$cache && !empty($data)) { + if (!empty($data)) { return $data; } }zmsdb/src/Zmsdb/Scope.php (1)
97-102: Simplify the cache existence check.Same redundant pattern as other files.
Apply this diff:
if (!$disableCache) { $data = App::$cache->get($cacheKey); - if (App::$cache && !empty($data)) { + if (!empty($data)) { return $data; } }zmsdb/src/Zmsdb/Application.php (3)
71-77: Consider externalizing cache configuration.The cache directory defaults to
__DIR__ . '/cache'(inside the source directory) and TTL defaults to 3600 seconds. For production deployments, the cache should be in a dedicated location (e.g.,/var/cache/zmsdb).While environment variables are supported, consider documenting these requirements prominently or providing a configuration file. Also consider making the defaults more production-ready:
private static function initializeCache(): void { - self::$CACHE_DIR = getenv('CACHE_DIR') ?: __DIR__ . '/cache'; + self::$CACHE_DIR = getenv('CACHE_DIR') ?: '/var/cache/zmsdb'; self::$SOURCE_CACHE_TTL = (int) (getenv('SOURCE_CACHE_TTL') ?: 3600); self::validateCacheDirectory(); self::setupCache(); }Also, document these environment variables in your deployment documentation or README.
79-88: Add error handling for cache directory validation.The directory creation and permission checks should have more robust error handling, especially for production environments. Consider logging the error before throwing the exception.
As per coding guidelines
private static function validateCacheDirectory(): void { if (!is_dir(self::$CACHE_DIR) && !mkdir(self::$CACHE_DIR, 0750, true)) { + $error = error_get_last(); + // If logging is available at this point, log the error throw new \RuntimeException(sprintf('Cache directory "%s" could not be created', self::$CACHE_DIR)); } if (!is_writable(self::$CACHE_DIR)) { + // If logging is available at this point, log the error throw new \RuntimeException(sprintf('Cache directory "%s" is not writable', self::$CACHE_DIR)); } }Note: Proper Monolog logging should be added here if the logger is initialized before cache initialization.
71-99: Code duplication across Application classes.The cache initialization logic (initializeCache, validateCacheDirectory, setupCache) is duplicated across multiple Application classes (zmsdb, zmsapi, zmscitizenapi as seen in relevant snippets). This violates the DRY principle.
Consider extracting this logic into a shared trait or abstract base class:
trait CacheInitializationTrait { public static ?CacheInterface $cache = null; public static string $CACHE_DIR; public static int $SOURCE_CACHE_TTL; private static function initializeCache(string $namespace): void { self::$CACHE_DIR = getenv('CACHE_DIR') ?: '/var/cache/' . $namespace; self::$SOURCE_CACHE_TTL = (int) (getenv('SOURCE_CACHE_TTL') ?: 3600); self::validateCacheDirectory(); self::setupCache($namespace); } private static function validateCacheDirectory(): void { if (!is_dir(self::$CACHE_DIR) && !mkdir(self::$CACHE_DIR, 0750, true)) { throw new \RuntimeException(sprintf('Cache directory "%s" could not be created', self::$CACHE_DIR)); } if (!is_writable(self::$CACHE_DIR)) { throw new \RuntimeException(sprintf('Cache directory "%s" is not writable', self::$CACHE_DIR)); } } private static function setupCache(string $namespace): void { $psr6 = new FilesystemAdapter(namespace: $namespace, defaultLifetime: self::$SOURCE_CACHE_TTL, directory: self::$CACHE_DIR); self::$cache = new Psr16Cache($psr6); } }Then use it in Application classes:
class Application extends \BO\Slim\Application { + use CacheInitializationTrait; + const IDENTIFIER = 'zms'; const MODULE_NAME = 'zmsdb'; // ... other constants - public static ?CacheInterface $cache = null; - public static string $CACHE_DIR; - public static int $SOURCE_CACHE_TTL; - - private static function initializeCache(): void - { - self::$CACHE_DIR = getenv('CACHE_DIR') ?: __DIR__ . '/cache'; - self::$SOURCE_CACHE_TTL = (int) (getenv('SOURCE_CACHE_TTL') ?: 3600); - self::validateCacheDirectory(); - self::setupCache(); - } - - private static function validateCacheDirectory(): void - { - if (!is_dir(self::$CACHE_DIR) && !mkdir(self::$CACHE_DIR, 0750, true)) { - throw new \RuntimeException(sprintf('Cache directory "%s" could not be created', self::$CACHE_DIR)); - } - - if (!is_writable(self::$CACHE_DIR)) { - throw new \RuntimeException(sprintf('Cache directory "%s" is not writable', self::$CACHE_DIR)); - } - } - - private static function setupCache(): void - { - $psr6 = new FilesystemAdapter(namespace: '', defaultLifetime: self::$SOURCE_CACHE_TTL, directory: self::$CACHE_DIR); - self::$cache = new Psr16Cache($psr6); - } public static function initialize(): void { - self::initializeCache(); + self::initializeCache('zmsdb'); } }
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (2)
zmsapi/composer.lockis excluded by!**/*.lockzmsdb/composer.lockis excluded by!**/*.lock
📒 Files selected for processing (19)
zmsapi/composer.json(1 hunks)zmsapi/src/Zmsapi/Application.php(3 hunks)zmsapi/src/Zmsapi/UseraccountSearchByDepartment.php(1 hunks)zmsdb/composer.json(1 hunks)zmsdb/src/Zmsdb/Application.php(3 hunks)zmsdb/src/Zmsdb/Calldisplay.php(1 hunks)zmsdb/src/Zmsdb/Closure.php(3 hunks)zmsdb/src/Zmsdb/Cluster.php(3 hunks)zmsdb/src/Zmsdb/Config.php(2 hunks)zmsdb/src/Zmsdb/DayOff.php(3 hunks)zmsdb/src/Zmsdb/Department.php(3 hunks)zmsdb/src/Zmsdb/Provider.php(2 hunks)zmsdb/src/Zmsdb/Request.php(3 hunks)zmsdb/src/Zmsdb/Scope.php(5 hunks)zmsdb/src/Zmsdb/Source.php(3 hunks)zmsdb/src/Zmsdb/Useraccount.php(1 hunks)zmsentities/src/Zmsentities/Collection/DepartmentList.php(1 hunks)zmsticketprinter/templates/block/content/print.twig(1 hunks)zmsticketprinter/templates/page/process.twig(2 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
**/*
⚙️ CodeRabbit configuration file
**/*: Apply the following Clean Code guidelines to all files, as summarized by wojteklu's Clean Code gist:Code is clean if it can be understood easily – by everyone on the team. Clean code can be read and enhanced by a developer other than its original author. With understandability comes readability, changeability, extensibility and maintainability.
General rules
- Follow standard conventions.
- Keep it simple stupid. Simpler is always better. Reduce complexity as much as possible.
- Boy scout rule. Leave the campground cleaner than you found it.
- Always find root cause. Always look for the root cause of a problem.
Design rules
- Keep configurable data at high levels.
- Prefer polymorphism to if/else or switch/case.
- Separate multi-threading code.
- Prevent over-configurability.
- Use dependency injection.
- Follow Law of Demeter. A class should know only its direct dependencies.
Understandability tips
- Be consistent. If you do something a certain way, do all similar things in the same way.
- Use explanatory variables.
- Encapsulate boundary conditions. Boundary conditions are hard to keep track of. Put the processing for them in one place.
- Prefer dedicated value objects to primitive type.
- Avoid logical dependency. Don't write methods which works correctly depending on something else in the same class.
- Avoid negative conditionals.
Names rules
- Choose descriptive and unambiguous names.
- Make meaningful distinction.
- Use pronounceable names.
- Use searchable names.
- Replace magic numbers with named constants.
- Avoid encodings. Don't append prefixes or type information.
Functions rules
- Small.
- Do one thing.
- Use descriptive names.
- Prefer fewer arguments.
- Have no side effects.
- Don't use flag arguments. Split method into several independent methods that can b...
Files:
zmsticketprinter/templates/block/content/print.twigzmsticketprinter/templates/page/process.twigzmsdb/composer.jsonzmsentities/src/Zmsentities/Collection/DepartmentList.phpzmsapi/src/Zmsapi/UseraccountSearchByDepartment.phpzmsdb/src/Zmsdb/Request.phpzmsdb/src/Zmsdb/Scope.phpzmsdb/src/Zmsdb/DayOff.phpzmsdb/src/Zmsdb/Department.phpzmsdb/src/Zmsdb/Application.phpzmsapi/src/Zmsapi/Application.phpzmsdb/src/Zmsdb/Config.phpzmsdb/src/Zmsdb/Source.phpzmsdb/src/Zmsdb/Cluster.phpzmsapi/composer.jsonzmsdb/src/Zmsdb/Useraccount.phpzmsdb/src/Zmsdb/Calldisplay.phpzmsdb/src/Zmsdb/Closure.phpzmsdb/src/Zmsdb/Provider.php
**/*.php
⚙️ CodeRabbit configuration file
**/*.php: Flag any usage of error_log() as it should be replaced with proper Monolog logging mechanisms provided by zmsslim which should be setup in the App/Application of each module:
- For error handling: Use the proper Monolog logging framework with error levels
- For application info logs: Use the proper Monolog logging framework with info levels
- For debugging: Use a dedicated debug logger or remove debug statements
- For CLI output: Use a CLI output handler or symfony/console
- The application log levels are as follows DEBUG, INFO, NOTICE, WARNING, ERROR, CRITICAL, ALERT, and EMERGENCY
Example replacement:
// Instead of: error_log("Import failed - " . $e->getMessage()); // Use: $log->error("Import failed", ['error' => $e->getMessage()]);Flag specific logging violations:
- error_log(), var_dump(), print_r(), die(), exit() usage (except proper error logging in catch blocks)
- Any debug output that should use proper Monolog logging
- Debug constants like DEBUG = true
- Debug logging that should be removed in production
- Commented debug code that should be cleaned up
Example replacements:
// Instead of: error_log("Error occurred"); var_dump($data); die('debug point'); // Use: $log->error("Error occurred", ['context' => 'processing']); $log->debug('Data dump', ['data' => $data]); // Remove die() statements entirelyException handling should use proper logging:
// Instead of: try { $result = $this->process(); } catch (Exception $e) { error_log("Processing failed: " . $e->getMessage()); } // Use: try { $result = $this->process(); } catch (Exception $e) { $log->error("Processing failed", ['error' => $e->getMessage(), 'trace' => $e->getTraceAsString()]); } ```...
Files:
zmsentities/src/Zmsentities/Collection/DepartmentList.phpzmsapi/src/Zmsapi/UseraccountSearchByDepartment.phpzmsdb/src/Zmsdb/Request.phpzmsdb/src/Zmsdb/Scope.phpzmsdb/src/Zmsdb/DayOff.phpzmsdb/src/Zmsdb/Department.phpzmsdb/src/Zmsdb/Application.phpzmsapi/src/Zmsapi/Application.phpzmsdb/src/Zmsdb/Config.phpzmsdb/src/Zmsdb/Source.phpzmsdb/src/Zmsdb/Cluster.phpzmsdb/src/Zmsdb/Useraccount.phpzmsdb/src/Zmsdb/Calldisplay.phpzmsdb/src/Zmsdb/Closure.phpzmsdb/src/Zmsdb/Provider.php
🧬 Code graph analysis (14)
zmsentities/src/Zmsentities/Collection/DepartmentList.php (1)
zmsentities/src/Zmsentities/Collection/Base.php (1)
addEntity(109-113)
zmsapi/src/Zmsapi/UseraccountSearchByDepartment.php (1)
zmsdb/src/Zmsdb/Useraccount.php (1)
readSearchByDepartmentId(276-296)
zmsdb/src/Zmsdb/Request.php (7)
zmsdb/src/Zmsdb/Application.php (1)
Application(19-100)zmsdb/src/Zmsdb/Cluster.php (1)
readEntity(27-51)zmsdb/src/Zmsdb/Config.php (1)
readEntity(14-30)zmsdb/src/Zmsdb/Department.php (1)
readEntity(22-49)zmsdb/src/Zmsdb/Provider.php (1)
readEntity(11-33)zmsdb/src/Zmsdb/Scope.php (1)
readEntity(21-45)zmsdb/src/Zmsdb/Source.php (1)
readEntity(25-49)
zmsdb/src/Zmsdb/Scope.php (1)
zmsdb/src/Zmsdb/Application.php (1)
Application(19-100)
zmsdb/src/Zmsdb/DayOff.php (1)
zmsdb/src/Zmsdb/Application.php (1)
Application(19-100)
zmsdb/src/Zmsdb/Department.php (1)
zmsdb/src/Zmsdb/Application.php (1)
Application(19-100)
zmsdb/src/Zmsdb/Application.php (2)
zmsapi/src/Zmsapi/Application.php (5)
initializeCache(124-130)validateCacheDirectory(132-141)setupCache(143-147)initialize(149-152)Application(25-153)zmscitizenapi/src/Zmscitizenapi/Application.php (5)
initializeCache(108-114)validateCacheDirectory(146-155)setupCache(157-161)initialize(63-70)Application(16-208)
zmsapi/src/Zmsapi/Application.php (2)
zmsdb/src/Zmsdb/Application.php (5)
initializeCache(71-77)validateCacheDirectory(79-88)setupCache(90-94)initialize(96-99)Application(19-100)zmscitizenapi/src/Zmscitizenapi/Application.php (5)
initializeCache(108-114)validateCacheDirectory(146-155)setupCache(157-161)initialize(63-70)Application(16-208)
zmsdb/src/Zmsdb/Config.php (2)
zmsdb/src/Zmsdb/Application.php (1)
Application(19-100)zmsdb/src/Zmsdb/Cluster.php (1)
readEntity(27-51)
zmsdb/src/Zmsdb/Source.php (7)
zmsdb/src/Zmsdb/Application.php (1)
Application(19-100)zmsdb/src/Zmsdb/Cluster.php (1)
readEntity(27-51)zmsdb/src/Zmsdb/Config.php (1)
readEntity(14-30)zmsdb/src/Zmsdb/Department.php (1)
readEntity(22-49)zmsdb/src/Zmsdb/Provider.php (1)
readEntity(11-33)zmsdb/src/Zmsdb/Request.php (1)
readEntity(15-42)zmsdb/src/Zmsdb/Scope.php (1)
readEntity(21-45)
zmsdb/src/Zmsdb/Cluster.php (4)
zmsdb/src/Zmsdb/Application.php (1)
Application(19-100)zmsdb/src/Zmsdb/Config.php (1)
readEntity(14-30)zmsdb/src/Zmsdb/Department.php (2)
readEntity(22-49)readResolvedReferences(51-62)zmsdb/src/Zmsdb/Source.php (2)
readEntity(25-49)readResolvedReferences(79-94)
zmsdb/src/Zmsdb/Useraccount.php (2)
zmsentities/src/Zmsentities/Schema/Entity.php (1)
toProperty(264-267)zmsentities/src/Zmsentities/Collection/Base.php (1)
addEntity(109-113)
zmsdb/src/Zmsdb/Closure.php (1)
zmsdb/src/Zmsdb/Application.php (1)
Application(19-100)
zmsdb/src/Zmsdb/Provider.php (4)
zmsdb/src/Zmsdb/Application.php (1)
Application(19-100)zmsdb/src/Zmsdb/Cluster.php (1)
readEntity(27-51)zmsdb/src/Zmsdb/Request.php (1)
readEntity(15-42)zmsdb/src/Zmsdb/Source.php (1)
readEntity(25-49)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (13)
- GitHub Check: call-php-code-quality / module-code-quality (zmsticketprinter, 8.3)
- GitHub Check: call-php-code-quality / module-code-quality (zmsslim, 8.3)
- GitHub Check: call-php-code-quality / module-code-quality (zmsstatistic, 8.3)
- GitHub Check: call-php-code-quality / module-code-quality (zmsdb, 8.3)
- GitHub Check: call-php-code-quality / module-code-quality (zmsdldb, 8.3)
- GitHub Check: call-php-code-quality / module-code-quality (zmsentities, 8.3)
- GitHub Check: call-php-code-quality / module-code-quality (zmsclient, 8.3)
- GitHub Check: call-php-unit-tests / module-test (zmscitizenapi, 8.3)
- GitHub Check: call-php-code-quality / module-code-quality (zmscitizenapi, 8.3)
- GitHub Check: call-php-code-quality / module-code-quality (zmscalldisplay, 8.3)
- GitHub Check: call-php-code-quality / module-code-quality (zmsadmin, 8.3)
- GitHub Check: call-php-unit-tests / zmsapi-test
- GitHub Check: call-php-unit-tests / zmsdb-test
🔇 Additional comments (5)
zmsticketprinter/templates/page/process.twig (1)
22-22: Verify queue number format and downstream handling
- Notification handler now uses
waitingNumber(validated as numeric; correctly wired).- Confirm
process.queue.numbermatches the previousprocess.displayNumberformat/type and update any validation or logic still relying ondisplayNumber.zmsticketprinter/templates/block/content/print.twig (1)
4-4: Confirmprocess.queue.numberis provided and non-null
Ensure the waitingnumber API response includes a validqueue.numberfield and that Twig renders it correctly, adding a fallback if it can be null.zmsdb/src/Zmsdb/Config.php (1)
32-61: LGTM! Cache invalidation is properly implemented.The cache invalidation on line 34 correctly clears the cached config before updating, and the method returns freshly cached data from
readEntity(). This ensures cache consistency.zmsapi/src/Zmsapi/UseraccountSearchByDepartment.php (1)
36-36: LGTM! Cleaner data resolution approach.Moving the data resolution control from post-processing (
->withLessData()) to a method parameter ($resolveReferences) improves separation of concerns by handling data shaping within the data access layer.zmsdb/src/Zmsdb/DayOff.php (1)
5-5: LGTM!The import alias is necessary for accessing the cache infrastructure introduced in this PR.
| "symfony/cache": "^6.0", | ||
| "psr/simple-cache": "^3.0", |
There was a problem hiding this comment.
Upgrade symfony/cache to a maintained branch.
symfony/cache constrained to ^6.0 pulls an EOL release line (ended support Jan 2023). Please target a supported branch (e.g., 6.4 LTS or 7.x) so we keep receiving security patches. Based on learnings
🤖 Prompt for AI Agents
In zmsdb/composer.json around lines 37 to 38, the symfony/cache dependency is
pinned to the EOL ^6.0 branch; update the constraint to a maintained release
(for example "^6.4" for the current LTS or "^7.0" if you prefer the next major),
then run composer update symfony/cache (or composer update) to refresh
composer.lock and verify the updated versions; run your test suite and fix any
deprecations or breaking changes introduced by the newer symfony/cache release.
| use Psr\SimpleCache\CacheInterface; | ||
| use Symfony\Component\Cache\Adapter\FilesystemAdapter; | ||
| use Symfony\Component\Cache\Psr16Cache; |
There was a problem hiding this comment.
🧩 Analysis chain
Verify Symfony Cache version compatibility.
The PR uses symfony/cache ^6.0 according to the learnings. Note that Symfony 6.0 reached end-of-life in January 2023 and no longer receives security updates.
Based on learnings
Also applies to: 32-35
🌐 Web query:
What is the latest stable version of Symfony Cache component, and what are the security implications of using symfony/cache 6.0?
💡 Result:
-
Latest stable symfony/cache: v7.3.4 (current Symfony stable 7.3 series). [1][2]
-
Security implications of using symfony/cache 6.0:
- Symfony 6.0 is end-of-life and no longer receives security fixes (active/security support ended Jan 31, 2023). Running 6.0 means you will not get patched fixes for newly discovered vulnerabilities. [3][4]
- Unpatched cache-related issues can expose sensitive data (e.g., cache poisoning or caching responses containing session/cookie data) and have led to real CVE-class issues in Symfony-based stacks. Using an EOL cache component therefore increases risk of cache poisoning, information disclosure, and related attacks. [5][6]
- Recommendation: upgrade to a maintained release (e.g., Symfony 7.3.x or the 6.4 LTS) to receive security fixes, or backport/patch and isolate caching layers until you can upgrade. [2][3]
Sources:
- Packagist — symfony/cache (v7.3.4).
- Symfony releases / “7.3 maintained” (latest patch 7.3.4).
- endoflife.date / eol.support — Symfony 6.0 EOL and last patch dates (security support ended Jan 31, 2023).
- Ubuntu security notice summarizing Symfony vulnerabilities.
- CVE examples and writeups showing cache/session-related issues (e.g., CVE-2024-27917).
Upgrade symfony/cache to a maintained release
symfony/cache ^6.0 is end-of-life since Jan 31, 2023 and no longer receives security fixes—risking cache poisoning and data exposure. Bump the dependency to ^6.4 (LTS) or ^7.3.x (v7.3.4+) to restore security support.
| private static function setupCache(): void | ||
| { | ||
| $psr6 = new FilesystemAdapter(namespace: '', defaultLifetime: self::$SOURCE_CACHE_TTL, directory: self::$CACHE_DIR); | ||
| self::$cache = new Psr16Cache($psr6); | ||
| } |
There was a problem hiding this comment.
Empty cache namespace creates collision risk.
The FilesystemAdapter is initialized with an empty namespace (namespace: ''). This means all modules using this cache (zmsdb, zmsapi, etc.) share the same cache namespace. If cache keys are not carefully prefixed (e.g., "cluster-", "scope-"), there's a risk of key collisions between modules.
Based on learnings
Apply this diff to namespace the cache by module:
private static function setupCache(): void
{
- $psr6 = new FilesystemAdapter(namespace: '', defaultLifetime: self::$SOURCE_CACHE_TTL, directory: self::$CACHE_DIR);
+ $psr6 = new FilesystemAdapter(namespace: 'zmsdb', defaultLifetime: self::$SOURCE_CACHE_TTL, directory: self::$CACHE_DIR);
self::$cache = new Psr16Cache($psr6);
}Ensure other modules (zmsapi, zmscitizenapi) also use module-specific namespaces to avoid collisions.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| private static function setupCache(): void | |
| { | |
| $psr6 = new FilesystemAdapter(namespace: '', defaultLifetime: self::$SOURCE_CACHE_TTL, directory: self::$CACHE_DIR); | |
| self::$cache = new Psr16Cache($psr6); | |
| } | |
| private static function setupCache(): void | |
| { | |
| $psr6 = new FilesystemAdapter(namespace: 'zmsdb', defaultLifetime: self::$SOURCE_CACHE_TTL, directory: self::$CACHE_DIR); | |
| self::$cache = new Psr16Cache($psr6); | |
| } |
🤖 Prompt for AI Agents
In zmsdb/src/Zmsdb/Application.php around lines 90-94, the FilesystemAdapter is
created with an empty namespace which risks cache key collisions across modules;
change the namespace argument to a module-specific value (for example "zmsdb" or
a constant like self::CACHE_NAMESPACE) so the adapter is initialized with
namespace: 'zmsdb' (or equivalent), keep defaultLifetime and directory as-is,
and ensure other modules (zmsapi, zmscitizenapi) are similarly updated to use
their own module-specific namespaces.
| if ($department->hasAccess($useraccount)) { | ||
| if ($useraccount->rights['organisation']) { | ||
| return clone $this; | ||
| return $this; |
There was a problem hiding this comment.
Returning $this breaks immutability pattern.
Returning the original instance instead of a new filtered list breaks the immutability pattern established by the with* method naming convention and other methods in this class (withOutClusterDuplicates, withMatchingScopes). This could lead to unintended mutations of the original list and potentially corrupt cached entities in the caching infrastructure introduced by this PR.
If you need to return all departments for organisation rights, consider:
- return $this;
+ return clone $this;Or, if the entire list should be returned without filtering (but maintaining the pattern):
public function withAccess(\BO\Zmsentities\Useraccount $useraccount)
{
+ if ($useraccount->rights['organisation']) {
+ return $this;
+ }
$list = new static();
foreach ($this as $department) {
if ($department->hasAccess($useraccount)) {
- if ($useraccount->rights['organisation']) {
- return $this;
- }
$list->addEntity($department);
}
}
return $list;
}🤖 Prompt for AI Agents
In zmsentities/src/Zmsentities/Collection/DepartmentList.php around line 61, the
method currently returns $this which breaks the immutability contract of the
with* methods; instead construct and return a new DepartmentList instance (or
clone populated via the existing factory/constructor) containing the appropriate
items (either the full list or the filtered subset) so the original list remains
unchanged and cached entity references are not mutated; ensure you copy any
internal metadata (scopes, clusters, etc.) into the new instance so behavior
remains identical except for the modified items.
| return $this; | ||
| } | ||
| $list->addEntity(clone $department); | ||
| $list->addEntity($department); |
There was a problem hiding this comment.
Inconsistent entity handling - consider cloning for immutability.
Adding the department directly without cloning is inconsistent with other methods in this class. Both withOutClusterDuplicates() (line 15) and withMatchingScopes() (line 73) clone entities before adding them to filtered lists. Not cloning risks mutations propagating back to cached entities.
For consistency with the class's immutability pattern, clone the department:
- $list->addEntity($department);
+ $list->addEntity(clone $department);🤖 Prompt for AI Agents
In zmsentities/src/Zmsentities/Collection/DepartmentList.php around line 63, the
code adds $department directly to the filtered list which is inconsistent with
other methods that clone entities first; change the code to clone the
$department (e.g., $departmentClone = clone $department) and add the clone to
the list so mutations on the filtered result cannot affect the original/cached
entity, keeping behavior consistent with withOutClusterDuplicates() and
withMatchingScopes().
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (4)
zmsapi/src/Zmsapi/Application.php (4)
34-37: Consider dependency injection for cache in future refactoring.While the static properties align with the existing class design, they break the Dependency Inversion Principle and make unit testing more difficult. For future improvements, consider using instance-based dependency injection to decouple the cache from global state. As per coding guidelines.
132-141: Consider adding logging for cache directory operations.While the error handling is correct, adding logging for directory creation and validation failures would improve observability. As per coding guidelines.
Example enhancement:
private static function validateCacheDirectory(): void { if (!is_dir(self::$CACHE_DIR)) { if (!mkdir(self::$CACHE_DIR, 0750, true)) { // Log error if logger is available throw new \RuntimeException(sprintf('Cache directory "%s" could not be created', self::$CACHE_DIR)); } // Log: Cache directory created at self::$CACHE_DIR } if (!is_writable(self::$CACHE_DIR)) { // Log error if logger is available throw new \RuntimeException(sprintf('Cache directory "%s" is not writable', self::$CACHE_DIR)); } }
143-147: Consider using an app-specific namespace for cache keys.The empty namespace string (
'') could lead to key collisions if multiple applications share the same cache directory. Using a namespace like'zmsapi'would provide better isolation. Based on learnings.Apply this diff to add a namespace:
private static function setupCache(): void { - $psr6 = new FilesystemAdapter(namespace: '', defaultLifetime: self::$SOURCE_CACHE_TTL, directory: self::$CACHE_DIR); + $psr6 = new FilesystemAdapter(namespace: 'zmsapi', defaultLifetime: self::$SOURCE_CACHE_TTL, directory: self::$CACHE_DIR); self::$cache = new Psr16Cache($psr6); }
155-155: Consider lazy initialization instead of eager initialization.Calling
initialize()at file load time introduces side effects during class loading. While this aligns with the existing pattern in other modules, consider lazy initialization where the cache is set up only when first accessed. This would improve testability and reduce coupling between file loading and initialization.Example lazy initialization pattern:
public static function getCache(): CacheInterface { if (self::$cache === null) { self::initialize(); } return self::$cache; }Then remove the
Application::initialize();call at the end of the file and access the cache viaApplication::getCache()instead ofApplication::$cache.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
zmsapi/src/Zmsapi/Application.php(3 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
**/*
⚙️ CodeRabbit configuration file
**/*: Apply the following Clean Code guidelines to all files, as summarized by wojteklu's Clean Code gist:Code is clean if it can be understood easily – by everyone on the team. Clean code can be read and enhanced by a developer other than its original author. With understandability comes readability, changeability, extensibility and maintainability.
General rules
- Follow standard conventions.
- Keep it simple stupid. Simpler is always better. Reduce complexity as much as possible.
- Boy scout rule. Leave the campground cleaner than you found it.
- Always find root cause. Always look for the root cause of a problem.
Design rules
- Keep configurable data at high levels.
- Prefer polymorphism to if/else or switch/case.
- Separate multi-threading code.
- Prevent over-configurability.
- Use dependency injection.
- Follow Law of Demeter. A class should know only its direct dependencies.
Understandability tips
- Be consistent. If you do something a certain way, do all similar things in the same way.
- Use explanatory variables.
- Encapsulate boundary conditions. Boundary conditions are hard to keep track of. Put the processing for them in one place.
- Prefer dedicated value objects to primitive type.
- Avoid logical dependency. Don't write methods which works correctly depending on something else in the same class.
- Avoid negative conditionals.
Names rules
- Choose descriptive and unambiguous names.
- Make meaningful distinction.
- Use pronounceable names.
- Use searchable names.
- Replace magic numbers with named constants.
- Avoid encodings. Don't append prefixes or type information.
Functions rules
- Small.
- Do one thing.
- Use descriptive names.
- Prefer fewer arguments.
- Have no side effects.
- Don't use flag arguments. Split method into several independent methods that can b...
Files:
zmsapi/src/Zmsapi/Application.php
**/*.php
⚙️ CodeRabbit configuration file
**/*.php: Flag any usage of error_log() as it should be replaced with proper Monolog logging mechanisms provided by zmsslim which should be setup in the App/Application of each module:
- For error handling: Use the proper Monolog logging framework with error levels
- For application info logs: Use the proper Monolog logging framework with info levels
- For debugging: Use a dedicated debug logger or remove debug statements
- For CLI output: Use a CLI output handler or symfony/console
- The application log levels are as follows DEBUG, INFO, NOTICE, WARNING, ERROR, CRITICAL, ALERT, and EMERGENCY
Example replacement:
// Instead of: error_log("Import failed - " . $e->getMessage()); // Use: $log->error("Import failed", ['error' => $e->getMessage()]);Flag specific logging violations:
- error_log(), var_dump(), print_r(), die(), exit() usage (except proper error logging in catch blocks)
- Any debug output that should use proper Monolog logging
- Debug constants like DEBUG = true
- Debug logging that should be removed in production
- Commented debug code that should be cleaned up
Example replacements:
// Instead of: error_log("Error occurred"); var_dump($data); die('debug point'); // Use: $log->error("Error occurred", ['context' => 'processing']); $log->debug('Data dump', ['data' => $data]); // Remove die() statements entirelyException handling should use proper logging:
// Instead of: try { $result = $this->process(); } catch (Exception $e) { error_log("Processing failed: " . $e->getMessage()); } // Use: try { $result = $this->process(); } catch (Exception $e) { $log->error("Processing failed", ['error' => $e->getMessage(), 'trace' => $e->getTraceAsString()]); } ```...
Files:
zmsapi/src/Zmsapi/Application.php
🧬 Code graph analysis (1)
zmsapi/src/Zmsapi/Application.php (2)
zmsdb/src/Zmsdb/Application.php (5)
initializeCache(71-77)validateCacheDirectory(79-88)setupCache(90-94)initialize(96-99)Application(19-100)zmscitizenapi/src/Zmscitizenapi/Application.php (5)
initializeCache(108-114)validateCacheDirectory(146-155)setupCache(157-161)initialize(63-70)Application(16-208)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: call-php-unit-tests / zmsdb-test
🔇 Additional comments (2)
zmsapi/src/Zmsapi/Application.php (2)
10-12: LGTM! Correct PSR-16 cache imports.The imports align with the symfony/cache and psr/simple-cache dependencies added in composer.json.
149-152: LGTM! Clean initialization method.The initialize method provides a clear entry point for cache setup and can be extended in the future if additional initialization steps are needed.
| private static function initializeCache(): void | ||
| { | ||
| self::$CACHE_DIR = getenv('CACHE_DIR') ?: __DIR__ . '/cache'; | ||
| self::$SOURCE_CACHE_TTL = (int) (getenv('SOURCE_CACHE_TTL') ?: 3600); | ||
| self::validateCacheDirectory(); | ||
| self::setupCache(); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Eliminate code duplication across Application classes.
The cache initialization logic is duplicated across zmsapi, zmsdb, and zmscitizenapi Application classes. This violates the DRY principle and increases maintenance burden. As per coding guidelines.
Consider extracting the common cache setup logic into a shared trait or base class helper:
Create a new trait:
trait CacheInitializationTrait
{
public static ?CacheInterface $cache = null;
public static string $CACHE_DIR;
public static int $SOURCE_CACHE_TTL;
private static function initializeCache(): void
{
self::$CACHE_DIR = getenv('CACHE_DIR') ?: __DIR__ . '/cache';
self::$SOURCE_CACHE_TTL = (int) (getenv('SOURCE_CACHE_TTL') ?: 3600);
if (self::$SOURCE_CACHE_TTL <= 0) {
throw new \RuntimeException(sprintf('SOURCE_CACHE_TTL must be positive, got: %d', self::$SOURCE_CACHE_TTL));
}
self::validateCacheDirectory();
self::setupCache();
}
private static function validateCacheDirectory(): void
{
if (!is_dir(self::$CACHE_DIR) && !mkdir(self::$CACHE_DIR, 0750, true)) {
throw new \RuntimeException(sprintf('Cache directory "%s" could not be created', self::$CACHE_DIR));
}
if (!is_writable(self::$CACHE_DIR)) {
throw new \RuntimeException(sprintf('Cache directory "%s" is not writable', self::$CACHE_DIR));
}
}
private static function setupCache(): void
{
$psr6 = new FilesystemAdapter(namespace: '', defaultLifetime: self::$SOURCE_CACHE_TTL, directory: self::$CACHE_DIR);
self::$cache = new Psr16Cache($psr6);
}
}Then use the trait in each Application class:
class Application extends \BO\Slim\Application
{
use CacheInitializationTrait;
// ... rest of class
}🤖 Prompt for AI Agents
In zmsapi/src/Zmsapi/Application.php around lines 124-130 the cache
initialization logic is duplicated across multiple Application classes; extract
the shared logic into a new reusable trait (e.g., CacheInitializationTrait) that
declares the cache properties and implements initializeCache(),
validateCacheDirectory(), and setupCache() (including the getenv fallbacks,
positive TTL validation, directory creation/writability checks, and PSR-16/PSR-6
adapter setup), then remove the duplicated methods from each Application class
and adopt the trait via "use CacheInitializationTrait" so all apps share a
single implementation.
Validate SOURCE_CACHE_TTL to prevent invalid values.
The TTL cast to int could result in zero or negative values (e.g., if the environment variable is an invalid string), which may cause unexpected cache behavior.
Apply this diff to add validation:
private static function initializeCache(): void
{
self::$CACHE_DIR = getenv('CACHE_DIR') ?: __DIR__ . '/cache';
self::$SOURCE_CACHE_TTL = (int) (getenv('SOURCE_CACHE_TTL') ?: 3600);
+ if (self::$SOURCE_CACHE_TTL <= 0) {
+ throw new \RuntimeException(sprintf('SOURCE_CACHE_TTL must be positive, got: %d', self::$SOURCE_CACHE_TTL));
+ }
self::validateCacheDirectory();
self::setupCache();
}🤖 Prompt for AI Agents
In zmsapi/src/Zmsapi/Application.php around lines 124 to 130, the cast of
SOURCE_CACHE_TTL to int can yield zero or negative values for invalid env input;
change the initialization to read the env value, validate it is numeric and
greater than zero, and only then set self::$SOURCE_CACHE_TTL to that value,
otherwise fall back to the default 3600 (optionally log a warning). Ensure
validation uses strict numeric check (is_numeric) and an explicit > 0 check so
invalid strings or negative numbers don't become the TTL.
There was a problem hiding this comment.
Actionable comments posted: 8
♻️ Duplicate comments (2)
zmsdb/src/Zmsdb/Scope.php (2)
25-30: Fix cache access (null-guard + proper hit check).Guard App::$cache and use has(); also guard set():
- if (!$disableCache) { - $data = App::$cache->get($cacheKey); - if (App::$cache && !empty($data)) { - return $data; - } - } + if (!$disableCache && App::$cache && App::$cache->has($cacheKey)) { + return App::$cache->get($cacheKey); + } @@ - App::$cache->set($cacheKey, self::$cache[$cacheKey]); + if (App::$cache) { + App::$cache->set($cacheKey, self::$cache[$cacheKey]); + }Also applies to: 42-42
97-103: Same cache issue in readByProviderId.Apply the same guard/has()/set() pattern here:
- if (!$disableCache) { - $data = App::$cache->get($cacheKey); - if (App::$cache && !empty($data)) { - return $data; - } - } + if (!$disableCache && App::$cache && App::$cache->has($cacheKey)) { + return App::$cache->get($cacheKey); + } @@ - App::$cache->set($cacheKey, $scopeList); + if (App::$cache) { + App::$cache->set($cacheKey, $scopeList); + }Also applies to: 129-130
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
zmsdb/src/Zmsdb/Cluster.php(5 hunks)zmsdb/src/Zmsdb/Config.php(3 hunks)zmsdb/src/Zmsdb/Department.php(4 hunks)zmsdb/src/Zmsdb/Request.php(4 hunks)zmsdb/src/Zmsdb/Scope.php(6 hunks)zmsdb/src/Zmsdb/Source.php(4 hunks)zmsdb/tests/Zmsdb/ScopeTest.php(1 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
**/*
⚙️ CodeRabbit configuration file
**/*: Apply the following Clean Code guidelines to all files, as summarized by wojteklu's Clean Code gist:Code is clean if it can be understood easily – by everyone on the team. Clean code can be read and enhanced by a developer other than its original author. With understandability comes readability, changeability, extensibility and maintainability.
General rules
- Follow standard conventions.
- Keep it simple stupid. Simpler is always better. Reduce complexity as much as possible.
- Boy scout rule. Leave the campground cleaner than you found it.
- Always find root cause. Always look for the root cause of a problem.
Design rules
- Keep configurable data at high levels.
- Prefer polymorphism to if/else or switch/case.
- Separate multi-threading code.
- Prevent over-configurability.
- Use dependency injection.
- Follow Law of Demeter. A class should know only its direct dependencies.
Understandability tips
- Be consistent. If you do something a certain way, do all similar things in the same way.
- Use explanatory variables.
- Encapsulate boundary conditions. Boundary conditions are hard to keep track of. Put the processing for them in one place.
- Prefer dedicated value objects to primitive type.
- Avoid logical dependency. Don't write methods which works correctly depending on something else in the same class.
- Avoid negative conditionals.
Names rules
- Choose descriptive and unambiguous names.
- Make meaningful distinction.
- Use pronounceable names.
- Use searchable names.
- Replace magic numbers with named constants.
- Avoid encodings. Don't append prefixes or type information.
Functions rules
- Small.
- Do one thing.
- Use descriptive names.
- Prefer fewer arguments.
- Have no side effects.
- Don't use flag arguments. Split method into several independent methods that can b...
Files:
zmsdb/src/Zmsdb/Scope.phpzmsdb/src/Zmsdb/Config.phpzmsdb/src/Zmsdb/Department.phpzmsdb/src/Zmsdb/Source.phpzmsdb/src/Zmsdb/Cluster.phpzmsdb/tests/Zmsdb/ScopeTest.phpzmsdb/src/Zmsdb/Request.php
**/*.php
⚙️ CodeRabbit configuration file
**/*.php: Flag any usage of error_log() as it should be replaced with proper Monolog logging mechanisms provided by zmsslim which should be setup in the App/Application of each module:
- For error handling: Use the proper Monolog logging framework with error levels
- For application info logs: Use the proper Monolog logging framework with info levels
- For debugging: Use a dedicated debug logger or remove debug statements
- For CLI output: Use a CLI output handler or symfony/console
- The application log levels are as follows DEBUG, INFO, NOTICE, WARNING, ERROR, CRITICAL, ALERT, and EMERGENCY
Example replacement:
// Instead of: error_log("Import failed - " . $e->getMessage()); // Use: $log->error("Import failed", ['error' => $e->getMessage()]);Flag specific logging violations:
- error_log(), var_dump(), print_r(), die(), exit() usage (except proper error logging in catch blocks)
- Any debug output that should use proper Monolog logging
- Debug constants like DEBUG = true
- Debug logging that should be removed in production
- Commented debug code that should be cleaned up
Example replacements:
// Instead of: error_log("Error occurred"); var_dump($data); die('debug point'); // Use: $log->error("Error occurred", ['context' => 'processing']); $log->debug('Data dump', ['data' => $data]); // Remove die() statements entirelyException handling should use proper logging:
// Instead of: try { $result = $this->process(); } catch (Exception $e) { error_log("Processing failed: " . $e->getMessage()); } // Use: try { $result = $this->process(); } catch (Exception $e) { $log->error("Processing failed", ['error' => $e->getMessage(), 'trace' => $e->getTraceAsString()]); } ```...
Files:
zmsdb/src/Zmsdb/Scope.phpzmsdb/src/Zmsdb/Config.phpzmsdb/src/Zmsdb/Department.phpzmsdb/src/Zmsdb/Source.phpzmsdb/src/Zmsdb/Cluster.phpzmsdb/tests/Zmsdb/ScopeTest.phpzmsdb/src/Zmsdb/Request.php
🧬 Code graph analysis (7)
zmsdb/src/Zmsdb/Scope.php (6)
zmsdb/src/Zmsdb/Application.php (1)
Application(19-100)zmsdb/src/Zmsdb/Cluster.php (1)
readEntity(27-51)zmsdb/src/Zmsdb/Department.php (1)
readEntity(22-49)zmsdb/src/Zmsdb/Request.php (1)
readEntity(15-42)zmsdb/src/Zmsdb/Source.php (1)
readEntity(25-49)zmsdb/src/Zmsdb/Provider.php (1)
readEntity(11-33)
zmsdb/src/Zmsdb/Config.php (3)
zmsdb/src/Zmsdb/Application.php (1)
Application(19-100)zmsdb/src/Zmsdb/Cluster.php (1)
readEntity(27-51)zmsdb/src/Zmsdb/Request.php (1)
readEntity(15-42)
zmsdb/src/Zmsdb/Department.php (7)
zmsdb/src/Zmsdb/Application.php (1)
Application(19-100)zmsdb/src/Zmsdb/Cluster.php (1)
readEntity(27-51)zmsdb/src/Zmsdb/Config.php (1)
readEntity(14-30)zmsdb/src/Zmsdb/Request.php (1)
readEntity(15-42)zmsdb/src/Zmsdb/Scope.php (1)
readEntity(21-45)zmsdb/src/Zmsdb/Source.php (1)
readEntity(25-49)zmsdb/src/Zmsdb/Provider.php (1)
readEntity(11-33)
zmsdb/src/Zmsdb/Source.php (7)
zmsdb/src/Zmsdb/Application.php (1)
Application(19-100)zmsdb/src/Zmsdb/Cluster.php (1)
readEntity(27-51)zmsdb/src/Zmsdb/Config.php (1)
readEntity(14-30)zmsdb/src/Zmsdb/Department.php (1)
readEntity(22-49)zmsdb/src/Zmsdb/Request.php (1)
readEntity(15-42)zmsdb/src/Zmsdb/Scope.php (1)
readEntity(21-45)zmsdb/src/Zmsdb/Provider.php (1)
readEntity(11-33)
zmsdb/src/Zmsdb/Cluster.php (7)
zmsdb/src/Zmsdb/Application.php (1)
Application(19-100)zmsdb/src/Zmsdb/Config.php (1)
readEntity(14-30)zmsdb/src/Zmsdb/Department.php (2)
readEntity(22-49)readResolvedReferences(51-62)zmsdb/src/Zmsdb/Request.php (1)
readEntity(15-42)zmsdb/src/Zmsdb/Scope.php (2)
readEntity(21-45)readResolvedReferences(47-56)zmsdb/src/Zmsdb/Source.php (2)
readEntity(25-49)readResolvedReferences(79-94)zmsdb/src/Zmsdb/Provider.php (1)
readEntity(11-33)
zmsdb/tests/Zmsdb/ScopeTest.php (1)
zmsdb/src/Zmsdb/Scope.php (1)
readEntity(21-45)
zmsdb/src/Zmsdb/Request.php (7)
zmsdb/src/Zmsdb/Application.php (1)
Application(19-100)zmsdb/src/Zmsdb/Cluster.php (1)
readEntity(27-51)zmsdb/src/Zmsdb/Config.php (1)
readEntity(14-30)zmsdb/src/Zmsdb/Department.php (1)
readEntity(22-49)zmsdb/src/Zmsdb/Scope.php (1)
readEntity(21-45)zmsdb/src/Zmsdb/Source.php (1)
readEntity(25-49)zmsdb/src/Zmsdb/Provider.php (1)
readEntity(11-33)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: call-php-unit-tests / zmsdb-test
- GitHub Check: call-php-unit-tests / module-test (zmscitizenapi, 8.3)
- GitHub Check: call-php-unit-tests / zmsapi-test
🔇 Additional comments (8)
zmsdb/tests/Zmsdb/ScopeTest.php (1)
122-122: Good use of the new signature to bypass cache. Ensure Application::initialize() is invoked in your test setup to prevent App::$cache remaining null and causing fatal errors in other tests.zmsdb/src/Zmsdb/Source.php (4)
5-5: LGTM!The Application import alias is consistent with the caching pattern established across the codebase.
25-34: LGTM!The cache implementation follows the consistent pattern used across other entities (Cluster, Department, Request, Scope). The defensive check
if (App::$cache && !empty($data))is appropriate given thatApp::$cacheis nullable.
47-48: LGTM!Caching the resolved entity after reference resolution is correct and matches the pattern in other entity classes.
120-120: LGTM!Correctly bypasses the cache after writing to ensure fresh data is returned.
zmsdb/src/Zmsdb/Cluster.php (3)
5-5: LGTM!The Application import alias is consistent with the caching pattern established across the codebase.
27-35: LGTM!The cache implementation follows the consistent pattern used across other entities. The defensive check for
App::$cacheis appropriate.
47-50: LGTM!Correctly caches the resolved cluster and returns it.
| $cacheKey = "config"; | ||
| if (!$disableCache) { | ||
| $data = App::$cache->get($cacheKey); | ||
| if (App::$cache && !empty($data)) { | ||
| return $data; | ||
| } | ||
| } |
There was a problem hiding this comment.
Fix cache access: guard against null and use has() instead of empty().
- Potential fatal: calling App::$cache->get()/delete()/set() when App::$cache is null.
!empty($data)drops valid cached values; usehas()or a sentinel default.
Apply:
- if (!$disableCache) {
- $data = App::$cache->get($cacheKey);
- if (App::$cache && !empty($data)) {
- return $data;
- }
- }
+ if (!$disableCache && App::$cache && App::$cache->has($cacheKey)) {
+ return App::$cache->get($cacheKey);
+ }
@@
- App::$cache->set($cacheKey, $config);
+ if (App::$cache) {
+ App::$cache->set($cacheKey, $config);
+ }
@@
- App::$cache->delete('config');
+ if (App::$cache) {
+ App::$cache->delete('config');
+ }Also applies to: 27-28, 34-36
🤖 Prompt for AI Agents
In zmsdb/src/Zmsdb/Config.php around lines 16-22 (and also adjust similar logic
at 27-28 and 34-36), the code calls App::$cache->get()/set()/delete() without
first checking App::$cache for null and uses !empty($data) which can reject
valid cached falsy values; change the logic to first guard with if (App::$cache)
before any get/set/delete calls, and replace the !empty($data) check with a
proper has()/exists() check on the cache (or compare get() return against a
sentinel/default) so valid cached values like 0, "0", false, or empty array are
preserved; update all three locations accordingly to avoid fatal errors and
preserve falsy cache entries.
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (3)
zmsdb/src/Zmsdb/Scope.php (3)
23-30: Fix cache miss check and add null-guard (prevents fatal on uninitialized cache).
- Calling App::$cache->get(...) without ensuring App::$cache is set can fatally error.
- Using !empty($data) treats valid cached values ([], 0, '', false) as misses. Prefer has() or a strict sentinel.
Apply:
- if (!$disableCache) { - $data = App::$cache->get($cacheKey); - if (App::$cache && !empty($data)) { - return $data; - } - } + if (!$disableCache && App::$cache) { + if (App::$cache->has($cacheKey)) { + return App::$cache->get($cacheKey); + } + }Optionally run a repo-wide check to fix the same pattern elsewhere:
#!/bin/bash # Find cache get()+empty() anti-pattern and missing null-guards rg -n --type=php -C2 "App::\\$cache->get\\(" rg -n --type=php -C2 "App::\\$cache && !empty\\(\\$data\\)"
93-103: Same cache check issue in readByProviderId + guard cache write.
- Add null-guard for App::$cache before get()/set().
- Replace !empty() with has() (or strict sentinel).
- public function readByProviderId($providerId, $resolveReferences = 0, $disableCache = false) + public function readByProviderId($providerId, $resolveReferences = 0, $disableCache = false) { $cacheKey = "scopeReadByProviderId-$providerId-$resolveReferences"; - - if (!$disableCache) { - $data = App::$cache->get($cacheKey); - if (App::$cache && !empty($data)) { - return $data; - } - } + if (!$disableCache && App::$cache) { + if (App::$cache->has($cacheKey)) { + return App::$cache->get($cacheKey); + } + } @@ - App::$cache->set($cacheKey, $scopeList); + if (App::$cache) { + App::$cache->set($cacheKey, $scopeList); + }Also applies to: 129-131
493-494: Invalidate related cache keys on emergency update.Clear stale cache entries before re-reading to avoid serving outdated data.
- return $this->readEntity($scopeId, 0, true); + if (App::$cache) { + foreach ([0, 1, 2] as $level) { + App::$cache->delete("scope-$scopeId-$level"); + } + } + return $this->readEntity($scopeId, 0, true);
🧹 Nitpick comments (3)
zmsdb/src/Zmsdb/Scope.php (2)
21-21: Avoid flag parameters; provide explicit no-cache variants.Boolean $disableCache flags harm readability (Clean Code). Consider dedicated methods:
- readEntityNoCache($scopeId, $resolveReferences = 0)
- readByProviderIdNoCache($providerId, $resolveReferences = 0)
This keeps APIs intention‑revealing and avoids branching inside.
As per coding guidelines
Also applies to: 93-93
21-45: Consolidate dual caches to reduce drift.Maintaining both self::$cache and App::$cache risks inconsistencies. Prefer one authoritative layer (PSR-16) and optionally add an in-request static cache with strict invalidation on writes.
As per coding guidelines
zmsdb/src/Zmsdb/Organisation.php (1)
31-31: Remove commented debug statement; use Monolog if needed.Delete stale
//error_log(...)or replace with proper logger at appropriate level.As per coding guidelines
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
zmsdb/src/Zmsdb/Organisation.php(1 hunks)zmsdb/src/Zmsdb/Scope.php(7 hunks)zmsdb/tests/Zmsdb/ConfigTest.php(1 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
**/*
⚙️ CodeRabbit configuration file
**/*: Apply the following Clean Code guidelines to all files, as summarized by wojteklu's Clean Code gist:Code is clean if it can be understood easily – by everyone on the team. Clean code can be read and enhanced by a developer other than its original author. With understandability comes readability, changeability, extensibility and maintainability.
General rules
- Follow standard conventions.
- Keep it simple stupid. Simpler is always better. Reduce complexity as much as possible.
- Boy scout rule. Leave the campground cleaner than you found it.
- Always find root cause. Always look for the root cause of a problem.
Design rules
- Keep configurable data at high levels.
- Prefer polymorphism to if/else or switch/case.
- Separate multi-threading code.
- Prevent over-configurability.
- Use dependency injection.
- Follow Law of Demeter. A class should know only its direct dependencies.
Understandability tips
- Be consistent. If you do something a certain way, do all similar things in the same way.
- Use explanatory variables.
- Encapsulate boundary conditions. Boundary conditions are hard to keep track of. Put the processing for them in one place.
- Prefer dedicated value objects to primitive type.
- Avoid logical dependency. Don't write methods which works correctly depending on something else in the same class.
- Avoid negative conditionals.
Names rules
- Choose descriptive and unambiguous names.
- Make meaningful distinction.
- Use pronounceable names.
- Use searchable names.
- Replace magic numbers with named constants.
- Avoid encodings. Don't append prefixes or type information.
Functions rules
- Small.
- Do one thing.
- Use descriptive names.
- Prefer fewer arguments.
- Have no side effects.
- Don't use flag arguments. Split method into several independent methods that can b...
Files:
zmsdb/src/Zmsdb/Scope.phpzmsdb/tests/Zmsdb/ConfigTest.phpzmsdb/src/Zmsdb/Organisation.php
**/*.php
⚙️ CodeRabbit configuration file
**/*.php: Flag any usage of error_log() as it should be replaced with proper Monolog logging mechanisms provided by zmsslim which should be setup in the App/Application of each module:
- For error handling: Use the proper Monolog logging framework with error levels
- For application info logs: Use the proper Monolog logging framework with info levels
- For debugging: Use a dedicated debug logger or remove debug statements
- For CLI output: Use a CLI output handler or symfony/console
- The application log levels are as follows DEBUG, INFO, NOTICE, WARNING, ERROR, CRITICAL, ALERT, and EMERGENCY
Example replacement:
// Instead of: error_log("Import failed - " . $e->getMessage()); // Use: $log->error("Import failed", ['error' => $e->getMessage()]);Flag specific logging violations:
- error_log(), var_dump(), print_r(), die(), exit() usage (except proper error logging in catch blocks)
- Any debug output that should use proper Monolog logging
- Debug constants like DEBUG = true
- Debug logging that should be removed in production
- Commented debug code that should be cleaned up
Example replacements:
// Instead of: error_log("Error occurred"); var_dump($data); die('debug point'); // Use: $log->error("Error occurred", ['context' => 'processing']); $log->debug('Data dump', ['data' => $data]); // Remove die() statements entirelyException handling should use proper logging:
// Instead of: try { $result = $this->process(); } catch (Exception $e) { error_log("Processing failed: " . $e->getMessage()); } // Use: try { $result = $this->process(); } catch (Exception $e) { $log->error("Processing failed", ['error' => $e->getMessage(), 'trace' => $e->getTraceAsString()]); } ```...
Files:
zmsdb/src/Zmsdb/Scope.phpzmsdb/tests/Zmsdb/ConfigTest.phpzmsdb/src/Zmsdb/Organisation.php
🧬 Code graph analysis (3)
zmsdb/src/Zmsdb/Scope.php (7)
zmsdb/src/Zmsdb/Application.php (1)
Application(19-100)zmsdb/src/Zmsdb/Request.php (1)
readEntity(15-42)zmsdb/src/Zmsdb/Config.php (1)
readEntity(14-30)zmsdb/src/Zmsdb/Department.php (1)
readEntity(22-49)zmsdb/src/Zmsdb/Source.php (1)
readEntity(25-49)zmsdb/src/Zmsdb/Cluster.php (1)
readEntity(27-51)zmsdb/src/Zmsdb/Provider.php (1)
readEntity(11-33)
zmsdb/tests/Zmsdb/ConfigTest.php (2)
zmsdb/src/Zmsdb/Organisation.php (1)
readEntity(15-26)zmsdb/src/Zmsdb/Scope.php (1)
readEntity(21-45)
zmsdb/src/Zmsdb/Organisation.php (1)
zmsdb/src/Zmsdb/Scope.php (1)
readEntity(21-45)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: call-php-unit-tests / zmsdb-test
🔇 Additional comments (1)
zmsdb/tests/Zmsdb/ConfigTest.php (1)
27-27: LGTM: test updated to use cache-bypass flag.Change aligns with Config::readEntity($disableCache = false).
| return $this->readEntity($organisationId, 1, true); | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
Too many arguments to readEntity; will emit warning or fatal (PHP ≥8).
readEntity accepts 2 params; this call passes 3. Fix now.
Minimal fix:
- return $this->readEntity($organisationId, 1, true);
+ return $this->readEntity($organisationId, 1);Alternatively, align Organisation with new caching API by extending its signature and adding cache handling like Scope::readEntity.
Please confirm PHP runtime version; on PHP 8+, “Too many arguments” can be fatal depending on setup.
🏁 Script executed:
#!/bin/bash
rg -nP "function\s+readEntity\s*\(" -g "*.php"Length of output: 4017
🏁 Script executed:
#!/bin/bash
rg -n "error_log" -g "zmsdb/src/Zmsdb/Organisation.php"Length of output: 101
Remove invalid third argument and clean up debug code
- zmsdb/src/Zmsdb/Organisation.php line 183:
readEntitysignature is($itemId, $resolveReferences = 0)—remove the thirdtrueargument:
- return $this->readEntity($organisationId, 1, true);
+ return $this->readEntity($organisationId, 1);- zmsdb/src/Zmsdb/Organisation.php line 31: remove the commented
//error_log("Organisation Level $resolveReferences");or replace with proper Monolog logging.
🤖 Prompt for AI Agents
In zmsdb/src/Zmsdb/Organisation.php around lines 183-184 and line 31, the call
return $this->readEntity($organisationId, 1, true); uses an invalid third
argument—remove the third parameter so it becomes readEntity($organisationId,
1); and also delete the commented debug line on line 31
(//error_log("Organisation Level $resolveReferences");) or replace it with a
proper Monolog logger call if runtime logging is required.
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
zmsapi/src/Zmsapi/ProcessListByClusterAndDate.php (1)
77-77: Replace error_log() with Monolog logging.The use of
error_log()violates the coding guidelines. Replace it with proper Monolog logging.Apply this diff:
if ($archivedProcesses instanceof ProcessListCollection) { $allArchivedProcesses = $archivedProcesses; } else { - error_log("Expected ProcessListCollection, received " . gettype($archivedProcesses)); + \App::$log->error("Archived processes type mismatch", [ + 'expected' => ProcessListCollection::class, + 'received' => gettype($archivedProcesses), + 'scopeIds' => $scopeIds, + 'dates' => array_map(fn($d) => $d->format('Y-m-d'), $dates) + ]); }Note: Ensure
\App::$logis properly initialized in your Application class, as per coding guidelines.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
zmsapi/src/Zmsapi/ProcessListByClusterAndDate.php(1 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
**/*
⚙️ CodeRabbit configuration file
**/*: Apply the following Clean Code guidelines to all files, as summarized by wojteklu's Clean Code gist:Code is clean if it can be understood easily – by everyone on the team. Clean code can be read and enhanced by a developer other than its original author. With understandability comes readability, changeability, extensibility and maintainability.
General rules
- Follow standard conventions.
- Keep it simple stupid. Simpler is always better. Reduce complexity as much as possible.
- Boy scout rule. Leave the campground cleaner than you found it.
- Always find root cause. Always look for the root cause of a problem.
Design rules
- Keep configurable data at high levels.
- Prefer polymorphism to if/else or switch/case.
- Separate multi-threading code.
- Prevent over-configurability.
- Use dependency injection.
- Follow Law of Demeter. A class should know only its direct dependencies.
Understandability tips
- Be consistent. If you do something a certain way, do all similar things in the same way.
- Use explanatory variables.
- Encapsulate boundary conditions. Boundary conditions are hard to keep track of. Put the processing for them in one place.
- Prefer dedicated value objects to primitive type.
- Avoid logical dependency. Don't write methods which works correctly depending on something else in the same class.
- Avoid negative conditionals.
Names rules
- Choose descriptive and unambiguous names.
- Make meaningful distinction.
- Use pronounceable names.
- Use searchable names.
- Replace magic numbers with named constants.
- Avoid encodings. Don't append prefixes or type information.
Functions rules
- Small.
- Do one thing.
- Use descriptive names.
- Prefer fewer arguments.
- Have no side effects.
- Don't use flag arguments. Split method into several independent methods that can b...
Files:
zmsapi/src/Zmsapi/ProcessListByClusterAndDate.php
**/*.php
⚙️ CodeRabbit configuration file
**/*.php: Flag any usage of error_log() as it should be replaced with proper Monolog logging mechanisms provided by zmsslim which should be setup in the App/Application of each module:
- For error handling: Use the proper Monolog logging framework with error levels
- For application info logs: Use the proper Monolog logging framework with info levels
- For debugging: Use a dedicated debug logger or remove debug statements
- For CLI output: Use a CLI output handler or symfony/console
- The application log levels are as follows DEBUG, INFO, NOTICE, WARNING, ERROR, CRITICAL, ALERT, and EMERGENCY
Example replacement:
// Instead of: error_log("Import failed - " . $e->getMessage()); // Use: $log->error("Import failed", ['error' => $e->getMessage()]);Flag specific logging violations:
- error_log(), var_dump(), print_r(), die(), exit() usage (except proper error logging in catch blocks)
- Any debug output that should use proper Monolog logging
- Debug constants like DEBUG = true
- Debug logging that should be removed in production
- Commented debug code that should be cleaned up
Example replacements:
// Instead of: error_log("Error occurred"); var_dump($data); die('debug point'); // Use: $log->error("Error occurred", ['context' => 'processing']); $log->debug('Data dump', ['data' => $data]); // Remove die() statements entirelyException handling should use proper logging:
// Instead of: try { $result = $this->process(); } catch (Exception $e) { error_log("Processing failed: " . $e->getMessage()); } // Use: try { $result = $this->process(); } catch (Exception $e) { $log->error("Processing failed", ['error' => $e->getMessage(), 'trace' => $e->getTraceAsString()]); } ```...
Files:
zmsapi/src/Zmsapi/ProcessListByClusterAndDate.php
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: call-php-unit-tests / zmsdb-test
- GitHub Check: call-php-unit-tests / zmsapi-test
|
|
||
| $query = new Query(); | ||
| $cluster = $query->readEntity($args['id'], 0); | ||
| $cluster = $query->readEntity($args['id'], 0, 1); |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
🧩 Analysis chain
Verify the cache bypass decision.
The third parameter 1 disables caching for this cluster read. Consider the following:
- Is bypassing the cache necessary here? Cluster data is typically stable and would benefit from caching.
- This creates inconsistency: the
readQueueListcall at line 54 doesn't disable caching. - If cache bypass is required (e.g., to ensure fresh cluster configuration), add a comment explaining why.
Run the following script to check how cache bypass is used across similar read operations:
🏁 Script executed:
#!/bin/bash
# Description: Check cache bypass patterns in readEntity calls across the codebase
# Search for readEntity calls with the cache bypass parameter
rg -nP --type=php -C3 '->readEntity\([^)]+,\s*\d+,\s*1\)'Length of output: 404
🏁 Script executed:
#!/bin/bash
# Description: Find all readEntity calls disabling cache (third parameter = 1)
rg -n --type=php -C3 -e '->readEntity\([^,]+,\s*[^,]+,\s*1\s*\)'Length of output: 6905
🏁 Script executed:
#!/bin/bash
# Display the top of ProcessListByClusterAndDate to inspect imports and Query reference
sed -n '1,60p' zmsapi/src/Zmsapi/ProcessListByClusterAndDate.phpLength of output: 1892
Replace error_log() with Monolog logging.
The error_log() call (around line 77) must be removed and replaced with the application’s Monolog logger (e.g. $this->logger->error('Message', ['error'=>$e->getMessage()]);).
🤖 Prompt for AI Agents
In zmsapi/src/Zmsapi/ProcessListByClusterAndDate.php around line 47, remove the
error_log() call and replace it with the application's Monolog logger;
specifically, call $this->logger->error with a descriptive message and include
the exception message in the context array (e.g. ['error' => $e->getMessage()]);
ensure you reference the existing $this->logger instance and keep the same
exception variable ($e) used in the catch block.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (2)
zmsapi/src/Zmsapi/ProcessListByClusterAndDate.php (2)
77-77: Replace error_log() with Monolog logging.The
error_log()call violates the project's logging guidelines. Replace it with the application's Monolog logger.As per coding guidelines, apply this diff:
- error_log("Expected ProcessListCollection, received " . gettype($archivedProcesses)); + $this->logger->error("Expected ProcessListCollection", [ + 'receivedType' => gettype($archivedProcesses), + 'scopeIds' => $scopeIds, + 'dates' => array_map(fn($d) => $d->format('Y-m-d'), $dates) + ]);
47-47: Reconsider the cache bypass decision.The third parameter
truedisables caching for cluster reads, which seems counterproductive in a PR implementing caching. This also creates inconsistency: thereadQueueListcall at line 54 uses caching, while this cluster read bypasses it. Cluster data is typically stable and would benefit from caching.If cache bypass is required for this specific use case, add a comment explaining the rationale.
🧹 Nitpick comments (1)
zmsapi/tests/Zmsapi/ProcessListByClusterAndDateTest.php (1)
6-6: Remove unused import.The
BO\Zmsclient\Httpimport is not referenced anywhere in this test file.Apply this diff:
-use BO\Zmsclient\Http;
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
zmsapi/src/Zmsapi/ProcessListByClusterAndDate.php(1 hunks)zmsapi/tests/Zmsapi/ProcessListByClusterAndDateTest.php(1 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
**/*
⚙️ CodeRabbit configuration file
**/*: Apply the following Clean Code guidelines to all files, as summarized by wojteklu's Clean Code gist:Code is clean if it can be understood easily – by everyone on the team. Clean code can be read and enhanced by a developer other than its original author. With understandability comes readability, changeability, extensibility and maintainability.
General rules
- Follow standard conventions.
- Keep it simple stupid. Simpler is always better. Reduce complexity as much as possible.
- Boy scout rule. Leave the campground cleaner than you found it.
- Always find root cause. Always look for the root cause of a problem.
Design rules
- Keep configurable data at high levels.
- Prefer polymorphism to if/else or switch/case.
- Separate multi-threading code.
- Prevent over-configurability.
- Use dependency injection.
- Follow Law of Demeter. A class should know only its direct dependencies.
Understandability tips
- Be consistent. If you do something a certain way, do all similar things in the same way.
- Use explanatory variables.
- Encapsulate boundary conditions. Boundary conditions are hard to keep track of. Put the processing for them in one place.
- Prefer dedicated value objects to primitive type.
- Avoid logical dependency. Don't write methods which works correctly depending on something else in the same class.
- Avoid negative conditionals.
Names rules
- Choose descriptive and unambiguous names.
- Make meaningful distinction.
- Use pronounceable names.
- Use searchable names.
- Replace magic numbers with named constants.
- Avoid encodings. Don't append prefixes or type information.
Functions rules
- Small.
- Do one thing.
- Use descriptive names.
- Prefer fewer arguments.
- Have no side effects.
- Don't use flag arguments. Split method into several independent methods that can b...
Files:
zmsapi/tests/Zmsapi/ProcessListByClusterAndDateTest.phpzmsapi/src/Zmsapi/ProcessListByClusterAndDate.php
**/*.php
⚙️ CodeRabbit configuration file
**/*.php: Flag any usage of error_log() as it should be replaced with proper Monolog logging mechanisms provided by zmsslim which should be setup in the App/Application of each module:
- For error handling: Use the proper Monolog logging framework with error levels
- For application info logs: Use the proper Monolog logging framework with info levels
- For debugging: Use a dedicated debug logger or remove debug statements
- For CLI output: Use a CLI output handler or symfony/console
- The application log levels are as follows DEBUG, INFO, NOTICE, WARNING, ERROR, CRITICAL, ALERT, and EMERGENCY
Example replacement:
// Instead of: error_log("Import failed - " . $e->getMessage()); // Use: $log->error("Import failed", ['error' => $e->getMessage()]);Flag specific logging violations:
- error_log(), var_dump(), print_r(), die(), exit() usage (except proper error logging in catch blocks)
- Any debug output that should use proper Monolog logging
- Debug constants like DEBUG = true
- Debug logging that should be removed in production
- Commented debug code that should be cleaned up
Example replacements:
// Instead of: error_log("Error occurred"); var_dump($data); die('debug point'); // Use: $log->error("Error occurred", ['context' => 'processing']); $log->debug('Data dump', ['data' => $data]); // Remove die() statements entirelyException handling should use proper logging:
// Instead of: try { $result = $this->process(); } catch (Exception $e) { error_log("Processing failed: " . $e->getMessage()); } // Use: try { $result = $this->process(); } catch (Exception $e) { $log->error("Processing failed", ['error' => $e->getMessage(), 'trace' => $e->getTraceAsString()]); } ```...
Files:
zmsapi/tests/Zmsapi/ProcessListByClusterAndDateTest.phpzmsapi/src/Zmsapi/ProcessListByClusterAndDate.php
🧬 Code graph analysis (1)
zmsapi/tests/Zmsapi/ProcessListByClusterAndDateTest.php (1)
zmsclient/src/Zmsclient/Http.php (1)
Http(16-260)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: Analyze (javascript-typescript)
- GitHub Check: call-php-unit-tests / zmsdb-test
🔇 Additional comments (1)
zmsapi/tests/Zmsapi/ProcessListByClusterAndDateTest.php (1)
13-19: Cache mock setup looks good.The
setUp()method correctly initializes a cache mock and assigns it to\App::$cache, enabling the tests to work with the new caching infrastructure.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
zmsapi/tests/Zmsapi/ProcessListByClusterAndDateTest.php (1)
6-6: Remove unused import.The
BO\Zmsclient\Httpimport is not used anywhere in this test file.Apply this diff to remove the unused import:
-use BO\Zmsclient\Http;
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
zmsapi/tests/Zmsapi/ProcessListByClusterAndDateTest.php(1 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
**/*
⚙️ CodeRabbit configuration file
**/*: Apply the following Clean Code guidelines to all files, as summarized by wojteklu's Clean Code gist:Code is clean if it can be understood easily – by everyone on the team. Clean code can be read and enhanced by a developer other than its original author. With understandability comes readability, changeability, extensibility and maintainability.
General rules
- Follow standard conventions.
- Keep it simple stupid. Simpler is always better. Reduce complexity as much as possible.
- Boy scout rule. Leave the campground cleaner than you found it.
- Always find root cause. Always look for the root cause of a problem.
Design rules
- Keep configurable data at high levels.
- Prefer polymorphism to if/else or switch/case.
- Separate multi-threading code.
- Prevent over-configurability.
- Use dependency injection.
- Follow Law of Demeter. A class should know only its direct dependencies.
Understandability tips
- Be consistent. If you do something a certain way, do all similar things in the same way.
- Use explanatory variables.
- Encapsulate boundary conditions. Boundary conditions are hard to keep track of. Put the processing for them in one place.
- Prefer dedicated value objects to primitive type.
- Avoid logical dependency. Don't write methods which works correctly depending on something else in the same class.
- Avoid negative conditionals.
Names rules
- Choose descriptive and unambiguous names.
- Make meaningful distinction.
- Use pronounceable names.
- Use searchable names.
- Replace magic numbers with named constants.
- Avoid encodings. Don't append prefixes or type information.
Functions rules
- Small.
- Do one thing.
- Use descriptive names.
- Prefer fewer arguments.
- Have no side effects.
- Don't use flag arguments. Split method into several independent methods that can b...
Files:
zmsapi/tests/Zmsapi/ProcessListByClusterAndDateTest.php
**/*.php
⚙️ CodeRabbit configuration file
**/*.php: Flag any usage of error_log() as it should be replaced with proper Monolog logging mechanisms provided by zmsslim which should be setup in the App/Application of each module:
- For error handling: Use the proper Monolog logging framework with error levels
- For application info logs: Use the proper Monolog logging framework with info levels
- For debugging: Use a dedicated debug logger or remove debug statements
- For CLI output: Use a CLI output handler or symfony/console
- The application log levels are as follows DEBUG, INFO, NOTICE, WARNING, ERROR, CRITICAL, ALERT, and EMERGENCY
Example replacement:
// Instead of: error_log("Import failed - " . $e->getMessage()); // Use: $log->error("Import failed", ['error' => $e->getMessage()]);Flag specific logging violations:
- error_log(), var_dump(), print_r(), die(), exit() usage (except proper error logging in catch blocks)
- Any debug output that should use proper Monolog logging
- Debug constants like DEBUG = true
- Debug logging that should be removed in production
- Commented debug code that should be cleaned up
Example replacements:
// Instead of: error_log("Error occurred"); var_dump($data); die('debug point'); // Use: $log->error("Error occurred", ['context' => 'processing']); $log->debug('Data dump', ['data' => $data]); // Remove die() statements entirelyException handling should use proper logging:
// Instead of: try { $result = $this->process(); } catch (Exception $e) { error_log("Processing failed: " . $e->getMessage()); } // Use: try { $result = $this->process(); } catch (Exception $e) { $log->error("Processing failed", ['error' => $e->getMessage(), 'trace' => $e->getTraceAsString()]); } ```...
Files:
zmsapi/tests/Zmsapi/ProcessListByClusterAndDateTest.php
🧬 Code graph analysis (1)
zmsapi/tests/Zmsapi/ProcessListByClusterAndDateTest.php (1)
zmsclient/src/Zmsclient/Http.php (1)
Http(16-260)
🪛 GitHub Actions: Combined Workflow
zmsapi/tests/Zmsapi/ProcessListByClusterAndDateTest.php
[error] 17-17: PHPUnit error: Creation of dynamic property BO\Zmsapi\Tests\ProcessListByClusterAndDateTest::$cacheMock is deprecated.
[error] 17-17: PHPUnit error: Creation of dynamic property BO\Zmsapi\Tests\ProcessListByClusterAndDateTest::$cacheMock is deprecated.
| public function setUp(): void | ||
| { | ||
| parent::setUp(); | ||
|
|
||
| $this->cacheMock = $this->createMock(CacheInterface::class); | ||
| \App::$cache = $this->cacheMock; | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
Declare $cacheMock as a class property to fix deprecation warning.
The pipeline failure indicates that creating $this->cacheMock dynamically is deprecated in PHP 8.2+. This will become a fatal error in PHP 9.0.
Add the property declaration to the class:
class ProcessListByClusterAndDateTest extends Base
{
protected $classname = "ProcessListByClusterAndDate";
+
+ protected $cacheMock;
public function setUp(): voidAdditionally, consider whether the cache mock needs expectations configured. Currently, it's created but not configured to return any values or verify interactions. If the code under test uses the cache, you may want to add expectations like:
$this->cacheMock->expects($this->any())
->method('get')
->willReturn(null);🏁 Script executed:
#!/bin/bash
set -e
echo "Class declaration context:"
rg -n 'class ProcessListByClusterAndDateTest' -C3 zmsapi/tests/Zmsapi/ProcessListByClusterAndDateTest.php
echo
echo "Occurrences of cacheMock:"
rg -n 'cacheMock' -C1 zmsapi/tests/Zmsapi/ProcessListByClusterAndDateTest.phpLength of output: 371
Declare cacheMock as a class property
Add the property to avoid PHP 8.2’s dynamic‐property deprecation:
class ProcessListByClusterAndDateTest extends Base
{
protected $classname = "ProcessListByClusterAndDate";
+ protected CacheInterface $cacheMock;(Optional) configure expectations on this mock if the code under test invokes the cache.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| public function setUp(): void | |
| { | |
| parent::setUp(); | |
| $this->cacheMock = $this->createMock(CacheInterface::class); | |
| \App::$cache = $this->cacheMock; | |
| } | |
| class ProcessListByClusterAndDateTest extends Base | |
| { | |
| protected $classname = "ProcessListByClusterAndDate"; | |
| protected CacheInterface $cacheMock; | |
| public function setUp(): void | |
| { | |
| parent::setUp(); | |
| $this->cacheMock = $this->createMock(CacheInterface::class); | |
| \App::$cache = $this->cacheMock; | |
| } | |
| // ... | |
| } |
🧰 Tools
🪛 GitHub Actions: Combined Workflow
[error] 17-17: PHPUnit error: Creation of dynamic property BO\Zmsapi\Tests\ProcessListByClusterAndDateTest::$cacheMock is deprecated.
[error] 17-17: PHPUnit error: Creation of dynamic property BO\Zmsapi\Tests\ProcessListByClusterAndDateTest::$cacheMock is deprecated.
🤖 Prompt for AI Agents
In zmsapi/tests/Zmsapi/ProcessListByClusterAndDateTest.php around lines 13 to
19, the test assigns $this->cacheMock dynamically which triggers PHP 8.2 dynamic
property deprecation; declare a class property (e.g. private or protected
CacheInterface $cacheMock;) at the top of the test class to make the mock a
typed property, and then keep the setUp assignment; optionally, configure
expectations on this mock in setUp or individual tests if the code under test
calls the cache.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
zmsapi/tests/Zmsapi/ProcessListByClusterAndDateTest.php (1)
6-6: Remove unused import.The
BO\Zmsclient\Httpimport is not used anywhere in this test file.Apply this diff to remove the unused import:
-use BO\Zmsclient\Http;
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
zmsapi/tests/Zmsapi/ProcessListByClusterAndDateTest.php(1 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
**/*
⚙️ CodeRabbit configuration file
**/*: Apply the following Clean Code guidelines to all files, as summarized by wojteklu's Clean Code gist:Code is clean if it can be understood easily – by everyone on the team. Clean code can be read and enhanced by a developer other than its original author. With understandability comes readability, changeability, extensibility and maintainability.
General rules
- Follow standard conventions.
- Keep it simple stupid. Simpler is always better. Reduce complexity as much as possible.
- Boy scout rule. Leave the campground cleaner than you found it.
- Always find root cause. Always look for the root cause of a problem.
Design rules
- Keep configurable data at high levels.
- Prefer polymorphism to if/else or switch/case.
- Separate multi-threading code.
- Prevent over-configurability.
- Use dependency injection.
- Follow Law of Demeter. A class should know only its direct dependencies.
Understandability tips
- Be consistent. If you do something a certain way, do all similar things in the same way.
- Use explanatory variables.
- Encapsulate boundary conditions. Boundary conditions are hard to keep track of. Put the processing for them in one place.
- Prefer dedicated value objects to primitive type.
- Avoid logical dependency. Don't write methods which works correctly depending on something else in the same class.
- Avoid negative conditionals.
Names rules
- Choose descriptive and unambiguous names.
- Make meaningful distinction.
- Use pronounceable names.
- Use searchable names.
- Replace magic numbers with named constants.
- Avoid encodings. Don't append prefixes or type information.
Functions rules
- Small.
- Do one thing.
- Use descriptive names.
- Prefer fewer arguments.
- Have no side effects.
- Don't use flag arguments. Split method into several independent methods that can b...
Files:
zmsapi/tests/Zmsapi/ProcessListByClusterAndDateTest.php
**/*.php
⚙️ CodeRabbit configuration file
**/*.php: Flag any usage of error_log() as it should be replaced with proper Monolog logging mechanisms provided by zmsslim which should be setup in the App/Application of each module:
- For error handling: Use the proper Monolog logging framework with error levels
- For application info logs: Use the proper Monolog logging framework with info levels
- For debugging: Use a dedicated debug logger or remove debug statements
- For CLI output: Use a CLI output handler or symfony/console
- The application log levels are as follows DEBUG, INFO, NOTICE, WARNING, ERROR, CRITICAL, ALERT, and EMERGENCY
Example replacement:
// Instead of: error_log("Import failed - " . $e->getMessage()); // Use: $log->error("Import failed", ['error' => $e->getMessage()]);Flag specific logging violations:
- error_log(), var_dump(), print_r(), die(), exit() usage (except proper error logging in catch blocks)
- Any debug output that should use proper Monolog logging
- Debug constants like DEBUG = true
- Debug logging that should be removed in production
- Commented debug code that should be cleaned up
Example replacements:
// Instead of: error_log("Error occurred"); var_dump($data); die('debug point'); // Use: $log->error("Error occurred", ['context' => 'processing']); $log->debug('Data dump', ['data' => $data]); // Remove die() statements entirelyException handling should use proper logging:
// Instead of: try { $result = $this->process(); } catch (Exception $e) { error_log("Processing failed: " . $e->getMessage()); } // Use: try { $result = $this->process(); } catch (Exception $e) { $log->error("Processing failed", ['error' => $e->getMessage(), 'trace' => $e->getTraceAsString()]); } ```...
Files:
zmsapi/tests/Zmsapi/ProcessListByClusterAndDateTest.php
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: call-php-unit-tests / zmsdb-test
- GitHub Check: call-php-unit-tests / zmsapi-test
| public function setUp(): void | ||
| { | ||
| parent::setUp(); | ||
|
|
||
| $this->cacheMock = $this->createMock(CacheInterface::class); | ||
| \App::$cache = $this->cacheMock; | ||
| } |
There was a problem hiding this comment.
Add tearDown to ensure test isolation.
The cache mock is assigned to the global static property \App::$cache but there's no tearDown() method to clean up this state after each test. This can cause test pollution if other tests in the suite also interact with \App::$cache.
Add a tearDown() method to reset the static state:
+ public function tearDown(): void
+ {
+ \App::$cache = null;
+ parent::tearDown();
+ }🤖 Prompt for AI Agents
In zmsapi/tests/Zmsapi/ProcessListByClusterAndDateTest.php around lines 14 to
20, the setUp assigns a cache mock to the global static \App::$cache but no
tearDown is provided to reset that static, risking test pollution; add a public
function tearDown(): void that sets \App::$cache = null (or restores previous
value if you saved one) and calls parent::tearDown() to ensure the mock is
cleared after each test.
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
zmsdb/src/Zmsdb/DayOff.php (1)
68-96: Guard App::$cache usage and ensure cache initialization
- Wrap all
App::$cache->get(…)andApp::$cache->set(…)calls in a null check (e.g.if (App::$cache) { … }) in DayOff.php (lines 75, 94) and likewise in other DB classes.- Invoke
Application::initialize()in every entry point (index.php, bootstrap.php, console.php) soApp::$cacheis never null at runtime.
♻️ Duplicate comments (7)
zmsdb/src/Zmsdb/Scope.php (4)
23-30: This comment duplicates a previous review.The cache check logic issues identified in the previous review remain unaddressed.
42-42: This comment duplicates a previous review.The missing null check before cache write remains unaddressed.
103-108: This comment duplicates a previous review.The cache check logic issues identified in the previous review remain unaddressed.
503-503: This comment duplicates a previous review.The cache invalidation concern identified in the previous review remains unaddressed.
zmsdb/src/Zmsdb/Cluster.php (2)
370-370: Use booleantrueinstead of integer1for type consistency.The third parameter
$disableCacheexpects a boolean but receives an integer1. While PHP will coerce this totrue, it violates type expectations and clean code principles.Apply this diff:
- return $this->readEntity($lastInsertId, 1, 1); + return $this->readEntity($lastInsertId, 1, true);
391-391: Use booleantrueinstead of integer1for type consistency.The third parameter
$disableCacheexpects a boolean but receives an integer1. While PHP will coerce this totrue, it violates type expectations and clean code principles.Apply this diff:
- return $this->readEntity($clusterId, 1, 1); + return $this->readEntity($clusterId, 1, true);zmsdb/src/Zmsdb/DayOff.php (1)
94-94: Fix cache null-check before set.Calling
App::$cache->set()will fail ifApp::$cacheis null. This critical issue was flagged in the previous review but remains unfixed. Add a null check before attempting to cache the data.Apply this diff to fix the issue:
- - App::$cache->set($cacheKey, $dayOffList); - + + if (App::$cache) { + App::$cache->set($cacheKey, $dayOffList); + } + return $dayOffListCommon->addList($dayOffList);
🧹 Nitpick comments (5)
zmsdb/src/Zmsdb/Closure.php (1)
12-12: Add type hints for parameters.The method parameters lack type declarations, which reduces type safety and IDE support.
Apply this diff to add type hints:
- public function readByScopeId($scopeId = 0, $disableCache = false) + public function readByScopeId(int $scopeId = 0, bool $disableCache = false)zmsdb/src/Zmsdb/Scope.php (4)
47-56: Add type hint for the$disableCacheparameter.The method signature is missing a type hint for the
$disableCacheparameter, which reduces type safety and clarity.Apply this diff to add the type hint:
public function readResolvedReferences( \BO\Zmsentities\Schema\Entity $scope, $resolveReferences, - $disableCache = false + bool $disableCache = false ) {
59-96: Add type hint for the$disableCacheparameter.Similar to
readResolvedReferences, this method is missing a type hint for the$disableCacheparameter.Apply this diff:
public function readByClusterId( $clusterId, $resolveReferences = 0, - $disableCache = false + bool $disableCache = false ) {
99-99: Add type hint for the$disableCacheparameter.This method also needs a type hint for the
$disableCacheparameter to maintain consistency.Apply this diff:
- public function readByProviderId($providerId, $resolveReferences = 0, $disableCache = false) + public function readByProviderId($providerId, $resolveReferences = 0, bool $disableCache = false)
99-142: Consider unifying the caching strategy.
readEntity()uses both a static local cache (self::$cache) and the application-level cache (App::$cache), whilereadByProviderId()only usesApp::$cache. This inconsistency may lead to different performance characteristics and cache invalidation behavior.Consider either:
- Adding local cache to
readByProviderId()for consistency withreadEntity(), or- Documenting why different strategies are used for these methods.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
zmsdb/src/Zmsdb/Closure.php(2 hunks)zmsdb/src/Zmsdb/Cluster.php(5 hunks)zmsdb/src/Zmsdb/DayOff.php(4 hunks)zmsdb/src/Zmsdb/Scope.php(7 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
**/*
⚙️ CodeRabbit configuration file
**/*: Apply the following Clean Code guidelines to all files, as summarized by wojteklu's Clean Code gist:Code is clean if it can be understood easily – by everyone on the team. Clean code can be read and enhanced by a developer other than its original author. With understandability comes readability, changeability, extensibility and maintainability.
General rules
- Follow standard conventions.
- Keep it simple stupid. Simpler is always better. Reduce complexity as much as possible.
- Boy scout rule. Leave the campground cleaner than you found it.
- Always find root cause. Always look for the root cause of a problem.
Design rules
- Keep configurable data at high levels.
- Prefer polymorphism to if/else or switch/case.
- Separate multi-threading code.
- Prevent over-configurability.
- Use dependency injection.
- Follow Law of Demeter. A class should know only its direct dependencies.
Understandability tips
- Be consistent. If you do something a certain way, do all similar things in the same way.
- Use explanatory variables.
- Encapsulate boundary conditions. Boundary conditions are hard to keep track of. Put the processing for them in one place.
- Prefer dedicated value objects to primitive type.
- Avoid logical dependency. Don't write methods which works correctly depending on something else in the same class.
- Avoid negative conditionals.
Names rules
- Choose descriptive and unambiguous names.
- Make meaningful distinction.
- Use pronounceable names.
- Use searchable names.
- Replace magic numbers with named constants.
- Avoid encodings. Don't append prefixes or type information.
Functions rules
- Small.
- Do one thing.
- Use descriptive names.
- Prefer fewer arguments.
- Have no side effects.
- Don't use flag arguments. Split method into several independent methods that can b...
Files:
zmsdb/src/Zmsdb/Cluster.phpzmsdb/src/Zmsdb/Closure.phpzmsdb/src/Zmsdb/DayOff.phpzmsdb/src/Zmsdb/Scope.php
**/*.php
⚙️ CodeRabbit configuration file
**/*.php: Flag any usage of error_log() as it should be replaced with proper Monolog logging mechanisms provided by zmsslim which should be setup in the App/Application of each module:
- For error handling: Use the proper Monolog logging framework with error levels
- For application info logs: Use the proper Monolog logging framework with info levels
- For debugging: Use a dedicated debug logger or remove debug statements
- For CLI output: Use a CLI output handler or symfony/console
- The application log levels are as follows DEBUG, INFO, NOTICE, WARNING, ERROR, CRITICAL, ALERT, and EMERGENCY
Example replacement:
// Instead of: error_log("Import failed - " . $e->getMessage()); // Use: $log->error("Import failed", ['error' => $e->getMessage()]);Flag specific logging violations:
- error_log(), var_dump(), print_r(), die(), exit() usage (except proper error logging in catch blocks)
- Any debug output that should use proper Monolog logging
- Debug constants like DEBUG = true
- Debug logging that should be removed in production
- Commented debug code that should be cleaned up
Example replacements:
// Instead of: error_log("Error occurred"); var_dump($data); die('debug point'); // Use: $log->error("Error occurred", ['context' => 'processing']); $log->debug('Data dump', ['data' => $data]); // Remove die() statements entirelyException handling should use proper logging:
// Instead of: try { $result = $this->process(); } catch (Exception $e) { error_log("Processing failed: " . $e->getMessage()); } // Use: try { $result = $this->process(); } catch (Exception $e) { $log->error("Processing failed", ['error' => $e->getMessage(), 'trace' => $e->getTraceAsString()]); } ```...
Files:
zmsdb/src/Zmsdb/Cluster.phpzmsdb/src/Zmsdb/Closure.phpzmsdb/src/Zmsdb/DayOff.phpzmsdb/src/Zmsdb/Scope.php
🧬 Code graph analysis (4)
zmsdb/src/Zmsdb/Cluster.php (2)
zmsdb/src/Zmsdb/Application.php (1)
Application(19-100)zmsdb/src/Zmsdb/Scope.php (4)
readEntity(21-45)readResolvedReferences(47-57)Scope(17-608)readByClusterId(59-97)
zmsdb/src/Zmsdb/Closure.php (3)
zmsdb/src/Zmsdb/Application.php (1)
Application(19-100)zmsdb/src/Zmsdb/Query/Closure.php (1)
Closure(8-107)zmsdb/src/Zmsdb/DayOff.php (1)
readByScopeId(68-97)
zmsdb/src/Zmsdb/DayOff.php (3)
zmsdb/src/Zmsdb/Application.php (1)
Application(19-100)zmsdb/src/Zmsdb/Closure.php (1)
readByScopeId(12-39)zmsentities/src/Zmsentities/Collection/Base.php (1)
addList(145-151)
zmsdb/src/Zmsdb/Scope.php (8)
zmsdb/src/Zmsdb/Application.php (1)
Application(19-100)zmsdb/src/Zmsdb/Cluster.php (3)
readResolvedReferences(53-60)readByScopeId(98-111)readEntity(27-51)zmsdb/src/Zmsdb/Source.php (2)
readResolvedReferences(79-94)readEntity(25-49)zmsdb/src/Zmsdb/Department.php (3)
readResolvedReferences(51-62)readByScopeId(82-91)readEntity(22-49)zmsdb/src/Zmsdb/DayOff.php (2)
DayOff(9-206)readByScopeId(68-97)zmsdb/src/Zmsdb/Closure.php (3)
readByScopeId(12-39)Closure(10-107)readEntity(99-106)zmsdb/src/Zmsdb/Request.php (1)
readEntity(15-42)zmsdb/src/Zmsdb/Provider.php (1)
readEntity(11-33)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (10)
- GitHub Check: Analyze (python)
- GitHub Check: Analyze (javascript-typescript)
- GitHub Check: call-php-code-quality / module-code-quality (zmsmessaging, 8.3)
- GitHub Check: call-php-code-quality / module-code-quality (zmsadmin, 8.3)
- GitHub Check: call-php-code-quality / module-code-quality (zmsstatistic, 8.3)
- GitHub Check: call-php-code-quality / module-code-quality (zmsticketprinter, 8.3)
- GitHub Check: call-php-code-quality / module-code-quality (zmsdldb, 8.3)
- GitHub Check: call-php-unit-tests / zmsapi-test
- GitHub Check: call-php-unit-tests / zmsdb-test
- GitHub Check: build (zmscitizenview)
🔇 Additional comments (9)
zmsdb/src/Zmsdb/Closure.php (2)
5-5: LGTM!The import alias follows the established pattern across other files in the codebase.
14-21: LGTM!The cache read logic properly guards against null cache and respects the
$disableCacheflag. The early return pattern is clean and efficient.zmsdb/src/Zmsdb/Scope.php (2)
7-7: LGTM!The import statement correctly adds the Application class alias needed for the caching functionality.
449-449: LGTM!Correctly passes
truefor$disableCacheto ensure fresh data is returned after the update operation.zmsdb/src/Zmsdb/Cluster.php (3)
5-5: LGTM!The import alias enables access to the static cache instance. This follows the pattern established in other files implementing the caching layer.
27-35: LGTM!The cache read logic correctly implements the read-through pattern with proper guards:
- Checks both
$disableCacheandApp::$cachebefore accessing cache- Returns early on cache hit
- Cache key format matches the pattern used in other entities
53-60: LGTM!The signature update correctly adds the
$disableCacheparameter and threads it through to the nested scope read. The default value offalsemaintains backward compatibility with existing callers.zmsdb/src/Zmsdb/DayOff.php (2)
5-5: LGTM!The import alias enables clean access to the application-wide cache and follows the pattern used in related files.
68-79: Cache null-check order fixed correctly.The previous critical issue regarding cache access order has been resolved. The code now properly checks
!$disableCache && App::$cachebefore callingget(), preventing potential null pointer errors. The addition of the$disableCacheparameter provides a useful bypass mechanism.
zmsdb/src/Zmsdb/Closure.php
Outdated
| } | ||
| } | ||
|
|
||
| App::$cache->set($cacheKey, $closureList); |
There was a problem hiding this comment.
Critical: Unconditional cache write violates safety and parameter contract.
The cache write operation lacks essential guards:
- Null pointer risk: If
App::$cacheis null (e.g., initialization skipped in tests), this line throws a fatal error. - Contract violation: Writing to cache when
$disableCacheis true violates the explicit intent of the parameter.
The cache read at line 16 properly checks !$disableCache && App::$cache, but the write omits these guards entirely.
Apply this diff to add proper guards:
- App::$cache->set($cacheKey, $closureList);
+ if (!$disableCache && App::$cache) {
+ App::$cache->set($cacheKey, $closureList);
+ }🤖 Prompt for AI Agents
In zmsdb/src/Zmsdb/Closure.php around line 36, the unconditional cache write
violates safety and the $disableCache contract; update the code to only call
App::$cache->set($cacheKey, $closureList) when !$disableCache and App::$cache is
truthy (i.e., mirror the guard used on cache read at line 16) so you avoid null
pointer errors and respect the disable flag.
zmsdb/src/Zmsdb/Cluster.php
Outdated
| $cluster = $this->readResolvedReferences($cluster, $resolveReferences, $disableCache); | ||
| App::$cache->set($cacheKey, $cluster); | ||
|
|
||
| return $cluster; |
There was a problem hiding this comment.
Add null check before cache write to prevent fatal error.
Line 48 calls App::$cache->set() without verifying that App::$cache is not null, which could cause a fatal error if cache initialization fails. Additionally, the cache write doesn't respect the $disableCache flag, creating inconsistent behavior where cache reads are skipped but writes still occur.
Apply this diff to add proper guards:
$cluster = $this->readResolvedReferences($cluster, $resolveReferences, $disableCache);
- App::$cache->set($cacheKey, $cluster);
+ if (!$disableCache && App::$cache) {
+ App::$cache->set($cacheKey, $cluster);
+ }
return $cluster;📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| $cluster = $this->readResolvedReferences($cluster, $resolveReferences, $disableCache); | |
| App::$cache->set($cacheKey, $cluster); | |
| return $cluster; | |
| $cluster = $this->readResolvedReferences($cluster, $resolveReferences, $disableCache); | |
| if (!$disableCache && App::$cache) { | |
| App::$cache->set($cacheKey, $cluster); | |
| } | |
| return $cluster; |
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)
zmsdb/src/Zmsdb/Provider.php (1)
115-122: Fix import path: removeCache() receives wrong type and is never using writeImportEntity’s returnThis causes the pipeline TypeError and fails to invalidate correct keys.
Apply:
- foreach ($providerList as $provider) { - $this->writeImportEntity($provider, $source); - $this->removeCache($provider); - } + foreach ($providerList as $providerData) { + $providerEntity = $this->writeImportEntity($providerData, $source); + if ($providerEntity instanceof Entity) { + $this->removeCache($providerEntity); + } else { + // Fallback: evict by identifiers when entity not returned + $this->removeCacheById($source, (int) ($providerData['id'] ?? 0)); + } + }
♻️ Duplicate comments (5)
zmsdb/src/Zmsdb/Closure.php (1)
36-37: Guard cache write and honor disableCacheSet is unconditional; can NPE when cache is null and ignores disableCache flag.
- App::$cache->set($cacheKey, $closureList); + if (!$disableCache && App::$cache) { + App::$cache->set($cacheKey, $closureList); + }zmsdb/src/Zmsdb/Cluster.php (2)
438-455: Extract cache invalidation logic to reduce duplication.This method duplicates the cache invalidation pattern seen in Source.php, Request.php, Department.php, and Scope.php. Consider extracting to a shared helper to follow DRY principles. Based on coding guidelines.
48-50: Guard cache write with$disableCachecheck.The cache write occurs unconditionally, even when
$disableCacheis true. This creates inconsistent behavior where cache reads are skipped but writes still happen. Based on past review comments.Apply this diff:
- if (App::$cache) { - App::$cache->set($cacheKey, $cluster); - } + if (!$disableCache && App::$cache) { + App::$cache->set($cacheKey, $cluster); + }zmsdb/src/Zmsdb/Request.php (1)
231-263: Extract cache invalidation logic to reduce duplication.This method shares the same cache invalidation pattern with Source.php, Cluster.php, Department.php, and Scope.php. Consider extracting to a shared helper to follow DRY principles. Based on coding guidelines.
zmsdb/src/Zmsdb/Scope.php (1)
655-684: Extract cache invalidation logic to reduce duplication.This method shares the same cache invalidation pattern with Source.php, Cluster.php, Request.php, and Department.php. Consider extracting to a shared helper to follow DRY principles. Based on coding guidelines.
🧹 Nitpick comments (10)
zmsdb/src/Zmsdb/Department.php (3)
24-41: Avoid caching misses; respect disableCache on writeOnly cache successful reads. Also mirror the read guard on writes.
Apply:
- if (empty($department)) { + if (empty($department)) { $query = new Query\Department(Query\Base::SELECT); $query->addEntityMapping() ->addResolvedReferences($resolveReferences) ->addConditionDepartmentId($departmentId); $department = $this->fetchOne($query, new Entity()); - - if (App::$cache) { - App::$cache->set($cacheKey, $department); - } + if (!$disableCache && App::$cache && isset($department['id']) && $department['id']) { + App::$cache->set($cacheKey, $department); + } }
185-187: Creation path: removeCache uses unsaved entity idOn insert, $entity usually has id=0; invalidation is ineffective.
Either drop this invalidation or use the new id:
- $this->removeCache($entity); + // Optional: no invalidation needed for create unless list caches exist + // If you keep it, use the persisted id: + // $persisted = $this->readEntity($lastInsertId, 0, true); + // $this->removeCache($persisted);
413-430: Prefer id-based cache invalidation APIremoveCache($department) couples call sites to entity shape and fails on null. Use an id-based variant.
Example:
- public function removeCache($department) + public function removeCacheById(int $departmentId): void { if (!App::$cache) { return; } - - if (App::$cache->has("department-$department->id-0")) { - App::$cache->delete("department-$department->id-0"); + foreach ([0,1,2] as $lvl) { + $key = "department-$departmentId-$lvl"; + if (App::$cache->has($key)) { + App::$cache->delete($key); + } } - - if (App::$cache->has("department-$department->id-1")) { - App::$cache->delete("department-$department->id-1"); - } - - if (App::$cache->has("department-$department->id-2")) { - App::$cache->delete("department-$department->id-2"); - } }Then call removeCacheById($departmentId) where appropriate.
zmsdb/src/Zmsdb/DayOff.php (4)
52-57: Use has() for cache existence checkPrefer has() over get()+empty() for clarity and to avoid edge cases.
- if (!$disableCache && App::$cache) { - $data = App::$cache->get($cacheKey); - if (!empty($data)) { - return $data; - } - } + if (!$disableCache && App::$cache && App::$cache->has($cacheKey)) { + return App::$cache->get($cacheKey); + }
83-90: Propagate disableCache and use has() for scope cache check
- Pass $disableCache into readCommon() for consistent bypass.
- Prefer has() for existence check.
- $dayOffListCommon = $this->readCommon(); + $dayOffListCommon = $this->readCommon($disableCache); @@ - if (!$disableCache && App::$cache) { - $data = App::$cache->get($cacheKey); - if (!empty($data)) { - return $dayOffListCommon->addList($data); - } - } + if (!$disableCache && App::$cache && App::$cache->has($cacheKey)) { + return $dayOffListCommon->addList(App::$cache->get($cacheKey)); + }
105-109: Respect disableCache when writing cacheAlign set() with the bypass flag.
- if (App::$cache) { + if (!$disableCache && App::$cache) { App::$cache->set($cacheKey, $dayOffList); }
227-236: Add per-scope cache invalidation helperAdd a helper to evict "dayOffsByScope-$scopeId" and call it from write/delete operations that affect scope-specific dayoffs.
Example:
public function removeCache() { if (!App::$cache) { return; } if (App::$cache->has("dayOffs")) { App::$cache->delete("dayOffs"); } } + + public function removeCacheByScope(int $scopeId): void + { + if (!App::$cache) { + return; + } + $key = "dayOffsByScope-$scopeId"; + if (App::$cache->has($key)) { + App::$cache->delete($key); + } + }Then call removeCacheByScope() from callers that modify scope-related dayoffs (e.g., after writeDepartmentDayoffs).
zmsdb/src/Zmsdb/Closure.php (1)
16-21: Use has() for cache existenceConsistency with other modules and avoids false negatives.
- if (!$disableCache && App::$cache) { - $data = App::$cache->get($cacheKey); - if (!empty($data)) { - return $data; - } - } + if (!$disableCache && App::$cache && App::$cache->has($cacheKey)) { + return App::$cache->get($cacheKey); + }zmsdb/src/Zmsdb/Source.php (1)
172-189: Extract cache invalidation logic to reduce duplication.The
removeCachemethod hardcodes resolve reference levels (0, 1, 2) and repeats this pattern across multiple files (Cluster, Request, Scope, Department). Consider extracting this to a shared helper method to follow DRY principles and make future changes easier. Based on coding guidelines.Example refactor:
// In Application.php or a new CacheHelper class public static function invalidateEntityCache(string $keyPrefix, $entityId, array $levels = [0, 1, 2]): void { if (!self::$cache) { return; } foreach ($levels as $level) { $key = "$keyPrefix-$entityId-$level"; if (self::$cache->has($key)) { self::$cache->delete($key); } } }Then use it:
public function removeCache($sourceName) { App::invalidateEntityCache('source', $sourceName); }zmsdb/src/Zmsdb/Request.php (1)
155-175: Bypass cache when reading after write for consistency.The method calls
removeCache()but then returnsreadEntity()without passingdisableCache=true. This means the read might still fetch stale cached data if another process cached it between the invalidation and read. For consistency withwriteImportEntity(line 201) and similar patterns in other files, consider bypassing the cache. Based on coding guidelines.Apply this diff:
- return $this->readEntity($entity->getSource(), $entity->getId()); + return $this->readEntity($entity->getSource(), $entity->getId(), 0, true);
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
zmsapi/src/Zmsapi/AvailabilityClosureToggle.php(1 hunks)zmsdb/src/Zmsdb/Closure.php(4 hunks)zmsdb/src/Zmsdb/Cluster.php(9 hunks)zmsdb/src/Zmsdb/Config.php(5 hunks)zmsdb/src/Zmsdb/DayOff.php(5 hunks)zmsdb/src/Zmsdb/Department.php(6 hunks)zmsdb/src/Zmsdb/Provider.php(9 hunks)zmsdb/src/Zmsdb/Request.php(8 hunks)zmsdb/src/Zmsdb/Scope.php(12 hunks)zmsdb/src/Zmsdb/Source.php(6 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
**/*
⚙️ CodeRabbit configuration file
**/*: Apply the following Clean Code guidelines to all files, as summarized by wojteklu's Clean Code gist:Code is clean if it can be understood easily – by everyone on the team. Clean code can be read and enhanced by a developer other than its original author. With understandability comes readability, changeability, extensibility and maintainability.
General rules
- Follow standard conventions.
- Keep it simple stupid. Simpler is always better. Reduce complexity as much as possible.
- Boy scout rule. Leave the campground cleaner than you found it.
- Always find root cause. Always look for the root cause of a problem.
Design rules
- Keep configurable data at high levels.
- Prefer polymorphism to if/else or switch/case.
- Separate multi-threading code.
- Prevent over-configurability.
- Use dependency injection.
- Follow Law of Demeter. A class should know only its direct dependencies.
Understandability tips
- Be consistent. If you do something a certain way, do all similar things in the same way.
- Use explanatory variables.
- Encapsulate boundary conditions. Boundary conditions are hard to keep track of. Put the processing for them in one place.
- Prefer dedicated value objects to primitive type.
- Avoid logical dependency. Don't write methods which works correctly depending on something else in the same class.
- Avoid negative conditionals.
Names rules
- Choose descriptive and unambiguous names.
- Make meaningful distinction.
- Use pronounceable names.
- Use searchable names.
- Replace magic numbers with named constants.
- Avoid encodings. Don't append prefixes or type information.
Functions rules
- Small.
- Do one thing.
- Use descriptive names.
- Prefer fewer arguments.
- Have no side effects.
- Don't use flag arguments. Split method into several independent methods that can b...
Files:
zmsdb/src/Zmsdb/Source.phpzmsdb/src/Zmsdb/DayOff.phpzmsdb/src/Zmsdb/Provider.phpzmsdb/src/Zmsdb/Closure.phpzmsdb/src/Zmsdb/Config.phpzmsdb/src/Zmsdb/Request.phpzmsdb/src/Zmsdb/Scope.phpzmsdb/src/Zmsdb/Department.phpzmsdb/src/Zmsdb/Cluster.phpzmsapi/src/Zmsapi/AvailabilityClosureToggle.php
**/*.php
⚙️ CodeRabbit configuration file
**/*.php: Flag any usage of error_log() as it should be replaced with proper Monolog logging mechanisms provided by zmsslim which should be setup in the App/Application of each module:
- For error handling: Use the proper Monolog logging framework with error levels
- For application info logs: Use the proper Monolog logging framework with info levels
- For debugging: Use a dedicated debug logger or remove debug statements
- For CLI output: Use a CLI output handler or symfony/console
- The application log levels are as follows DEBUG, INFO, NOTICE, WARNING, ERROR, CRITICAL, ALERT, and EMERGENCY
Example replacement:
// Instead of: error_log("Import failed - " . $e->getMessage()); // Use: $log->error("Import failed", ['error' => $e->getMessage()]);Flag specific logging violations:
- error_log(), var_dump(), print_r(), die(), exit() usage (except proper error logging in catch blocks)
- Any debug output that should use proper Monolog logging
- Debug constants like DEBUG = true
- Debug logging that should be removed in production
- Commented debug code that should be cleaned up
Example replacements:
// Instead of: error_log("Error occurred"); var_dump($data); die('debug point'); // Use: $log->error("Error occurred", ['context' => 'processing']); $log->debug('Data dump', ['data' => $data]); // Remove die() statements entirelyException handling should use proper logging:
// Instead of: try { $result = $this->process(); } catch (Exception $e) { error_log("Processing failed: " . $e->getMessage()); } // Use: try { $result = $this->process(); } catch (Exception $e) { $log->error("Processing failed", ['error' => $e->getMessage(), 'trace' => $e->getTraceAsString()]); } ```...
Files:
zmsdb/src/Zmsdb/Source.phpzmsdb/src/Zmsdb/DayOff.phpzmsdb/src/Zmsdb/Provider.phpzmsdb/src/Zmsdb/Closure.phpzmsdb/src/Zmsdb/Config.phpzmsdb/src/Zmsdb/Request.phpzmsdb/src/Zmsdb/Scope.phpzmsdb/src/Zmsdb/Department.phpzmsdb/src/Zmsdb/Cluster.phpzmsapi/src/Zmsapi/AvailabilityClosureToggle.php
🧬 Code graph analysis (10)
zmsdb/src/Zmsdb/Source.php (5)
zmsdb/src/Zmsdb/Application.php (1)
Application(19-100)zmsdb/src/Zmsdb/Cluster.php (4)
readEntity(27-53)readResolvedReferences(55-63)readList(82-99)removeCache(438-455)zmsdb/src/Zmsdb/Provider.php (4)
readEntity(11-34)Provider(9-197)readListBySource(51-67)removeCache(176-196)zmsdb/src/Zmsdb/Request.php (4)
readEntity(15-41)readListBySource(111-132)Request(13-264)removeCache(231-263)zmsdb/src/Zmsdb/Scope.php (6)
readEntity(21-45)readResolvedReferences(47-57)readListBySource(212-221)Scope(17-685)readList(241-270)removeCache(655-684)
zmsdb/src/Zmsdb/DayOff.php (3)
zmsdb/src/Zmsdb/Application.php (1)
Application(19-100)zmsentities/src/Zmsentities/Collection/Base.php (2)
addEntity(109-113)addList(145-151)zmsdb/src/Zmsdb/Closure.php (1)
readByScopeId(12-39)
zmsdb/src/Zmsdb/Provider.php (6)
zmsdb/src/Zmsdb/Application.php (1)
Application(19-100)zmsentities/src/Zmsentities/Provider.php (2)
Provider(5-104)getSource(64-67)zmsdb/src/Zmsdb/Query/Provider.php (4)
Provider(5-86)addConditionRequestCsv(63-77)addConditionProviderId(48-52)addConditionProviderSource(54-58)zmsentities/src/Zmsentities/Collection/ProviderList.php (1)
ProviderList(5-94)zmsdb/src/Zmsdb/Cluster.php (2)
readEntity(27-53)removeCache(438-455)zmsdb/src/Zmsdb/Request.php (2)
readEntity(15-41)removeCache(231-263)
zmsdb/src/Zmsdb/Closure.php (4)
zmsdb/src/Zmsdb/Application.php (1)
Application(19-100)zmsdb/src/Zmsdb/Query/Closure.php (2)
Closure(8-107)addConditionId(94-98)zmsentities/src/Zmsentities/Closure.php (1)
Closure(5-50)zmsdb/src/Zmsdb/DayOff.php (1)
readByScopeId(79-110)
zmsdb/src/Zmsdb/Config.php (5)
zmsdb/src/Zmsdb/Application.php (1)
Application(19-100)zmsdb/src/Zmsdb/Cluster.php (2)
readEntity(27-53)updateEntity(391-405)zmsdb/src/Zmsdb/Provider.php (1)
readEntity(11-34)zmsdb/src/Zmsdb/Scope.php (2)
readEntity(21-45)updateEntity(470-483)zmsdb/src/Zmsdb/Source.php (1)
readEntity(25-50)
zmsdb/src/Zmsdb/Request.php (5)
zmsdb/src/Zmsdb/Application.php (1)
Application(19-100)zmsdb/src/Zmsdb/Cluster.php (2)
readEntity(27-53)removeCache(438-455)zmsdb/src/Zmsdb/Department.php (2)
readEntity(22-48)removeCache(413-430)zmsdb/src/Zmsdb/Provider.php (3)
readEntity(11-34)testSource(169-174)removeCache(176-196)zmsdb/src/Zmsdb/Source.php (2)
readEntity(25-50)removeCache(172-189)
zmsdb/src/Zmsdb/Scope.php (7)
zmsdb/src/Zmsdb/Application.php (1)
Application(19-100)zmsdb/src/Zmsdb/Cluster.php (5)
readResolvedReferences(55-63)readByScopeId(101-114)readList(82-99)removeCache(438-455)readEntity(27-53)zmsdb/src/Zmsdb/Source.php (4)
readResolvedReferences(80-101)readList(57-73)removeCache(172-189)readEntity(25-50)zmsdb/src/Zmsdb/DayOff.php (3)
DayOff(9-237)readByScopeId(79-110)removeCache(227-236)zmsdb/src/Zmsdb/Closure.php (3)
readByScopeId(12-39)Closure(10-117)readEntity(109-116)zmsdb/src/Zmsdb/Provider.php (2)
removeCache(176-196)readEntity(11-34)zmsdb/src/Zmsdb/Request.php (2)
removeCache(231-263)readEntity(15-41)
zmsdb/src/Zmsdb/Department.php (3)
zmsdb/src/Zmsdb/Application.php (1)
Application(19-100)zmsdb/src/Zmsdb/Cluster.php (3)
readResolvedReferences(55-63)removeCache(438-455)readEntity(27-53)zmsdb/src/Zmsdb/Scope.php (3)
readResolvedReferences(47-57)removeCache(655-684)readEntity(21-45)
zmsdb/src/Zmsdb/Cluster.php (3)
zmsdb/src/Zmsdb/Application.php (1)
Application(19-100)zmsdb/src/Zmsdb/Department.php (3)
readEntity(22-48)readResolvedReferences(50-61)removeCache(413-430)zmsdb/src/Zmsdb/Scope.php (5)
readEntity(21-45)readResolvedReferences(47-57)Scope(17-685)readByClusterId(59-113)removeCache(655-684)
zmsapi/src/Zmsapi/AvailabilityClosureToggle.php (1)
zmsdb/src/Zmsdb/Closure.php (2)
Closure(10-117)deleteEntity(75-85)
🪛 GitHub Actions: Combined Workflow
zmsdb/src/Zmsdb/Provider.php
[error] 176-176: Uncaught TypeError: BO\Zmsdb\Provider::removeCache(): Argument #1 ($provider) must be of type BO\Zmsentities\Provider, BO\Zmsdldb\Entity\Location given.
zmsdb/src/Zmsdb/Scope.php
[error] 241-241: PHPCS: Arguments with default values must be at the end of the argument list.
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Analyze (javascript-typescript)
🔇 Additional comments (5)
zmsapi/src/Zmsapi/AvailabilityClosureToggle.php (1)
53-55: Call signature aligned with new API — OKPassing the entity to deleteEntity() matches the updated zmsdb Closure::deleteEntity signature.
zmsdb/src/Zmsdb/Config.php (3)
14-27: Config caching path looks goodGuards with has() and null-checks; set() behind App::$cache guard. LGTM.
59-64: Cache invalidation on update — OKClearing "config" after update and re-reading bypassing cache is correct.
77-79: Property mutation invalidation — OKDeleting "config" cache before replace/delete keeps reads consistent.
Also applies to: 98-102
zmsdb/src/Zmsdb/Provider.php (1)
11-17: readEntity cache path — OKhas()+get guarded by disableCache. LGTM.
| public function deleteEntity($itemId) | ||
| { | ||
| $query = new Query\DayOff(Query\Base::DELETE); | ||
| $query->addConditionDayOffId($itemId); | ||
|
|
||
| $this->removeCache(); | ||
|
|
||
| return ($this->deleteItem($query)); | ||
| } |
There was a problem hiding this comment.
Missing invalidation for per-scope caches leads to stale reads
deleteEntity() only clears the global "dayOffs" cache. Keys like "dayOffsByScope-$scopeId" remain stale.
Consider one of:
- Look up the entity’s owning scope/department and also delete "dayOffsByScope-$scopeId".
- Add removeCacheByScope(int $scopeId) and call it from write paths that know the scope(s).
| public function removeCache(Entity $provider) | ||
| { | ||
| if (!App::$cache) { | ||
| return; | ||
| } | ||
|
|
||
| $source = $provider->getSource(); | ||
| $providerId = $provider->getId(); | ||
|
|
||
| if (App::$cache->has("request-$source-$providerId-0")) { | ||
| App::$cache->delete("request-$source-$providerId-0"); | ||
| } | ||
|
|
||
| if (App::$cache->has("request-$source-$providerId-1")) { | ||
| App::$cache->delete("request-$source-$providerId-1"); | ||
| } | ||
|
|
||
| if (App::$cache->has("request-$source-$providerId-2")) { | ||
| App::$cache->delete("request-$source-$providerId-2"); | ||
| } | ||
| } |
There was a problem hiding this comment.
removeCache() deletes wrong keys and has an overly strict parameter type
- Wrong keys: it deletes request-* instead of provider-*.
- Typehint causes TypeError in import path.
Apply:
- public function removeCache(Entity $provider)
+ public function removeCache($provider): void
{
if (!App::$cache) {
return;
}
-
- $source = $provider->getSource();
- $providerId = $provider->getId();
-
- if (App::$cache->has("request-$source-$providerId-0")) {
- App::$cache->delete("request-$source-$providerId-0");
- }
-
- if (App::$cache->has("request-$source-$providerId-1")) {
- App::$cache->delete("request-$source-$providerId-1");
- }
-
- if (App::$cache->has("request-$source-$providerId-2")) {
- App::$cache->delete("request-$source-$providerId-2");
- }
+ // Resolve identifiers
+ if ($provider instanceof Entity) {
+ $source = $provider->getSource();
+ $providerId = $provider->getId();
+ } elseif (is_array($provider)) {
+ $source = $provider['source'] ?? 'dldb';
+ $providerId = (int) ($provider['id'] ?? 0);
+ } else {
+ return;
+ }
+ foreach ([0,1,2] as $lvl) {
+ $key = "provider-$source-$providerId-$lvl";
+ if (App::$cache->has($key)) {
+ App::$cache->delete($key);
+ }
+ }
}Optionally, expose a dedicated helper to invalidate by identifiers:
+ public function removeCacheById(string $source, int $providerId): void
+ {
+ if (!App::$cache) return;
+ foreach ([0,1,2] as $lvl) {
+ $key = "provider-$source-$providerId-$lvl";
+ if (App::$cache->has($key)) {
+ App::$cache->delete($key);
+ }
+ }
+ }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| public function removeCache(Entity $provider) | |
| { | |
| if (!App::$cache) { | |
| return; | |
| } | |
| $source = $provider->getSource(); | |
| $providerId = $provider->getId(); | |
| if (App::$cache->has("request-$source-$providerId-0")) { | |
| App::$cache->delete("request-$source-$providerId-0"); | |
| } | |
| if (App::$cache->has("request-$source-$providerId-1")) { | |
| App::$cache->delete("request-$source-$providerId-1"); | |
| } | |
| if (App::$cache->has("request-$source-$providerId-2")) { | |
| App::$cache->delete("request-$source-$providerId-2"); | |
| } | |
| } | |
| public function removeCache($provider): void | |
| { | |
| if (!App::$cache) { | |
| return; | |
| } | |
| // Resolve identifiers | |
| if ($provider instanceof Entity) { | |
| $source = $provider->getSource(); | |
| $providerId = $provider->getId(); | |
| } elseif (is_array($provider)) { | |
| $source = $provider['source'] ?? 'dldb'; | |
| $providerId = (int) ($provider['id'] ?? 0); | |
| } else { | |
| return; | |
| } | |
| foreach ([0, 1, 2] as $lvl) { | |
| $key = "provider-$source-$providerId-$lvl"; | |
| if (App::$cache->has($key)) { | |
| App::$cache->delete($key); | |
| } | |
| } | |
| } | |
| public function removeCacheById(string $source, int $providerId): void | |
| { | |
| if (!App::$cache) { | |
| return; | |
| } | |
| foreach ([0, 1, 2] as $lvl) { | |
| $key = "provider-$source-$providerId-$lvl"; | |
| if (App::$cache->has($key)) { | |
| App::$cache->delete($key); | |
| } | |
| } | |
| } |
🧰 Tools
🪛 GitHub Actions: Combined Workflow
[error] 176-176: Uncaught TypeError: BO\Zmsdb\Provider::removeCache(): Argument #1 ($provider) must be of type BO\Zmsentities\Provider, BO\Zmsdldb\Entity\Location given.
🤖 Prompt for AI Agents
In zmsdb/src/Zmsdb/Provider.php around lines 176-196, remove the strict Entity
typehint on removeCache and change the deleted cache keys from "request-..." to
"provider-..."; update the method to accept a generic $provider (or mixed),
derive $source and $providerId safely (e.g., if $provider is an object call
getSource()/getId(), otherwise treat as identifier), and call
App::$cache->delete for "provider-$source-$providerId-0", "-1", and "-2" only
when those keys exist; optionally extract this logic into a small helper
invalidateProviderCacheByIdentifiers($source, $providerId) and call it from
removeCache.
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
zmsdb/src/Zmsdb/Provider.php (1)
51-64: Rename readListBySource’s$providerIdCsvto$requestIdCsv
Method currently uses the fourth parameter to filter by request IDs. Update the signature, conditional,addConditionRequestCsvcall, and all invocations.- public function readListBySource($source, $resolveReferences = 0, $isAssigned = null, $providerIdCsv = null) + public function readListBySource($source, $resolveReferences = 0, $isAssigned = null, $requestIdCsv = null) { … - if (null !== $providerIdCsv) { - $query->addConditionRequestCsv($providerIdCsv, $source); + if (null !== $requestIdCsv) { + $query->addConditionRequestCsv($requestIdCsv, $source); }Run
rg -nP '\breadListBySource\s*\('to update tests, API wrappers, and helper code.
♻️ Duplicate comments (2)
zmsdb/src/Zmsdb/Provider.php (2)
123-149: Fix import mapping (country/region) and return a Provider entity (not mixed array/entity)
- contact__country and contact__region are set from city → data corruption.
- Method returns array or Entity depending on branch → brittle API.
public function writeImportEntity($provider, $source = 'dldb') { if ($provider['address']['postal_code']) { $query = new Query\Provider(Query\Base::REPLACE); $query->addValues([ 'source' => $source, 'id' => $provider['id'], 'name' => $provider['name'], 'display_name' => $provider['displayName'] ?? $provider['name'], 'contact__city' => $provider['address']['city'], - 'contact__country' => $provider['address']['city'], + 'contact__country' => $provider['address']['country'] ?? '', 'contact__lat' => $provider['geo']['lat'], 'contact__lon' => $provider['geo']['lon'], 'contact__postalCode' => intval($provider['address']['postal_code']), - 'contact__region' => $provider['address']['city'], + 'contact__region' => $provider['address']['region'] ?? '', 'contact__street' => $provider['address']['street'], 'contact__streetNumber' => $provider['address']['house_number'], 'link' => ('dldb' == $source) ? 'https://service.berlin.de/standort/' . $provider['id'] . '/' : ((isset($provider['link'])) ? $provider['link'] : ''), 'data' => json_encode($provider) ]); $this->writeItem($query); - $provider = $this->readEntity($source, $provider['id'], 0, true); + $entity = $this->readEntity($source, $provider['id'], 0, true); + return $entity; } - return $provider; + return null; }
175-195: Fix invalidation: wrong cache keys and overly strict parameter type
- Deletes request-* instead of provider-*.
- Typehint causes TypeError when a non-Provider is passed during imports/cleanup.
Apply:
- public function removeCache(Entity $provider) + public function removeCache($provider): void { if (!App::$cache) { return; } - - $source = $provider->getSource(); - $providerId = $provider->getId(); - - if (App::$cache->has("request-$source-$providerId-0")) { - App::$cache->delete("request-$source-$providerId-0"); - } - - if (App::$cache->has("request-$source-$providerId-1")) { - App::$cache->delete("request-$source-$providerId-1"); - } - - if (App::$cache->has("request-$source-$providerId-2")) { - App::$cache->delete("request-$source-$providerId-2"); - } + // Resolve identifiers + if ($provider instanceof Entity) { + $source = $provider->getSource(); + $providerId = $provider->getId(); + } elseif (is_array($provider)) { + $source = $provider['source'] ?? 'dldb'; + $providerId = (int) ($provider['id'] ?? 0); + } else { + return; + } + foreach ([0, 1, 2] as $lvl) { + $key = "provider-$source-$providerId-$lvl"; + if (App::$cache->has($key)) { + App::$cache->delete($key); + } + } }
🧹 Nitpick comments (2)
zmsdb/src/Zmsdb/Provider.php (2)
110-111: Duplicate cache invalidationwriteEntity() already calls removeCache(). This second call is redundant.
- $this->removeCache($provider);
11-17: Caching read-through looks fine; consider not caching empty entitiesOptional: Align with Cluster/Source behavior and return null when entity not found to avoid caching empty shells.
Would you like a patch to add hasId() check and return null consistently?
Also applies to: 29-31
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
zmsdb/src/Zmsdb/Provider.php(8 hunks)zmsdb/src/Zmsdb/Scope.php(12 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
**/*
⚙️ CodeRabbit configuration file
**/*: Apply the following Clean Code guidelines to all files, as summarized by wojteklu's Clean Code gist:Code is clean if it can be understood easily – by everyone on the team. Clean code can be read and enhanced by a developer other than its original author. With understandability comes readability, changeability, extensibility and maintainability.
General rules
- Follow standard conventions.
- Keep it simple stupid. Simpler is always better. Reduce complexity as much as possible.
- Boy scout rule. Leave the campground cleaner than you found it.
- Always find root cause. Always look for the root cause of a problem.
Design rules
- Keep configurable data at high levels.
- Prefer polymorphism to if/else or switch/case.
- Separate multi-threading code.
- Prevent over-configurability.
- Use dependency injection.
- Follow Law of Demeter. A class should know only its direct dependencies.
Understandability tips
- Be consistent. If you do something a certain way, do all similar things in the same way.
- Use explanatory variables.
- Encapsulate boundary conditions. Boundary conditions are hard to keep track of. Put the processing for them in one place.
- Prefer dedicated value objects to primitive type.
- Avoid logical dependency. Don't write methods which works correctly depending on something else in the same class.
- Avoid negative conditionals.
Names rules
- Choose descriptive and unambiguous names.
- Make meaningful distinction.
- Use pronounceable names.
- Use searchable names.
- Replace magic numbers with named constants.
- Avoid encodings. Don't append prefixes or type information.
Functions rules
- Small.
- Do one thing.
- Use descriptive names.
- Prefer fewer arguments.
- Have no side effects.
- Don't use flag arguments. Split method into several independent methods that can b...
Files:
zmsdb/src/Zmsdb/Provider.phpzmsdb/src/Zmsdb/Scope.php
**/*.php
⚙️ CodeRabbit configuration file
**/*.php: Flag any usage of error_log() as it should be replaced with proper Monolog logging mechanisms provided by zmsslim which should be setup in the App/Application of each module:
- For error handling: Use the proper Monolog logging framework with error levels
- For application info logs: Use the proper Monolog logging framework with info levels
- For debugging: Use a dedicated debug logger or remove debug statements
- For CLI output: Use a CLI output handler or symfony/console
- The application log levels are as follows DEBUG, INFO, NOTICE, WARNING, ERROR, CRITICAL, ALERT, and EMERGENCY
Example replacement:
// Instead of: error_log("Import failed - " . $e->getMessage()); // Use: $log->error("Import failed", ['error' => $e->getMessage()]);Flag specific logging violations:
- error_log(), var_dump(), print_r(), die(), exit() usage (except proper error logging in catch blocks)
- Any debug output that should use proper Monolog logging
- Debug constants like DEBUG = true
- Debug logging that should be removed in production
- Commented debug code that should be cleaned up
Example replacements:
// Instead of: error_log("Error occurred"); var_dump($data); die('debug point'); // Use: $log->error("Error occurred", ['context' => 'processing']); $log->debug('Data dump', ['data' => $data]); // Remove die() statements entirelyException handling should use proper logging:
// Instead of: try { $result = $this->process(); } catch (Exception $e) { error_log("Processing failed: " . $e->getMessage()); } // Use: try { $result = $this->process(); } catch (Exception $e) { $log->error("Processing failed", ['error' => $e->getMessage(), 'trace' => $e->getTraceAsString()]); } ```...
Files:
zmsdb/src/Zmsdb/Provider.phpzmsdb/src/Zmsdb/Scope.php
🧬 Code graph analysis (2)
zmsdb/src/Zmsdb/Provider.php (7)
zmsapi/src/Zmsapi/Application.php (1)
Application(25-153)zmsdb/src/Zmsdb/Application.php (1)
Application(19-100)zmsdb/src/Zmsdb/Scope.php (2)
readEntity(21-45)removeCache(655-684)zmsdb/src/Zmsdb/Cluster.php (2)
readEntity(27-53)removeCache(438-455)zmsdb/src/Zmsdb/Source.php (2)
readEntity(25-50)removeCache(172-189)zmsdb/src/Zmsdb/Request.php (2)
readEntity(15-41)removeCache(231-263)zmsdb/src/Zmsdb/Department.php (2)
readEntity(22-48)removeCache(413-430)
zmsdb/src/Zmsdb/Scope.php (8)
zmsdb/src/Zmsdb/Application.php (1)
Application(19-100)zmsdb/src/Zmsdb/Cluster.php (4)
readResolvedReferences(55-63)readByScopeId(101-114)removeCache(438-455)readEntity(27-53)zmsdb/src/Zmsdb/Source.php (3)
readResolvedReferences(80-101)removeCache(172-189)readEntity(25-50)zmsdb/src/Zmsdb/DayOff.php (3)
DayOff(9-237)readByScopeId(79-110)removeCache(227-236)zmsdb/src/Zmsdb/Closure.php (3)
readByScopeId(12-39)Closure(10-117)readEntity(109-116)zmsdb/src/Zmsdb/Provider.php (2)
removeCache(175-195)readEntity(11-34)zmsdb/src/Zmsdb/Request.php (2)
removeCache(231-263)readEntity(15-41)zmsdb/src/Zmsdb/Config.php (1)
readEntity(14-30)
zmsdb/src/Zmsdb/Scope.php
Outdated
| if (App::$cache->has("scope-$scope->id-0")) { | ||
| App::$cache->delete("scope-$scope->id-0"); | ||
| } | ||
|
|
||
| if (App::$cache->has("scope-$scope->id-1")) { | ||
| App::$cache->delete("scope-$scope->id-1"); | ||
| } | ||
|
|
||
| if (App::$cache->has("scope-$scope->id-2")) { | ||
| App::$cache->delete("scope-$scope->id-2"); | ||
| } |
There was a problem hiding this comment.
Fix cache key interpolation for object properties
"scope-$scope->id-*" in double quotes won’t interpolate as intended; use concatenation or braces.
- if (App::$cache->has("scope-$scope->id-0")) {
- App::$cache->delete("scope-$scope->id-0");
- }
-
- if (App::$cache->has("scope-$scope->id-1")) {
- App::$cache->delete("scope-$scope->id-1");
- }
-
- if (App::$cache->has("scope-$scope->id-2")) {
- App::$cache->delete("scope-$scope->id-2");
- }
+ foreach ([0, 1, 2] as $lvl) {
+ $key = 'scope-' . $scope->id . '-' . $lvl;
+ if (App::$cache->has($key)) {
+ App::$cache->delete($key);
+ }
+ }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if (App::$cache->has("scope-$scope->id-0")) { | |
| App::$cache->delete("scope-$scope->id-0"); | |
| } | |
| if (App::$cache->has("scope-$scope->id-1")) { | |
| App::$cache->delete("scope-$scope->id-1"); | |
| } | |
| if (App::$cache->has("scope-$scope->id-2")) { | |
| App::$cache->delete("scope-$scope->id-2"); | |
| } | |
| foreach ([0, 1, 2] as $lvl) { | |
| $key = 'scope-' . $scope->id . '-' . $lvl; | |
| if (App::$cache->has($key)) { | |
| App::$cache->delete($key); | |
| } | |
| } |
🤖 Prompt for AI Agents
In zmsdb/src/Zmsdb/Scope.php around lines 673 to 683 the cache key strings use
"scope-$scope->id-*" which does not interpolate the object property; replace
them with either concatenation (e.g. 'scope-' . $scope->id . '-0') or brace
syntax inside double quotes (e.g. "scope-{$scope->id}-0") for each of the three
keys so the correct id value is used when calling App::$cache->has and
App::$cache->delete.
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
zmsdb/src/Zmsdb/Scope.php (1)
260-268: Propagate$disableCachetoreadResolvedReferences.The
$disableCacheflag is not passed toreadResolvedReferencesat line 264, which means nested reads (DayOff, Closure) won't respect the cache bypass setting.Apply this diff:
if (count($result)) { foreach ($result as $entity) { if ($entity instanceof Entity) { - $entity = $this->readResolvedReferences($entity, $resolveReferences); + $entity = $this->readResolvedReferences($entity, $resolveReferences, $disableCache); $scopeList->addEntity($entity); } }
♻️ Duplicate comments (1)
zmsdb/src/Zmsdb/Scope.php (1)
673-683: Fix cache key interpolation for object properties.String interpolation with object properties in double quotes doesn't work as expected:
"scope-$scope->id-0"will not interpolate$scope->idcorrectly. Use concatenation or brace syntax.Apply this diff:
- if (App::$cache->has("scope-$scope->id-0")) { - App::$cache->delete("scope-$scope->id-0"); - } - - if (App::$cache->has("scope-$scope->id-1")) { - App::$cache->delete("scope-$scope->id-1"); - } - - if (App::$cache->has("scope-$scope->id-2")) { - App::$cache->delete("scope-$scope->id-2"); - } + foreach ([0, 1, 2] as $lvl) { + $key = 'scope-' . $scope->id . '-' . $lvl; + if (App::$cache->has($key)) { + App::$cache->delete($key); + } + }
🧹 Nitpick comments (4)
zmsdb/src/Zmsdb/Scope.php (4)
162-174: Consider propagating$disableCacheparameter.The
readByRequestIdmethod callsreadByProviderIdwithout a$disableCacheparameter, which means it cannot bypass caching when needed. For consistency with other read methods, consider adding a$disableCacheparameter to this method and propagating it through.Example signature:
- public function readByRequestId($requestId, $source, $resolveReferences = 0) + public function readByRequestId($requestId, $source, $resolveReferences = 0, $disableCache = false) { $scopeList = new Collection(); $providerList = (new Provider())->readListBySource($source, 0, true, $requestId); foreach ($providerList as $provider) { - $scopeListByProvider = $this->readByProviderId($provider->getId(), $resolveReferences); + $scopeListByProvider = $this->readByProviderId($provider->getId(), $resolveReferences, $disableCache);
11-14: Consider addressing suppressed warnings.The class suppresses multiple static analysis warnings including Complexity and TooManyMethods. While this is not introduced by the current changes, consider breaking down this class in future refactoring to improve maintainability.
19-19: Remove legacyself::$cachefrom Scope
The static$cacheproperty inScopeis only ever cleared (no reads) and tests reset it, while actual caching usesApp::$cache. Remove the unused$cachedeclaration and itsself::$cache = []calls inwriteEntity,updateEntity,updateEmergency, anddeleteEntityto clean up dead code.
673-683: Use explicit concatenation for cache keys
Switch from string interpolation to concatenation for clarity and to avoid interpolation pitfalls:if (App::$cache->has('scope-' . $scope->id . '-0')) { App::$cache->delete('scope-' . $scope->id . '-0'); } if (App::$cache->has('scope-' . $scope->id . '-1')) { App::$cache->delete('scope-' . $scope->id . '-1'); } if (App::$cache->has('scope-' . $scope->id . '-2')) { App::$cache->delete('scope-' . $scope->id . '-2'); }
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
zmsdb/src/Zmsdb/Scope.php(12 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
**/*
⚙️ CodeRabbit configuration file
**/*: Apply the following Clean Code guidelines to all files, as summarized by wojteklu's Clean Code gist:Code is clean if it can be understood easily – by everyone on the team. Clean code can be read and enhanced by a developer other than its original author. With understandability comes readability, changeability, extensibility and maintainability.
General rules
- Follow standard conventions.
- Keep it simple stupid. Simpler is always better. Reduce complexity as much as possible.
- Boy scout rule. Leave the campground cleaner than you found it.
- Always find root cause. Always look for the root cause of a problem.
Design rules
- Keep configurable data at high levels.
- Prefer polymorphism to if/else or switch/case.
- Separate multi-threading code.
- Prevent over-configurability.
- Use dependency injection.
- Follow Law of Demeter. A class should know only its direct dependencies.
Understandability tips
- Be consistent. If you do something a certain way, do all similar things in the same way.
- Use explanatory variables.
- Encapsulate boundary conditions. Boundary conditions are hard to keep track of. Put the processing for them in one place.
- Prefer dedicated value objects to primitive type.
- Avoid logical dependency. Don't write methods which works correctly depending on something else in the same class.
- Avoid negative conditionals.
Names rules
- Choose descriptive and unambiguous names.
- Make meaningful distinction.
- Use pronounceable names.
- Use searchable names.
- Replace magic numbers with named constants.
- Avoid encodings. Don't append prefixes or type information.
Functions rules
- Small.
- Do one thing.
- Use descriptive names.
- Prefer fewer arguments.
- Have no side effects.
- Don't use flag arguments. Split method into several independent methods that can b...
Files:
zmsdb/src/Zmsdb/Scope.php
**/*.php
⚙️ CodeRabbit configuration file
**/*.php: Flag any usage of error_log() as it should be replaced with proper Monolog logging mechanisms provided by zmsslim which should be setup in the App/Application of each module:
- For error handling: Use the proper Monolog logging framework with error levels
- For application info logs: Use the proper Monolog logging framework with info levels
- For debugging: Use a dedicated debug logger or remove debug statements
- For CLI output: Use a CLI output handler or symfony/console
- The application log levels are as follows DEBUG, INFO, NOTICE, WARNING, ERROR, CRITICAL, ALERT, and EMERGENCY
Example replacement:
// Instead of: error_log("Import failed - " . $e->getMessage()); // Use: $log->error("Import failed", ['error' => $e->getMessage()]);Flag specific logging violations:
- error_log(), var_dump(), print_r(), die(), exit() usage (except proper error logging in catch blocks)
- Any debug output that should use proper Monolog logging
- Debug constants like DEBUG = true
- Debug logging that should be removed in production
- Commented debug code that should be cleaned up
Example replacements:
// Instead of: error_log("Error occurred"); var_dump($data); die('debug point'); // Use: $log->error("Error occurred", ['context' => 'processing']); $log->debug('Data dump', ['data' => $data]); // Remove die() statements entirelyException handling should use proper logging:
// Instead of: try { $result = $this->process(); } catch (Exception $e) { error_log("Processing failed: " . $e->getMessage()); } // Use: try { $result = $this->process(); } catch (Exception $e) { $log->error("Processing failed", ['error' => $e->getMessage(), 'trace' => $e->getTraceAsString()]); } ```...
Files:
zmsdb/src/Zmsdb/Scope.php
🧬 Code graph analysis (1)
zmsdb/src/Zmsdb/Scope.php (8)
zmsdb/src/Zmsdb/Application.php (1)
Application(19-100)zmsdb/src/Zmsdb/Cluster.php (5)
readResolvedReferences(55-63)readByScopeId(101-114)readList(82-99)removeCache(438-455)readEntity(27-53)zmsdb/src/Zmsdb/Department.php (5)
readResolvedReferences(50-61)readByScopeId(81-90)readList(63-79)removeCache(413-430)readEntity(22-48)zmsdb/src/Zmsdb/Source.php (4)
readResolvedReferences(80-101)readList(57-73)removeCache(172-189)readEntity(25-50)zmsdb/src/Zmsdb/DayOff.php (3)
DayOff(9-237)readByScopeId(79-110)removeCache(227-236)zmsdb/src/Zmsdb/Closure.php (3)
readByScopeId(12-39)Closure(10-117)readEntity(109-116)zmsdb/src/Zmsdb/Provider.php (2)
removeCache(175-195)readEntity(11-34)zmsdb/src/Zmsdb/Request.php (2)
removeCache(231-263)readEntity(15-41)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: call-php-unit-tests / zmsdb-test
- GitHub Check: call-php-unit-tests / zmsapi-test
🔇 Additional comments (7)
zmsdb/src/Zmsdb/Scope.php (7)
7-7: LGTM!The import enables clean access to the application-wide cache instance. Consistent with other files in the codebase.
39-41: LGTM!Cache write is properly guarded against null cache instance.
47-57: LGTM!The
$disableCacheparameter is properly added with a default value and correctly propagated to nestedreadByScopeIdcalls.
84-86: LGTM!The cache write correctly guards against both null cache and non-cacheable
PDOStatementobjects.
148-152: LGTM!The
$disableCacheparameter is correctly propagated toreadResolvedReferences.
457-457: LGTM!Cache invalidation is correctly invoked after all mutations. This ensures consistency between cached and persisted data.
Also applies to: 480-480, 497-497, 519-519, 542-542, 636-636, 652-652
661-671: Fix cache key interpolation for object properties.Cache keys using
"scopeReadByProviderId-$scope->getProviderId()-*"won't interpolate correctly. PHP doesn't parse method calls in double-quoted strings. Use concatenation instead.Apply this diff:
- if (App::$cache->has('scopeReadByProviderId-' . $scope->getProviderId() . '-0')) { - App::$cache->delete('scopeReadByProviderId-' . $scope->getProviderId() . '-0'); + $providerId = $scope->getProviderId(); + foreach ([0, 1, 2] as $lvl) { + $key = 'scopeReadByProviderId-' . $providerId . '-' . $lvl; + if (App::$cache->has($key)) { + App::$cache->delete($key); + } } - - if (App::$cache->has('scopeReadByProviderId-' . $scope->getProviderId() . '-1')) { - App::$cache->delete('scopeReadByProviderId-' . $scope->getProviderId() . '-1'); - } - - if (App::$cache->has('scopeReadByProviderId-' . $scope->getProviderId() . '-2')) { - App::$cache->delete('scopeReadByProviderId-' . $scope->getProviderId() . '-2'); - }Likely an incorrect or invalid review comment.
| $cacheKey = "scope-$scopeId-$resolveReferences"; | ||
|
|
||
| if (!$disableCache && App::$cache && App::$cache->has($cacheKey)) { | ||
| $scope = App::$cache->get($cacheKey); | ||
| } | ||
|
|
||
| if (empty($scope)) { |
There was a problem hiding this comment.
Initialize $scope before use.
The variable $scope is read from cache at line 26 but is never initialized beforehand. If the cache check at line 25 fails, PHP will create $scope as null implicitly, but this is not explicit and may trigger warnings depending on error reporting levels.
Apply this diff:
public function readEntity($scopeId, $resolveReferences = 0, $disableCache = false)
{
$cacheKey = "scope-$scopeId-$resolveReferences";
+ $scope = null;
if (!$disableCache && App::$cache && App::$cache->has($cacheKey)) {
$scope = App::$cache->get($cacheKey);
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| $cacheKey = "scope-$scopeId-$resolveReferences"; | |
| if (!$disableCache && App::$cache && App::$cache->has($cacheKey)) { | |
| $scope = App::$cache->get($cacheKey); | |
| } | |
| if (empty($scope)) { | |
| public function readEntity($scopeId, $resolveReferences = 0, $disableCache = false) | |
| { | |
| $cacheKey = "scope-$scopeId-$resolveReferences"; | |
| $scope = null; | |
| if (!$disableCache && App::$cache && App::$cache->has($cacheKey)) { | |
| $scope = App::$cache->get($cacheKey); | |
| } | |
| if (empty($scope)) { | |
| // … | |
| } |
🤖 Prompt for AI Agents
In zmsdb/src/Zmsdb/Scope.php around lines 23 to 29, $scope is read from the
cache but never explicitly initialized which can trigger notices; initialize the
variable before the cache lookup (e.g. set $scope = null;) so it is defined
regardless of the cache branch, then keep the existing cache-get and empty-check
logic as-is.
| $cacheKey = "scopeReadByProviderId-$providerId-$resolveReferences"; | ||
|
|
||
| if (!$disableCache && App::$cache && App::$cache->has($cacheKey)) { | ||
| $result = App::$cache->get($cacheKey); | ||
| } | ||
|
|
||
| if (empty($result)) { |
There was a problem hiding this comment.
Initialize $result before use.
Same issue as in previous methods: $result needs explicit initialization.
Apply this diff:
{
$cacheKey = "scopeReadByProviderId-$providerId-$resolveReferences";
+ $result = null;
if (!$disableCache && App::$cache && App::$cache->has($cacheKey)) {
$result = App::$cache->get($cacheKey);
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| $cacheKey = "scopeReadByProviderId-$providerId-$resolveReferences"; | |
| if (!$disableCache && App::$cache && App::$cache->has($cacheKey)) { | |
| $result = App::$cache->get($cacheKey); | |
| } | |
| if (empty($result)) { | |
| $cacheKey = "scopeReadByProviderId-$providerId-$resolveReferences"; | |
| $result = null; | |
| if (!$disableCache && App::$cache && App::$cache->has($cacheKey)) { | |
| $result = App::$cache->get($cacheKey); | |
| } | |
| if (empty($result)) { | |
| // ... | |
| } |
🤖 Prompt for AI Agents
In zmsdb/src/Zmsdb/Scope.php around lines 117 to 123, $result is referenced
before being initialized which can cause undefined variable notices; initialize
$result (e.g., set $result = null or an empty array) before the cacheKey and
cache-check block so subsequent checks and assignments operate on a defined
variable.
| $cacheKey = "scopeReadList-$resolveReferences"; | ||
|
|
||
| if (!$disableCache && App::$cache && App::$cache->has($cacheKey)) { | ||
| $result = App::$cache->get($cacheKey); | ||
| } | ||
|
|
||
| if (empty($result)) { |
There was a problem hiding this comment.
Initialize $result before use.
Same pattern issue: $result needs explicit initialization.
Apply this diff:
{
$cacheKey = "scopeReadList-$resolveReferences";
+ $result = null;
if (!$disableCache && App::$cache && App::$cache->has($cacheKey)) {
$result = App::$cache->get($cacheKey);
}🤖 Prompt for AI Agents
In zmsdb/src/Zmsdb/Scope.php around lines 243 to 249, $result may be referenced
before assignment; ensure $result is explicitly initialized (for example to null
or an empty array) before the cache-check block so subsequent checks like
empty($result) are safe; add a single initialization statement immediately
before $cacheKey is set.
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
zmsdb/src/Zmsdb/Request.php (1)
187-202: Add cache invalidation before cache-bypassed read.While line 201 uses cache bypass to get fresh data, the old cache entries for this request are never invalidated. If
readEntity()is called elsewhere without cache bypass, it will return stale data. Add aremoveCachecall or construct a minimal entity for invalidation.Apply this diff:
$this->writeItem($query); + + // Invalidate cache before reading fresh data + $entity = new Entity(['source' => $source, 'id' => $request['id']]); + $this->removeCache($entity); + return $this->readEntity($source, $request['id'], 0, true); }
♻️ Duplicate comments (2)
zmsdb/src/Zmsdb/Provider.php (2)
123-149: Ensure writeImportEntity always returns a Provider entity.This method may return a mixed type - either an array (the input
$providerparameter when the condition at line 125 is false) or an Entity (when the condition is true and line 146 executes). Downstream code, including cache invalidation, expects a consistent Provider entity withgetSource()andgetId()methods.Apply this diff to always return an entity or null:
public function writeImportEntity($provider, $source = 'dldb') { if ($provider['address']['postal_code']) { $query = new Query\Provider(Query\Base::REPLACE); $query->addValues([ 'source' => $source, 'id' => $provider['id'], 'name' => $provider['name'], 'display_name' => $provider['displayName'] ?? $provider['name'], 'contact__city' => $provider['address']['city'], 'contact__country' => $provider['address']['city'], 'contact__lat' => $provider['geo']['lat'], 'contact__lon' => $provider['geo']['lon'], 'contact__postalCode' => intval($provider['address']['postal_code']), 'contact__region' => $provider['address']['city'], 'contact__street' => $provider['address']['street'], 'contact__streetNumber' => $provider['address']['house_number'], 'link' => ('dldb' == $source) ? 'https://service.berlin.de/standort/' . $provider['id'] . '/' : ((isset($provider['link'])) ? $provider['link'] : ''), 'data' => json_encode($provider) ]); $this->writeItem($query); - $provider = $this->readEntity($source, $provider['id'], 0, true); + return $this->readEntity($source, $provider['id'], 0, true); } - return $provider; + return null; }
175-195: Critical: removeCache() deletes wrong cache keys and has overly strict parameter type.Two critical issues:
- Wrong keys: The method deletes
"request-$source-$providerId-{0,1,2}"keys instead of"provider-$source-$providerId-{0,1,2}"keys, breaking cache invalidation entirely.- Type error: The strict
Entitytypehint causesTypeErrorwhen called fromwriteImportEntity(line 148) which may pass an array.Apply this diff:
- public function removeCache(Entity $provider) + public function removeCache($provider): void { if (!App::$cache) { return; } - - $source = $provider->getSource(); - $providerId = $provider->getId(); - - if (App::$cache->has("request-$source-$providerId-0")) { - App::$cache->delete("request-$source-$providerId-0"); - } - - if (App::$cache->has("request-$source-$providerId-1")) { - App::$cache->delete("request-$source-$providerId-1"); - } - - if (App::$cache->has("request-$source-$providerId-2")) { - App::$cache->delete("request-$source-$providerId-2"); - } + + // Resolve identifiers from entity or array + if ($provider instanceof Entity) { + $source = $provider->getSource(); + $providerId = $provider->getId(); + } elseif (is_array($provider)) { + $source = $provider['source'] ?? 'dldb'; + $providerId = (int) ($provider['id'] ?? 0); + } else { + return; + } + + foreach ([0, 1, 2] as $lvl) { + $key = "provider-$source-$providerId-$lvl"; + if (App::$cache->has($key)) { + App::$cache->delete($key); + } + } }
🧹 Nitpick comments (5)
zmsdb/src/Zmsdb/Provider.php (2)
51-64: Consider renaming the query method for consistency.While renaming the parameter to
$providerIdCsvimproves clarity, the query method is still namedaddConditionRequestCsv, which creates a naming inconsistency. Consider updating the query method name toaddConditionProviderCsvfor better alignment.
105-112: Remove redundant cache invalidation.Line 110 calls
removeCache($provider), butwriteEntity()at line 109 already callsremoveCache()internally. This duplicate invalidation is unnecessary.Apply this diff:
public function writeListBySource(\BO\Zmsentities\Source $source) { $this->writeDeleteListBySource($source->getSource()); foreach ($source->getProviderList() as $provider) { $this->writeEntity($provider); - $this->removeCache($provider); } return $this->readListBySource($source->getSource()); }zmsdb/src/Zmsdb/Request.php (3)
155-175: Consider using cache bypass after write for consistency.Line 174 reads the entity without cache bypass (
disableCache=true), unlike the Provider implementation (line 102 in Provider.php). For consistency and to ensure fresh data after a write, consider using cache bypass.Apply this diff:
$this->writeItem($query); $this->removeCache($entity); - return $this->readEntity($entity->getSource(), $entity->getId()); + return $this->readEntity($entity->getSource(), $entity->getId(), 0, true); }
177-185: Remove redundant cache invalidation.Line 182 calls
removeCache($request), butwriteEntity()at line 181 already callsremoveCache()internally. This duplicate invalidation is unnecessary.Apply this diff:
public function writeListBySource(\BO\Zmsentities\Source $source) { $this->writeDeleteListBySource($source->getSource()); foreach ($source->getRequestList() as $request) { $this->writeEntity($request); - $this->removeCache($request); } return $this->readListBySource($source->getSource()); }
227-259: Consider relaxing the parameter type for consistency.While the cache keys are correct for Request entities, the strict
Entitytypehint could cause issues in edge cases where the method might be called with array data. For consistency with the suggested Provider.php fix and defensive coding, consider accepting mixed types.Apply this diff:
- public function removeCache(Entity $request) + public function removeCache($request): void { if (!App::$cache) { return; } - $source = $request->getSource(); - $requestId = $request->getId(); + // Resolve identifiers from entity or array + if ($request instanceof Entity) { + $source = $request->getSource(); + $requestId = $request->getId(); + } elseif (is_array($request)) { + $source = $request['source'] ?? 'dldb'; + $requestId = $request['id'] ?? ''; + } else { + return; + } if (App::$cache->has("request-$source-$requestId-0")) { App::$cache->delete("request-$source-$requestId-0"); } if (App::$cache->has("request-$source-$requestId-1")) { App::$cache->delete("request-$source-$requestId-1"); } if (App::$cache->has("request-$source-$requestId-2")) { App::$cache->delete("request-$source-$requestId-2"); } if (App::$cache->has("requestReadListBySource-$source-0")) { App::$cache->delete("requestReadListBySource-$source-0"); } if (App::$cache->has("requestReadListBySource-$source-1")) { App::$cache->delete("requestReadListBySource-$source-1"); } if (App::$cache->has("requestReadListBySource-$source-2")) { App::$cache->delete("requestReadListBySource-$source-2"); } }
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
zmsdb/src/Zmsdb/Provider.php(7 hunks)zmsdb/src/Zmsdb/Request.php(8 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
**/*
⚙️ CodeRabbit configuration file
**/*: Apply the following Clean Code guidelines to all files, as summarized by wojteklu's Clean Code gist:Code is clean if it can be understood easily – by everyone on the team. Clean code can be read and enhanced by a developer other than its original author. With understandability comes readability, changeability, extensibility and maintainability.
General rules
- Follow standard conventions.
- Keep it simple stupid. Simpler is always better. Reduce complexity as much as possible.
- Boy scout rule. Leave the campground cleaner than you found it.
- Always find root cause. Always look for the root cause of a problem.
Design rules
- Keep configurable data at high levels.
- Prefer polymorphism to if/else or switch/case.
- Separate multi-threading code.
- Prevent over-configurability.
- Use dependency injection.
- Follow Law of Demeter. A class should know only its direct dependencies.
Understandability tips
- Be consistent. If you do something a certain way, do all similar things in the same way.
- Use explanatory variables.
- Encapsulate boundary conditions. Boundary conditions are hard to keep track of. Put the processing for them in one place.
- Prefer dedicated value objects to primitive type.
- Avoid logical dependency. Don't write methods which works correctly depending on something else in the same class.
- Avoid negative conditionals.
Names rules
- Choose descriptive and unambiguous names.
- Make meaningful distinction.
- Use pronounceable names.
- Use searchable names.
- Replace magic numbers with named constants.
- Avoid encodings. Don't append prefixes or type information.
Functions rules
- Small.
- Do one thing.
- Use descriptive names.
- Prefer fewer arguments.
- Have no side effects.
- Don't use flag arguments. Split method into several independent methods that can b...
Files:
zmsdb/src/Zmsdb/Provider.phpzmsdb/src/Zmsdb/Request.php
**/*.php
⚙️ CodeRabbit configuration file
**/*.php: Flag any usage of error_log() as it should be replaced with proper Monolog logging mechanisms provided by zmsslim which should be setup in the App/Application of each module:
- For error handling: Use the proper Monolog logging framework with error levels
- For application info logs: Use the proper Monolog logging framework with info levels
- For debugging: Use a dedicated debug logger or remove debug statements
- For CLI output: Use a CLI output handler or symfony/console
- The application log levels are as follows DEBUG, INFO, NOTICE, WARNING, ERROR, CRITICAL, ALERT, and EMERGENCY
Example replacement:
// Instead of: error_log("Import failed - " . $e->getMessage()); // Use: $log->error("Import failed", ['error' => $e->getMessage()]);Flag specific logging violations:
- error_log(), var_dump(), print_r(), die(), exit() usage (except proper error logging in catch blocks)
- Any debug output that should use proper Monolog logging
- Debug constants like DEBUG = true
- Debug logging that should be removed in production
- Commented debug code that should be cleaned up
Example replacements:
// Instead of: error_log("Error occurred"); var_dump($data); die('debug point'); // Use: $log->error("Error occurred", ['context' => 'processing']); $log->debug('Data dump', ['data' => $data]); // Remove die() statements entirelyException handling should use proper logging:
// Instead of: try { $result = $this->process(); } catch (Exception $e) { error_log("Processing failed: " . $e->getMessage()); } // Use: try { $result = $this->process(); } catch (Exception $e) { $log->error("Processing failed", ['error' => $e->getMessage(), 'trace' => $e->getTraceAsString()]); } ```...
Files:
zmsdb/src/Zmsdb/Provider.phpzmsdb/src/Zmsdb/Request.php
🧬 Code graph analysis (2)
zmsdb/src/Zmsdb/Provider.php (5)
zmsdb/src/Zmsdb/Application.php (1)
Application(19-100)zmsentities/src/Zmsentities/Provider.php (2)
Provider(5-104)getSource(64-67)zmsdb/src/Zmsdb/Query/Provider.php (2)
Provider(5-86)addConditionRequestCsv(63-77)zmsdb/src/Zmsdb/Request.php (2)
readEntity(15-41)removeCache(227-259)zmsdb/src/Zmsdb/Source.php (3)
readEntity(25-50)removeCache(172-189)Source(18-190)
zmsdb/src/Zmsdb/Request.php (8)
zmsapi/src/Zmsapi/Application.php (1)
Application(25-153)zmsdb/src/Zmsdb/Application.php (1)
Application(19-100)zmsdb/src/Zmsdb/Provider.php (3)
readEntity(11-34)testSource(168-173)removeCache(175-195)zmsdb/src/Zmsdb/Source.php (2)
readEntity(25-50)removeCache(172-189)zmsdb/src/Zmsdb/Cluster.php (2)
readEntity(27-53)removeCache(438-455)zmsdb/src/Zmsdb/Department.php (2)
readEntity(22-48)removeCache(413-430)zmsdb/src/Zmsdb/Scope.php (3)
readEntity(21-45)testSource(223-228)removeCache(655-684)zmsentities/src/Zmsentities/Request.php (2)
Request(5-83)getSource(44-47)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: build (zmscitizenview)
- GitHub Check: call-php-unit-tests / zmsdb-test
🔇 Additional comments (6)
zmsdb/src/Zmsdb/Provider.php (3)
5-5: LGTM!The Application alias follows the established pattern used consistently across the codebase for cache access.
11-34: LGTM!The caching implementation correctly:
- Guards cache access with
App::$cachecheck- Uses
has()beforeget()to avoid false negatives- Implements read-through cache pattern consistently with other modules
99-102: LGTM!Cache invalidation followed by cache-bypassed read ensures consistency and prevents stale data.
zmsdb/src/Zmsdb/Request.php (3)
5-5: LGTM!The Application alias follows the established pattern consistently.
15-41: LGTM!The caching implementation correctly guards cache access and uses
has()beforeget()to properly handle falsy cached values.
111-132: LGTM!The list caching implementation is consistent with the entity-level caching and follows best practices.
| public function writeDeleteEntity($providerId, $source) | ||
| { | ||
| $provider = $this->readEntity($source, $providerId); | ||
| $query = new Query\Provider(Query\Base::DELETE); | ||
| $query->addConditionProviderId($providerId); | ||
| $query->addConditionProviderSource($source); | ||
| $this->removeCache($provider); | ||
| return $this->deleteItem($query); | ||
| } |
There was a problem hiding this comment.
Handle non-existent provider in writeDeleteEntity.
Line 153 calls readEntity() which throws an exception if the provider doesn't exist. This prevents the deletion query from executing for non-existent providers, which could cause issues in cleanup scenarios.
Consider one of these approaches:
public function writeDeleteEntity($providerId, $source)
{
- $provider = $this->readEntity($source, $providerId);
+ try {
+ $provider = $this->readEntity($source, $providerId);
+ $this->removeCache($provider);
+ } catch (\Exception $e) {
+ // Provider doesn't exist, still proceed with deletion attempt
+ }
$query = new Query\Provider(Query\Base::DELETE);
$query->addConditionProviderId($providerId);
$query->addConditionProviderSource($source);
- $this->removeCache($provider);
return $this->deleteItem($query);
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| public function writeDeleteEntity($providerId, $source) | |
| { | |
| $provider = $this->readEntity($source, $providerId); | |
| $query = new Query\Provider(Query\Base::DELETE); | |
| $query->addConditionProviderId($providerId); | |
| $query->addConditionProviderSource($source); | |
| $this->removeCache($provider); | |
| return $this->deleteItem($query); | |
| } | |
| public function writeDeleteEntity($providerId, $source) | |
| { | |
| try { | |
| $provider = $this->readEntity($source, $providerId); | |
| $this->removeCache($provider); | |
| } catch (\Exception $e) { | |
| // Provider doesn't exist, still proceed with deletion attempt | |
| } | |
| $query = new Query\Provider(Query\Base::DELETE); | |
| $query->addConditionProviderId($providerId); | |
| $query->addConditionProviderSource($source); | |
| return $this->deleteItem($query); | |
| } |
🤖 Prompt for AI Agents
In zmsdb/src/Zmsdb/Provider.php around lines 151 to 159, readEntity() is called
and will throw if the provider does not exist, preventing the deletion query
from running; change this by calling readEntity() inside a try/catch (or by
checking for a null/false return if readEntity is changed to return null) and
only call removeCache($provider) when a provider object was successfully
returned, but always construct the DELETE Query\Provider and call
deleteItem($query) so deletions proceed even when the entity was missing; ensure
the method returns the deleteItem result and does not rethrow the not-found
exception.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
zmsdb/src/Zmsdb/Scope.php (1)
675-685: Fix cache key interpolation to avoid fatal parse error.Inside double-quoted strings PHP cannot dereference
$scope->id; this code will crash at load time. Build the keys via concatenation (or braces) before calling the cache API.Apply this diff:
- if (App::$cache->has("scope-$scope->id-0")) { - App::$cache->delete("scope-$scope->id-0"); - } - - if (App::$cache->has("scope-$scope->id-1")) { - App::$cache->delete("scope-$scope->id-1"); - } - - if (App::$cache->has("scope-$scope->id-2")) { - App::$cache->delete("scope-$scope->id-2"); - } + foreach ([0, 1, 2] as $level) { + $key = 'scope-' . $scope->id . '-' . $level; + if (App::$cache->has($key)) { + App::$cache->delete($key); + } + }
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
zmsdb/src/Zmsdb/Scope.php(12 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
**/*
⚙️ CodeRabbit configuration file
**/*: Apply the following Clean Code guidelines to all files, as summarized by wojteklu's Clean Code gist:Code is clean if it can be understood easily – by everyone on the team. Clean code can be read and enhanced by a developer other than its original author. With understandability comes readability, changeability, extensibility and maintainability.
General rules
- Follow standard conventions.
- Keep it simple stupid. Simpler is always better. Reduce complexity as much as possible.
- Boy scout rule. Leave the campground cleaner than you found it.
- Always find root cause. Always look for the root cause of a problem.
Design rules
- Keep configurable data at high levels.
- Prefer polymorphism to if/else or switch/case.
- Separate multi-threading code.
- Prevent over-configurability.
- Use dependency injection.
- Follow Law of Demeter. A class should know only its direct dependencies.
Understandability tips
- Be consistent. If you do something a certain way, do all similar things in the same way.
- Use explanatory variables.
- Encapsulate boundary conditions. Boundary conditions are hard to keep track of. Put the processing for them in one place.
- Prefer dedicated value objects to primitive type.
- Avoid logical dependency. Don't write methods which works correctly depending on something else in the same class.
- Avoid negative conditionals.
Names rules
- Choose descriptive and unambiguous names.
- Make meaningful distinction.
- Use pronounceable names.
- Use searchable names.
- Replace magic numbers with named constants.
- Avoid encodings. Don't append prefixes or type information.
Functions rules
- Small.
- Do one thing.
- Use descriptive names.
- Prefer fewer arguments.
- Have no side effects.
- Don't use flag arguments. Split method into several independent methods that can b...
Files:
zmsdb/src/Zmsdb/Scope.php
**/*.php
⚙️ CodeRabbit configuration file
**/*.php: Flag any usage of error_log() as it should be replaced with proper Monolog logging mechanisms provided by zmsslim which should be setup in the App/Application of each module:
- For error handling: Use the proper Monolog logging framework with error levels
- For application info logs: Use the proper Monolog logging framework with info levels
- For debugging: Use a dedicated debug logger or remove debug statements
- For CLI output: Use a CLI output handler or symfony/console
- The application log levels are as follows DEBUG, INFO, NOTICE, WARNING, ERROR, CRITICAL, ALERT, and EMERGENCY
Example replacement:
// Instead of: error_log("Import failed - " . $e->getMessage()); // Use: $log->error("Import failed", ['error' => $e->getMessage()]);Flag specific logging violations:
- error_log(), var_dump(), print_r(), die(), exit() usage (except proper error logging in catch blocks)
- Any debug output that should use proper Monolog logging
- Debug constants like DEBUG = true
- Debug logging that should be removed in production
- Commented debug code that should be cleaned up
Example replacements:
// Instead of: error_log("Error occurred"); var_dump($data); die('debug point'); // Use: $log->error("Error occurred", ['context' => 'processing']); $log->debug('Data dump', ['data' => $data]); // Remove die() statements entirelyException handling should use proper logging:
// Instead of: try { $result = $this->process(); } catch (Exception $e) { error_log("Processing failed: " . $e->getMessage()); } // Use: try { $result = $this->process(); } catch (Exception $e) { $log->error("Processing failed", ['error' => $e->getMessage(), 'trace' => $e->getTraceAsString()]); } ```...
Files:
zmsdb/src/Zmsdb/Scope.php
🧬 Code graph analysis (1)
zmsdb/src/Zmsdb/Scope.php (8)
zmsdb/src/Zmsdb/Application.php (1)
Application(19-100)zmsdb/src/Zmsdb/Cluster.php (5)
readResolvedReferences(55-63)readByScopeId(101-114)readList(82-99)removeCache(438-455)readEntity(27-53)zmsdb/src/Zmsdb/Source.php (4)
readResolvedReferences(80-101)readList(57-73)removeCache(172-189)readEntity(25-50)zmsdb/src/Zmsdb/Department.php (5)
readResolvedReferences(50-61)readByScopeId(81-90)readList(63-79)removeCache(413-430)readEntity(22-48)zmsdb/src/Zmsdb/DayOff.php (3)
DayOff(9-237)readByScopeId(79-110)removeCache(227-236)zmsdb/src/Zmsdb/Closure.php (3)
readByScopeId(12-39)Closure(10-117)readEntity(109-116)zmsdb/src/Zmsdb/Provider.php (2)
removeCache(175-195)readEntity(11-34)zmsdb/src/Zmsdb/Request.php (2)
removeCache(227-259)readEntity(15-41)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: call-php-unit-tests / zmsdb-test
| public function removeCache($scope) | ||
| { | ||
| if (!App::$cache) { | ||
| return; | ||
| } | ||
|
|
||
| if (isset($scope->provider)) { | ||
| if (App::$cache->has('scopeReadByProviderId-' . $scope->getProviderId() . '-0')) { | ||
| App::$cache->delete('scopeReadByProviderId-' . $scope->getProviderId() . '-0'); | ||
| } | ||
|
|
||
| if (App::$cache->has('scopeReadByProviderId-' . $scope->getProviderId() . '-1')) { | ||
| App::$cache->delete('scopeReadByProviderId-' . $scope->getProviderId() . '-1'); | ||
| } | ||
|
|
||
| if (App::$cache->has('scopeReadByProviderId-' . $scope->getProviderId() . '-2')) { | ||
| App::$cache->delete('scopeReadByProviderId-' . $scope->getProviderId() . '-2'); | ||
| } | ||
| } | ||
|
|
||
| if (App::$cache->has("scope-$scope->id-0")) { | ||
| App::$cache->delete("scope-$scope->id-0"); | ||
| } | ||
|
|
||
| if (App::$cache->has("scope-$scope->id-1")) { | ||
| App::$cache->delete("scope-$scope->id-1"); | ||
| } | ||
|
|
||
| if (App::$cache->has("scope-$scope->id-2")) { | ||
| App::$cache->delete("scope-$scope->id-2"); | ||
| } | ||
| } |
There was a problem hiding this comment.
Extend cache invalidation to cover cluster/list keys.
removeCache() only deletes provider and per-scope entries, leaving scopeReadByClusterId-* and scopeReadList-* caches stale after writes/updates/deletes. Add deletions for those keys (iterate over resolve levels as above and use the scope’s cluster identifier) so downstream reads don’t serve outdated data.
Apply this diff for the list keys, and add corresponding cluster-key invalidation using the available cluster identifier on the scope entity:
if (isset($scope->provider)) {
…
}
+ foreach ([0, 1, 2] as $level) {
+ $listKey = 'scopeReadList-' . $level;
+ if (App::$cache->has($listKey)) {
+ App::$cache->delete($listKey);
+ }
+ }(Remember to delete the scopeReadByClusterId-<clusterId>-* entries as well, using whichever accessor (getClusterId() or property) exposes the scope’s cluster id.)
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| public function removeCache($scope) | |
| { | |
| if (!App::$cache) { | |
| return; | |
| } | |
| if (isset($scope->provider)) { | |
| if (App::$cache->has('scopeReadByProviderId-' . $scope->getProviderId() . '-0')) { | |
| App::$cache->delete('scopeReadByProviderId-' . $scope->getProviderId() . '-0'); | |
| } | |
| if (App::$cache->has('scopeReadByProviderId-' . $scope->getProviderId() . '-1')) { | |
| App::$cache->delete('scopeReadByProviderId-' . $scope->getProviderId() . '-1'); | |
| } | |
| if (App::$cache->has('scopeReadByProviderId-' . $scope->getProviderId() . '-2')) { | |
| App::$cache->delete('scopeReadByProviderId-' . $scope->getProviderId() . '-2'); | |
| } | |
| } | |
| if (App::$cache->has("scope-$scope->id-0")) { | |
| App::$cache->delete("scope-$scope->id-0"); | |
| } | |
| if (App::$cache->has("scope-$scope->id-1")) { | |
| App::$cache->delete("scope-$scope->id-1"); | |
| } | |
| if (App::$cache->has("scope-$scope->id-2")) { | |
| App::$cache->delete("scope-$scope->id-2"); | |
| } | |
| } | |
| public function removeCache($scope) | |
| { | |
| if (!App::$cache) { | |
| return; | |
| } | |
| if (isset($scope->provider)) { | |
| if (App::$cache->has('scopeReadByProviderId-' . $scope->getProviderId() . '-0')) { | |
| App::$cache->delete('scopeReadByProviderId-' . $scope->getProviderId() . '-0'); | |
| } | |
| if (App::$cache->has('scopeReadByProviderId-' . $scope->getProviderId() . '-1')) { | |
| App::$cache->delete('scopeReadByProviderId-' . $scope->getProviderId() . '-1'); | |
| } | |
| if (App::$cache->has('scopeReadByProviderId-' . $scope->getProviderId() . '-2')) { | |
| App::$cache->delete('scopeReadByProviderId-' . $scope->getProviderId() . '-2'); | |
| } | |
| } | |
| foreach ([0, 1, 2] as $level) { | |
| $listKey = 'scopeReadList-' . $level; | |
| if (App::$cache->has($listKey)) { | |
| App::$cache->delete($listKey); | |
| } | |
| } | |
| if (App::$cache->has("scope-$scope->id-0")) { | |
| App::$cache->delete("scope-$scope->id-0"); | |
| } | |
| if (App::$cache->has("scope-$scope->id-1")) { | |
| App::$cache->delete("scope-$scope->id-1"); | |
| } | |
| if (App::$cache->has("scope-$scope->id-2")) { | |
| App::$cache->delete("scope-$scope->id-2"); | |
| } | |
| } |
🤖 Prompt for AI Agents
In zmsdb/src/Zmsdb/Scope.php around lines 655 to 686, removeCache currently only
deletes provider-based and per-scope cache keys but misses the
scopeReadByClusterId-* and scopeReadList-* entries; update the method to also
delete scopeReadList-<scopeId>-<level> and
scopeReadByClusterId-<clusterId>-<level> for the same resolve levels (0,1,2)
used elsewhere. Use the scope's cluster identifier via the appropriate accessor
(e.g. $scope->getClusterId() or $scope->clusterId) and iterate levels 0..2 to
call App::$cache->has(...) and App::$cache->delete(...) for each constructed
key, mirroring the existing pattern.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (5)
zmsdb/src/Zmsdb/Scope.php (5)
243-249: Initialize$resultbefore use.The variable
$resultshould be initialized before the cache check block to prevent undefined variable notices.Apply this diff:
{ $cacheKey = "scopeReadList-$resolveReferences"; + $result = null; if (!$disableCache && App::$cache && App::$cache->has($cacheKey)) { $result = App::$cache->get($cacheKey); }
117-123: Initialize$resultbefore use.The variable
$resultneeds explicit initialization before the cache check to avoid undefined variable notices.Apply this diff:
{ $cacheKey = "scopeReadByProviderId-$providerId-$resolveReferences"; + $result = null; if (!$disableCache && App::$cache && App::$cache->has($cacheKey)) { $result = App::$cache->get($cacheKey); }
655-686: Extend cache invalidation to cover cluster and list keys.The
removeCache()method only deletes provider-based and per-scope cache keys, leavingscopeReadByClusterId-*andscopeReadList-*caches stale after write/update/delete operations. This can serve outdated data on subsequent reads.Apply this diff to also invalidate cluster and list cache keys:
if (App::$cache->has("scope-$scope->id-2")) { App::$cache->delete("scope-$scope->id-2"); } + + // Invalidate cluster-based cache entries + if (isset($scope->cluster) && isset($scope->cluster->id)) { + foreach ([0, 1, 2] as $level) { + $clusterKey = 'scopeReadByClusterId-' . $scope->cluster->id . '-' . $level; + if (App::$cache->has($clusterKey)) { + App::$cache->delete($clusterKey); + } + } + } + + // Invalidate list cache entries + foreach ([0, 1, 2] as $level) { + $listKey = 'scopeReadList-' . $level; + if (App::$cache->has($listKey)) { + App::$cache->delete($listKey); + } + } }
64-70: Initialize$resultbefore use.Similar to
readEntity, the variable$resultis assigned at line 67 only when the cache check passes, but is referenced at line 70 without prior initialization.Apply this diff:
{ $cacheKey = "scopeReadByClusterId-$clusterId-$resolveReferences"; + $result = null; if (!$disableCache && App::$cache && App::$cache->has($cacheKey)) { $result = App::$cache->get($cacheKey); }
23-29: Initialize$scopebefore use.The variable
$scopeis assigned at line 26 only if the cache check passes, but line 29 references it without initialization. If the cache condition fails, this causes an undefined variable notice.Apply this diff:
public function readEntity($scopeId, $resolveReferences = 0, $disableCache = false) { $cacheKey = "scope-$scopeId-$resolveReferences"; + $scope = null; if (!$disableCache && App::$cache && App::$cache->has($cacheKey)) { $scope = App::$cache->get($cacheKey); }
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
zmsdb/src/Zmsdb/Scope.php(12 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
**/*
⚙️ CodeRabbit configuration file
**/*: Apply the following Clean Code guidelines to all files, as summarized by wojteklu's Clean Code gist:Code is clean if it can be understood easily – by everyone on the team. Clean code can be read and enhanced by a developer other than its original author. With understandability comes readability, changeability, extensibility and maintainability.
General rules
- Follow standard conventions.
- Keep it simple stupid. Simpler is always better. Reduce complexity as much as possible.
- Boy scout rule. Leave the campground cleaner than you found it.
- Always find root cause. Always look for the root cause of a problem.
Design rules
- Keep configurable data at high levels.
- Prefer polymorphism to if/else or switch/case.
- Separate multi-threading code.
- Prevent over-configurability.
- Use dependency injection.
- Follow Law of Demeter. A class should know only its direct dependencies.
Understandability tips
- Be consistent. If you do something a certain way, do all similar things in the same way.
- Use explanatory variables.
- Encapsulate boundary conditions. Boundary conditions are hard to keep track of. Put the processing for them in one place.
- Prefer dedicated value objects to primitive type.
- Avoid logical dependency. Don't write methods which works correctly depending on something else in the same class.
- Avoid negative conditionals.
Names rules
- Choose descriptive and unambiguous names.
- Make meaningful distinction.
- Use pronounceable names.
- Use searchable names.
- Replace magic numbers with named constants.
- Avoid encodings. Don't append prefixes or type information.
Functions rules
- Small.
- Do one thing.
- Use descriptive names.
- Prefer fewer arguments.
- Have no side effects.
- Don't use flag arguments. Split method into several independent methods that can b...
Files:
zmsdb/src/Zmsdb/Scope.php
**/*.php
⚙️ CodeRabbit configuration file
**/*.php: Flag any usage of error_log() as it should be replaced with proper Monolog logging mechanisms provided by zmsslim which should be setup in the App/Application of each module:
- For error handling: Use the proper Monolog logging framework with error levels
- For application info logs: Use the proper Monolog logging framework with info levels
- For debugging: Use a dedicated debug logger or remove debug statements
- For CLI output: Use a CLI output handler or symfony/console
- The application log levels are as follows DEBUG, INFO, NOTICE, WARNING, ERROR, CRITICAL, ALERT, and EMERGENCY
Example replacement:
// Instead of: error_log("Import failed - " . $e->getMessage()); // Use: $log->error("Import failed", ['error' => $e->getMessage()]);Flag specific logging violations:
- error_log(), var_dump(), print_r(), die(), exit() usage (except proper error logging in catch blocks)
- Any debug output that should use proper Monolog logging
- Debug constants like DEBUG = true
- Debug logging that should be removed in production
- Commented debug code that should be cleaned up
Example replacements:
// Instead of: error_log("Error occurred"); var_dump($data); die('debug point'); // Use: $log->error("Error occurred", ['context' => 'processing']); $log->debug('Data dump', ['data' => $data]); // Remove die() statements entirelyException handling should use proper logging:
// Instead of: try { $result = $this->process(); } catch (Exception $e) { error_log("Processing failed: " . $e->getMessage()); } // Use: try { $result = $this->process(); } catch (Exception $e) { $log->error("Processing failed", ['error' => $e->getMessage(), 'trace' => $e->getTraceAsString()]); } ```...
Files:
zmsdb/src/Zmsdb/Scope.php
🧬 Code graph analysis (1)
zmsdb/src/Zmsdb/Scope.php (8)
zmsdb/src/Zmsdb/Application.php (1)
Application(19-100)zmsdb/src/Zmsdb/Cluster.php (5)
readResolvedReferences(55-63)readByScopeId(101-114)readList(82-99)removeCache(438-455)readEntity(27-53)zmsdb/src/Zmsdb/Department.php (5)
readResolvedReferences(50-61)readByScopeId(81-90)readList(63-79)removeCache(413-430)readEntity(22-48)zmsdb/src/Zmsdb/Source.php (4)
readResolvedReferences(80-101)readList(57-73)removeCache(172-189)readEntity(25-50)zmsdb/src/Zmsdb/DayOff.php (3)
DayOff(9-237)readByScopeId(79-110)removeCache(227-236)zmsdb/src/Zmsdb/Closure.php (3)
readByScopeId(12-39)Closure(10-117)readEntity(109-116)zmsdb/src/Zmsdb/Provider.php (2)
removeCache(175-195)readEntity(11-34)zmsdb/src/Zmsdb/Request.php (2)
removeCache(227-259)readEntity(15-41)
🔇 Additional comments (4)
zmsdb/src/Zmsdb/Scope.php (4)
7-7: LGTM!The import alias for
Application as Appis clean and consistent with the caching pattern used throughout this PR.
47-57: LGTM!The addition of the
$disableCacheparameter and its propagation to nested reads (DayOff,Closure) correctly implements the cache bypass mechanism.
457-457: LGTM!Cache invalidation via
removeCache()is correctly invoked after all mutation operations (writeEntity,updateEntity,replacePreferences,updateGhostWorkstationCount,updateEmergency,deleteEntity,deletePreferences), ensuring cache consistency.Also applies to: 480-480, 497-497, 519-519, 542-542, 636-636, 652-652
675-685: LGTM!The cache key construction using string interpolation (
"scope-$scope->id-0") is correct and consistent with patterns used in other repository classes (Cluster,Department, etc.).
| if (isset($scope->provider) && isset($this->provider->id)) { | ||
| if (App::$cache->has('scopeReadByProviderId-' . $scope->getProviderId() . '-0')) { | ||
| App::$cache->delete('scopeReadByProviderId-' . $scope->getProviderId() . '-0'); | ||
| } | ||
|
|
||
| if (App::$cache->has('scopeReadByProviderId-' . $scope->getProviderId() . '-1')) { | ||
| App::$cache->delete('scopeReadByProviderId-' . $scope->getProviderId() . '-1'); | ||
| } | ||
|
|
||
| if (App::$cache->has('scopeReadByProviderId-' . $scope->getProviderId() . '-2')) { | ||
| App::$cache->delete('scopeReadByProviderId-' . $scope->getProviderId() . '-2'); | ||
| } | ||
| } |
There was a problem hiding this comment.
Fix incorrect property access in provider check.
Line 661 checks isset($this->provider->id), which incorrectly references the Scope repository class instead of the $scope entity parameter. This causes the provider-related cache invalidation to fail or behave incorrectly.
Apply this diff:
- if (isset($scope->provider) && isset($this->provider->id)) {
+ if (isset($scope->provider) && isset($scope->provider->id)) {
if (App::$cache->has('scopeReadByProviderId-' . $scope->getProviderId() . '-0')) {
App::$cache->delete('scopeReadByProviderId-' . $scope->getProviderId() . '-0');
}Or simplify to:
- if (isset($scope->provider) && isset($this->provider->id)) {
+ if (isset($scope->provider)) {
if (App::$cache->has('scopeReadByProviderId-' . $scope->getProviderId() . '-0')) {since getProviderId() is called anyway.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if (isset($scope->provider) && isset($this->provider->id)) { | |
| if (App::$cache->has('scopeReadByProviderId-' . $scope->getProviderId() . '-0')) { | |
| App::$cache->delete('scopeReadByProviderId-' . $scope->getProviderId() . '-0'); | |
| } | |
| if (App::$cache->has('scopeReadByProviderId-' . $scope->getProviderId() . '-1')) { | |
| App::$cache->delete('scopeReadByProviderId-' . $scope->getProviderId() . '-1'); | |
| } | |
| if (App::$cache->has('scopeReadByProviderId-' . $scope->getProviderId() . '-2')) { | |
| App::$cache->delete('scopeReadByProviderId-' . $scope->getProviderId() . '-2'); | |
| } | |
| } | |
| // In removeCache(): only invalidate when the scope entity actually has a provider | |
| if (isset($scope->provider)) { | |
| if (App::$cache->has('scopeReadByProviderId-' . $scope->getProviderId() . '-0')) { | |
| App::$cache->delete('scopeReadByProviderId-' . $scope->getProviderId() . '-0'); | |
| } | |
| if (App::$cache->has('scopeReadByProviderId-' . $scope->getProviderId() . '-1')) { | |
| App::$cache->delete('scopeReadByProviderId-' . $scope->getProviderId() . '-1'); | |
| } | |
| if (App::$cache->has('scopeReadByProviderId-' . $scope->getProviderId() . '-2')) { | |
| App::$cache->delete('scopeReadByProviderId-' . $scope->getProviderId() . '-2'); | |
| } | |
| } |
🤖 Prompt for AI Agents
In zmsdb/src/Zmsdb/Scope.php around lines 661-673, the condition uses
isset($this->provider->id) which incorrectly references the repository instead
of the passed $scope entity; change that to check the entity (either use
isset($scope->provider->id) or simply check if $scope->getProviderId() !== null)
so the provider-related cache invalidation runs against the correct object, then
keep the existing cache delete calls using $scope->getProviderId().
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
zmsdb/src/Zmsdb/Request.php (1)
15-41: Initialize$requestbefore use.Similar to Department.php, the variable
$requestshould be initialized to avoid potential undefined variable notices.Apply this diff:
public function readEntity($source, $requestId, $resolveReferences = 0, $disableCache = false) { $cacheKey = "request-$source-$requestId-$resolveReferences"; + $request = null; if (!$disableCache && App::$cache && App::$cache->has($cacheKey)) { return App::$cache->get($cacheKey); }Note: Since this method returns early on cache hit, the initialization may not be strictly necessary here, but it's good defensive practice.
zmsdb/src/Zmsdb/Scope.php (1)
21-45: Initialize$scopebefore use.The variable
$scopeis not initialized before line 26, which can cause undefined variable notices when the cache check doesn't execute.Apply this diff:
public function readEntity($scopeId, $resolveReferences = 0, $disableCache = false) { $cacheKey = "scope-$scopeId-$resolveReferences"; + $scope = null; if (!$disableCache && App::$cache && App::$cache->has($cacheKey)) { $scope = App::$cache->get($cacheKey); }
♻️ Duplicate comments (5)
zmsdb/src/Zmsdb/Scope.php (4)
115-160: Initialize$resultbefore use.Same issue as
readByClusterId:$resultneeds initialization.Apply this diff:
public function readByProviderId($providerId, $resolveReferences = 0, $disableCache = false) { $cacheKey = "scopeReadByProviderId-$providerId-$resolveReferences"; + $result = null; if (!$disableCache && App::$cache && App::$cache->has($cacheKey)) { $result = App::$cache->get($cacheKey); }
241-270: Initialize$resultbefore use.Same pattern:
$resultneeds initialization before the cache check.Apply this diff:
public function readList($resolveReferences = 0, $disableCache = false) { $cacheKey = "scopeReadList-$resolveReferences"; + $result = null; if (!$disableCache && App::$cache && App::$cache->has($cacheKey)) { $result = App::$cache->get($cacheKey); }
59-113: Initialize$resultbefore use.The variable
$resultis not initialized before line 67, which can cause undefined variable notices.Apply this diff:
public function readByClusterId( $clusterId, $resolveReferences = 0, $disableCache = false ) { $cacheKey = "scopeReadByClusterId-$clusterId-$resolveReferences"; + $result = null; if (!$disableCache && App::$cache && App::$cache->has($cacheKey)) { $result = App::$cache->get($cacheKey); }
655-688: Critical: Fix incorrect property access in removeCache.Line 661 accesses
$this->provider->id(the repository class property) instead of$scope->provider->id(the entity parameter property). This breaks provider-related cache invalidation.Apply this diff:
- if (isset($scope->provider) && isset($this->provider->id)) { + if (isset($scope->provider) && isset($scope->provider->id)) { if (App::$cache->has('scopeReadByProviderId-' . $scope->getProviderId() . '-0')) { App::$cache->delete('scopeReadByProviderId-' . $scope->getProviderId() . '-0'); }Or simplify since
getProviderId()is called anyway:- if (isset($scope->provider) && isset($this->provider->id)) { + if (isset($scope->provider)) { if (App::$cache->has('scopeReadByProviderId-' . $scope->getProviderId() . '-0')) {zmsdb/src/Zmsdb/Provider.php (1)
175-195: Critical: Fix copy-paste errors in removeCache.Two critical bugs prevent proper cache invalidation:
- Line 177 checks
$department->idinstead of$provider->id- Lines 184-194 delete
request-*keys instead ofprovider-*keysApply this diff:
public function removeCache(Entity $provider) { - if (!App::$cache || !isset($department->id)) { + if (!App::$cache || !isset($provider->id)) { return; } $source = $provider->getSource(); $providerId = $provider->getId(); - if (App::$cache->has("request-$source-$providerId-0")) { - App::$cache->delete("request-$source-$providerId-0"); + if (App::$cache->has("provider-$source-$providerId-0")) { + App::$cache->delete("provider-$source-$providerId-0"); } - if (App::$cache->has("request-$source-$providerId-1")) { - App::$cache->delete("request-$source-$providerId-1"); + if (App::$cache->has("provider-$source-$providerId-1")) { + App::$cache->delete("provider-$source-$providerId-1"); } - if (App::$cache->has("request-$source-$providerId-2")) { - App::$cache->delete("request-$source-$providerId-2"); + if (App::$cache->has("provider-$source-$providerId-2")) { + App::$cache->delete("provider-$source-$providerId-2"); } }
🧹 Nitpick comments (1)
zmsdb/src/Zmsdb/Department.php (1)
413-430: Fix copy-paste error: use$departmentconsistently.Line 415 references
$department->idbut the parameter is named$department, making this check correct by accident. However, looking at the pattern in other removeCache methods (Cluster.php line 440, etc.), the intent is clear. The implementation is actually correct but could add a null check for safety.Consider adding an explicit null check for defensive programming:
public function removeCache($department) { - if (!App::$cache || !isset($department->id)) { + if (!App::$cache || !$department || !isset($department->id)) { return; }
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
zmsdb/src/Zmsdb/Cluster.php(9 hunks)zmsdb/src/Zmsdb/Department.php(6 hunks)zmsdb/src/Zmsdb/Provider.php(7 hunks)zmsdb/src/Zmsdb/Request.php(8 hunks)zmsdb/src/Zmsdb/Scope.php(12 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
**/*
⚙️ CodeRabbit configuration file
**/*: Apply the following Clean Code guidelines to all files, as summarized by wojteklu's Clean Code gist:Code is clean if it can be understood easily – by everyone on the team. Clean code can be read and enhanced by a developer other than its original author. With understandability comes readability, changeability, extensibility and maintainability.
General rules
- Follow standard conventions.
- Keep it simple stupid. Simpler is always better. Reduce complexity as much as possible.
- Boy scout rule. Leave the campground cleaner than you found it.
- Always find root cause. Always look for the root cause of a problem.
Design rules
- Keep configurable data at high levels.
- Prefer polymorphism to if/else or switch/case.
- Separate multi-threading code.
- Prevent over-configurability.
- Use dependency injection.
- Follow Law of Demeter. A class should know only its direct dependencies.
Understandability tips
- Be consistent. If you do something a certain way, do all similar things in the same way.
- Use explanatory variables.
- Encapsulate boundary conditions. Boundary conditions are hard to keep track of. Put the processing for them in one place.
- Prefer dedicated value objects to primitive type.
- Avoid logical dependency. Don't write methods which works correctly depending on something else in the same class.
- Avoid negative conditionals.
Names rules
- Choose descriptive and unambiguous names.
- Make meaningful distinction.
- Use pronounceable names.
- Use searchable names.
- Replace magic numbers with named constants.
- Avoid encodings. Don't append prefixes or type information.
Functions rules
- Small.
- Do one thing.
- Use descriptive names.
- Prefer fewer arguments.
- Have no side effects.
- Don't use flag arguments. Split method into several independent methods that can b...
Files:
zmsdb/src/Zmsdb/Cluster.phpzmsdb/src/Zmsdb/Request.phpzmsdb/src/Zmsdb/Scope.phpzmsdb/src/Zmsdb/Provider.phpzmsdb/src/Zmsdb/Department.php
**/*.php
⚙️ CodeRabbit configuration file
**/*.php: Flag any usage of error_log() as it should be replaced with proper Monolog logging mechanisms provided by zmsslim which should be setup in the App/Application of each module:
- For error handling: Use the proper Monolog logging framework with error levels
- For application info logs: Use the proper Monolog logging framework with info levels
- For debugging: Use a dedicated debug logger or remove debug statements
- For CLI output: Use a CLI output handler or symfony/console
- The application log levels are as follows DEBUG, INFO, NOTICE, WARNING, ERROR, CRITICAL, ALERT, and EMERGENCY
Example replacement:
// Instead of: error_log("Import failed - " . $e->getMessage()); // Use: $log->error("Import failed", ['error' => $e->getMessage()]);Flag specific logging violations:
- error_log(), var_dump(), print_r(), die(), exit() usage (except proper error logging in catch blocks)
- Any debug output that should use proper Monolog logging
- Debug constants like DEBUG = true
- Debug logging that should be removed in production
- Commented debug code that should be cleaned up
Example replacements:
// Instead of: error_log("Error occurred"); var_dump($data); die('debug point'); // Use: $log->error("Error occurred", ['context' => 'processing']); $log->debug('Data dump', ['data' => $data]); // Remove die() statements entirelyException handling should use proper logging:
// Instead of: try { $result = $this->process(); } catch (Exception $e) { error_log("Processing failed: " . $e->getMessage()); } // Use: try { $result = $this->process(); } catch (Exception $e) { $log->error("Processing failed", ['error' => $e->getMessage(), 'trace' => $e->getTraceAsString()]); } ```...
Files:
zmsdb/src/Zmsdb/Cluster.phpzmsdb/src/Zmsdb/Request.phpzmsdb/src/Zmsdb/Scope.phpzmsdb/src/Zmsdb/Provider.phpzmsdb/src/Zmsdb/Department.php
🧬 Code graph analysis (5)
zmsdb/src/Zmsdb/Cluster.php (3)
zmsdb/src/Zmsdb/Application.php (1)
Application(19-100)zmsdb/src/Zmsdb/Department.php (3)
readEntity(22-48)readResolvedReferences(50-61)removeCache(413-430)zmsdb/src/Zmsdb/Scope.php (5)
readEntity(21-45)readResolvedReferences(47-57)Scope(17-689)readByClusterId(59-113)removeCache(655-688)
zmsdb/src/Zmsdb/Request.php (7)
zmsapi/src/Zmsapi/Application.php (1)
Application(25-153)zmsdb/src/Zmsdb/Application.php (1)
Application(19-100)zmsdb/src/Zmsdb/Cluster.php (2)
readEntity(27-53)removeCache(438-455)zmsdb/src/Zmsdb/Department.php (2)
readEntity(22-48)removeCache(413-430)zmsdb/src/Zmsdb/Provider.php (3)
readEntity(11-34)testSource(168-173)removeCache(175-195)zmsdb/src/Zmsdb/Scope.php (3)
readEntity(21-45)testSource(223-228)removeCache(655-688)zmsdb/src/Zmsdb/Source.php (2)
readEntity(25-50)removeCache(172-189)
zmsdb/src/Zmsdb/Scope.php (8)
zmsdb/src/Zmsdb/Application.php (1)
Application(19-100)zmsdb/src/Zmsdb/Cluster.php (4)
readResolvedReferences(55-63)readByScopeId(101-114)removeCache(438-455)readEntity(27-53)zmsdb/src/Zmsdb/Department.php (4)
readResolvedReferences(50-61)readByScopeId(81-90)removeCache(413-430)readEntity(22-48)zmsdb/src/Zmsdb/Source.php (3)
readResolvedReferences(80-101)removeCache(172-189)readEntity(25-50)zmsdb/src/Zmsdb/DayOff.php (3)
DayOff(9-237)readByScopeId(79-110)removeCache(227-236)zmsdb/src/Zmsdb/Closure.php (3)
readByScopeId(12-39)Closure(10-117)readEntity(109-116)zmsdb/src/Zmsdb/Provider.php (2)
removeCache(175-195)readEntity(11-34)zmsdb/src/Zmsdb/Request.php (2)
removeCache(227-261)readEntity(15-41)
zmsdb/src/Zmsdb/Provider.php (5)
zmsdb/src/Zmsdb/Application.php (1)
Application(19-100)zmsentities/src/Zmsentities/Provider.php (2)
Provider(5-104)getSource(64-67)zmsdb/src/Zmsdb/Query/Provider.php (2)
Provider(5-86)addConditionRequestCsv(63-77)zmsdb/src/Zmsdb/Cluster.php (1)
readEntity(27-53)zmsdb/src/Zmsdb/Request.php (1)
readEntity(15-41)
zmsdb/src/Zmsdb/Department.php (4)
zmsdb/src/Zmsdb/Application.php (1)
Application(19-100)zmsdb/src/Zmsdb/Cluster.php (3)
readResolvedReferences(55-63)removeCache(438-455)readEntity(27-53)zmsdb/src/Zmsdb/Scope.php (3)
readResolvedReferences(47-57)removeCache(655-688)readEntity(21-45)zmsdb/src/Zmsdb/Source.php (3)
readResolvedReferences(80-101)removeCache(172-189)readEntity(25-50)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: Analyze (javascript-typescript)
- GitHub Check: call-php-unit-tests / zmsdb-test
- GitHub Check: call-php-unit-tests / zmsapi-test
🔇 Additional comments (3)
zmsdb/src/Zmsdb/Cluster.php (2)
27-53: LGTM: Proper cache implementation with initialization.The cache integration is correctly implemented:
- Variable
$clusteris initialized to null before use- Cache checks use proper
has()beforeget()- Cache is guarded with null checks before
set()- Cache bypass parameter is properly propagated
438-455: LGTM: Cache invalidation properly structured.The
removeCache()method correctly:
- Guards against null cache and missing entity ID
- Uses proper cache key format matching
readEntity()- Covers all resolution levels (0, 1, 2)
zmsdb/src/Zmsdb/Department.php (1)
22-48: Initialize$departmentbefore cache lookup.The variable
$departmentis undefined when the cache check at line 26 doesn't execute or returns false. This can cause PHP notices when line 30 evaluatesempty($department).Apply this diff:
public function readEntity($departmentId, $resolveReferences = 0, $disableCache = false) { $cacheKey = "department-$departmentId-$resolveReferences"; + $department = null; if (!$disableCache && App::$cache && App::$cache->has($cacheKey)) { $department = App::$cache->get($cacheKey); }Likely an incorrect or invalid review comment.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
zmsdb/src/Zmsdb/Provider.php (1)
175-195: CRITICAL: removeCache deletes wrong cache keys and has overly strict type.Lines 184-194 delete
"request-*"keys instead of"provider-*"keys, invalidating the wrong entity type. Additionally, the strictEntity $providertypehint causes TypeError when called with non-Entity types (as shown in the past review comment pipeline error).Apply this diff:
- public function removeCache(Entity $provider) + public function removeCache($provider): void { - if (!App::$cache || !isset($provider->id)) { + if (!App::$cache) { return; } - $source = $provider->getSource(); - $providerId = $provider->getId(); + // Resolve identifiers from entity or array + if ($provider instanceof Entity) { + $source = $provider->getSource(); + $providerId = $provider->getId(); + } elseif (is_array($provider)) { + $source = $provider['source'] ?? 'dldb'; + $providerId = (int) ($provider['id'] ?? 0); + } else { + return; + } + + if (!$providerId) { + return; + } - if (App::$cache->has("request-$source-$providerId-0")) { - App::$cache->delete("request-$source-$providerId-0"); - } - - if (App::$cache->has("request-$source-$providerId-1")) { - App::$cache->delete("request-$source-$providerId-1"); - } - - if (App::$cache->has("request-$source-$providerId-2")) { - App::$cache->delete("request-$source-$providerId-2"); + foreach ([0, 1, 2] as $level) { + $cacheKey = "provider-$source-$providerId-$level"; + if (App::$cache->has($cacheKey)) { + App::$cache->delete($cacheKey); + } } }
🧹 Nitpick comments (1)
zmsdb/src/Zmsdb/Provider.php (1)
51-67: Clarify parameter naming for request-based filtering.The parameter
$providerIdCsvis misleading since line 63 callsaddConditionRequestCsv($providerIdCsv, $source), which filters providers by request IDs (as seen in Query/Provider.php). Consider renaming to$requestIdCsvfor clarity.Apply this diff:
- public function readListBySource($source, $resolveReferences = 0, $isAssigned = null, $providerIdCsv = null) + public function readListBySource($source, $resolveReferences = 0, $isAssigned = null, $requestIdCsv = null) { $this->testSource($source); $query = new Query\Provider(Query\Base::SELECT); $query->setDistinctSelect(); $query->setResolveLevel($resolveReferences); $query->addEntityMapping(); $query->addConditionProviderSource($source); if (null !== $isAssigned) { $query->addConditionIsAssigned($isAssigned); } - if (null !== $providerIdCsv) { - $query->addConditionRequestCsv($providerIdCsv, $source); + if (null !== $requestIdCsv) { + $query->addConditionRequestCsv($requestIdCsv, $source); } $providerList = $this->readCollection($query); return ($providerList->count()) ? $providerList->sortById() : $providerList; }
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
zmsdb/src/Zmsdb/Provider.php(7 hunks)zmsdb/src/Zmsdb/Request.php(8 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
**/*
⚙️ CodeRabbit configuration file
**/*: Apply the following Clean Code guidelines to all files, as summarized by wojteklu's Clean Code gist:Code is clean if it can be understood easily – by everyone on the team. Clean code can be read and enhanced by a developer other than its original author. With understandability comes readability, changeability, extensibility and maintainability.
General rules
- Follow standard conventions.
- Keep it simple stupid. Simpler is always better. Reduce complexity as much as possible.
- Boy scout rule. Leave the campground cleaner than you found it.
- Always find root cause. Always look for the root cause of a problem.
Design rules
- Keep configurable data at high levels.
- Prefer polymorphism to if/else or switch/case.
- Separate multi-threading code.
- Prevent over-configurability.
- Use dependency injection.
- Follow Law of Demeter. A class should know only its direct dependencies.
Understandability tips
- Be consistent. If you do something a certain way, do all similar things in the same way.
- Use explanatory variables.
- Encapsulate boundary conditions. Boundary conditions are hard to keep track of. Put the processing for them in one place.
- Prefer dedicated value objects to primitive type.
- Avoid logical dependency. Don't write methods which works correctly depending on something else in the same class.
- Avoid negative conditionals.
Names rules
- Choose descriptive and unambiguous names.
- Make meaningful distinction.
- Use pronounceable names.
- Use searchable names.
- Replace magic numbers with named constants.
- Avoid encodings. Don't append prefixes or type information.
Functions rules
- Small.
- Do one thing.
- Use descriptive names.
- Prefer fewer arguments.
- Have no side effects.
- Don't use flag arguments. Split method into several independent methods that can b...
Files:
zmsdb/src/Zmsdb/Request.phpzmsdb/src/Zmsdb/Provider.php
**/*.php
⚙️ CodeRabbit configuration file
**/*.php: Flag any usage of error_log() as it should be replaced with proper Monolog logging mechanisms provided by zmsslim which should be setup in the App/Application of each module:
- For error handling: Use the proper Monolog logging framework with error levels
- For application info logs: Use the proper Monolog logging framework with info levels
- For debugging: Use a dedicated debug logger or remove debug statements
- For CLI output: Use a CLI output handler or symfony/console
- The application log levels are as follows DEBUG, INFO, NOTICE, WARNING, ERROR, CRITICAL, ALERT, and EMERGENCY
Example replacement:
// Instead of: error_log("Import failed - " . $e->getMessage()); // Use: $log->error("Import failed", ['error' => $e->getMessage()]);Flag specific logging violations:
- error_log(), var_dump(), print_r(), die(), exit() usage (except proper error logging in catch blocks)
- Any debug output that should use proper Monolog logging
- Debug constants like DEBUG = true
- Debug logging that should be removed in production
- Commented debug code that should be cleaned up
Example replacements:
// Instead of: error_log("Error occurred"); var_dump($data); die('debug point'); // Use: $log->error("Error occurred", ['context' => 'processing']); $log->debug('Data dump', ['data' => $data]); // Remove die() statements entirelyException handling should use proper logging:
// Instead of: try { $result = $this->process(); } catch (Exception $e) { error_log("Processing failed: " . $e->getMessage()); } // Use: try { $result = $this->process(); } catch (Exception $e) { $log->error("Processing failed", ['error' => $e->getMessage(), 'trace' => $e->getTraceAsString()]); } ```...
Files:
zmsdb/src/Zmsdb/Request.phpzmsdb/src/Zmsdb/Provider.php
🧬 Code graph analysis (2)
zmsdb/src/Zmsdb/Request.php (8)
zmsdb/src/Zmsdb/Application.php (1)
Application(19-100)zmsdb/src/Zmsdb/Provider.php (3)
readEntity(11-34)testSource(168-173)removeCache(175-195)zmsdb/src/Zmsdb/Cluster.php (2)
readEntity(27-53)removeCache(438-455)zmsdb/src/Zmsdb/Department.php (2)
readEntity(22-48)removeCache(413-430)zmsdb/src/Zmsdb/Scope.php (3)
readEntity(21-45)testSource(223-228)removeCache(655-688)zmsdb/src/Zmsdb/Source.php (2)
readEntity(25-50)removeCache(172-189)zmsentities/src/Zmsentities/Request.php (2)
Request(5-83)getSource(44-47)zmsdb/src/Zmsdb/Query/Request.php (3)
Request(5-107)addConditionRequestSource(83-87)addConditionRequestId(35-39)
zmsdb/src/Zmsdb/Provider.php (5)
zmsapi/src/Zmsapi/Application.php (1)
Application(25-153)zmsdb/src/Zmsdb/Application.php (1)
Application(19-100)zmsdb/src/Zmsdb/Query/Provider.php (2)
Provider(5-86)addConditionRequestCsv(63-77)zmsdb/src/Zmsdb/Request.php (3)
readEntity(15-41)readListBySource(111-132)removeCache(227-261)zmsdb/src/Zmsdb/Source.php (3)
readEntity(25-50)removeCache(172-189)Source(18-190)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (7)
- GitHub Check: call-php-build-images / build (zmsticketprinter, 8.3)
- GitHub Check: call-php-build-images / build (zmsadmin, 8.3)
- GitHub Check: call-php-build-images / build (zmsapi, 8.3)
- GitHub Check: call-php-build-images / build (zmsstatistic, 8.3)
- GitHub Check: call-php-build-images / build (zmscitizenapi, 8.3)
- GitHub Check: call-php-build-images / build (zmsmessaging, 8.3)
- GitHub Check: call-php-build-images / build (zmscalldisplay, 8.3)
| public function writeDeleteEntity($requestId, $source) | ||
| { | ||
| $query = new Query\Request(Query\Base::DELETE); | ||
| $query->addConditionRequestId($requestId); | ||
| $query->addConditionRequestSource($source); | ||
|
|
||
| return $this->deleteItem($query); | ||
| } |
There was a problem hiding this comment.
Add cache invalidation before deletion.
writeDeleteEntity should invalidate cache entries before deletion, similar to how Provider::writeDeleteEntity (lines 151-159) reads the entity and calls removeCache() before deletion. This ensures stale cache entries don't persist after a request is deleted.
Apply this diff:
public function writeDeleteEntity($requestId, $source)
{
+ try {
+ $request = $this->readEntity($source, $requestId, 0, true);
+ $this->removeCache($request);
+ } catch (\Exception $e) {
+ // Request doesn't exist, proceed with deletion anyway
+ }
$query = new Query\Request(Query\Base::DELETE);
$query->addConditionRequestId($requestId);
$query->addConditionRequestSource($source);
return $this->deleteItem($query);
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| public function writeDeleteEntity($requestId, $source) | |
| { | |
| $query = new Query\Request(Query\Base::DELETE); | |
| $query->addConditionRequestId($requestId); | |
| $query->addConditionRequestSource($source); | |
| return $this->deleteItem($query); | |
| } | |
| public function writeDeleteEntity($requestId, $source) | |
| { | |
| try { | |
| $request = $this->readEntity($source, $requestId, 0, true); | |
| $this->removeCache($request); | |
| } catch (\Exception $e) { | |
| // Request doesn't exist, proceed with deletion anyway | |
| } | |
| $query = new Query\Request(Query\Base::DELETE); | |
| $query->addConditionRequestId($requestId); | |
| $query->addConditionRequestSource($source); | |
| return $this->deleteItem($query); | |
| } |
🤖 Prompt for AI Agents
In zmsdb/src/Zmsdb/Request.php around lines 204 to 211, before constructing and
executing the delete Query, load the entity using the existing read/get method
(same approach as Provider::writeDeleteEntity at lines 151-159), call
removeCache() on that entity if it exists to invalidate any cached entries, then
proceed to build the DELETE Query and call deleteItem($query); ensure null
checks around the read result to avoid errors when entity is missing.
zmsdb/src/Zmsdb/Closure.php
Outdated
| } | ||
| } | ||
|
|
||
| App::$cache->set($cacheKey, $closureList); |
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (5)
zmsdb/src/Zmsdb/Calldisplay.php (1)
73-91: Add null safety check for $image.The method can return
nullif both database queries fail, but there's no explicit handling for this case. Callers expecting an array with image data (including the now-missingmimefield) may encounter null reference errors.Consider adding a null check and either returning a default image structure or throwing an appropriate exception.
Example fix:
if (! $image) { $image = $this->getReader() ->fetchOne((new Query\Calldisplay(Query\Base::SELECT)) ->getQueryImage(), ['name' => "logo.png"]); } + + if (! $image) { + // Return a safe default or throw an exception + return null; // Or throw new Exception\Calldisplay\ImageNotFound(); + } - //$mime = pathinfo($image['name'], PATHINFO_EXTENSION); - //$image['mime'] = ($mime == 'jpg') ? 'jpeg' : $mime; + $mime = pathinfo($image['name'], PATHINFO_EXTENSION); + $image['mime'] = ($mime == 'jpg') ? 'jpeg' : $mime; return $image;zmsdb/src/Zmsdb/Link.php (3)
16-43: Add type hints and extract cache key generation.The caching implementation is functionally correct, but has the following maintainability concerns:
- Missing type hints for
$departmentIdparameter and return type- Cache key string
"linksReadByDepartmentId-$departmentId"is duplicated inwriteEntity(line 61), violating DRYApply this diff to add type hints and extract cache key generation:
+ private function getCacheKey(int $departmentId): string + { + return "linksReadByDepartmentId-$departmentId"; + } + /** * Fetch status from db * * @return \BO\Zmsentities\Collection\LinkList */ - public function readByDepartmentId($departmentId, $disableCache = false) + public function readByDepartmentId(int $departmentId, bool $disableCache = false): Collection { - $cacheKey = "linksReadByDepartmentId-$departmentId"; + $cacheKey = $this->getCacheKey($departmentId); if (!$disableCache && App::$cache && App::$cache->has($cacheKey)) {Then update
writeEntityto use the same method:public function writeEntity(Entity $entity, $departmentId) { $query = new Query\Link(Query\Base::INSERT); $values = $query->reverseEntityMapping($entity, $departmentId); $query->addValues($values); if (App::$cache) { - App::$cache->delete("linksReadByDepartmentId-$departmentId"); + App::$cache->delete($this->getCacheKey($departmentId)); } return $this->writeItem($query); }
54-65: Add type hints to writeEntity.The cache invalidation logic is correct, but the method signature lacks type hints for consistency and type safety.
Apply this diff:
- public function writeEntity(Entity $entity, $departmentId) + public function writeEntity(Entity $entity, int $departmentId): int {
67-72: Critical: Missing cache invalidation in deleteEntity requires departmentId parameter.The
deleteEntitymethod performs a write operation without invalidating the cache. When a link is deleted viaDepartment.php:285, the cache key based ondepartmentId(set byreadByDepartmentId) is never cleared, causing deleted links to persist in cache until expiration.The calling code in
Department.php:281-285has$departmentIdavailable but cannot pass it todeleteEntity:$existingLinks = (new Link())->readByDepartmentId($departmentId); if ($existingLinks->count()) { foreach ($existingLinks as $item) { $query = new Link(); $query->deleteEntity($item->getId()); // Missing departmentId } }Required changes:
- Link.php:67 – Add
$departmentIdparameter and invalidate cache:- public function deleteEntity($itemId) + public function deleteEntity($itemId, $departmentId) { $query = new Query\Link(Query\Base::DELETE); $query->addConditionLinkId($itemId); + + if (App::$cache) { + App::$cache->delete($this->getCacheKey($departmentId)); + } + return ($this->deleteItem($query)); }
- Department.php:285 – Pass
$departmentIdto deleteEntity:- $query->deleteEntity($item->getId()); + $query->deleteEntity($item->getId(), $departmentId);zmsdb/src/Zmsdb/Department.php (1)
129-136: Use fresh data for deletion guard.Bypass cache when checking for non-empty scopes/clusters before delete.
- $entity = $this->readEntity($departmentId, 1); + $entity = $this->readEntity($departmentId, 1, true);
♻️ Duplicate comments (8)
zmsdb/src/Zmsdb/Calldisplay.php (1)
88-89: Restore MIME type detection for backward compatibility.The commented-out MIME detection code must be restored. Consumers of this method rely on
$image['mime']to set HTTP response headers (e.g.,Content-Type). Without it, clients will fail to render images correctly or fall back to incorrect defaults.This issue has been flagged multiple times in previous reviews and remains unaddressed.
Apply this diff to restore the MIME detection:
- //$mime = pathinfo($image['name'], PATHINFO_EXTENSION); - //$image['mime'] = ($mime == 'jpg') ? 'jpeg' : $mime; + $mime = pathinfo($image['name'], PATHINFO_EXTENSION); + $image['mime'] = ($mime == 'jpg') ? 'jpeg' : $mime; return $image;zmsdb/src/Zmsdb/Scope.php (5)
25-41: Init variable and guard writes with disableCache.
- Initialize
$scopebefore empty().- Don’t set cache when
$disableCacheis true.- if (!$disableCache && App::$cache && App::$cache->has($cacheKey)) { - $scope = App::$cache->get($cacheKey); - } + $scope = null; + if (!$disableCache && App::$cache && App::$cache->has($cacheKey)) { + $scope = App::$cache->get($cacheKey); + } @@ - if (App::$cache) { + if (!$disableCache && App::$cache) { App::$cache->set($cacheKey, $scope); }
66-87: Guard all cache set() calls with disableCache and init$result.Apply consistently across readByClusterId/readByProviderId/readByDepartmentId/readList.
- if (!$disableCache && App::$cache && App::$cache->has($cacheKey)) { - $result = App::$cache->get($cacheKey); - } + $result = null; + if (!$disableCache && App::$cache && App::$cache->has($cacheKey)) { + $result = App::$cache->get($cacheKey); + } @@ - if (App::$cache && !($result instanceof \PDOStatement)) { + if (!$disableCache && App::$cache && !($result instanceof \PDOStatement)) { App::$cache->set($cacheKey, $result); }Repeat the
if (!$disableCache && App::$cache) { App::$cache->set(...) }pattern at other set() sites shown above.Also applies to: 119-133, 194-196, 260-273
676-688: Critical: wrong variable in provider check prevents invalidation.You reference
$this->provider->idinstead of the entity’s provider. Fix condition.- if (isset($scope->provider) && isset($this->provider->id)) { + if (isset($scope->provider)) { if (App::$cache->has('scopeReadByProviderId-' . $scope->getProviderId() . '-0')) { App::$cache->delete('scopeReadByProviderId-' . $scope->getProviderId() . '-0'); } if (App::$cache->has('scopeReadByProviderId-' . $scope->getProviderId() . '-1')) { App::$cache->delete('scopeReadByProviderId-' . $scope->getProviderId() . '-1'); } if (App::$cache->has('scopeReadByProviderId-' . $scope->getProviderId() . '-2')) { App::$cache->delete('scopeReadByProviderId-' . $scope->getProviderId() . '-2'); } }
690-702: Critical: fix object property interpolation in cache keys.Use concatenation or braces; current strings won’t interpolate
$scope->id.- if (App::$cache->has("scope-$scope->id-0")) { - App::$cache->delete("scope-$scope->id-0"); - } - if (App::$cache->has("scope-$scope->id-1")) { - App::$cache->delete("scope-$scope->id-1"); - } - if (App::$cache->has("scope-$scope->id-2")) { - App::$cache->delete("scope-$scope->id-2"); - } + foreach ([0,1,2] as $lvl) { + $key = 'scope-' . $scope->id . '-' . $lvl; + if (App::$cache->has($key)) { + App::$cache->delete($key); + } + }
670-703: Broaden invalidation to cover list and cluster keys.Stale "scopeReadList-" and "scopeReadByClusterId--" can persist after scope changes.
public function removeCache($scope) { if (!App::$cache) { return; } @@ + // Invalidate list caches + foreach ([0,1,2] as $lvl) { + $listKey = 'scopeReadList-' . $lvl; + if (App::$cache->has($listKey)) { + App::$cache->delete($listKey); + } + } + + // Invalidate cluster list caches when clusterId is available + $clusterId = null; + if (method_exists($scope, 'getClusterId')) { + $clusterId = $scope->getClusterId(); + } elseif (isset($scope->clusterId)) { + $clusterId = $scope->clusterId; + } + if ($clusterId !== null) { + foreach ([0,1,2] as $lvl) { + $key = 'scopeReadByClusterId-' . (int)$clusterId . '-' . $lvl; + if (App::$cache->has($key)) { + App::$cache->delete($key); + } + } + }zmsdb/src/Zmsdb/Closure.php (1)
36-38: Respect disableCache on write to avoid unintended cache pollution.You still write even when
$disableCache === true. Align write with read guard.- if (App::$cache) { - App::$cache->set($cacheKey, $closureList); - } + if (!$disableCache && App::$cache) { + App::$cache->set($cacheKey, $closureList); + }zmsdb/src/Zmsdb/Cluster.php (1)
48-50: Respect disableCache on write.Skip set() when cache is bypassed.
- if (App::$cache) { + if (!$disableCache && App::$cache) { App::$cache->set($cacheKey, $cluster); }
🧹 Nitpick comments (11)
zmsdb/src/Zmsdb/Calldisplay.php (3)
9-9: Verify if @SuppressWarnings is justified.The
@SuppressWarnings(CouplingBetweenObjects)annotation suppresses coupling warnings. While this class does coordinate multiple queries (Scope, Cluster, Organisation, Department), consider whether the coupling can be reduced through refactoring (e.g., introducing a service layer or repository pattern) rather than suppressing the warning.
29-33: Remove or implement commented exception handling.The commented exception code violates clean code principles. If exception handling is needed, implement it properly. If the code is obsolete or the validation is performed elsewhere, remove the commented lines entirely.
As per coding guidelines: "Don't comment out code. Just remove."
43-47: Remove or implement commented exception handling.Similar to lines 29-33, this commented exception code should either be implemented or removed entirely.
As per coding guidelines: "Don't comment out code. Just remove."
zmsdb/src/Zmsdb/Closure.php (2)
16-21: Optional: use has() for clarity and avoid truthiness pitfalls.Minor readability win and consistent with other files.
- if (!$disableCache && App::$cache) { - $data = App::$cache->get($cacheKey); - if (!empty($data)) { - return $data; - } - } + if (!$disableCache && App::$cache && App::$cache->has($cacheKey)) { + return App::$cache->get($cacheKey); + }
12-12: Optional: avoid flag argument.Split into readByScopeId() and readByScopeIdNoCache() or introduce a small CachePolicy enum. Improves readability.
Based on coding guidelines.zmsdb/src/Zmsdb/DayOff.php (1)
31-41: Consistent cache get checks.Use has()+get() symmetrically with other modules for consistency.
- if (!$disableCache && App::$cache) { - $data = App::$cache->get($cacheKey); - if (!empty($data)) { - return $data; - } - } + if (!$disableCache && App::$cache && App::$cache->has($cacheKey)) { + return App::$cache->get($cacheKey); + }Also applies to: 62-72, 93-101
zmsdb/src/Zmsdb/Department.php (3)
24-31: Initialize $department before empty() for clarity.Avoid relying on empty() on undefined variable.
- if (!$disableCache && App::$cache && App::$cache->has($cacheKey)) { + $department = null; + if (!$disableCache && App::$cache && App::$cache->has($cacheKey)) { $department = App::$cache->get($cacheKey); }
192-193: Ensure cache is actually invalidated on write/update.removeCache($entity) may no-op if
$entity->idis not set. Prefer invalidating by ID.Option A (change calls):
- $this->removeCache($entity); + $this->removeCache((object)['id' => $lastInsertId]);- $this->removeCache($entity); + $this->removeCache((object)['id' => $departmentId]);Option B (change signature): make removeCache accept either Entity or int and branch on type.
Also applies to: 228-229
241-265: Also invalidate DayOff department cache after dayoff changes.Call DayOff::removeCacheByDepartmentId() after updating department dayoffs to prevent stale lists.
foreach ($dayoffList as $dayoff) { $query = new Query\DayOff(Query\Base::INSERT); $query->addValues( [ 'behoerdenid' => $departmentId, 'Feiertag' => $dayoff['name'], 'Datum' => (new \DateTimeImmutable('@' . $dayoff['date']))->format('Y-m-d') ] ); $this->writeItem($query); } + (new DayOff())->removeCacheByDepartmentId($departmentId);zmsdb/src/Zmsdb/Cluster.php (1)
378-381: Ensure invalidation targets correct key.removeCache($entity) may not delete anything if
$entity->idis missing. Use the ID you have.- $this->removeCache($entity); + $this->removeCache((object)['id' => $lastInsertId]);- $this->removeCache($entity); + $this->removeCache((object)['id' => $clusterId]);Also applies to: 402-405
zmsdb/src/Zmsdb/Scope.php (1)
176-201: Optional: consider caching resolveReferences=0 branch too.You currently skip caching for the “simple match” path; if intentional, ignore. Otherwise, align with other list reads.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
zmsdb/src/Zmsdb/Calldisplay.php(1 hunks)zmsdb/src/Zmsdb/Closure.php(4 hunks)zmsdb/src/Zmsdb/Cluster.php(11 hunks)zmsdb/src/Zmsdb/DayOff.php(6 hunks)zmsdb/src/Zmsdb/Department.php(6 hunks)zmsdb/src/Zmsdb/Link.php(4 hunks)zmsdb/src/Zmsdb/Provider.php(5 hunks)zmsdb/src/Zmsdb/Scope.php(14 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- zmsdb/src/Zmsdb/Provider.php
🧰 Additional context used
📓 Path-based instructions (2)
**/*
⚙️ CodeRabbit configuration file
**/*: Apply the following Clean Code guidelines to all files, as summarized by wojteklu's Clean Code gist:Code is clean if it can be understood easily – by everyone on the team. Clean code can be read and enhanced by a developer other than its original author. With understandability comes readability, changeability, extensibility and maintainability.
General rules
- Follow standard conventions.
- Keep it simple stupid. Simpler is always better. Reduce complexity as much as possible.
- Boy scout rule. Leave the campground cleaner than you found it.
- Always find root cause. Always look for the root cause of a problem.
Design rules
- Keep configurable data at high levels.
- Prefer polymorphism to if/else or switch/case.
- Separate multi-threading code.
- Prevent over-configurability.
- Use dependency injection.
- Follow Law of Demeter. A class should know only its direct dependencies.
Understandability tips
- Be consistent. If you do something a certain way, do all similar things in the same way.
- Use explanatory variables.
- Encapsulate boundary conditions. Boundary conditions are hard to keep track of. Put the processing for them in one place.
- Prefer dedicated value objects to primitive type.
- Avoid logical dependency. Don't write methods which works correctly depending on something else in the same class.
- Avoid negative conditionals.
Names rules
- Choose descriptive and unambiguous names.
- Make meaningful distinction.
- Use pronounceable names.
- Use searchable names.
- Replace magic numbers with named constants.
- Avoid encodings. Don't append prefixes or type information.
Functions rules
- Small.
- Do one thing.
- Use descriptive names.
- Prefer fewer arguments.
- Have no side effects.
- Don't use flag arguments. Split method into several independent methods that can b...
Files:
zmsdb/src/Zmsdb/Link.phpzmsdb/src/Zmsdb/Cluster.phpzmsdb/src/Zmsdb/DayOff.phpzmsdb/src/Zmsdb/Closure.phpzmsdb/src/Zmsdb/Department.phpzmsdb/src/Zmsdb/Calldisplay.phpzmsdb/src/Zmsdb/Scope.php
**/*.php
⚙️ CodeRabbit configuration file
**/*.php: Flag any usage of error_log() as it should be replaced with proper Monolog logging mechanisms provided by zmsslim which should be setup in the App/Application of each module:
- For error handling: Use the proper Monolog logging framework with error levels
- For application info logs: Use the proper Monolog logging framework with info levels
- For debugging: Use a dedicated debug logger or remove debug statements
- For CLI output: Use a CLI output handler or symfony/console
- The application log levels are as follows DEBUG, INFO, NOTICE, WARNING, ERROR, CRITICAL, ALERT, and EMERGENCY
Example replacement:
// Instead of: error_log("Import failed - " . $e->getMessage()); // Use: $log->error("Import failed", ['error' => $e->getMessage()]);Flag specific logging violations:
- error_log(), var_dump(), print_r(), die(), exit() usage (except proper error logging in catch blocks)
- Any debug output that should use proper Monolog logging
- Debug constants like DEBUG = true
- Debug logging that should be removed in production
- Commented debug code that should be cleaned up
Example replacements:
// Instead of: error_log("Error occurred"); var_dump($data); die('debug point'); // Use: $log->error("Error occurred", ['context' => 'processing']); $log->debug('Data dump', ['data' => $data]); // Remove die() statements entirelyException handling should use proper logging:
// Instead of: try { $result = $this->process(); } catch (Exception $e) { error_log("Processing failed: " . $e->getMessage()); } // Use: try { $result = $this->process(); } catch (Exception $e) { $log->error("Processing failed", ['error' => $e->getMessage(), 'trace' => $e->getTraceAsString()]); } ```...
Files:
zmsdb/src/Zmsdb/Link.phpzmsdb/src/Zmsdb/Cluster.phpzmsdb/src/Zmsdb/DayOff.phpzmsdb/src/Zmsdb/Closure.phpzmsdb/src/Zmsdb/Department.phpzmsdb/src/Zmsdb/Calldisplay.phpzmsdb/src/Zmsdb/Scope.php
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: call-php-unit-tests / zmsdb-test
🔇 Additional comments (7)
zmsdb/src/Zmsdb/Calldisplay.php (1)
93-112: LGTM!The
readContactDatamethod has clean logic and properly handles both cluster and scope lists to build the contact name.zmsdb/src/Zmsdb/Link.php (1)
5-5: LGTM - Import added for cache access.The alias import is necessary for accessing the cache facility and aligns with the caching pattern used across other zmsdb components.
zmsdb/src/Zmsdb/Department.php (2)
50-66: Good propagation of disableCache to nested reads.
420-437: Critical: fix string interpolation of object property in cache keys.Double-quoted "department-$department->id-*" does not interpolate as intended. Use concatenation or braces.
- if (App::$cache->has("department-$department->id-0")) { - App::$cache->delete("department-$department->id-0"); - } - if (App::$cache->has("department-$department->id-1")) { - App::$cache->delete("department-$department->id-1"); - } - if (App::$cache->has("department-$department->id-2")) { - App::$cache->delete("department-$department->id-2"); - } + foreach ([0,1,2] as $lvl) { + $key = 'department-' . $department->id . '-' . $lvl; + if (App::$cache->has($key)) { + App::$cache->delete($key); + } + }Likely an incorrect or invalid review comment.
zmsdb/src/Zmsdb/Cluster.php (2)
55-63: Correct propagation of disableCache to nested scope reads.
438-455: Critical: fix string interpolation in cache keys."cluster-$cluster->id-*" won’t interpolate the property. Use concatenation/brace syntax.
- if (App::$cache->has("cluster-$cluster->id-0")) { - App::$cache->delete("cluster-$cluster->id-0"); - } - if (App::$cache->has("cluster-$cluster->id-1")) { - App::$cache->delete("cluster-$cluster->id-1"); - } - if (App::$cache->has("cluster-$cluster->id-2")) { - App::$cache->delete("cluster-$cluster->id-2"); - } + foreach ([0,1,2] as $lvl) { + $key = 'cluster-' . $cluster->id . '-' . $lvl; + if (App::$cache->has($key)) { + App::$cache->delete($key); + } + }Likely an incorrect or invalid review comment.
zmsdb/src/Zmsdb/Scope.php (1)
52-57: Good: disableCache propagated to DayOff and Closure reads.
| if (App::$cache) { | ||
| App::$cache->set($cacheKey, $dayOffList); | ||
| } |
There was a problem hiding this comment.
Guard cache writes with disableCache.
Writes should be skipped when $disableCache is true, mirroring read guards.
- if (App::$cache) {
+ if (!$disableCache && App::$cache) {
App::$cache->set($cacheKey, $dayOffList);
}Apply similarly at other set() sites in this file.
Also applies to: 86-88, 119-121
🤖 Prompt for AI Agents
In zmsdb/src/Zmsdb/DayOff.php around lines 55-57 (and similarly at 86-88 and
119-121), the cache set() calls are unconditional; update each to skip writes
when $disableCache is true by wrapping the set call with a guard that checks
!$disableCache as well as App::$cache (e.g. if (!$disableCache && App::$cache) {
App::$cache->set($cacheKey, $dayOffList); }), applying the same pattern at the
other set() sites so cache writes mirror the existing read guards.
| $dayOffListCommon = $this->readCommon(); | ||
|
|
||
| if (!$disableCache && App::$cache) { | ||
| $data = App::$cache->get($cacheKey); | ||
| if (!empty($data)) { | ||
| return $dayOffListCommon->addList($data); | ||
| } | ||
| } | ||
|
|
There was a problem hiding this comment.
Critical: avoid mutating cached common list and honor disableCache.
- readCommon() ignores
$disableCache. - Returning
$dayOffListCommon->addList(...)likely mutates the cached common list, polluting future reads.
- $dayOffListCommon = $this->readCommon();
+ $dayOffListCommon = $this->readCommon($disableCache);
- if (!$disableCache && App::$cache) {
- $data = App::$cache->get($cacheKey);
- if (!empty($data)) {
- return $dayOffListCommon->addList($data);
- }
- }
+ if (!$disableCache && App::$cache && App::$cache->has($cacheKey)) {
+ $data = App::$cache->get($cacheKey);
+ $combined = new Collection();
+ $combined->addList($dayOffListCommon);
+ $combined->addList($data);
+ return $combined;
+ }
- return $dayOffListCommon->addList($dayOffList);
+ $combined = new Collection();
+ $combined->addList($dayOffListCommon);
+ $combined->addList($dayOffList);
+ return $combined;Also applies to: 123-124
🤖 Prompt for AI Agents
In zmsdb/src/Zmsdb/DayOff.php around lines 97-105 (and similarly at 123-124),
readCommon() is called without honoring $disableCache and the code returns
$dayOffListCommon->addList($data) which mutates the common list and pollutes
cached state; change the flow so that when $disableCache is true you bypass
App::$cache entirely, and when reading from cache you do not call addList on the
cached object directly — instead create a new DayOffList (or deep-clone
$dayOffListCommon) and call addList on that new instance (or merge immutably)
before returning; ensure readCommon accepts/propagates the $disableCache flag if
needed or call a non-cached variant, and never mutate the cached shared object.
| public function removeCache() | ||
| { | ||
| if (!App::$cache) { | ||
| return; | ||
| } | ||
|
|
||
| if (App::$cache->has("dayOffs")) { | ||
| App::$cache->delete("dayOffs"); | ||
| } | ||
| } |
There was a problem hiding this comment.
Extend invalidation: add per-department and per-scope removals.
Only deleting "dayOffs" leaves "dayOffsReadOnlyByDepartmentId-" and "dayOffsByScope-" stale after writes/deletes.
public function removeCache()
{
if (!App::$cache) {
return;
}
if (App::$cache->has("dayOffs")) {
App::$cache->delete("dayOffs");
}
+
+ // Optional helpers (call from write/delete paths when IDs are known)
+ // Remove per-department cache
+ // Usage: $this->removeCacheByDepartmentId($departmentId);
+ }
+
+ public function removeCacheByDepartmentId($departmentId): void
+ {
+ if (!App::$cache) { return; }
+ $key = 'dayOffsReadOnlyByDepartmentId-' . (int)$departmentId;
+ if (App::$cache->has($key)) {
+ App::$cache->delete($key);
+ }
+ }
+
+ public function removeCacheByScopeId($scopeId): void
+ {
+ if (!App::$cache) { return; }
+ $key = 'dayOffsByScope-' . (int)$scopeId;
+ if (App::$cache->has($key)) {
+ App::$cache->delete($key);
+ }
}Follow-up: call removeCacheByDepartmentId() in Department::writeDepartmentDayoffs() after modifications. Based on coding guidelines.
🤖 Prompt for AI Agents
In zmsdb/src/Zmsdb/DayOff.php around lines 241-250, extend removeCache() so it
not only deletes the "dayOffs" key but also removes per-department and per-scope
entries to avoid stale cache: detect and delete all keys matching
"dayOffsReadOnlyByDepartmentId-*" and "dayOffsByScope-*" (use the cache
provider's key listing / pattern deletion API or iterate keys and delete
matches), and add a new public removeCacheByDepartmentId($departmentId) helper
that deletes "dayOffsReadOnlyByDepartmentId-{$departmentId}" and the global
"dayOffs" plus any scope keys for that department; then, per the follow-up, call
removeCacheByDepartmentId($deptId) from Department::writeDepartmentDayoffs()
immediately after making modifications.
| if (App::$cache) { | ||
| App::$cache->set($cacheKey, $department); | ||
| } |
There was a problem hiding this comment.
Respect disableCache on write.
Skip set() when bypassing cache.
- if (App::$cache) {
+ if (!$disableCache && App::$cache) {
App::$cache->set($cacheKey, $department);
}🤖 Prompt for AI Agents
In zmsdb/src/Zmsdb/Department.php around lines 37 to 39, the current code always
calls App::$cache->set($cacheKey, $department) when App::$cache exists; update
it to respect the cache bypass flag by only calling set when caching is enabled
(e.g., check that App::$disableCache is not true): change the condition to
require App::$cache and that cache is not disabled, and leave behavior unchanged
otherwise.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
zmsdb/tests/Zmsdb/DepartmentTest.php (1)
15-15: LGTM! Consider adding test coverage forwithDayOff=false.The addition of the third parameter
truecorrectly aligns with the test's assertions that verify dayoff data (lines 21-22). The change properly reflects the updated method signature.For more comprehensive test coverage, consider adding a test case that verifies the behavior when
withDayOff=falseis passed to ensure the parameter works as expected in both scenarios. For example:public function testReadWithoutDayOff() { $query = new Query(); $input = $this->getTestEntity(); $entity = $query->writeEntity($input, 75); $entity = $query->readEntity($entity->id, 2, false); $this->assertEquals(0, count($entity->dayoff)); }
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
zmsdb/tests/Zmsdb/DepartmentTest.php(1 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
**/*
⚙️ CodeRabbit configuration file
**/*: Apply the following Clean Code guidelines to all files, as summarized by wojteklu's Clean Code gist:Code is clean if it can be understood easily – by everyone on the team. Clean code can be read and enhanced by a developer other than its original author. With understandability comes readability, changeability, extensibility and maintainability.
General rules
- Follow standard conventions.
- Keep it simple stupid. Simpler is always better. Reduce complexity as much as possible.
- Boy scout rule. Leave the campground cleaner than you found it.
- Always find root cause. Always look for the root cause of a problem.
Design rules
- Keep configurable data at high levels.
- Prefer polymorphism to if/else or switch/case.
- Separate multi-threading code.
- Prevent over-configurability.
- Use dependency injection.
- Follow Law of Demeter. A class should know only its direct dependencies.
Understandability tips
- Be consistent. If you do something a certain way, do all similar things in the same way.
- Use explanatory variables.
- Encapsulate boundary conditions. Boundary conditions are hard to keep track of. Put the processing for them in one place.
- Prefer dedicated value objects to primitive type.
- Avoid logical dependency. Don't write methods which works correctly depending on something else in the same class.
- Avoid negative conditionals.
Names rules
- Choose descriptive and unambiguous names.
- Make meaningful distinction.
- Use pronounceable names.
- Use searchable names.
- Replace magic numbers with named constants.
- Avoid encodings. Don't append prefixes or type information.
Functions rules
- Small.
- Do one thing.
- Use descriptive names.
- Prefer fewer arguments.
- Have no side effects.
- Don't use flag arguments. Split method into several independent methods that can b...
Files:
zmsdb/tests/Zmsdb/DepartmentTest.php
**/*.php
⚙️ CodeRabbit configuration file
**/*.php: Flag any usage of error_log() as it should be replaced with proper Monolog logging mechanisms provided by zmsslim which should be setup in the App/Application of each module:
- For error handling: Use the proper Monolog logging framework with error levels
- For application info logs: Use the proper Monolog logging framework with info levels
- For debugging: Use a dedicated debug logger or remove debug statements
- For CLI output: Use a CLI output handler or symfony/console
- The application log levels are as follows DEBUG, INFO, NOTICE, WARNING, ERROR, CRITICAL, ALERT, and EMERGENCY
Example replacement:
// Instead of: error_log("Import failed - " . $e->getMessage()); // Use: $log->error("Import failed", ['error' => $e->getMessage()]);Flag specific logging violations:
- error_log(), var_dump(), print_r(), die(), exit() usage (except proper error logging in catch blocks)
- Any debug output that should use proper Monolog logging
- Debug constants like DEBUG = true
- Debug logging that should be removed in production
- Commented debug code that should be cleaned up
Example replacements:
// Instead of: error_log("Error occurred"); var_dump($data); die('debug point'); // Use: $log->error("Error occurred", ['context' => 'processing']); $log->debug('Data dump', ['data' => $data]); // Remove die() statements entirelyException handling should use proper logging:
// Instead of: try { $result = $this->process(); } catch (Exception $e) { error_log("Processing failed: " . $e->getMessage()); } // Use: try { $result = $this->process(); } catch (Exception $e) { $log->error("Processing failed", ['error' => $e->getMessage(), 'trace' => $e->getTraceAsString()]); } ```...
Files:
zmsdb/tests/Zmsdb/DepartmentTest.php
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
zmsdb/src/Zmsdb/Connection/Select.php(1 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
**/*
⚙️ CodeRabbit configuration file
**/*: Apply the following Clean Code guidelines to all files, as summarized by wojteklu's Clean Code gist:Code is clean if it can be understood easily – by everyone on the team. Clean code can be read and enhanced by a developer other than its original author. With understandability comes readability, changeability, extensibility and maintainability.
General rules
- Follow standard conventions.
- Keep it simple stupid. Simpler is always better. Reduce complexity as much as possible.
- Boy scout rule. Leave the campground cleaner than you found it.
- Always find root cause. Always look for the root cause of a problem.
Design rules
- Keep configurable data at high levels.
- Prefer polymorphism to if/else or switch/case.
- Separate multi-threading code.
- Prevent over-configurability.
- Use dependency injection.
- Follow Law of Demeter. A class should know only its direct dependencies.
Understandability tips
- Be consistent. If you do something a certain way, do all similar things in the same way.
- Use explanatory variables.
- Encapsulate boundary conditions. Boundary conditions are hard to keep track of. Put the processing for them in one place.
- Prefer dedicated value objects to primitive type.
- Avoid logical dependency. Don't write methods which works correctly depending on something else in the same class.
- Avoid negative conditionals.
Names rules
- Choose descriptive and unambiguous names.
- Make meaningful distinction.
- Use pronounceable names.
- Use searchable names.
- Replace magic numbers with named constants.
- Avoid encodings. Don't append prefixes or type information.
Functions rules
- Small.
- Do one thing.
- Use descriptive names.
- Prefer fewer arguments.
- Have no side effects.
- Don't use flag arguments. Split method into several independent methods that can b...
Files:
zmsdb/src/Zmsdb/Connection/Select.php
**/*.php
⚙️ CodeRabbit configuration file
**/*.php: Flag any usage of error_log() as it should be replaced with proper Monolog logging mechanisms provided by zmsslim which should be setup in the App/Application of each module:
- For error handling: Use the proper Monolog logging framework with error levels
- For application info logs: Use the proper Monolog logging framework with info levels
- For debugging: Use a dedicated debug logger or remove debug statements
- For CLI output: Use a CLI output handler or symfony/console
- The application log levels are as follows DEBUG, INFO, NOTICE, WARNING, ERROR, CRITICAL, ALERT, and EMERGENCY
Example replacement:
// Instead of: error_log("Import failed - " . $e->getMessage()); // Use: $log->error("Import failed", ['error' => $e->getMessage()]);Flag specific logging violations:
- error_log(), var_dump(), print_r(), die(), exit() usage (except proper error logging in catch blocks)
- Any debug output that should use proper Monolog logging
- Debug constants like DEBUG = true
- Debug logging that should be removed in production
- Commented debug code that should be cleaned up
Example replacements:
// Instead of: error_log("Error occurred"); var_dump($data); die('debug point'); // Use: $log->error("Error occurred", ['context' => 'processing']); $log->debug('Data dump', ['data' => $data]); // Remove die() statements entirelyException handling should use proper logging:
// Instead of: try { $result = $this->process(); } catch (Exception $e) { error_log("Processing failed: " . $e->getMessage()); } // Use: try { $result = $this->process(); } catch (Exception $e) { $log->error("Processing failed", ['error' => $e->getMessage(), 'trace' => $e->getTraceAsString()]); } ```...
Files:
zmsdb/src/Zmsdb/Connection/Select.php
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: Analyze (javascript-typescript)
- GitHub Check: call-php-unit-tests / zmsapi-test
- GitHub Check: call-php-unit-tests / zmsdb-test
| $pdoOptions = array_merge([ | ||
| Pdo::ATTR_PERSISTENT => true | ||
| ], self::$pdoOptions); |
There was a problem hiding this comment.
🧩 Analysis chain
Make persistent connections configurable, not hardcoded.
Hardcoding ATTR_PERSISTENT => true violates the principle of keeping configurable data at high levels. Persistent connections can have significant operational impact and should be controllable via configuration or environment variables.
Additionally, persistent connections will cause session-level settings (lines 123, 126-127, 164, 170, 219-220, 225, 234-238) to persist across requests, which may lead to unexpected behavior if connections are reused in ways that weren't anticipated.
As per coding guidelines.
Apply this diff to make it configurable:
+ /**
+ * @var Bool $enablePersistentConnections
+ */
+ public static $enablePersistentConnections = false;
+
protected static function createPdoConnection($dataSourceName)
{
try {
- $pdoOptions = array_merge([
- Pdo::ATTR_PERSISTENT => true
- ], self::$pdoOptions);
+ $pdoOptions = self::$pdoOptions;
+ if (self::$enablePersistentConnections) {
+ $pdoOptions[Pdo::ATTR_PERSISTENT] = true;
+ }
$pdo = new Pdo($dataSourceName, self::$username, self::$password, $pdoOptions);Then set it during application initialization:
// In application bootstrap
\BO\Zmsdb\Connection\Select::$enablePersistentConnections =
(getenv('DB_PERSISTENT_CONNECTIONS') === 'true');Run the following script to check if persistent connections are already configurable elsewhere or if there are patterns for this in the codebase:
🏁 Script executed:
#!/bin/bash
# Search for other persistent connection configuration patterns
rg -n -C3 'ATTR_PERSISTENT|enablePersistent|persistent.*connection' --type=php
# Search for how other static properties in Select are configured
rg -n -C3 'Select::\$' --type=php -g '!**/Connection/Select.php'Length of output: 14501
Hardcoded persistent connections must be made configurable.
The verification confirms this is the only occurrence of ATTR_PERSISTENT => true in the codebase, and there is no existing configuration mechanism for it. The codebase establishes a clear pattern for externalizing connection settings through static properties configured during bootstrap (see zmsdb/config.example.php lines 21–35, and zmsdldb/bootstrap.php lines 22–30, where $enableProfiling, $readSourceName, $writeSourceName, and $pdoOptions are all externalized). Hardcoding persistent connections violates this design and the principle of keeping configurable data at high levels.
Additionally, persistent connections cause session-level settings (lines 123, 126–127, 164, 170, 219–220, 225, 234–238) to persist across requests, creating operational risks that should be controllable.
Apply the suggested diff to make it configurable via the static property pattern already in use throughout the codebase, then set it during application bootstrap using an environment variable.
🤖 Prompt for AI Agents
In zmsdb/src/Zmsdb/Connection/Select.php around lines 119 to 121, remove the
hardcoded Pdo::ATTR_PERSISTENT => true and make the persistent option
configurable by reading it from the existing static pdo options pattern
(self::$pdoOptions) so that the default merge does not force persistence; ensure
callers/bootstrap set self::$pdoOptions['ATTR_PERSISTENT'] (or the correct Pdo
constant key) from an environment variable during application bootstrap (e.g.,
read env var like ZMSDB_PDO_PERSISTENT and cast to boolean) so persistent
connections are opt-in and controlled at startup.
2 participants
Pull Request Checklist (Feature Branch to
next):nextBranch in meinen Feature-Branch gemergt.Summary by CodeRabbit
Chores
Refactor
Tests