fix(pm): harden readpmsg save/delete flow#49
Merged
mambax7 merged 9 commits intoXOOPS:masterfrom May 6, 2026
Merged
Conversation
…InnoDB+utf8mb4
Three correctness fixes in pm/readpmsg.php plus a fresh-install schema
upgrade for pm/sql/mysql.sql.
readpmsg.php (case 'save'):
Build a $saveResults array of each operation's return value instead
of computing `$res = $res1 && $res2` against variables that may have
been undefined when their conditional branch was skipped. The new
shape (`!empty($saveResults) && !in_array(false, $saveResults,
true)`) accurately reflects "all attempted operations succeeded"
for any subset of {recipient, sender} × {delete, move}.
readpmsg.php (single-PM fetch):
Replace `[$pm] = $pm_handler->getObjects(...)` with
`$pmObjects = ...; $pm = $pmObjects[0] ?? null;` — the destructure
form raises "Undefined array key 0" on PHP 8 when getObjects()
returns an empty array. The explicit fallback keeps $pm at null
so the existing `is_object($pm)` guard handles the empty case.
readpmsg.php ($message default):
Initialise $message = null before the `if (is_object($pm))` block
so the trailing `assign('message', $message)` can never see an
undefined variable.
pm/sql/mysql.sql (fresh-install only):
- Switch ENGINE from MyISAM to InnoDB with utf8mb4 charset and
utf8mb4_unicode_ci collation. Brings pm_messages in line with
other XOOPS modern tables, supports full Unicode (emoji, CJK)
in subjects and message text, and gives the table real
transaction support.
- Add KEY from_userid_msg_time (from_userid, msg_time) — outbox
queries that select by sender ordered by time get a usable
index instead of scanning the outbox(from_userid, read_msg)
index and re-sorting.
- Existing installs keep their tables; the schema change applies
only to fresh installs from this SQL.
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #49 +/- ##
============================================
- Coverage 18.08% 18.04% -0.05%
Complexity 7840 7840
============================================
Files 665 665
Lines 42982 43070 +88
============================================
- Hits 7775 7773 -2
- Misses 35207 35297 +90 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
Warning Rate limit exceeded
To continue reviewing without waiting, purchase usage credits in the billing tab. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (1)
WalkthroughGuarded module handler retrieval with try/catch and instance check; message lookup changed from destructuring to explicit indexed access with null coalescing; ChangesPM Handler Defensive Checks and Message Flow
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
Contributor
There was a problem hiding this comment.
Pull request overview
This PR targets correctness and PHP 8+ safety in the PM “read message” flow, and updates the PM module’s MySQL schema file to modern defaults (InnoDB/utf8mb4) plus an index intended to improve outbox ordering queries.
Changes:
- Fixes
readpmsg.php“save” action result aggregation to avoid undefined variables and to correctly represent “all attempted operations succeeded”. - Makes single-message fetch null-safe when
getObjects()returns an empty array, and ensures$messageis always defined before assigning to the template. - Updates
htdocs/modules/pm/sql/mysql.sqlto InnoDB + utf8mb4 and adds an index on(from_userid, msg_time).
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| htdocs/modules/pm/readpmsg.php | Fixes save-action result handling and PHP 8 undefined-variable / undefined-array-key warnings. |
| htdocs/modules/pm/sql/mysql.sql | Switches table engine/charset and adds an index for sender+time lookups. |
Comment on lines
75
to
88
| if (Request::hasVar('delete_message', 'POST')) { | ||
| $res1 = $pm_handler->setTodelete($pm); | ||
| $res1 = $res1 ? $pm_handler->setTosave($pm, 0) : false; | ||
| $res1 = $pm_handler->setTodelete($pm); | ||
| $saveResults[] = $res1 ? $pm_handler->setTosave($pm, 0) : false; | ||
| } elseif (Request::hasVar('move_message', 'POST')) { | ||
| $res1 = $pm_handler->setTosave($pm, 0); | ||
| $saveResults[] = $pm_handler->setTosave($pm, 0); | ||
| } | ||
| } | ||
| if ($pm->getVar('from_userid') == $GLOBALS['xoopsUser']->getVar('uid')) { | ||
| if (Request::hasVar('delete_message', 'POST')) { | ||
| $res2 = $pm_handler->setFromdelete($pm); | ||
| $res2 = $res2 ? $pm_handler->setFromsave($pm, 0) : false; | ||
| $res2 = $pm_handler->setFromdelete($pm); | ||
| $saveResults[] = $res2 ? $pm_handler->setFromsave($pm, 0) : false; | ||
| } elseif (Request::hasVar('move_message', 'POST')) { | ||
| $res2 = $pm_handler->setFromsave($pm, 0); | ||
| $saveResults[] = $pm_handler->setFromsave($pm, 0); | ||
| } |
Comment on lines
+18
to
+22
| KEY inbox (`to_userid`,`read_msg`), | ||
| KEY outbox (`from_userid`, `read_msg`), | ||
| KEY from_userid_msg_time (`from_userid`, `msg_time`), | ||
| KEY prune (`msg_time`, `read_msg`, `from_save`, `to_delete`) | ||
| ) ENGINE=MyISAM; No newline at end of file | ||
| ) ENGINE=InnoDB DEFAULT CHARSET=utf8mb4 COLLATE=utf8mb4_unicode_ci; |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@htdocs/modules/pm/readpmsg.php`:
- Around line 73-90: The code calls methods on $pm_handler (setTodelete,
setTosave, setFromdelete, setFromsave, etc.) but
xoops_getModuleHandler('message') can return false; add an immediate guard after
the xoops_getModuleHandler('message') assignment to check if $pm_handler ===
false (or ! $pm_handler) and handle the error path (log and bail out, return an
error response, or redirect) so subsequent calls to $pm_handler methods in
readpmsg.php won’t trigger a PHP TypeError.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 94a4289f-6ae9-4a94-bc07-fcc35fb01667
📒 Files selected for processing (2)
htdocs/modules/pm/readpmsg.phphtdocs/modules/pm/sql/mysql.sql
mambax7
changed the title
Three correctness fixes in pm/readpmsg.php for the message-save flow.
readpmsg.php (case 'save' undefined-variable bug):
Build a $saveResults array of each operation's return value
instead of computing `$res = $res1 && $res2` against vars that
may have been undefined when their conditional branch was
skipped. The new shape (`!empty($saveResults) && !in_array(
false, $saveResults, true)`) accurately reflects "all attempted
operations succeeded" for any subset of {recipient, sender} ×
{delete, move}.
readpmsg.php (case 'save' delete-then-save false-failure):
setTodelete() / setFromdelete() call parent::delete($pm) (real
row removal) when the OTHER party has already deleted —
documented in pm/class/message.php:97-124. After that, the
follow-up setTosave($pm, 0) UPDATE affects 0 rows because the
row is gone, and updateAll() reports that as failure. Re-fetch
by msg_id after delete; if the row no longer exists, treat the
delete as success and skip the save-flag follow-up.
readpmsg.php (single-PM fetch null-safety):
Replace `[$pm] = $pm_handler->getObjects(...)` with
`$pmObjects = ...; $pm = $pmObjects[0] ?? null;` so the
destructure form doesn't raise "Undefined array key 0" on PHP 8
when getObjects() returns an empty array.
readpmsg.php ($message default + handler guard):
- Initialise $message = null before the `if (is_object($pm))`
block so the trailing `assign('message', $message)` can never
see an undefined variable.
- Guard $pm_handler immediately after xoops_getModuleHandler() —
that call can return false, and the file dispatches methods
on it unconditionally a few lines down. The new branch
triggers an E_USER_WARNING and redirects to XOOPS_URL.
The pm/sql/mysql.sql file from the original PR was reverted: PM
uses the priv_msgs table, defined in
htdocs/install/sql/mysql.structure.sql:462. xoops_version.php has
$modversion['sqlfile'] commented out, so pm/sql/mysql.sql is not
loaded by the installer. A separate installer-touching PR can
upgrade the priv_msgs schema (InnoDB + utf8mb4 + better outbox
index) for fresh installs.
Comment on lines
102
to
110
| if ($pm->getVar('from_userid') == $GLOBALS['xoopsUser']->getVar('uid')) { | ||
| if (Request::hasVar('delete_message', 'POST')) { | ||
| $res2 = $pm_handler->setFromdelete($pm); | ||
| $res2 = $res2 ? $pm_handler->setFromsave($pm, 0) : false; | ||
| if ($res2 && !is_object($pm_handler->get($msgId))) { | ||
| $saveResults[] = true; | ||
| } else { | ||
| $saveResults[] = $res2 ? $pm_handler->setFromsave($pm, 0) : false; | ||
| } | ||
| } elseif (Request::hasVar('move_message', 'POST')) { |
Comment on lines
+19
to
+22
| KEY outbox (`from_userid`, `read_msg`), | ||
| KEY from_userid_msg_time (`from_userid`, `msg_time`), | ||
| KEY prune (`msg_time`, `read_msg`, `from_save`, `to_delete`) | ||
| ) ENGINE=MyISAM; No newline at end of file | ||
| ) ENGINE=InnoDB DEFAULT CHARSET=utf8mb4 COLLATE=utf8mb4_unicode_ci; |
…revert SQL
readpmsg.php (case 'save' self-message edge case):
Add a $messageDeleted flag tracked across both branches. When
from_userid == to_userid == current uid, both branches fire on
the same row. If the recipient branch already deleted the row
(other party's flag was already set, so setTodelete() called
parent::delete()), the sender branch's setFromdelete() /
setFromsave() would affect 0 rows and updateAll() reports that
as failure. Skip the second branch entirely once the row is
gone.
readpmsg.php (handler-guard exit):
Add explicit `exit();` after the redirect_header() in the
handler-unavailable branch — defensive against any custom
preload that intercepts redirect_header() before its internal
exit. Also switched the redirect message from a hardcoded
English string to the universal _NOPERM constant.
pm/sql/mysql.sql (proper revert):
The previous round's `git checkout HEAD -- mysql.sql` reverted
to HEAD's tree (which still had the schema change), not to
upstream/master. The SQL file is now properly reverted: PR diff
vs master no longer touches mysql.sql. PM uses priv_msgs
(defined in install/sql/mysql.structure.sql), and
pm/xoops_version.php has the module SQL file commented out, so
pm/sql/mysql.sql is effectively dead code. Schema modernisation
for priv_msgs belongs in a separate installer-touching PR
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@htdocs/modules/pm/readpmsg.php`:
- Around line 33-42: The guard currently uses is_object($pm_handler) which is
safe at runtime but doesn't let PHPStan infer PM-specific methods; update the
check to narrow the type by either replacing is_object($pm_handler) with
$pm_handler instanceof PmMessageHandler or, immediately after the existing
guard, add a local docblock annotation /** `@var` PmMessageHandler $pm_handler */
so calls to setTodelete(), setTosave(), setFromdelete(), setFromsave(), and
getObjects() on $pm_handler are recognized by static analysis.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 2cab6d19-1478-4c26-a5aa-0a35e22136f3
📒 Files selected for processing (1)
htdocs/modules/pm/readpmsg.php
| // xoops_getModuleHandler() returns false when the PM module/handler | ||
| // can't be loaded. Bail out before any method call would fatal. | ||
| trigger_error('PM module handler unavailable', E_USER_WARNING); | ||
| redirect_header(XOOPS_URL, 2, _NOPERM); |
Comment on lines
+95
to
+101
| $res1 = $pm_handler->setTodelete($pm); | ||
| $res1 = $res1 ? $pm_handler->setTosave($pm, 0) : false; | ||
| if ($res1 && !is_object($pm_handler->get($msgId))) { | ||
| // Row gone — delete succeeded, save-flag is moot. | ||
| $saveResults[] = true; | ||
| $messageDeleted = true; | ||
| } else { | ||
| $saveResults[] = $res1 ? $pm_handler->setTosave($pm, 0) : false; |
Comment on lines
114
to
+120
| $res2 = $pm_handler->setFromdelete($pm); | ||
| $res2 = $res2 ? $pm_handler->setFromsave($pm, 0) : false; | ||
| if ($res2 && !is_object($pm_handler->get($msgId))) { | ||
| $saveResults[] = true; | ||
| $messageDeleted = true; | ||
| } else { | ||
| $saveResults[] = $res2 ? $pm_handler->setFromsave($pm, 0) : false; | ||
| } |
| } | ||
| $msg_id = Request::hasVar('msg_id', 'POST') ? Request::getInt('msg_id', 0, 'POST') : Request::getInt('msg_id', 0, 'GET'); | ||
| $pm_handler = xoops_getModuleHandler('message'); | ||
| if (!is_object($pm_handler)) { |
mambax7
changed the title
…_ACTION_ERROR Address PR XOOPS#49 follow-up review: readpmsg.php (delete prediction without extra SELECT): Replace the post-delete `$pm_handler->get($msgId)` round-trip with a predicate computed BEFORE the call, using the same conditions the handler itself uses (pm/class/message.php): setTodelete hard-deletes when (from_delete != 0 && from_userid != 0) setFromdelete hard-deletes when (to_delete != 0) Avoids one extra SELECT per delete operation, and avoids conflating "row removed by design" with "SELECT failed" — both of which `get()` returns null for. readpmsg.php (handler guard): - Switch the `is_object($pm_handler)` guard to `$pm_handler instanceof PmMessageHandler`. Gives static analysers (PHPStan) the type narrowing they need to recognise the PM-specific methods (setTodelete etc.) used further down. Also added a `@var` docblock. - Use `_PM_ACTION_ERROR` (verified in pm/language/english/main.php:77) instead of `_NOPERM` for the redirect message — this is an internal handler-load failure, not an authorisation failure, and `_NOPERM` would mislead the user. - Wrap the `instanceof` in parens (`!($pm_handler instanceof PmMessageHandler)`) — pre-commit hook caught the precedence bug on the first attempt without parens. PR description: dropped the now-stale claims about pm/sql/mysql.sql changes (the SQL was reverted in the previous round). Title shortened to "fix(pm): harden readpmsg save/delete flow".
Comment on lines
+33
to
+47
| /** @var PmMessageHandler $pm_handler */ | ||
| $pm_handler = xoops_getModuleHandler('message'); | ||
| if (!($pm_handler instanceof PmMessageHandler)) { | ||
| // xoops_getModuleHandler() returns false when the PM module/handler | ||
| // can't be loaded. The instanceof guard (rather than plain is_object) | ||
| // also gives static analysers like PHPStan the type narrowing they | ||
| // need to recognise the PM-specific methods (setTodelete etc.) used | ||
| // below. _PM_ACTION_ERROR is a PM module language constant; using a | ||
| // generic error message here rather than _NOPERM, since this is an | ||
| // internal load failure, not an authorisation failure. | ||
| trigger_error('PM module handler unavailable', E_USER_WARNING); | ||
| redirect_header(XOOPS_URL, 2, _PM_ACTION_ERROR); | ||
| // redirect_header() calls exit() internally, but the explicit exit | ||
| // is defensive against custom preloads that might intercept it. | ||
| exit(); |
Address PR XOOPS#49 follow-up review: readpmsg.php (handler resolution): Wrap the `xoops_getModuleHandler('message')` call in a try/catch. Verified two throw paths in `htdocs/include/functions.php`: line 73: throw \Exception('No Module is loaded') when no $module_dir argument is passed AND there is no $xoopsModule context (direct script entry, preload misfire, etc.). line 95: throw \Exception('Handler does not exist') when the handler class cannot be resolved (unless the $optional = true argument is passed — which we don't, and even if we did, it only covers this second path). The previous `instanceof PmMessageHandler` guard ran AFTER the call, so the exception escaped uncaught and became a 500. The catch (\Throwable) handles both Exception variants (and any future error class). On failure: emit an E_USER_WARNING with the exception message for ops triage, then redirect with _PM_ACTION_ERROR and explicit exit. The `instanceof` check is retained AFTER the try/catch as a defensive guard and to give static analysers (PHPStan) the type narrowing for the PM-specific methods (setTodelete etc.) used further down.
Comment on lines
+45
to
+48
| } catch (\Throwable $e) { | ||
| trigger_error('PM module handler unavailable: ' . $e->getMessage(), E_USER_WARNING); | ||
| redirect_header(XOOPS_URL, 2, _PM_ACTION_ERROR); | ||
| // redirect_header() calls exit() internally, but the explicit exit |
…oad redirects Address PR XOOPS#49 follow-up review: readpmsg.php (handler resolution failure path): When xoops_getModuleHandler('message') throws "No Module is loaded" (no $xoopsModule context, e.g. direct script entry or preload misfire), the PM language file has not been loaded yet either. The catch block's redirect previously referenced _PM_ACTION_ERROR directly, which on PHP 8+ raises an Error for undefined constants and masks the original failure with a less informative fatal page. Resolve a safe fallback message BEFORE the try/catch: $pmActionError = defined('_PM_ACTION_ERROR') ? constant('_PM_ACTION_ERROR') : 'Operation failed'; and use it in both redirect paths (catch block and the post-catch instanceof guard). When the language file IS loaded, behaviour is unchanged; when it isn't, the user sees a generic English message instead of a 500.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@htdocs/modules/pm/readpmsg.php`:
- Around line 47-64: The two trigger_error calls in the handler-load guards
should prefix their messages with the script name using basename(__FILE__) to
follow project logging guidelines; update the first trigger_error inside the
catch (currently 'PM module handler unavailable: ' . $e->getMessage()) and the
second trigger_error (currently 'PM module handler unavailable') to include the
basename(__FILE__) at the start of the message while preserving E_USER_WARNING
and the existing redirect_header(XOOPS_URL, 2, $pmActionError) behavior and
pm_handler variable usage.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 61e66e18-ab82-4d6f-8e6a-daa2e34ba444
📒 Files selected for processing (1)
htdocs/modules/pm/readpmsg.php
Address PR XOOPS#49 follow-up review (CodeRabbit r3193522028): readpmsg.php (handler-load failure path): Prefix the two trigger_error() messages in the handler-load guard with basename(__FILE__) so log lines are traceable to the originating script without exposing the absolute server path. Behaviour, the $pmActionError fallback, redirect target, and exit semantics are unchanged.
Comment on lines
+133
to
138
| } else { | ||
| $saveResults[] = $res1 ? $pm_handler->setTosave($pm, 0) : false; | ||
| } | ||
| } elseif (Request::hasVar('move_message', 'POST')) { | ||
| $res1 = $pm_handler->setTosave($pm, 0); | ||
| $saveResults[] = $pm_handler->setTosave($pm, 0); | ||
| } |
Comment on lines
+154
to
159
| } else { | ||
| $saveResults[] = $res2 ? $pm_handler->setFromsave($pm, 0) : false; | ||
| } | ||
| } elseif (Request::hasVar('move_message', 'POST')) { | ||
| $res2 = $pm_handler->setFromsave($pm, 0); | ||
| $saveResults[] = $pm_handler->setFromsave($pm, 0); | ||
| } |
Address PR XOOPS#49 follow-up review (Copilot r3193641619, r3193641692): readpmsg.php (case 'save', recipient and sender branches): setTosave($pm, 0) and setFromsave($pm, 0) call updateAll(), which returns false for 0 affected rows. When the target flag is already 0, the UPDATE is a true no-op but the call reports failure, and the combined $saveResults result is then dragged to false even though the user-visible action completed. Concrete failure: a self-message (from_userid == to_userid) saved with from_save = 1 and to_save = 0: - recipient branch's setTosave($pm, 0) UPDATEs 0 rows -> false - sender branch's setFromsave($pm, 0) succeeds - combined result: false -> user gets a spurious error toast Guard the save-flag clears at all four call sites (delete-message and move-message paths, recipient and sender branches): if the flag is already 0, treat the call as a successful no-op and skip the setter entirely. Same shape as the existing hard-delete sequencing that already handles the same updateAll() quirk for parent::delete() ordering. Codecov coverage of these branches still requires a PM controller test harness that does not exist yet; tracked as a separate effort.
Comment on lines
+46
to
+47
| // in both redirect paths. | ||
| $pmActionError = defined('_PM_ACTION_ERROR') ? constant('_PM_ACTION_ERROR') : 'Operation failed'; |
Comment on lines
+56
to
+65
| // redirect_header() calls exit() internally, but the explicit exit | ||
| // is defensive against custom preloads that might intercept it. | ||
| exit(); | ||
| } | ||
| if (!($pm_handler instanceof PmMessageHandler)) { | ||
| // Internal load failure (not an authorisation failure), so use the | ||
| // PM action-error message rather than _NOPERM. | ||
| trigger_error(basename(__FILE__) . ': PM module handler unavailable', E_USER_WARNING); | ||
| redirect_header(XOOPS_URL, 2, $pmActionError); | ||
| exit(); |
Address PR XOOPS#49 follow-up review (Copilot r3193689909, r3193689943): readpmsg.php (handler-load failure path): - Localize _PM_ACTION_ERROR fallback when possible: try xoops_loadLanguage('main', 'pm') first when the constant is undefined, then resolve. This keeps the redirect localized in the "No Module is loaded" case where the PM language file typically hasn't been auto-loaded yet but the loader itself is available. The literal 'Operation failed' fallback remains if loading fails, so the catch path stays bulletproof. - Drop the explicit exit() after redirect_header() in both the catch block and the instanceof guard. redirect_header() ends with an unconditional exit() (htdocs/include/functions.php:858), so the trailing exit() is unreachable. This also matches the existing style in this same file (lines 25, 73, 197 — every other redirect_header() call has no trailing exit()). The earlier rationale about "defensive against preload interception" was unfounded — PHP's exit() has no userland interception path.
|
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Correctness fixes in pm/readpmsg.php for the message save/delete flow.
What changed
htdocs/modules/pm/readpmsg.php:$res = $res1 && $res2against vars that may have been undefined when the corresponding branch was skipped. Switched to a$saveResults[]array; final$resis!empty($saveResults) && !in_array(false, $saveResults, true).setTodelete()/setFromdelete()(inpm/class/message.php:97-124) hard-delete the row when the OTHER party has already deleted. The follow-upsetTosave()/setFromsave()then affects 0 rows andupdateAll()reports that as failure. Now predicts the hard-delete using the SAME predicates the handler itself uses (from_delete != 0 && from_userid != 0for setTodelete;to_delete != 0for setFromdelete) — no extra SELECT, distinguishes "row removed by design" from "SELECT failed".from_userid == to_userid == current uid, both branches fire on the same row. Once branch 1 hard-deletes the row, branch 2's UPDATE would affect 0 rows. A$messageDeletedflag now skips branch 2 once we know the row is gone.[$pm] = $pm_handler->getObjects(...)with$pmObjects = ...; $pm = $pmObjects[0] ?? null;so the destructure form doesn't raise "Undefined array key 0" on PHP 8 when getObjects() returns an empty array.$messagedefault + handler guard: initialise$message = nullbefore theis_object($pm)block so the trailingassign('message', $message)can never see an undefined variable. Guard$pm_handlerwithinstanceof PmMessageHandler(also gives PHPStan the type narrowing for the PM-specific methods used below) —xoops_getModuleHandler()can returnfalse. Redirect with_PM_ACTION_ERRORand explicitexit().NOT in this PR
The
pm/sql/mysql.sqlschema upgrade (InnoDB + utf8mb4 + new outbox index) was reverted: PM uses thepriv_msgstable defined inhtdocs/install/sql/mysql.structure.sql:462.pm/xoops_version.phphas$modversion['sqlfile']commented out, sopm/sql/mysql.sqlis effectively dead code. A separate installer-touching PR can upgradepriv_msgsfor fresh installs.Test plan
from_save = 1/to_save = 1) — confirm success pathSummary by CodeRabbit