fix(database): return the documented failure value for a non-result argument - #143
Merged
Conversation
…rgument The result-set methods pass their argument straight to mysqli. A failed query returns false, and a mis-wired caller can pass the SQL string itself -- PHP 8 turns both into an uncaught TypeError inside mysqli, so a single bad call from a single module blanks the whole page. The '@' in front of those calls never covered it: it suppresses diagnostics, not thrown errors. Seen on xoops.org, where modules/core/brokenfile.php reached getRowsNum() with a string and took the page down. Each method now returns the failure value it already documents -- false for the fetch family, 0 for the counts, '' for the field names -- which is what every caller is written to handle, since that is what these return at end of data or on error anyway. The check reuses the existing public isResultSet(); only the reporting is new. The misuse is reported through addExtra() plus E_USER_WARNING, the same channel this class already uses when a caller misuses query()/exec(), so the module bug stays visible instead of quietly becoming an empty result.
…e guard The 2.7.3 note claimed an existing site picks up the new logging "only once its mainfile.php is updated from the new dist". That is wrong: include/common.php reads debug.php itself, so file logging works with no change to mainfile.php at all. Updating mainfile.php adds the XOOPS_DEBUG and XOOPS_DB_LEGACY_LOG constants and raises error reporting earlier than common.php runs, which matters only for bootstrap failures. Adds the visibility note that belongs beside it. debugLevel in xoops_data/configs/xoopsconfig.php clamps error_reporting to 0 for non-admins, but it does so after authentication, while display_errors is applied much earlier -- so errors during the database connection, config loading or preloads are still shown to any visitor.
|
Caution The consumer version of Gemini Code Assist on GitHub has been sunset. All code review activity has officially ceased. |
Contributor
Reviewer's GuideAdds defensive checks around MySQL result-set helper methods to avoid PHP 8 TypeErrors when callers pass non-result values, reporting misuse via the existing logging channel and returning the already-documented failure values, plus corrects and clarifies a note in the 2.7.3 changelog about debug/logging behavior and error visibility. Sequence diagram for invalid MySQL result handling in helper methodssequenceDiagram
actor ModuleCaller
participant XoopsMySQLDatabase
participant XoopsLogger
participant PHPErrorHandler
ModuleCaller->>XoopsMySQLDatabase: getRowsNum(result)
XoopsMySQLDatabase->>XoopsMySQLDatabase: isResultSet(result)
alt [result is not a result set]
XoopsMySQLDatabase->>XoopsMySQLDatabase: reportInvalidResult(__FUNCTION__, result)
XoopsMySQLDatabase->>XoopsLogger: addExtra(DB, message)
XoopsMySQLDatabase->>PHPErrorHandler: trigger_error(message, E_USER_WARNING)
XoopsMySQLDatabase-->>ModuleCaller: 0
else [result is a result set]
XoopsMySQLDatabase->>XoopsMySQLDatabase: mysqli_num_rows(result)
XoopsMySQLDatabase-->>ModuleCaller: rows_count
end
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
|
Important Review skippedAuto reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ 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 |
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Hey - I've found 1 issue
Prompt for AI Agents
Please address the comments from this code review:
## Individual Comments
### Comment 1
<location path="docs/changelog.270.txt" line_range="15-16" />
<code_context>
+- note on visibility: display_errors from debug.php is applied at include/common.php, while the debugLevel setting in xoops_data/configs/xoopsconfig.php clamps error_reporting to 0 for non-admins later in the same file, after authentication. Errors raised before that point — the database connection, config loading, preloads — are therefore still displayed to any visitor. Set display_errors to false on a public site and read the log instead; it records the same bootstrap window (mamba) in #139
+
+Database:
+- return the documented failure value instead of fataling when a result-set method is handed something that is not one. A failed query returns false and a mis-wired caller can pass the SQL string itself; PHP 8 turns both into an uncaught TypeError inside mysqli, so one bad call from one module blanked the whole page. Observed on xoops.org from modules/core/brokenfile.php. The misuse is still reported, so the caller's bug stays visible (mamba)
Security:
</code_context>
<issue_to_address>
**suggestion (typo):** Consider rephrasing 'fataling', which is not standard usage.
The phrase "instead of fataling" uses a non-standard verb and could be confusing. Consider something like "instead of causing a fatal error" or "instead of failing with a fatal error" for clarity.
```suggestion
Database:
- return the documented failure value instead of causing a fatal error when a result-set method is handed something that is not one. A failed query returns false and a mis-wired caller can pass the SQL string itself; PHP 8 turns both into an uncaught TypeError inside mysqli, so one bad call from one module blanked the whole page. Observed on xoops.org from modules/core/brokenfile.php. The misuse is still reported, so the caller's bug stays visible (mamba)
```
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Comment on lines
+15
to
+16
| Database: | ||
| - return the documented failure value instead of fataling when a result-set method is handed something that is not one. A failed query returns false and a mis-wired caller can pass the SQL string itself; PHP 8 turns both into an uncaught TypeError inside mysqli, so one bad call from one module blanked the whole page. Observed on xoops.org from modules/core/brokenfile.php. The misuse is still reported, so the caller's bug stays visible (mamba) |
Contributor
There was a problem hiding this comment.
suggestion (typo): Consider rephrasing 'fataling', which is not standard usage.
The phrase "instead of fataling" uses a non-standard verb and could be confusing. Consider something like "instead of causing a fatal error" or "instead of failing with a fatal error" for clarity.
Suggested change
| Database: | |
| - return the documented failure value instead of fataling when a result-set method is handed something that is not one. A failed query returns false and a mis-wired caller can pass the SQL string itself; PHP 8 turns both into an uncaught TypeError inside mysqli, so one bad call from one module blanked the whole page. Observed on xoops.org from modules/core/brokenfile.php. The misuse is still reported, so the caller's bug stays visible (mamba) | |
| Database: | |
| - return the documented failure value instead of causing a fatal error when a result-set method is handed something that is not one. A failed query returns false and a mis-wired caller can pass the SQL string itself; PHP 8 turns both into an uncaught TypeError inside mysqli, so one bad call from one module blanked the whole page. Observed on xoops.org from modules/core/brokenfile.php. The misuse is still reported, so the caller's bug stays visible (mamba) |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #143 +/- ##
============================================
+ Coverage 18.96% 19.18% +0.21%
- Complexity 8091 8198 +107
============================================
Files 670 672 +2
Lines 43925 44202 +277
============================================
+ Hits 8332 8481 +149
- Misses 35593 35721 +128 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
![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 `@docs/changelog.270.txt`:
- Around line 15-16: Update the Database changelog entry to state that
result-set methods return the documented failure value (`false`, `0`, or `''`),
covering fetch, count, and field metadata methods while preserving the existing
description of the TypeError scenario and misuse reporting.
🪄 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 Plus
Run ID: d5688173-e9c1-4091-afcc-9e78a3367150
📒 Files selected for processing (2)
docs/changelog.270.txthtdocs/class/database/mysqldatabase.php
Comment on lines
+15
to
+16
| Database: | ||
| - return the documented failure value instead of fataling when a result-set method is handed something that is not one. A failed query returns false and a mis-wired caller can pass the SQL string itself; PHP 8 turns both into an uncaught TypeError inside mysqli, so one bad call from one module blanked the whole page. Observed on xoops.org from modules/core/brokenfile.php. The misuse is still reported, so the caller's bug stays visible (mamba) |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Document all fallback values, not only false.
The implementation returns false for fetch methods, 0 for count methods, and '' for field metadata methods. Update this entry to say “the documented failure value (false, 0, or '')” so the release note accurately reflects the API contract.
🤖 Prompt for 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.
In `@docs/changelog.270.txt` around lines 15 - 16, Update the Database changelog
entry to state that result-set methods return the documented failure value
(`false`, `0`, or `''`), covering fetch, count, and field metadata methods while
preserving the existing description of the TypeError scenario and misuse
reporting.
mambax7
added a commit
that referenced
this pull request
Jul 28, 2026
…rect fix The 2.7.3 block carried two "Database:" headings, because #143's entry was added at the top of the section rather than beside the existing one from #130. They are one section now, and #143 gained the PR reference it was missing. #145 had no curated entry at all -- it landed after the section was written -- so the login redirect fix appears under a new "Redirects:" heading, with the detail the generated one-liner cannot carry: why a URL carrying an HTML entity hid the fault for exactly one hop, and what made it start accumulating.
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.
Summary
Stops a single module's bad call from blanking an entire page, and corrects a factual error in the 2.7.3 release notes.
Motivation
The result-set methods in
XoopsMySQLDatabasepass their argument straight to mysqli. Two things reach them in practice:falsePHP 8 turns both into an uncaught
TypeErrorinside mysqli, so one bad call takes the whole request down. The@on those calls never helped — it suppresses diagnostics, not thrown errors.Observed on xoops.org:
modules/core/brokenfile.phpreachedgetRowsNum()with a string.Approach
Each method returns the failure value it already documents —
falsefor the fetch family,0for the counts,''for field names. That is what these return at end of data or on error anyway, so every caller is already written to handle it.The check reuses the existing public
isResultSet(); only the reporting is new. Misuse goes throughaddExtra()plusE_USER_WARNING— the same channel this class already uses when a caller misusesquery()/exec()— so the module's bug stays visible rather than quietly becoming an empty result.Also: a correction to the 2.7.3 notes
The notes claimed a site picks up the new logging "only once its mainfile.php is updated from the new dist." That was wrong —
include/common.phpreadsdebug.phpitself, so file logging needs nomainfile.phpchange. Updatingmainfile.phpadds theXOOPS_DEBUG/XOOPS_DB_LEGACY_LOGconstants and raises reporting earlier thancommon.phpruns.A visibility note is added beside it:
debugLevelclampserror_reportingto 0 for non-admins, but after authentication, whiledisplay_errorsis applied much earlier — so bootstrap-time errors are still shown to any visitor.Testing
All nine guarded methods exercised against both production shapes (
falseand a SQL string), using the real base classes and the realXoopsLogger: 18/18 return the documented value rather than throwing, with 18 logger entries and 18 warnings raised.Summary by Sourcery
Guard MySQL result-set helper methods against invalid arguments to avoid PHP 8 type errors and ensure they return their documented failure values, and correct related release notes documentation.
Bug Fixes:
Enhancements:
Documentation:
Summary by CodeRabbit
Bug Fixes
Documentation