fix Using null as an array offset is deprecated Refactor cache subsystem to support multiple engines #1596
Conversation
…instead fix offset is deprecated, use an empty string instead replacing extract()
There was a problem hiding this comment.
Pull request overview
This PR addresses PHP deprecation warnings related to using null as an array offset by replacing extract() calls with explicit variable extraction and adding validation checks before using variables as array keys.
Key changes:
- Replaced
extract()calls inconfig()andwrite()methods with explicit array key checking and assignment - Added string type and empty string validation before using variables as array offsets
- Updated parameter documentation to reflect that null values are accepted
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 1 out of 1 changed files in this pull request and generated 2 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 1 out of 1 changed files in this pull request and generated 3 comments.
Comments suppressed due to low confidence (2)
htdocs/class/cache/xoopscache.php:435
- Potential null array offset issue: When
$_this->nameis null (which is its initial value), accessing$_this->configs[$_this->name]will use null as an array key. This should be guarded with a null check on$_this->namebefore attempting array access, similar to the pattern used in the updatedconfigmethod at lines 156-161.
if (!$engine && isset($_this->configs[$_this->name]['engine'])) {
$engine = $_this->configs[$_this->name]['engine'];
}
htdocs/class/cache/xoopscache.php:451
- Potential null array offset issue: When
$_this->nameis null (which is its initial value), accessing$_this->configs[$_this->name]will use null as an array key. This should be guarded with a null check on$_this->namebefore attempting array access, similar to the pattern used in the updatedconfigmethod at lines 156-161.
if (!$engine && isset($_this->configs[$_this->name]['engine'])) {
$engine = $_this->configs[$_this->name]['engine'];
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
|
||
|
|
There was a problem hiding this comment.
Excessive blank lines: There are three consecutive blank lines (lines 99-100) which is inconsistent with the rest of the codebase that typically uses single blank lines between methods.
| $config = $_this->resolveEngineConfig($config); | ||
| if ($config === false) { | ||
| return false; | ||
| } |
There was a problem hiding this comment.
Inconsistent return type behavior: The method returns false when config resolution fails (line 338), but the PHPDoc at line 328 states the return type is mixed (cached data or false). While this is technically correct, it creates an ambiguity where false could mean either "config failed" or "cached data is false". Consider documenting this nuance or using a different sentinel value for configuration failures.
| } | ||
|
|
||
| $engine = $config['engine']; | ||
|
|
There was a problem hiding this comment.
Missing null check: The code assumes $_this->engine[$engine] exists and is not null before calling gc() on it. If the engine initialization failed (line 224 sets it to null), this will cause a fatal error. An isInitialized check should be added before accessing the engine, similar to the pattern used in write, read, delete, and clear methods.
| if (!isset($_this->engine[$engine]) || $_this->engine[$engine] === null) { | |
| return false; | |
| } |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 1 out of 1 changed files in this pull request and generated 6 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| * @var array<string, XoopsCacheEngine|null> | ||
| * @access protected | ||
| */ | ||
| protected $engine = []; |
There was a problem hiding this comment.
The $engine property is now initialized as an empty array, but throughout the codebase it's used with string keys (e.g., $_this->engine[$name]). Consider adding a check in the engine() method to ensure that $_this->engine[$name] is not just isset() but also an instance of the expected cache engine class before calling methods on it, to prevent potential type errors if the array is manipulated incorrectly elsewhere.
| $config = $name; | ||
|
|
||
| if (isset($config['name']) && is_string($config['name'])) { | ||
| $name = $config['name']; | ||
| } |
There was a problem hiding this comment.
When $name is an array, it's assigned to $config, but if $config['name'] is not set or not a string, $name remains an array. This could lead to type errors in subsequent code that expects $name to be a string. Consider setting a default string value (e.g., 'default') if the name cannot be extracted from the config array.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 1 out of 1 changed files in this pull request and generated 2 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 1 out of 1 changed files in this pull request and generated 5 comments.
Comments suppressed due to low confidence (1)
htdocs/class/cache/xoopscache.php:346
trigger_error('Cache write not initialized: ' . $engine)omits the error type, so it defaults toE_USER_NOTICE. Elsewhere in this class non-fatal cache failures useE_USER_WARNING; consider passingE_USER_WARNINGhere as well for consistent severity/visibility.
if (!$_this->isInitialized($engine)) {
trigger_error('Cache write not initialized: ' . $engine);
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughReplaces single-engine cache storage with a named engine map, adds XoopsCacheEngineInterface and engine loader/resolution helpers, and refactors public API (config/engine/read/write/delete/clear/gc/isInitialized/settings) to normalize inputs, initialize engines, and standardize return types. Changes
Sequence Diagram(s)sequenceDiagram
actor Client
participant XoopsCache
participant Resolver as resolveEngineConfig
participant Loader as loadEngine
participant CacheEngine
Client->>XoopsCache: write(key, value, duration_or_config)
activate XoopsCache
XoopsCache->>Resolver: resolveEngineConfig(config_or_null)
Resolver-->>XoopsCache: {engine, settings} or false
alt config valid
XoopsCache->>Loader: loadEngine(engine_name)
Loader-->>XoopsCache: engine instance or failure
alt engine available
XoopsCache->>CacheEngine: write(key, value, duration)
CacheEngine-->>XoopsCache: bool
XoopsCache-->>Client: bool
else engine missing/invalid
XoopsCache-->>Client: false
end
else invalid config
XoopsCache-->>Client: false
end
deactivate XoopsCache
sequenceDiagram
actor Admin
participant XoopsCache
participant Resolver as resolveEngineName
participant Loader as loadEngine
participant CacheEngine
Admin->>XoopsCache: engine(name, settings)
activate XoopsCache
XoopsCache->>Resolver: resolveEngineName(name)
Resolver-->>XoopsCache: engine_name or null
alt valid name
XoopsCache->>Loader: loadEngine(engine_name)
Loader-->>XoopsCache: engine instance or failure
alt initialized
XoopsCache->>CacheEngine: init(settings)
CacheEngine-->>XoopsCache: void/bool
XoopsCache-->>Admin: true/false
else failed
XoopsCache-->>Admin: false
end
else invalid name
XoopsCache-->>Admin: false
end
deactivate XoopsCache
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Tip Issue Planner is now in beta. Read the docs and try it out! Share your feedback on Discord. 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 |
| interface XoopsCacheEngineInterface | ||
| { | ||
| /** | ||
| * Initialize the cache engine | ||
| * | ||
| * @param array $settings Associative array of parameters for the engine | ||
| * @return bool True if the engine has been successfully initialized, false if not | ||
| */ | ||
| public function init($settings = []); | ||
|
|
||
| /** | ||
| * Garbage collection - permanently remove all expired and deleted data | ||
| * | ||
| * @return bool|null True on success, false on failure, null for no-op | ||
| */ | ||
| public function gc(); | ||
|
|
||
| /** | ||
| * Write value for a key into cache | ||
| * | ||
| * @param string $key Identifier for the data | ||
| * @param mixed $value Data to be cached | ||
| * @param int|null $duration How long to cache the data, in seconds | ||
| * @return bool True if the data was successfully cached, false on failure | ||
| */ | ||
| public function write($key, $value, $duration = null); | ||
|
|
||
| /** | ||
| * Read a key from the cache | ||
| * | ||
| * @param string $key Identifier for the data | ||
| * @return mixed The cached data, or false if the data doesn't exist or has expired | ||
| */ | ||
| public function read($key); | ||
|
|
||
| /** | ||
| * Delete a key from the cache | ||
| * | ||
| * @param string $key Identifier for the data | ||
| * @return bool True if the value was successfully deleted, false otherwise | ||
| */ | ||
| public function delete($key); | ||
|
|
||
| /** | ||
| * Delete all keys from the cache | ||
| * | ||
| * @param bool $check If true will check expiration, otherwise delete all | ||
| * @return bool True if the cache was successfully cleared, false otherwise | ||
| */ | ||
| public function clear($check); | ||
|
|
||
| /** | ||
| * Return the settings for this cache engine | ||
| * | ||
| * @return array Associative array of current engine settings | ||
| */ | ||
| public function settings(); | ||
| } | ||
|
|
||
| /** | ||
| * Abstract class for storage engine for caching | ||
| * | ||
| * @package core | ||
| * @subpackage cache | ||
| */ | ||
| class XoopsCacheEngine | ||
| abstract class XoopsCacheEngine implements XoopsCacheEngineInterface | ||
| { |
There was a problem hiding this comment.
This PR introduces significant refactoring to the cache subsystem including new interface requirements, multi-engine support, and changes to return type semantics. However, there are no automated tests for the cache system. Consider adding unit tests to cover the new functionality, especially the interface implementation checks, engine resolution logic, configuration parsing, and return type normalization to prevent regressions.
| * @access public | ||
| */ | ||
| public static function delete($key, $config = null) | ||
| public static function delete($key, $config = null): bool |
There was a problem hiding this comment.
The return type declaration : bool is added to the delete() method signature, but the XoopsCacheEngineInterface interface doesn't specify return type declarations in its method signatures (see lines 598-646). For consistency and to ensure all implementations remain compatible with the interface, consider whether to add return type declarations to the interface methods as well, or remove them from the implementing methods. Note that PHP 7.4 supports return type declarations, so this could be done safely for the target PHP version.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
htdocs/class/cache/xoopscache.php (1)
446-476:⚠️ Potential issue | 🟠 MajorCast engine delete result to bool to satisfy the return type.
XoopsCache::delete()declares: boolbut returns the engine's raw result. Bothwrite()(line 401) andclear()(line 504) cast their engine calls to bool, butdelete()does not. This inconsistency can trigger aTypeErrorif any engine returnsnullor a non-bool value. Apply the same pattern as the other static methods.Proposed fix
- $success = $_this->engine[$engine]->delete($key); + $success = (bool) $_this->engine[$engine]->delete($key);
| $engine = (!empty($settings['engine']) && is_string($settings['engine'])) | ||
| ? $settings['engine'] | ||
| : 'file'; | ||
|
|
||
| // Normalize: ensure the resolved engine name is stored in settings |
There was a problem hiding this comment.
Here $engine is derived from the (possibly merged/overridden) $settings, but later config() returns $settings from $_this->configs[$name]. If config() is called with overrides while $name is already the active config, the method can return an {engine, settings} pair that is inconsistent (engine reflects the overrides, settings reflects the stored config) and the overrides may never be applied. Consider ensuring the returned $settings corresponds to the resolved $engine (e.g., return the merged $settings and/or reinitialize/update the config even when $name === $_this->name).
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 2 out of 2 changed files in this pull request and generated 14 comments.
Comments suppressed due to low confidence (2)
htdocs/class/cache/xoopscache.php:509
- At line 509, the engine's init() is called AFTER the clear() operation completes. This seems backwards - typically you would want to ensure the engine is properly initialized before performing operations on it. If the engine's state was modified by the clear() operation, re-initializing it afterward might reset or invalidate the operation. Consider moving the init() call to before the clear() operation, similar to the pattern used in write(), read(), and delete() methods.
return $success;
}
htdocs/class/cache/xoopscache.php:152
- The logic at line 137 has a potential security flaw. When the candidate file path exists but realpath() fails (returns false), the condition 'if ($file !== false && ...)' evaluates to false, and execution falls through to the else block which reports "file not found". However, if realpath($candidate) returns false but file_exists($candidate) is true, you should not include the file because you couldn't validate its path. Consider adding an explicit check using file_exists() to distinguish between: 1) file doesn't exist (legitimate error), 2) realpath() failed for security/permission reasons (should trigger a security warning), and 3) file exists and is in the correct directory (safe to include).
trigger_error(
'Class XoopsCache' . ucfirst($name) . ' not found after including ' . basename($file),
E_USER_WARNING
);
return false;
}
} else {
trigger_error(
'File: ' . basename($candidate) . ' not found in file: ' . basename(__FILE__) . ' at line: ' . __LINE__,
E_USER_WARNING
);
return false;
}
}
return true;
| * miss; callers SHOULD treat any falsy return value as a cache miss. | ||
| */ | ||
| public function read($key); | ||
|
|
||
| /** |
There was a problem hiding this comment.
The interface documentation at line 618-620 states that read() can return "false or null" for cache misses, making it engine-dependent. This inconsistency makes it difficult for callers to reliably detect cache misses. Consider standardizing on a single return value (preferably false) for all cache miss scenarios across all engine implementations, and update the interface documentation to reflect this guarantee. This would make the API more predictable and easier to use correctly.
| if (isset($config['engine']) && !isset($settings['engine'])) { | ||
| $settings['engine'] = $config['engine']; | ||
| } | ||
| } else { | ||
| // Back-compat: treat array input as settings when no nested "settings" exists | ||
| $settings = $config; |
There was a problem hiding this comment.
In the array handling logic, when the input is an array with a 'settings' key (line 184), the code extracts settings and carries forward the top-level 'engine' key into settings if not already set (lines 187-189). However, this means the original $config array might have 'engine' at the top level that doesn't get used if 'settings' already has an 'engine' key. This could lead to confusion where the top-level engine specification is silently ignored. Consider either rejecting configurations where both top-level and settings-level 'engine' keys exist but differ, or at least documenting which one takes precedence.
| if ($_this->isInitialized($engine)) { | ||
| $_this->engine[$engine]->init($settings); | ||
| } | ||
| $_this->configs[$name] = $_this->settings($engine); | ||
| } | ||
|
|
||
| $settings = $_this->configs[$name]; |
There was a problem hiding this comment.
At line 241, when the engine is already initialized and new settings are provided, the code calls init($settings) directly on the engine. However, unlike the first initialization path (line 230), this doesn't check if the re-initialization succeeded. If init() returns false, the config will still be updated at line 243 with potentially invalid settings. Consider checking the return value of init() and handling the failure case, similar to the pattern at line 230-234.
|
|
||
| if (!$_this->isInitialized($engine)) { | ||
| trigger_error('Cache write not initialized: ' . $engine); | ||
| trigger_error('Cache write not initialized: ' . $engine, E_USER_WARNING); |
There was a problem hiding this comment.
At line 380-382, if the settings don't have a 'duration' key, it's set to 0. However, this default value (0) might not be appropriate for all cache engines. Some engines might interpret 0 as "cache forever" while others might treat it as "don't cache at all". The interface documentation doesn't specify what duration=0 means, and different engines might handle it differently. Consider either documenting the expected behavior of duration=0 in the interface, or using a more sensible default like 3600 (1 hour) that's more universally meaningful across engines.
| @@ -477,7 +728,10 @@ public function read($key) | |||
| * @return boolean True if the value was successfully deleted, false if it didn't exist or couldn't be removed | |||
| * @access public | |||
| */ | |||
| public function delete($key) {} | |||
| public function delete($key) | |||
| { | |||
| return false; | |||
| } | |||
|
|
|||
| /** | |||
| * Delete all keys from the cache | |||
| @@ -486,7 +740,10 @@ public function delete($key) {} | |||
| * @return boolean True if the cache was successfully cleared, false otherwise | |||
| * @access public | |||
| */ | |||
| public function clear($check) {} | |||
| public function clear($check) | |||
| { | |||
| return false; | |||
| } | |||
|
|
|||
| /** | |||
There was a problem hiding this comment.
The abstract class provides default implementations for gc(), delete(), and clear() methods. However, delete() and clear() return false by default (lines 736, 748), while gc() returns true (line 698). This inconsistency could be confusing for implementers. If an engine doesn't override these methods, gc() will silently succeed (returning true) while delete() and clear() will fail (returning false). This creates an asymmetric API. Consider either making gc() also return false by default for consistency, or documenting why gc() succeeds by default (e.g., "no garbage to collect" is a valid success state) while the others don't.
| return true; | ||
| } | ||
|
|
There was a problem hiding this comment.
The garbage collection probability check at line 297 has a potential division by zero issue. When probability is 0, the condition 'time() % $probability === 0' will cause a fatal error "Modulo by zero". The check 'if ($probability > 0 && ...)' correctly guards against this. However, consider whether probability = 0 should mean "never run GC" or if it has a different semantic meaning. If 0 means "always run GC", the logic should be adjusted. If 0 means "never run GC", the current implementation is correct but could benefit from a code comment explaining this behavior.
| !is_string($config['engine']) || | ||
| !is_array($config['settings']) | ||
| ) { | ||
| return false; | ||
| } | ||
|
|
There was a problem hiding this comment.
The validation at lines 88-93 checks if 'engine' and 'settings' keys exist and have the correct types, but it doesn't validate that the engine name is non-empty. An empty string for the engine name would pass this validation but would fail later when trying to load or initialize the engine. Consider adding a check like '|| $config['engine'] === ''' to ensure the engine name is not an empty string.
| && isset($_this->configs[$_this->name]) | ||
| ) { | ||
| $name = $_this->name; | ||
| $settings = $_this->configs[$_this->name]; | ||
| } else { |
There was a problem hiding this comment.
The condition at line 204-208 checks if $_this->name is a non-empty string before using it, but this same pattern is used multiple times in the codebase. Earlier in the method (line 196-197), there's already a guard ensuring $name is a non-empty string. However, $_this->name could theoretically be null (as declared at line 54 with type string|null). Consider initializing $_this->name to a default value like 'default' in the constructor or getInstance() to simplify these repeated null checks, or document why null is a valid state for the name property.
…ig, harden loadEngine Bug fixes: - Move init() call before clear() to match write/read/delete pattern - Check init() return value on engine re-initialization in config() - Reject empty engine name in resolveEngineConfig() - Distinguish file-not-found from path validation failure in loadEngine() Documentation: - Clarify gc() return semantics for no-op and auto-expiring engines - Document subclass responsibility for resource cleanup on repeated init() Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
The resolve-config + check-initialized + init-engine pattern was duplicated across gc(), write(), read(), delete(), and clear(). Extract into a private prepareEngine() method to bring duplication below SonarCloud's 3% threshold. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
| @@ -127,10 +250,13 @@ public function config($name = 'default', $settings = []) | |||
| ]; | |||
| } | |||
There was a problem hiding this comment.
When the same config name is used but with different settings (lines 242-251), if the engine is not initialized (isInitialized($engine) returns false at line 245), the code silently continues without initializing the engine or returning an error. This means $_this->settings($engine) at line 251 will return an empty array (per the settings() method implementation), potentially leading to inconsistent cache configuration.
Consider adding an else clause to handle the case where the engine is not initialized, either by calling $_this->engine($engine, $settings) or returning false with an appropriate error message.
| $duration = null; | ||
| } | ||
| } elseif ( |
There was a problem hiding this comment.
Setting $settings['duration'] to 0 as a fallback (line 389) will cause the write operation to fail at line 411-413, where any duration less than 1 causes the method to return false.
If the intent is to use a default duration when none is provided, consider using a positive default value (such as the engine's default duration) or removing this defensive check entirely, as the engine's init() method should already provide a default duration. Alternatively, if 0 duration caches should be rejected, this logic is correct but the settings should not default to 0.
|
|
||
| if (!$duration) { |
There was a problem hiding this comment.
The return value of init($settings) is not checked before attempting the cache operation. While the engine is verified to be initialized at line 392-396, the init() call at line 414 could theoretically fail if the new settings are invalid or cause initialization problems.
Consider checking the return value and returning false early if re-initialization fails, similar to the pattern at lines 246-249 in the config() method. This would provide more predictable error handling and clearer failure modes.
| } | ||
| $_this->engine[$engine]->init($settings); | ||
| $success = $_this->engine[$engine]->read($key); | ||
|
|
There was a problem hiding this comment.
The return value of init($settings) is not checked before attempting the read operation. While the engine is verified to be initialized at line 445-446, the init() call could theoretically fail with new settings.
Consider checking the return value and returning false early if re-initialization fails, for consistency with error handling patterns elsewhere in the codebase.
| * @return boolean True if the cache was successfully cleared, false otherwise | ||
| * @access public |
There was a problem hiding this comment.
The return value of init($settings) is not checked before attempting the delete operation. While the engine is verified to be initialized at line 478-479, the init() call could theoretically fail with new settings.
Consider checking the return value and returning false early if re-initialization fails, for consistency with error handling patterns elsewhere in the codebase.
| } | ||
|
|
There was a problem hiding this comment.
The return value of init($settings) is not checked before attempting the clear operation. While the engine is verified to be initialized at line 512-513, the init() call could theoretically fail with new settings.
Consider checking the return value and returning false early if re-initialization fails, for consistency with error handling patterns elsewhere in the codebase.
Mark the interface as deprecated in favor of the future Xoops\Core\Cache\EngineInterface in XOOPS 4.0. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…gine in config re-init - prepareEngine() now returns false if init() fails, preventing operations on a misconfigured engine - config() now attempts full engine initialization when same config name is reused but engine was not previously initialized Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
|
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #1596 +/- ##
==============================
==============================
☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Summary by CodeRabbit
Refactor
Bug Fixes