perf(ZMSKVR): cache schema files in zmsentities to prevent redundant disk I/O #1731
Conversation
…disk I/O - Add PSR-16 cache support to zmsentities module - Cache parsed schema objects and JSON strings in Loader - Use App::$cache for persistent caching across PHP-FPM workers - Add cache hit/set logging via Monolog - Update Validator to use cached Loader::asJson() method - Cache persists across process restarts and benefits all workers
WalkthroughAdds PSR‑cache-backed schema loading: Loader caches raw JSON and serialized Schema with mtime freshness checks, JSON parse handling, and logging; Validator now loads schema content via Loader::asJson() instead of reading files directly. Changes
Sequence DiagramsequenceDiagram
participant Validator
participant Loader
participant Cache
participant Filesystem
Validator->>Loader: asJson(schemaFilename)
Loader->>Loader: compute cacheKey & resolve fullPath
Loader->>Cache: get(cacheKey)
alt cache hit
Cache-->>Loader: {data: JSON or serialized Schema, mtime}
Loader->>Filesystem: stat(fullPath) -> current mtime
alt mtimes match
Loader-->>Validator: return cached JSON / reconstructed Schema
Note right of Loader: optional logCacheHit
else stale
Loader->>Filesystem: read fullPath -> JSON
Filesystem-->>Loader: JSON + mtime
Loader->>Loader: parseJsonSchema(JSON)
alt parse success
Loader->>Cache: set(cacheKey -> {JSON, mtime})
Loader-->>Validator: return JSON
else parse fail
Loader-->>Validator: throw parse error
end
end
else cache miss
Loader->>Filesystem: read fullPath -> JSON
Filesystem-->>Loader: JSON + mtime
Loader->>Loader: parseJsonSchema(JSON)
alt parse success
Loader->>Cache: set(cacheKey -> {JSON, mtime})
Loader-->>Validator: return JSON
else parse fail
Loader-->>Validator: throw parse error
end
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ 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). (4)
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: 3
🧹 Nitpick comments (3)
zmsentities/src/Zmsentities/Schema/Loader.php (3)
30-30: Use allowed_classes option with unserialize for safer deserialization.Using
unserialize()without restrictions can be a security risk if the cache is ever compromised. Specify the expected class to prevent object injection attacks.- return unserialize($cached); + return unserialize($cached, ['allowed_classes' => [Schema::class]]);
17-34: Consider extracting cache retrieval logic to reduce duplication.The cache retrieval pattern (check mtime, get cached value, log hit) is duplicated between
asArray()andasJson(). This violates the DRY principle and makes maintenance harder.Consider extracting a private helper method:
private static function getFromCache(string $cacheKey, string $filePath, string $type): mixed { if (!class_exists('\App') || !isset(\App::$cache) || !\App::$cache || !file_exists($filePath)) { return null; } $cacheMtime = \App::$cache->get($cacheKey . '_mtime'); if ($cacheMtime === null || $cacheMtime < filemtime($filePath)) { return null; } $cached = \App::$cache->get($cacheKey); if ($cached !== null && class_exists('\App') && isset(\App::$log) && \App::$log) { \App::$log->debug('Schema cache hit', ['schema' => $cacheKey, 'type' => $type]); } return $cached; }Also applies to: 85-102
58-60: Store filemtime result to avoid redundant syscalls on cache miss.When the cache is missed,
filemtime()is called again at line 60 (and similarly at line 110 inasJson). Consider storing the mtime value earlier for reuse.+ $fileMtime = file_exists($fullPath) ? filemtime($fullPath) : null; + // Try to get from cache if (class_exists('\App') && isset(\App::$cache) && \App::$cache && file_exists($fullPath)) { $cacheMtime = \App::$cache->get($cacheKey . '_mtime'); if ($cacheMtime !== null) { - $fileMtime = filemtime($fullPath); if ($cacheMtime >= $fileMtime) { // ... } } } // ... later ... - \App::$cache->set($cacheKey . '_mtime', filemtime($fullPath)); + \App::$cache->set($cacheKey . '_mtime', $fileMtime);
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
zmsentities/composer.lockis excluded by!**/*.lock
📒 Files selected for processing (3)
zmsentities/composer.json(1 hunks)zmsentities/src/Zmsentities/Schema/Loader.php(3 hunks)zmsentities/src/Zmsentities/Schema/Validator.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:
zmsentities/src/Zmsentities/Schema/Validator.phpzmsentities/src/Zmsentities/Schema/Loader.phpzmsentities/composer.json
**/*.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/Schema/Validator.phpzmsentities/src/Zmsentities/Schema/Loader.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). (8)
- GitHub Check: call-php-code-quality / module-code-quality (zmsdb, 8.3)
- GitHub Check: call-php-code-quality / module-code-quality (zmsclient, 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 (zmsstatistic, 8.3)
- GitHub Check: call-zmsautomation-tests / zmsapiautomation
- GitHub Check: call-php-unit-tests / zmsapi-test
- GitHub Check: call-php-unit-tests / zmsdb-test
🔇 Additional comments (1)
zmsentities/src/Zmsentities/Schema/Validator.php (1)
53-59: LGTM!The integration with
Loader::asJson()is clean and maintains the existing schema registration behavior while enabling cache-backed loading. The variable rename to$schemaFilenameimproves clarity.
There was a problem hiding this comment.
Actionable comments posted: 4
♻️ Duplicate comments (3)
zmsentities/src/Zmsentities/Schema/Loader.php (3)
30-35: Duplicate: INFO log level already flagged in past review.As noted in a previous review comment, cache hit/set events should use DEBUG level instead of INFO to avoid excessive log noise.
48-53: Duplicate: INFO log level already flagged in past review.As noted in a previous review comment, cache set events should use DEBUG level instead of INFO to avoid excessive log noise.
120-121: Duplicate:file_get_contents()error handling already flagged in past review.A previous review comment already identified that
file_get_contents()can returnfalseon failure, which would then be cached and cause downstream errors.
🧹 Nitpick comments (1)
zmsentities/src/Zmsentities/Schema/Loader.php (1)
9-9: Add return type declarations for better type safety.All four methods in this file lack return type declarations. Adding them would improve IDE support, catch potential bugs earlier, and make the API clearer.
- private static function getCachedSchema(string $cacheKey, string $fullPath, string $schemaFilename, string $type) + private static function getCachedSchema(string $cacheKey, string $fullPath, string $schemaFilename, string $type): ?string - private static function setCachedSchema(string $cacheKey, string $fullPath, string $schemaFilename, string $type, $data): void + private static function setCachedSchema(string $cacheKey, string $fullPath, string $schemaFilename, string $type, string $data): void - public static function asArray($schemaFilename) + public static function asArray(string $schemaFilename): Schema - public static function asJson($schemaFilename) + public static function asJson(string $schemaFilename): stringAlso applies to: 40-40, 76-76, 102-102
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
zmsentities/src/Zmsentities/Schema/Loader.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:
zmsentities/src/Zmsentities/Schema/Loader.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/Schema/Loader.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). (8)
- GitHub Check: build (zmscitizenview)
- GitHub Check: Analyze (python)
- GitHub Check: Analyze (javascript-typescript)
- GitHub Check: call-php-unit-tests / module-test (zmsmessaging, 8.3)
- GitHub Check: call-php-unit-tests / module-test (zmsadmin, 8.3)
- GitHub Check: call-zmsautomation-tests / zmsapiautomation
- GitHub Check: call-php-unit-tests / zmsdb-test
- GitHub Check: call-php-unit-tests / zmsapi-test
🔇 Additional comments (1)
zmsentities/src/Zmsentities/Schema/Loader.php (1)
56-74: LGTM!The JSON parsing logic includes comprehensive error handling with clear, specific error messages for different JSON error types.
- Change log level from info to debug for cache hits and sets - Prevents test failures by avoiding unexpected log output - Debug logs still available when DEBUGLEVEL is set to DEBUG
- Check if file_get_contents() returns false before caching - Throw SchemaMissingJsonFile exception with clear error message - Prevents caching false values that would cause json_decode errors
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
zmsentities/src/Zmsentities/Schema/Loader.php (1)
9-127: Good caching implementation with proper mtime-based freshness checks.The overall caching strategy is well-designed:
- Separate cache keys for JSON and parsed schemas
- mtime-based cache invalidation ensures freshness
- Proper fallback when cache is unavailable
- Debug-level logging is appropriate for cache operations (addresses previous feedback)
Consider adding type declarations to improve code clarity and catch type errors earlier:
private static function getCachedSchema( string $cacheKey, string $fullPath, string $schemaFilename, string $type ): ?string { /* ... */ } private static function setCachedSchema( string $cacheKey, string $fullPath, string $schemaFilename, string $type, string $data ): void { /* ... */ } public static function asArray(string $schemaFilename): Schema { /* ... */ } public static function asJson(string $schemaFilename): string { /* ... */ }As per coding guidelines, proper type hints improve maintainability and readability.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
zmsentities/src/Zmsentities/Schema/Loader.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:
zmsentities/src/Zmsentities/Schema/Loader.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/Schema/Loader.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). (4)
- GitHub Check: Analyze (javascript-typescript)
- GitHub Check: call-php-unit-tests / zmsapi-test
- GitHub Check: call-zmsautomation-tests / zmsapiautomation
- GitHub Check: call-php-unit-tests / zmsdb-test
- Change symfony/cache from ^6.0 to ^5.4 - Matches existing symfony/translation and symfony/twig-bridge versions - Prevents potential dependency conflicts and runtime issues
- Check cache first before validating file existence - Allows cache to be checked even if file is temporarily unavailable - Still validates file exists before returning cached data (schema files should exist) - File existence check remains in setCachedSchema for write operations
- Check if filemtime() returns false before comparing with cached mtime - Prevents incorrect cache invalidation when file access fails - Also handle filemtime() failure in setCachedSchema to avoid caching false values
- Store JSON string instead of serialized Schema object in cache - Reconstruct Schema object from JSON on cache retrieval - Eliminates security risk of arbitrary code execution via unserialize() - JSON is safer than PHP serialization for cached data
…tor bug - Add absolute path handling to asArray() to match asJson() behavior - Fix double-separator bug when absolute paths are provided - Ensures consistent path resolution across both methods
- Extract isCacheAvailable() helper method - Extract validateCacheMtime() helper method - Extract logCacheHit() helper method - Reduces NPath complexity from 512 to below threshold
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
zmsentities/src/Zmsentities/Schema/Loader.php (1)
22-24: Reconsider: Redundant file existence check may hinder caching effectiveness.This explicit
file_exists($fullPath)check is redundant becausefilemtime()on line 26 already returnsfalsewhen the file doesn't exist, which is properly handled on lines 27-29. The explicit check prevents returning cached data when the source file has been deleted, which may defeat the caching purpose in scenarios where cached schemas should remain available even after file removal.Per the previous review discussion, the
filemtime()check alone provides sufficient validation while allowing the cache to work independently of current file presence.Consider removing the redundant check:
- // Validate file exists and compare modification times - if (!file_exists($fullPath)) { - return null; - } - $fileMtime = filemtime($fullPath); if ($fileMtime === false) { return null; }The
filemtime === falsecheck already handles missing files gracefully while allowing valid cached data to be served when appropriate.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
zmsentities/composer.lockis excluded by!**/*.lock
📒 Files selected for processing (2)
zmsentities/composer.json(1 hunks)zmsentities/src/Zmsentities/Schema/Loader.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:
zmsentities/composer.jsonzmsentities/src/Zmsentities/Schema/Loader.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/Schema/Loader.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). (11)
- GitHub Check: call-zmsautomation-tests / zmsapiautomation
- 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 (zmsapi, 8.3)
- GitHub Check: call-php-unit-tests / module-test (zmsslim, 8.3)
- GitHub Check: call-php-unit-tests / module-test (zmsstatistic, 8.3)
- GitHub Check: call-php-unit-tests / module-test (zmscitizenapi, 8.3)
- GitHub Check: call-php-unit-tests / zmsclient-test
- GitHub Check: call-php-unit-tests / zmsdb-test
- GitHub Check: call-php-unit-tests / zmsapi-test
🔇 Additional comments (8)
zmsentities/composer.json (1)
24-25: LGTM! Cache dependencies properly aligned.The addition of
symfony/cacheandpsr/simple-cacheat version^5.4and^3.0respectively properly supports the PSR-16 caching infrastructure implemented in Loader.php. All Symfony packages maintain consistent versioning at ^5.4.zmsentities/src/Zmsentities/Schema/Loader.php (7)
9-12: LGTM! Clean cache availability check.The defensive checking pattern (class existence → isset → truthiness) properly guards against missing cache infrastructure.
37-45: LGTM! Appropriate debug logging.Cache hit logging correctly uses DEBUG level for routine operational events and includes useful context (schema filename and type).
47-64: LGTM! Clean cache retrieval logic.The method properly delegates to helpers (
isCacheAvailable,validateCacheMtime,logCacheHit) and uses clear early-return patterns.
66-83: LGTM! Robust cache storage with proper error handling.The method correctly:
- Validates file existence before caching (appropriate for cache writes)
- Handles
filemtime()failure (lines 73-76)- Logs at DEBUG level
- All past review issues properly addressed
85-103: LGTM! Comprehensive JSON parse error handling.The method provides detailed error messages for various JSON parsing failures and properly includes the schema filename for debugging. Good refactoring of existing logic.
105-140: LGTM! Secure cache-backed schema loading.The refactored method properly:
- Handles absolute paths (lines 111-116)
- Reconstructs Schema safely from cached JSON instead of
unserialize()(lines 122-126)- Delegates JSON parsing to the helper
- Caches JSON strings for safety
All past security and correctness issues properly addressed.
142-177: LGTM! Robust JSON loading with proper error handling.The refactored method properly:
- Handles absolute paths without double-separator issues (lines 149-154)
- Validates
file_get_contents()failure with clear exception (lines 167-171)- Integrates caching seamlessly
All past review issues properly addressed.
ThomasAFink
added
php
…prevent-disk-io-reads
…prevent-disk-io-reads
Pull Request Checklist (Feature Branch to
next):nextBranch in meinen Feature-Branch gemergt.Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.