fix Using null as an array offset is deprecated Refactor cache subsystem to support multiple engines #1596
+338
−78
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
…instead fix offset is deprecated, use an empty string instead replacing extract()
Contributor
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.
Contributor
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.
Contributor
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.
Comment on lines
99
to
100
|
|
||
|
|
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.
Suggested change
Comment on lines
+336
to
339
| $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.
Suggested change
| if (!isset($_this->engine[$engine]) || $_this->engine[$engine] === null) { | |
| return false; | |
| } |
Contributor
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.
Comment on lines
160
to
164
| $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.
Contributor
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.
Contributor
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:
📝 WalkthroughWalkthroughReplaces 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)
Comment |
![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)
709-718:⚠️ Potential issue | 🟡 MinorEmpty method stubs can cause TypeError when return value is used.
The
delete()andclear()methods have empty bodies that implicitly returnnull. However,XoopsCache::delete()(line 436) andXoopsCache::clear()(line 471) declare return type: booland return the result from these engine methods. If a concrete engine class doesn't override these methods, PHP will throw a TypeError.🔧 Proposed fix: return explicit bool values
public function delete($key) { + return false; } /** * Delete all keys from the cache * * `@param` boolean $check if true will check expiration, otherwise delete all * `@return` boolean True if the cache was successfully cleared, false otherwise * `@access` public */ public function clear($check) { + return false; }Alternatively, make these methods
abstractto force concrete implementations.
🧹 Nitpick comments (1)
htdocs/class/cache/xoopscache.php (1)
194-199: Minor: Redundant null check.The check
$_this->configs !== nullon line 195 is redundant since$this->configsis initialized as an empty array on line 47 and can never be null. Consider simplifying:- } elseif ( - $_this->configs !== null - && is_string($_this->name) + } elseif ( + is_string($_this->name) && $_this->name !== '' && isset($_this->configs[$_this->name]) ) {
Contributor
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.
Comments suppressed due to low confidence (1)
htdocs/class/cache/xoopscache.php:225
$enginedefaults to'file'when$settings['engine']is missing, but that default isn’t written back into$settings/configs. This can leave the active config without an'engine'key, which breaksresolveEngineName()/isInitialized()when called without an explicit engine. Consider normalizing by setting$settings['engine'] = $enginebefore initializing/storing the config.
$engine = (!empty($settings['engine']) && is_string($settings['engine']))
? $settings['engine']
: 'file';
if ($name !== $_this->name) {
if ($_this->engine($engine, $settings) === false) {
trigger_error("Cache Engine {$engine} is not set", E_USER_WARNING);
return false;
}
$_this->name = $name;
$_this->configs[$name] = $_this->settings($engine);
}
htdocs/class/cache/xoopscache.php
Outdated
| } | ||
|
|
||
| $gcResult = $_this->engine[$engine]->gc(); | ||
|
|
There was a problem hiding this comment.
gc() now returns (bool) $gcResult. For engines whose gc() returns null/void (e.g., engines that inherit the empty base implementation), this will always return false even though GC may be a no-op/success. Consider standardizing engine gc() to return a boolean, or treat null as success here (e.g., return true when $gcResult === null).
Suggested change
| if ($gcResult === null) { | |
| // Treat null/void gc() implementations as a successful no-op | |
| return true; | |
| } |
Comment on lines
168
to
183
| if (is_array($name)) { | ||
| extract($name); | ||
| $config = $name; | ||
|
|
||
| if (isset($config['name']) && is_string($config['name'])) { | ||
| $name = $config['name']; | ||
| unset($config['name']); | ||
| } else { | ||
| $name = 'default'; | ||
| } | ||
|
|
||
| if (isset($config['settings']) && is_array($config['settings'])) { | ||
| $settings = $config['settings']; | ||
| } else { | ||
| // Back-compat: treat array input as settings when no nested "settings" exists | ||
| $settings = $config; | ||
| } |
There was a problem hiding this comment.
When $name is passed as an array containing both top-level engine and nested settings, the current logic drops the top-level engine (because $settings is replaced with $config['settings']). This makes inputs like ['engine' => 'memcache', 'settings' => [...]] silently fall back to the default engine. Consider explicitly carrying engine (and possibly other top-level keys) into $settings or honoring the top-level engine when present.
Copyable Unit Test EditsCopyable Editstests/unit/htdocs/class/cache/XoopsCacheTest.phpThis is a new file. tests/unit/htdocs/include/VersionTest.phpThis is a new file. tests/unit/htdocs/kernel/XoopsModuleTest.phpThis is a new file. |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@htdocs/class/cache/apc.php`:
- Around line 84-87: Update the PHPDoc return types to match the actual boolean
casts: change the `@return` annotations for the write method (public function
write) and the delete method (public function delete) in the APC cache class
from array (or mixed/other) to bool, and scan the class for any other docblocks
that still claim array returns and update them to bool so the docs match the
(bool) casted return values from apc_store/apc_delete.
Comment on lines
+78
to
+86
| * @param string $key Identifier for the data | ||
| * @param mixed $value Data to be cached | ||
| * @param integer $duration How long to cache the data, in seconds | ||
| * @return bool|array Returns TRUE on success or FALSE on failure | array with error keys. | ||
| * @access public | ||
| */ | ||
| public function write($key, $value, $duration = null) | ||
| { | ||
| return apc_store($key, $value, $duration); | ||
| return (bool) apc_store($key, $value, $duration); |
There was a problem hiding this comment.
The write() return type in the docblock (bool|array) no longer matches the implementation since the result is now cast to bool. Please update the PHPDoc to reflect the actual bool return value (and clarify the behavior for array keys, if that’s still supported).
Comment on lines
+104
to
+110
| * @param string $key Identifier for the data | ||
| * @return bool|string[] Returns TRUE on success or FALSE on failure. For array of keys returns list of failed keys. | ||
| * @access public | ||
| */ | ||
| public function delete($key) | ||
| { | ||
| return apc_delete($key); | ||
| return (bool) apc_delete($key); |
There was a problem hiding this comment.
The delete() return type in the docblock (bool|string[]) no longer matches the implementation since the result is now cast to bool. Please update the PHPDoc to reflect the actual bool return value.
|
|
||
| $engine = $config['engine']; | ||
| $settings = $config['settings']; | ||
|
|
There was a problem hiding this comment.
resolveEngineConfig() provides $settings to write(), but write() later assumes keys like duration exist in $settings when $duration is not provided. For engines that implement XoopsCacheEngineInterface without extending XoopsCacheEngine (or that don’t populate defaults in settings()), this can lead to undefined index notices. Consider ensuring required defaults (e.g., duration) are present when resolving config / initializing engines, or defensively defaulting when the key is absent.
Suggested change
| if (!isset($settings['duration'])) { | |
| $settings['duration'] = 0; | |
| } |
Comment on lines
133
to
139
| if (!class_exists('XoopsCache' . ucfirst($name))) { | ||
| if (file_exists($file = __DIR__ . '/' . strtolower($name) . '.php')) { | ||
| $baseDir = realpath(__DIR__); | ||
| $candidate = __DIR__ . '/' . strtolower($name) . '.php'; | ||
| $file = $baseDir !== false ? realpath($candidate) : false; | ||
|
|
||
| if ($file !== false && strpos($file, $baseDir . DIRECTORY_SEPARATOR) === 0) { | ||
| include $file; |
There was a problem hiding this comment.
loadEngine() returns true after including the candidate file without re-checking that the expected engine class was actually declared. If the file exists but doesn’t define XoopsCache{Name} (or defines a wrong class), new $cacheClass() will fatally error. Re-check class_exists($cacheClass, false) after include and return false / warn if it’s still missing.
Comment on lines
406
to
412
| * @param string|array|null $config name of the configuration to use | ||
| * @return mixed The cached data, or false if: | ||
| * - Configuration is invalid | ||
| * - Data doesn't exist | ||
| * - Data has expired | ||
| * - Error occurred during retrieval | ||
| * @access public |
There was a problem hiding this comment.
The read() PHPDoc claims it returns false when data doesn’t exist/has expired, but at least one built-in engine (XoopsCacheModel::read) returns null on a miss. Either normalize null to false in XoopsCache::read() (to make behavior consistent across engines) or adjust the docblock to document possible null return values.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@htdocs/class/cache/xoopscache.php`:
- Around line 445-448: The code normalizes a cached null to false which makes a
stored null indistinguishable from a miss; change the return to preserve actual
cached values by returning $result directly instead of converting null to false
(i.e., remove the ternary normalization after calling
$_this->engine[$engine]->read($key)). If you need to distinguish misses from
stored nulls, add or use an explicit miss sentinel or a separate exists/has
method on the cache engine rather than coalescing null to false; reference the
engine read call ($_this->engine[$engine]->read) and the $result variable when
applying the change.
Comment on lines
+348
to
350
| public static function write($key, $value, $duration = null): bool | ||
| { | ||
| $key = substr(md5(XOOPS_URL), 0, 8) . '_' . $key; |
There was a problem hiding this comment.
XoopsCache::write() now has a : bool return type, but the underlying engine write() implementations are not guaranteed to return a boolean (e.g. the bundled DB/model engine ultimately calls XoopsPersistableObjectHandler::insert(), which returns an object ID). If a non-bool bubbles up, PHP will throw a TypeError due to the declared return type. Normalize the engine result before returning (e.g., cast to bool or map non-false values to true).
Comment on lines
590
to
656
| 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.
htdocs/class/cache/xoopscache.php
Outdated
| * @license GNU GPL 2 (https://www.gnu.org/licenses/gpl-2.0.html) | ||
| * @link https://xoops.org | ||
| * @since 2.5.12 | ||
| * @deprecated 4.0.0 This interface will be removed in a future version. |
There was a problem hiding this comment.
The interface XoopsCacheEngineInterface is marked as deprecated in version 4.0.0, but it's being newly introduced in this PR. Consider removing the deprecation notice since this is a new interface being added to the codebase. If the interface is truly intended to be deprecated immediately upon introduction, this should be explained in the documentation with a clear migration path.
Suggested change
| * @deprecated 4.0.0 This interface will be removed in a future version. |
htdocs/class/cache/xoopscache.php
Outdated
| /** | ||
| * Garbage collection - permanently remove all expired and deleted data | ||
| * | ||
| * @return bool|null True on success, false on failure, null for no-op |
There was a problem hiding this comment.
The interface documents that gc() returns bool|null with "null for no-op", but this is inconsistent with typical PHP interface design where methods should have predictable return types. Consider standardizing gc() to always return bool, where true indicates successful cleanup (or successful no-op) and false indicates failure. This would make the API more predictable and consistent with the other interface methods.
Suggested change
| * @return bool|null True on success, false on failure, null for no-op | |
| * @return bool True on success (including no-op when there is nothing to clean), false on failure |
| * @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.
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);
Comment on lines
+223
to
+227
| $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).
htdocs/class/cache/xoopscache.php
Outdated
| * 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 |
There was a problem hiding this comment.
The interface doc for read() states it returns false when data doesn't exist/expired, but at least one built-in engine (XoopsCacheModel::read()) returns null on a cache miss. To avoid misleading implementers and callers, either update the interface contract/docs to allow null (and/or other engine-specific miss values) or normalize built-in engines to consistently return false on a miss.
Suggested change
| * @return mixed The cached data, or false if the data doesn't exist or has expired | |
| * @return mixed The cached data, or a falsy value (such as false or null) if the data | |
| * doesn't exist or has expired. Implementations MUST NOT throw on cache | |
| * miss; callers SHOULD treat any falsy return value as a cache miss. |
|
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Summary by CodeRabbit
Refactor
Bug Fixes