-
-
Notifications
You must be signed in to change notification settings - Fork 60
fix for https://github.com/XOOPS/XoopsCore25/issues/1588 #1589
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR refactors the XOOPS session handler to address issue #1588 by splitting the implementation into PHP version-specific files. The main session.php file now acts as a loader that includes either session74.php (for PHP < 8.0) or session80.php (for PHP >= 8.0), enabling proper type declarations and interface implementations for each PHP version.
Key changes:
- Introduced version-specific session handlers with appropriate type hints for their respective PHP versions
- PHP 8.0+ handler implements SessionUpdateTimestampHandlerInterface for lazy timestamp updates
- Consolidated domain validation logic across both implementations
Reviewed Changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
| htdocs/kernel/session.php | Converted to a loader shim that conditionally includes the appropriate version-specific handler |
| htdocs/kernel/session74.php | New PHP 7.4-compatible handler without type declarations, implementing SessionHandlerInterface |
| htdocs/kernel/session80.php | New PHP 8.0+ handler with full type declarations and SessionUpdateTimestampHandlerInterface support |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
| * @copyright (c) 2000-2025 XOOPS Project (https://xoops.org) | ||
| */ | ||
|
|
||
| defined('XOOPS_ROOT_PATH') || exit('Restricted access'); |
Copilot
AI
Oct 23, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Duplicate security check: line 20 already contains this exact check, making this second check at line 32 redundant.
| defined('XOOPS_ROOT_PATH') || exit('Restricted access'); |
| $this->gc_force(); | ||
| return true; | ||
| } | ||
|
|
Copilot
AI
Oct 23, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The return type should be string|false to match SessionHandlerInterface requirements in PHP 8.0+. However, the method returns an empty string ('') when the session is not found (line 111), and returns false on storage failure (line 106). Consider documenting this distinction in a docblock, as it deviates from the typical pattern where both cases might return false.
| /** | |
| * Read session data from storage. | |
| * | |
| * Returns the session data as a string if found and valid. | |
| * Returns an empty string ('') if the session is not found or the IP does not match. | |
| * Returns false on storage failure (e.g., database error). | |
| * | |
| * This behaviour deviates from the typical pattern where both "not found" and "error" cases return false. | |
| * | |
| * @param string $sessionId Session ID | |
| * @return string|false Session data string, empty string if not found/IP mismatch, or false on storage failure | |
| */ |
| if ($row === false) { | ||
| return ''; // not found -> empty string | ||
| } | ||
| list($sess_data, $sess_ip) = $row; |
Copilot
AI
Oct 23, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[nitpick] Consider using array destructuring syntax [$sess_data, $sess_ip] = $row; instead of list() for consistency with the session80.php implementation (line 114) and modern PHP practices.
| list($sess_data, $sess_ip) = $row; | |
| [$sess_data, $sess_ip] = $row; |
| ); | ||
| $result = $this->db->queryF($sql); | ||
| if (!$this->db->isResultSet($result)) { | ||
| return ''; // failure -> empty string (7.4-safe) |
Copilot
AI
Oct 23, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Returning empty string on database failure differs from session80.php which returns false (line 106). This inconsistency in error handling between PHP versions could lead to different behavior. Consider aligning both implementations to return false on storage failures for consistency.
| return ''; // failure -> empty string (7.4-safe) | |
| return false; // failure -> false for consistency with session80.php |
That keeps aligned with PHP 8.4’s deprecations and cookies-only sessions.
No description provided.