Fix missing isResultSet() guard and remove @ error suppression in mod…#1646
Merged
mambax7 merged 6 commits intoMar 14, 2026
Merged
Conversation
…ule update scripts - Add proper isResultSet() + instanceof \mysqli_result check before fetchArray() loop in system update (SHOW INDEX query) - Replace @Unlink() with is_file() guard in pm and profile update scripts - Fix SourceFileTestTrait to detect git worktrees (is_file .git) - Add source-level convention tests for system, pm, and profile modules
|
Warning Rate limit exceeded
⌛ 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 (7)
WalkthroughTightens result-set validation (requires \mysqli_result), fixes operator-precedence checks, replaces suppressed unlink() calls with is_file() guards, adds PHPUnit checks enforcing these conventions across module scripts, and updates test helper to detect a Changes
Sequence Diagram(s)(omitted) Estimated code review effort🎯 3 (Moderate) | ⏱️ ~30 minutes Suggested reviewers
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
📝 Coding Plan
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 |
There was a problem hiding this comment.
Pull request overview
This PR hardens module update scripts by replacing fragile/unsafe DB result handling and removing @ error suppression, and adds unit tests that enforce these source-level conventions across system/pm/profile modules.
Changes:
- Add an
isResultSet()+instanceof \mysqli_resultguard before iterating over SHOW INDEX results in the system module update script. - Replace
@unlink()calls with anis_file()guard in pm/profile update scripts. - Improve the source-file test harness to support git worktrees and add new convention tests for system/pm/profile update/install scripts.
Reviewed changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
htdocs/modules/system/include/update.php |
Adds stricter result guarding before iterating over a SHOW INDEX query result. |
htdocs/modules/profile/include/update.php |
Removes @unlink() suppression and guards file deletion with is_file(). |
htdocs/modules/pm/include/update.php |
Removes @unlink() suppression and guards file deletion with is_file(). |
tests/unit/htdocs/modules/system/SourceFileTestTrait.php |
Updates repo-root detection to support .git being a file (git worktrees). |
tests/unit/htdocs/modules/system/SystemUpdateTest.php |
Adds source-level convention checks for system’s update script. |
tests/unit/htdocs/modules/profile/ProfileUpdateTest.php |
Adds source-level convention checks for profile’s update script. |
tests/unit/htdocs/modules/pm/PmUpdateTest.php |
Adds source-level convention checks for pm’s update script. |
tests/unit/htdocs/modules/pm/PmInstallTest.php |
Adds source-level convention checks for pm’s install script. |
Comment on lines
+81
to
83
| $result = $xoopsDB->query($sql); | ||
| if (!$xoopsDB->isResultSet($result) || !$result instanceof \mysqli_result) { | ||
| xoops_error($xoopsDB->error() . '<br>' . $sql); |
There was a problem hiding this comment.
The result-set guard mixes isResultSet($result) with a bare !$result instanceof \mysqli_result. For consistency with other code in this repo (e.g. patterns that check false === $result first and parenthesize the instanceof), consider rewriting this guard to (1) explicitly handle $result === false before calling isResultSet(), and (2) use !($result instanceof \mysqli_result) for clarity. This avoids relying on operator precedence and makes the failure path safer/readable.
Comment on lines
+55
to
+62
| public function updateScriptUsesIsResultSetBeforeFetchArray(): void | ||
| { | ||
| // Every fetchArray() call should be preceded by an isResultSet() check | ||
| // in the same function scope | ||
| $this->assertStringContainsString( | ||
| 'isResultSet($result)', | ||
| $this->sourceContent, | ||
| 'system/include/update.php must use isResultSet() before fetchArray()' |
There was a problem hiding this comment.
The test name/comment says it validates that every fetchArray() call is preceded by an isResultSet() check, but the assertion only checks that the file contains the substring isResultSet($result) somewhere. Either tighten the test to actually verify the intended guard near each fetch, or adjust the comment/test name so it matches what’s being asserted.
Suggested change
| public function updateScriptUsesIsResultSetBeforeFetchArray(): void | |
| { | |
| // Every fetchArray() call should be preceded by an isResultSet() check | |
| // in the same function scope | |
| $this->assertStringContainsString( | |
| 'isResultSet($result)', | |
| $this->sourceContent, | |
| 'system/include/update.php must use isResultSet() before fetchArray()' | |
| public function updateScriptUsesIsResultSetGuard(): void | |
| { | |
| // The update script should use an isResultSet() guard before working | |
| // with database result sets | |
| $this->assertStringContainsString( | |
| 'isResultSet($result)', | |
| $this->sourceContent, | |
| 'system/include/update.php must use an isResultSet() guard on result sets' |
Comment on lines
+69
to
+73
| // The SHOW INDEX query should use isResultSet() + instanceof guard | ||
| $this->assertMatchesRegularExpression( | ||
| '/isResultSet\(\$result\)\s*\|\|\s*!\$result\s+instanceof\s+\\\\mysqli_result/', | ||
| $this->sourceContent, | ||
| 'system/include/update.php must use isResultSet() || !$result instanceof \\mysqli_result pattern' |
There was a problem hiding this comment.
updateScriptUsesProperResultSetCheckPattern() currently hard-codes a very specific textual pattern for the SHOW INDEX guard. If the production code is updated to a safer/clearer variant (e.g., false === $result || ... || !($result instanceof \mysqli_result)), this test will fail even though behavior is correct. Consider relaxing the regex to accept equivalent guard forms (including parentheses around instanceof and/or an explicit $result === false check) to reduce brittleness.
Suggested change
| // The SHOW INDEX query should use isResultSet() + instanceof guard | |
| $this->assertMatchesRegularExpression( | |
| '/isResultSet\(\$result\)\s*\|\|\s*!\$result\s+instanceof\s+\\\\mysqli_result/', | |
| $this->sourceContent, | |
| 'system/include/update.php must use isResultSet() || !$result instanceof \\mysqli_result pattern' | |
| // The SHOW INDEX query should use isResultSet() together with a negated instanceof \mysqli_result guard | |
| $this->assertMatchesRegularExpression( | |
| '/isResultSet\(\$result\)[^;]*\|\|[^;]*!\s*\(?\$result\s+instanceof\s+\\\\mysqli_result\)?/', | |
| $this->sourceContent, | |
| 'system/include/update.php must combine isResultSet($result) with a negated $result instanceof \\mysqli_result guard for the SHOW INDEX query' |
Comment on lines
+77
to
+85
| if (str_contains($this->sourceContent, 'unlink(')) { | ||
| $this->assertStringContainsString( | ||
| 'is_file(', | ||
| $this->sourceContent, | ||
| 'profile/include/update.php should use is_file() guard before unlink()' | ||
| ); | ||
| } else { | ||
| $this->assertTrue(true, 'No unlink() calls found'); | ||
| } |
There was a problem hiding this comment.
updateScriptUsesIsFileBeforeUnlink() can produce false positives: it only asserts that is_file( appears somewhere in the file when unlink( exists, but it doesn’t verify that each unlink() call is actually guarded by an is_file() check. Consider matching the specific guard pattern around unlink() (e.g., via a regex that checks if (is_file(...)) { unlink(...); }) so the test fails if an unguarded unlink is reintroduced.
Suggested change
| if (str_contains($this->sourceContent, 'unlink(')) { | |
| $this->assertStringContainsString( | |
| 'is_file(', | |
| $this->sourceContent, | |
| 'profile/include/update.php should use is_file() guard before unlink()' | |
| ); | |
| } else { | |
| $this->assertTrue(true, 'No unlink() calls found'); | |
| } | |
| if (!str_contains($this->sourceContent, 'unlink(')) { | |
| $this->assertTrue(true, 'No unlink() calls found'); | |
| return; | |
| } | |
| $totalUnlinks = preg_match_all('/\bunlink\s*\(/', $this->sourceContent); | |
| $guardedUnlinks = preg_match_all( | |
| '/if\s*\([^)]*is_file\s*\([^)]*\)[^)]*\)\s*\{[^}]*\bunlink\s*\(/s', | |
| $this->sourceContent | |
| ); | |
| $this->assertSame( | |
| $totalUnlinks, | |
| $guardedUnlinks, | |
| 'profile/include/update.php should wrap each unlink() in an if (is_file(...)) { ... unlink(...); } guard' | |
| ); |
Comment on lines
+75
to
+87
| public function updateScriptUsesIsFileBeforeUnlink(): void | ||
| { | ||
| // If unlink() is used, it should be guarded by is_file() | ||
| if (str_contains($this->sourceContent, 'unlink(')) { | ||
| $this->assertStringContainsString( | ||
| 'is_file(', | ||
| $this->sourceContent, | ||
| 'pm/include/update.php should use is_file() guard before unlink()' | ||
| ); | ||
| } else { | ||
| $this->assertTrue(true, 'No unlink() calls found'); | ||
| } | ||
| } |
There was a problem hiding this comment.
updateScriptUsesIsFileBeforeUnlink() can pass even if unlink() is unguarded (it only checks that is_file( exists somewhere in the script). To make this test meaningful, assert the actual guard structure around the unlink() call (e.g., a regex for if (is_file(...)) followed by unlink(...) in the same block).
Replace 4 nearly-identical test files (SystemUpdateTest, PmUpdateTest, PmInstallTest, ProfileUpdateTest) with one data-driven ModuleScriptConventionsTest using @dataProvider. This eliminates code duplication flagged by SonarCloud and adds stronger structural checks (regex-based convention verification instead of substring matching). Also add missing instanceof \mysqli_result guards to isResultSet() checks in pm/update.php, pm/install.php, profile/update.php, and system/update.php to satisfy Scrutinizer static analysis.
| $sql = 'SHOW COLUMNS FROM ' . $xoopsDB->prefix('priv_msgs'); | ||
| $result = $xoopsDB->query($sql); | ||
| if (!$xoopsDB->isResultSet($result)) { | ||
| if (!$xoopsDB->isResultSet($result) || !$result instanceof \mysqli_result) { |
| $sql = 'SELECT * FROM `' . $GLOBALS['xoopsDB']->prefix('user_profile_field') . "` WHERE `field_name` NOT IN ('" . implode("', '", $skip_fields) . "')"; | ||
| $result = $GLOBALS['xoopsDB']->query($sql); | ||
| if (!$GLOBALS['xoopsDB']->isResultSet($result)) { | ||
| if (!$GLOBALS['xoopsDB']->isResultSet($result) || !$result instanceof \mysqli_result) { |
| /** | ||
| * Verify that module update/install scripts follow XOOPS 2.5.12 conventions: | ||
| * - No deprecated queryF() calls | ||
| * - No @ error suppression |
Comment on lines
+83
to
+96
| // Count fetch calls | ||
| $fetchCount = preg_match_all('/->fetch(Array|Row|Both)\s*\(/', $source); | ||
| if ($fetchCount === 0) { | ||
| $this->assertTrue(true, 'No fetch calls found'); | ||
| return; | ||
| } | ||
|
|
||
| // Count isResultSet guards | ||
| $guardCount = preg_match_all('/isResultSet\s*\(\s*\$result\s*\)/', $source); | ||
| $this->assertGreaterThanOrEqual( | ||
| $fetchCount, | ||
| $guardCount, | ||
| "$script must have at least one isResultSet() guard per fetch block" | ||
| ); |
Comment on lines
59
to
60
| if (!$xoopsDB->isResultSet($result) || !$result instanceof \mysqli_result) { | ||
| throw new \RuntimeException( |
Comment on lines
+81
to
83
| $result = $xoopsDB->query($sql); | ||
| if (!$xoopsDB->isResultSet($result) || !$result instanceof \mysqli_result) { | ||
| xoops_error($xoopsDB->error() . '<br>' . $sql); |
| $sql = 'SHOW COLUMNS FROM ' . $xoopsDB->prefix('priv_msgs'); | ||
| $result = $xoopsDB->query($sql); | ||
| if (!$xoopsDB->isResultSet($result)) { | ||
| if (!$xoopsDB->isResultSet($result) || !$result instanceof \mysqli_result) { |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@htdocs/modules/pm/include/install.php`:
- Line 29: The guard uses a negated instanceof with incorrect precedence: change
the second check so instanceof applies to $result (e.g., replace "!$result
instanceof \mysqli_result" with "!( $result instanceof \mysqli_result )" or
equivalent), so the condition reads $xoopsDB->isResultSet($result) || !($result
instanceof \mysqli_result) and correctly tests the type of $result.
In `@htdocs/modules/pm/include/update.php`:
- Around line 54-57: The deletion logic uses is_file() then unlink() on
$filePath without checking that it resides under $templateDirectory and it
ignores unlink() failures; update the block that computes $filePath (using
realpath() on the candidate and on $templateDirectory) and verify the resolved
path begins with the resolved template directory before calling unlink(), call
unlink() and check its return value, and on failure use trigger_error() with
E_USER_WARNING including only basename($filePath) in the message to avoid
leaking full server paths.
- Line 34: The type-check guard currently uses incorrect operator precedence:
change the condition that reads !$result instanceof \mysqli_result to explicitly
negate the instanceof expression so it becomes !($result instanceof
\mysqli_result); update the combined guard in the same if that calls
$xoopsDB->isResultSet($result) to use this parenthesized form to correctly
validate $result's type.
In `@htdocs/modules/profile/include/update.php`:
- Around line 123-126: Replace the naive is_file/unlink block by resolving and
checking boundaries: call realpath() on both $templateDirectory and the target
$filePath (e.g. $realTemplateDir = realpath($templateDirectory); $realFile =
realpath($filePath)), ensure $realFile !== false and that the $realFile path
begins with $realTemplateDir to prevent traversal, then attempt
unlink($realFile) and if unlink returns false call trigger_error() with
E_USER_WARNING using basename($v) in the message (do not expose full paths) so
failed deletions are reported non-fatally; keep existing is_file checks only as
a secondary guard.
In `@htdocs/modules/system/include/update.php`:
- Line 59: The boolean negation is applied to $result instead of the instanceof
expression due to operator precedence; change occurrences of "!$result
instanceof \mysqli_result" (seen in the conditional that also calls
$xoopsDB->isResultSet($result)) to "!($result instanceof \mysqli_result)" so the
code correctly negates the instanceof check; apply the same parenthesized fix to
the second identical check later in the file (the other instanceof check near
the update result handling).
In `@tests/unit/htdocs/ModuleScriptConventionsTest.php`:
- Around line 1-21: Add the standard XOOPS PHP source header block at the top of
the file and expand the class docblock for ModuleScriptConventionsTest to
include the required metadata tags: `@category`, `@package`, `@author`, `@copyright`,
`@license`, and `@link`; ensure the header matches the project's canonical XOOPS
header format and that the ModuleScriptConventionsTest docblock sits immediately
above the class declaration and contains all required fields filled
appropriately.
- Around line 41-141: Rename each PHPUnit test method so its name begins with
"test" to match the required convention: change noDeprecatedQueryF ->
testNoDeprecatedQueryF, noErrorSuppression -> testNoErrorSuppression,
noDeprecatedQuoteString -> testNoDeprecatedQuoteString,
everyFetchHasResultSetGuard -> testEveryFetchHasResultSetGuard,
resultSetGuardIncludesInstanceof -> testResultSetGuardIncludesInstanceof, and
everyUnlinkIsGuardedByIsFile -> testEveryUnlinkIsGuardedByIsFile; keep the
attributes (#[Test], #[DataProvider('scriptProvider')]) and method signatures
unchanged so tests and providers still wire up correctly.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 05f82832-4e4a-44ca-88a3-2fbec8ed79fa
📒 Files selected for processing (6)
htdocs/modules/pm/include/install.phphtdocs/modules/pm/include/update.phphtdocs/modules/profile/include/update.phphtdocs/modules/system/include/update.phptests/unit/htdocs/ModuleScriptConventionsTest.phptests/unit/htdocs/modules/system/SourceFileTestTrait.php
Comment on lines
+54
to
+57
| $filePath = $templateDirectory . $v; | ||
| if (is_file($filePath)) { | ||
| unlink($filePath); | ||
| } |
There was a problem hiding this comment.
Harden template file deletion with boundary checks and failure reporting.
The is_file() guard is good, but this still lacks realpath() boundary validation and ignores unlink() failures.
Proposed hardening
- $filePath = $templateDirectory . $v;
- if (is_file($filePath)) {
- unlink($filePath);
- }
+ $basePath = realpath($templateDirectory);
+ $filePath = realpath($templateDirectory . $v);
+ if ($basePath !== false
+ && $filePath !== false
+ && str_starts_with($filePath, $basePath . DIRECTORY_SEPARATOR)
+ && is_file($filePath)
+ && !unlink($filePath)
+ ) {
+ trigger_error('Failed to remove template file: ' . basename($filePath), E_USER_WARNING);
+ }As per coding guidelines, "Validate file paths with realpath() and boundary checks to prevent directory traversal" and "Use trigger_error() with E_USER_WARNING for non-fatal failures and use basename() in error messages to avoid exposing server paths."
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| $filePath = $templateDirectory . $v; | |
| if (is_file($filePath)) { | |
| unlink($filePath); | |
| } | |
| $basePath = realpath($templateDirectory); | |
| $filePath = realpath($templateDirectory . $v); | |
| if ($basePath !== false | |
| && $filePath !== false | |
| && str_starts_with($filePath, $basePath . DIRECTORY_SEPARATOR) | |
| && is_file($filePath) | |
| && !unlink($filePath) | |
| ) { | |
| trigger_error('Failed to remove template file: ' . basename($filePath), E_USER_WARNING); | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@htdocs/modules/pm/include/update.php` around lines 54 - 57, The deletion
logic uses is_file() then unlink() on $filePath without checking that it resides
under $templateDirectory and it ignores unlink() failures; update the block that
computes $filePath (using realpath() on the candidate and on $templateDirectory)
and verify the resolved path begins with the resolved template directory before
calling unlink(), call unlink() and check its return value, and on failure use
trigger_error() with E_USER_WARNING including only basename($filePath) in the
message to avoid leaking full server paths.
Comment on lines
+123
to
+126
| $filePath = $templateDirectory . $v; | ||
| if (is_file($filePath)) { | ||
| unlink($filePath); | ||
| } |
There was a problem hiding this comment.
Add boundary validation and non-fatal error reporting around unlink.
is_file() alone is not enough here; validate the resolved path is within the template directory and report failed deletions.
Proposed hardening
- $filePath = $templateDirectory . $v;
- if (is_file($filePath)) {
- unlink($filePath);
- }
+ $basePath = realpath($templateDirectory);
+ $filePath = realpath($templateDirectory . $v);
+ if ($basePath !== false
+ && $filePath !== false
+ && str_starts_with($filePath, $basePath . DIRECTORY_SEPARATOR)
+ && is_file($filePath)
+ && !unlink($filePath)
+ ) {
+ trigger_error('Failed to remove template file: ' . basename($filePath), E_USER_WARNING);
+ }As per coding guidelines, "Validate file paths with realpath() and boundary checks to prevent directory traversal" and "Use trigger_error() with E_USER_WARNING for non-fatal failures and use basename() in error messages to avoid exposing server paths."
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| $filePath = $templateDirectory . $v; | |
| if (is_file($filePath)) { | |
| unlink($filePath); | |
| } | |
| $basePath = realpath($templateDirectory); | |
| $filePath = realpath($templateDirectory . $v); | |
| if ($basePath !== false | |
| && $filePath !== false | |
| && str_starts_with($filePath, $basePath . DIRECTORY_SEPARATOR) | |
| && is_file($filePath) | |
| && !unlink($filePath) | |
| ) { | |
| trigger_error('Failed to remove template file: ' . basename($filePath), E_USER_WARNING); | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@htdocs/modules/profile/include/update.php` around lines 123 - 126, Replace
the naive is_file/unlink block by resolving and checking boundaries: call
realpath() on both $templateDirectory and the target $filePath (e.g.
$realTemplateDir = realpath($templateDirectory); $realFile =
realpath($filePath)), ensure $realFile !== false and that the $realFile path
begins with $realTemplateDir to prevent traversal, then attempt
unlink($realFile) and if unlink returns false call trigger_error() with
E_USER_WARNING using basename($v) in the message (do not expose full paths) so
failed deletions are reported non-fatally; keep existing is_file checks only as
a secondary guard.
Comment on lines
+1
to
+21
| <?php | ||
| declare(strict_types=1); | ||
|
|
||
| namespace modulescripts; | ||
|
|
||
| use PHPUnit\Framework\Attributes\CoversNothing; | ||
| use PHPUnit\Framework\Attributes\DataProvider; | ||
| use PHPUnit\Framework\Attributes\Test; | ||
| use PHPUnit\Framework\TestCase; | ||
|
|
||
| /** | ||
| * Verify that module update/install scripts follow XOOPS 2.5.12 conventions: | ||
| * - No deprecated queryF() calls | ||
| * - No @ error suppression | ||
| * - No deprecated quoteString() calls | ||
| * - isResultSet() guard before fetchArray/fetchRow/fetchBoth | ||
| * - is_file() guard before every unlink() | ||
| * - instanceof \mysqli_result used with isResultSet() | ||
| */ | ||
| #[CoversNothing] | ||
| class ModuleScriptConventionsTest extends TestCase |
There was a problem hiding this comment.
Add the standard XOOPS source header and complete class metadata tags.
This new file is missing the required XOOPS header block, and the class docblock is missing required tags (@category, @author, @copyright, @license, @link).
As per coding guidelines, "**/*.php: Every source file must begin with the XOOPS copyright header block" and "Class docblocks must include @category, @package, @author, @copyright, @license, and @link tags."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tests/unit/htdocs/ModuleScriptConventionsTest.php` around lines 1 - 21, Add
the standard XOOPS PHP source header block at the top of the file and expand the
class docblock for ModuleScriptConventionsTest to include the required metadata
tags: `@category`, `@package`, `@author`, `@copyright`, `@license`, and `@link`; ensure the
header matches the project's canonical XOOPS header format and that the
ModuleScriptConventionsTest docblock sits immediately above the class
declaration and contains all required fields filled appropriately.
Comment on lines
+41
to
+141
| #[Test] | ||
| #[DataProvider('scriptProvider')] | ||
| public function noDeprecatedQueryF(string $script): void | ||
| { | ||
| $source = $this->readScript($script); | ||
| $this->assertDoesNotMatchRegularExpression( | ||
| '/->queryF\s*\(/', | ||
| $source, | ||
| "$script must not use deprecated queryF()" | ||
| ); | ||
| } | ||
|
|
||
| #[Test] | ||
| #[DataProvider('scriptProvider')] | ||
| public function noErrorSuppression(string $script): void | ||
| { | ||
| $source = $this->readScript($script); | ||
| $this->assertDoesNotMatchRegularExpression( | ||
| '/@\s*(unlink|fopen|file_get_contents|mkdir|rmdir)\s*\(/', | ||
| $source, | ||
| "$script must not use @ error suppression" | ||
| ); | ||
| } | ||
|
|
||
| #[Test] | ||
| #[DataProvider('scriptProvider')] | ||
| public function noDeprecatedQuoteString(string $script): void | ||
| { | ||
| $source = $this->readScript($script); | ||
| $this->assertDoesNotMatchRegularExpression( | ||
| '/->quoteString\s*\(/', | ||
| $source, | ||
| "$script must not use deprecated quoteString()" | ||
| ); | ||
| } | ||
|
|
||
| #[Test] | ||
| #[DataProvider('scriptProvider')] | ||
| public function everyFetchHasResultSetGuard(string $script): void | ||
| { | ||
| $source = $this->readScript($script); | ||
|
|
||
| // Count fetch calls | ||
| $fetchCount = preg_match_all('/->fetch(Array|Row|Both)\s*\(/', $source); | ||
| if ($fetchCount === 0) { | ||
| $this->assertTrue(true, 'No fetch calls found'); | ||
| return; | ||
| } | ||
|
|
||
| // Count isResultSet guards | ||
| $guardCount = preg_match_all('/isResultSet\s*\(\s*\$result\s*\)/', $source); | ||
| $this->assertGreaterThanOrEqual( | ||
| $fetchCount, | ||
| $guardCount, | ||
| "$script must have at least one isResultSet() guard per fetch block" | ||
| ); | ||
| } | ||
|
|
||
| #[Test] | ||
| #[DataProvider('scriptProvider')] | ||
| public function resultSetGuardIncludesInstanceof(string $script): void | ||
| { | ||
| $source = $this->readScript($script); | ||
|
|
||
| if (!str_contains($source, 'isResultSet(')) { | ||
| $this->assertTrue(true, 'No isResultSet() calls found'); | ||
| return; | ||
| } | ||
|
|
||
| // Accept flexible guard forms: isResultSet() combined with instanceof \mysqli_result | ||
| $this->assertMatchesRegularExpression( | ||
| '/isResultSet\s*\(\s*\$result\s*\).*\$result\s+instanceof\s+\\\\mysqli_result/s', | ||
| $source, | ||
| "$script must combine isResultSet() with instanceof \\mysqli_result" | ||
| ); | ||
| } | ||
|
|
||
| #[Test] | ||
| #[DataProvider('scriptProvider')] | ||
| public function everyUnlinkIsGuardedByIsFile(string $script): void | ||
| { | ||
| $source = $this->readScript($script); | ||
|
|
||
| $totalUnlinks = preg_match_all('/\bunlink\s*\(/', $source); | ||
| if ($totalUnlinks === 0) { | ||
| $this->assertTrue(true, 'No unlink() calls found'); | ||
| return; | ||
| } | ||
|
|
||
| // Match if (is_file(...)) { ... unlink(...) ... } pattern | ||
| $guardedUnlinks = preg_match_all( | ||
| '/if\s*\([^)]*\bis_file\s*\([^)]*\)[^)]*\)\s*\{[^}]*\bunlink\s*\(/s', | ||
| $source | ||
| ); | ||
|
|
||
| $this->assertSame( | ||
| $totalUnlinks, | ||
| $guardedUnlinks, | ||
| "$script: every unlink() must be wrapped in an if (is_file(...)) guard" | ||
| ); | ||
| } |
There was a problem hiding this comment.
Rename test methods to the required test* convention.
Methods in this class should follow test{MethodName} naming.
Example rename pattern
- public function noDeprecatedQueryF(string $script): void
+ public function testNoDeprecatedQueryF(string $script): void
@@
- public function noErrorSuppression(string $script): void
+ public function testNoErrorSuppression(string $script): voidAs per coding guidelines, "tests/**/*.php: Name test methods test{MethodName} or test{MethodName}{Scenario}`."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tests/unit/htdocs/ModuleScriptConventionsTest.php` around lines 41 - 141,
Rename each PHPUnit test method so its name begins with "test" to match the
required convention: change noDeprecatedQueryF -> testNoDeprecatedQueryF,
noErrorSuppression -> testNoErrorSuppression, noDeprecatedQuoteString ->
testNoDeprecatedQuoteString, everyFetchHasResultSetGuard ->
testEveryFetchHasResultSetGuard, resultSetGuardIncludesInstanceof ->
testResultSetGuardIncludesInstanceof, and everyUnlinkIsGuardedByIsFile ->
testEveryUnlinkIsGuardedByIsFile; keep the attributes (#[Test],
#[DataProvider('scriptProvider')]) and method signatures unchanged so tests and
providers still wire up correctly.
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
htdocs/modules/pm/include/install.php (1)
17-21: 🧹 Nitpick | 🔵 TrivialMove
@paramand@returntags to the function's own docblock.The PHPDoc tags on lines 17-18 are attached to the file docblock rather than the function. While the type hint
XoopsModule $moduleis present, moving the documentation to the function improves IDE support and maintainability.Proposed fix
* `@author` Taiwen Jiang <phppp@users.sourceforge.net> - * `@param` $module - * `@return` bool */ +/** + * `@param` XoopsModule $module + * `@return` bool + */ function xoops_module_install_pm(XoopsModule $module)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@htdocs/modules/pm/include/install.php` around lines 17 - 21, The file-level PHPDoc currently contains the `@param` and `@return` tags; move those tags into a proper docblock immediately above the function xoops_module_install_pm so the function has its own /** ... */ block documenting the parameter ($module) and return type (bool). Update the function docblock to include the `@param` XoopsModule $module and `@return` bool tags (and any short description) and remove those tags from the file-level docblock so IDEs and static analyzers pick up the function signature correctly.
♻️ Duplicate comments (5)
tests/unit/htdocs/ModuleScriptConventionsTest.php (3)
1-21:⚠️ Potential issue | 🟡 MinorAdd the standard XOOPS source header and complete class metadata tags.
This new file is missing the required XOOPS header block, and the class docblock is incomplete (missing
@category,@author,@copyright,@license,@linktags).As per coding guidelines, "Every source file must begin with the XOOPS copyright header block" and "Class docblocks must include
@category,@package,@author,@copyright,@license, and@linktags."🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/unit/htdocs/ModuleScriptConventionsTest.php` around lines 1 - 21, Add the standard XOOPS source header block at the top of the file and complete the class docblock for ModuleScriptConventionsTest by adding the required metadata tags: `@category`, `@package`, `@author`, `@copyright`, `@license`, and `@link`, ensuring the header and docblock follow XOOPS formatting conventions and remain above the declare(strict_types=1) and class declaration.
41-43:⚠️ Potential issue | 🟠 MajorRename test methods to the required
test*convention.Methods
noDeprecatedQueryF,noErrorSuppression,noDeprecatedQuoteString,everyFetchHasResultSetGuard,resultSetGuardIncludesInstanceof, andeveryUnlinkIsGuardedByIsFileshould be prefixed withtest.While the
#[Test]attribute works in PHPUnit 10+, thetest*prefix convention ensures compatibility with PHPUnit 9.6 as required by coding guidelines.Example rename
- public function noDeprecatedQueryF(string $script): void + public function testNoDeprecatedQueryF(string $script): voidAs per coding guidelines, "Name test methods
test{MethodName}ortest{MethodName}{Scenario}" and "Tests must work across PHPUnit 9.6, 10.5, and 11.x."🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/unit/htdocs/ModuleScriptConventionsTest.php` around lines 41 - 43, The listed PHPUnit test methods must be renamed to start with the test prefix to ensure PHPUnit 9.6 compatibility: rename noDeprecatedQueryF to testNoDeprecatedQueryF, noErrorSuppression to testNoErrorSuppression, noDeprecatedQuoteString to testNoDeprecatedQuoteString, everyFetchHasResultSetGuard to testEveryFetchHasResultSetGuard, resultSetGuardIncludesInstanceof to testResultSetGuardIncludesInstanceof, and everyUnlinkIsGuardedByIsFile to testEveryUnlinkIsGuardedByIsFile; update the method declarations (preserving attributes like #[Test] and #[DataProvider('scriptProvider')] and their signatures) so the method names match the new names and run in older PHPUnit versions.
6-8:⚠️ Potential issue | 🟠 MajorPHPUnit 9.6 incompatibility: test methods lack backward-compatible naming.
The test methods use
#[Test]and#[DataProvider]attributes (introduced in PHPUnit 10) but omit thetest*method naming convention required for PHPUnit 9.6 compatibility. To support PHPUnit 9.6, 10.5, and 11.x without thephpunit/phpunit-attributespolyfill, rename methods to follow thetest*convention (e.g.,testNoDeprecatedQueryF,testNoErrorSuppression) while keeping the attributes—both styles are compatible with PHPUnit 10+.Alternatively, add
phpunit/phpunit-attributesto require-dev if polyfill support is preferred.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/unit/htdocs/ModuleScriptConventionsTest.php` around lines 6 - 8, Rename the test methods in ModuleScriptConventionsTest to use the PHPUnit 9.6-compatible "test*" naming convention while keeping the existing #[Test] and #[DataProvider] attributes (e.g., rename methods to testNoDeprecatedQueryF, testNoErrorSuppression or similar), so the suite runs under PHPUnit 9.6, 10.5 and 11.x without adding the phpunit/phpunit-attributes polyfill; update any references/calls to those methods (data provider names can remain) to match the new method names.htdocs/modules/pm/include/update.php (1)
54-57:⚠️ Potential issue | 🟠 MajorHarden template file deletion with boundary checks and failure reporting.
The
is_file()guard is good, but this still lacksrealpath()boundary validation and ignoresunlink()failures.Proposed hardening
- $filePath = $templateDirectory . $v; - if (is_file($filePath)) { - unlink($filePath); - } + $basePath = realpath($templateDirectory); + $filePath = realpath($templateDirectory . $v); + if ($basePath !== false + && $filePath !== false + && str_starts_with($filePath, $basePath . DIRECTORY_SEPARATOR) + && is_file($filePath) + && !unlink($filePath) + ) { + trigger_error('Failed to remove template file: ' . basename($filePath), E_USER_WARNING); + }As per coding guidelines, "Validate file paths with realpath() and boundary checks to prevent directory traversal" and "Use trigger_error() with E_USER_WARNING for non-fatal failures and use basename() in error messages to avoid exposing server paths."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@htdocs/modules/pm/include/update.php` around lines 54 - 57, Validate and safely delete template files by resolving $filePath with realpath(), ensure the resolved path is not false and starts with the realpath of $templateDirectory (boundary check) before deleting; retain the is_file() guard but replace direct unlink() calls with a check of unlink()'s return value and on failure call trigger_error() with E_USER_WARNING, using basename($filePath) in the error message to avoid exposing full server paths.htdocs/modules/profile/include/update.php (1)
123-126:⚠️ Potential issue | 🟠 MajorAdd boundary validation and non-fatal error reporting around unlink.
is_file()alone is not enough here; validate the resolved path is within the template directory and report failed deletions.Proposed hardening
- $filePath = $templateDirectory . $v; - if (is_file($filePath)) { - unlink($filePath); - } + $basePath = realpath($templateDirectory); + $filePath = realpath($templateDirectory . $v); + if ($basePath !== false + && $filePath !== false + && str_starts_with($filePath, $basePath . DIRECTORY_SEPARATOR) + && is_file($filePath) + && !unlink($filePath) + ) { + trigger_error('Failed to remove template file: ' . basename($filePath), E_USER_WARNING); + }As per coding guidelines, "Validate file paths with realpath() and boundary checks to prevent directory traversal" and "Use trigger_error() with E_USER_WARNING for non-fatal failures and use basename() in error messages to avoid exposing server paths."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@htdocs/modules/profile/include/update.php` around lines 123 - 126, Replace the raw is_file/unlink sequence by resolving and validating the path: compute $resolved = realpath($filePath) and ensure it is not false and that it begins with the realpath of $templateDirectory to prevent traversal; only then attempt unlink($resolved) and if unlink returns false call trigger_error() with E_USER_WARNING using basename($v) in the message (do not include full path) to report a non-fatal deletion failure; keep checks for is_file/is_readable as needed before unlink.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@tests/unit/htdocs/ModuleScriptConventionsTest.php`:
- Around line 130-134: The current regex used in $guardedUnlinks (the
preg_match_all pattern) only matches unlink() when it appears inside the same
brace block as an is_file() check, so it misses guarded unlinks that use
early-return patterns, ternary/null-coalescing guards, or more
complex/multi-line conditions; update the detection to cover those cases by
adding additional patterns and checks: keep the existing brace-scoped regex for
the simple case, add a second regex to match the early-return guard form like
if\s*\(\s*!?\s*is_file\s*\([^)]*\)\s*\)\s*\{\s*(?:return|continue|break)\b[^}]*\}\s*[^;]*unlink\s*\(
and another pattern to detect inline guards (ternary/null-coalescing) or broader
sequences where is_file(...) appears before an unlink(...) within a reasonable
token window, and aggregate results into $guardedUnlinks so all these
guarded-unlink forms are counted; reference the existing $guardedUnlinks
variable, the preg_match_all call and the regex string so you update/augment
those locations.
- Around line 110-116: The current test in ModuleScriptConventionsTest using
assertMatchesRegularExpression ties isResultSet(...) and the exact "$result
instanceof \\mysqli_result" ordering/variable name, which is brittle; update the
check to be more flexible by either (a) asserting presence of an instanceof
\\mysqli_result pattern and an isResultSet\\s*\\(\\s*\\$?\\w+\\s*\\) pattern
independently (so different variable names are allowed and order doesn't
matter), or (b) use a single regex that matches
isResultSet\\s*\\(\\s*\\$?\\w+\\s*\\).*?instanceof\\s+\\\\mysqli_result|instanceof\\s+\\\\mysqli_result.*?isResultSet\\s*\\(\\s*\\$?\\w+\\s*\\)
to allow swapped order and different variable names; modify the assertion that
currently references assertMatchesRegularExpression and the pattern string
accordingly.
---
Outside diff comments:
In `@htdocs/modules/pm/include/install.php`:
- Around line 17-21: The file-level PHPDoc currently contains the `@param` and
`@return` tags; move those tags into a proper docblock immediately above the
function xoops_module_install_pm so the function has its own /** ... */ block
documenting the parameter ($module) and return type (bool). Update the function
docblock to include the `@param` XoopsModule $module and `@return` bool tags (and
any short description) and remove those tags from the file-level docblock so
IDEs and static analyzers pick up the function signature correctly.
---
Duplicate comments:
In `@htdocs/modules/pm/include/update.php`:
- Around line 54-57: Validate and safely delete template files by resolving
$filePath with realpath(), ensure the resolved path is not false and starts with
the realpath of $templateDirectory (boundary check) before deleting; retain the
is_file() guard but replace direct unlink() calls with a check of unlink()'s
return value and on failure call trigger_error() with E_USER_WARNING, using
basename($filePath) in the error message to avoid exposing full server paths.
In `@htdocs/modules/profile/include/update.php`:
- Around line 123-126: Replace the raw is_file/unlink sequence by resolving and
validating the path: compute $resolved = realpath($filePath) and ensure it is
not false and that it begins with the realpath of $templateDirectory to prevent
traversal; only then attempt unlink($resolved) and if unlink returns false call
trigger_error() with E_USER_WARNING using basename($v) in the message (do not
include full path) to report a non-fatal deletion failure; keep checks for
is_file/is_readable as needed before unlink.
In `@tests/unit/htdocs/ModuleScriptConventionsTest.php`:
- Around line 1-21: Add the standard XOOPS source header block at the top of the
file and complete the class docblock for ModuleScriptConventionsTest by adding
the required metadata tags: `@category`, `@package`, `@author`, `@copyright`, `@license`,
and `@link`, ensuring the header and docblock follow XOOPS formatting conventions
and remain above the declare(strict_types=1) and class declaration.
- Around line 41-43: The listed PHPUnit test methods must be renamed to start
with the test prefix to ensure PHPUnit 9.6 compatibility: rename
noDeprecatedQueryF to testNoDeprecatedQueryF, noErrorSuppression to
testNoErrorSuppression, noDeprecatedQuoteString to testNoDeprecatedQuoteString,
everyFetchHasResultSetGuard to testEveryFetchHasResultSetGuard,
resultSetGuardIncludesInstanceof to testResultSetGuardIncludesInstanceof, and
everyUnlinkIsGuardedByIsFile to testEveryUnlinkIsGuardedByIsFile; update the
method declarations (preserving attributes like #[Test] and
#[DataProvider('scriptProvider')] and their signatures) so the method names
match the new names and run in older PHPUnit versions.
- Around line 6-8: Rename the test methods in ModuleScriptConventionsTest to use
the PHPUnit 9.6-compatible "test*" naming convention while keeping the existing
#[Test] and #[DataProvider] attributes (e.g., rename methods to
testNoDeprecatedQueryF, testNoErrorSuppression or similar), so the suite runs
under PHPUnit 9.6, 10.5 and 11.x without adding the phpunit/phpunit-attributes
polyfill; update any references/calls to those methods (data provider names can
remain) to match the new method names.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 88f45fba-c1a9-41f4-a765-3b7bb59e4c6e
📒 Files selected for processing (5)
htdocs/modules/pm/include/install.phphtdocs/modules/pm/include/update.phphtdocs/modules/profile/include/update.phphtdocs/modules/system/include/update.phptests/unit/htdocs/ModuleScriptConventionsTest.php
Comment on lines
+110
to
+116
| // Accept flexible guard forms: isResultSet() combined with instanceof \mysqli_result | ||
| $this->assertMatchesRegularExpression( | ||
| '/isResultSet\s*\(\s*\$result\s*\).*!\s*\(\s*\$result\s+instanceof\s+\\\\mysqli_result\s*\)/s', | ||
| $source, | ||
| "$script must combine isResultSet() with instanceof \\mysqli_result" | ||
| ); | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
The instanceof pattern regex is fragile and overly specific.
The regex on line 112 only matches one exact pattern shape (isResultSet(...).*!\s*\(\s*\$result\s+instanceof...). It won't match:
- Different variable names (e.g.,
$res,$queryResult) - Swapped condition order (
!($result instanceof ...) || !$xoopsDB->isResultSet(...)) - Multi-statement guards
Consider a more flexible approach or document that this test specifically validates the canonical pattern used in these scripts.
More flexible alternative
- $this->assertMatchesRegularExpression(
- '/isResultSet\s*\(\s*\$result\s*\).*!\s*\(\s*\$result\s+instanceof\s+\\\\mysqli_result\s*\)/s',
- $source,
- "$script must combine isResultSet() with instanceof \\mysqli_result"
- );
+ // Verify both guards exist in the file (order-independent)
+ $hasIsResultSet = preg_match('/isResultSet\s*\(/', $source);
+ $hasInstanceofGuard = preg_match('/!\s*\(\s*\$\w+\s+instanceof\s+\\\\mysqli_result\s*\)/', $source);
+ $this->assertTrue(
+ $hasIsResultSet && $hasInstanceofGuard,
+ "$script must combine isResultSet() with instanceof \\mysqli_result"
+ );📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| // Accept flexible guard forms: isResultSet() combined with instanceof \mysqli_result | |
| $this->assertMatchesRegularExpression( | |
| '/isResultSet\s*\(\s*\$result\s*\).*!\s*\(\s*\$result\s+instanceof\s+\\\\mysqli_result\s*\)/s', | |
| $source, | |
| "$script must combine isResultSet() with instanceof \\mysqli_result" | |
| ); | |
| } | |
| // Accept flexible guard forms: isResultSet() combined with instanceof \mysqli_result | |
| // Verify both guards exist in the file (order-independent) | |
| $hasIsResultSet = preg_match('/isResultSet\s*\(/', $source); | |
| $hasInstanceofGuard = preg_match('/!\s*\(\s*\$\w+\s+instanceof\s+\\\\mysqli_result\s*\)/', $source); | |
| $this->assertTrue( | |
| $hasIsResultSet && $hasInstanceofGuard, | |
| "$script must combine isResultSet() with instanceof \\mysqli_result" | |
| ); | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tests/unit/htdocs/ModuleScriptConventionsTest.php` around lines 110 - 116,
The current test in ModuleScriptConventionsTest using
assertMatchesRegularExpression ties isResultSet(...) and the exact "$result
instanceof \\mysqli_result" ordering/variable name, which is brittle; update the
check to be more flexible by either (a) asserting presence of an instanceof
\\mysqli_result pattern and an isResultSet\\s*\\(\\s*\\$?\\w+\\s*\\) pattern
independently (so different variable names are allowed and order doesn't
matter), or (b) use a single regex that matches
isResultSet\\s*\\(\\s*\\$?\\w+\\s*\\).*?instanceof\\s+\\\\mysqli_result|instanceof\\s+\\\\mysqli_result.*?isResultSet\\s*\\(\\s*\\$?\\w+\\s*\\)
to allow swapped order and different variable names; modify the assertion that
currently references assertMatchesRegularExpression and the pattern string
accordingly.
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)
upgrade/upd_2.5.11-to-2.5.12/index.php (1)
130-148: 🧹 Nitpick | 🔵 TrivialAdd defensive guards to
deleteFolder()for consistency with PR objectives.A few hardening opportunities in this utility method:
- Line 133:
scandir()returnsfalseon failure, which causesarray_diff()to throw aTypeErrorin PHP 8.x.- Line 142:
unlink()without anis_file()guard — the PR adds these guards in pm/profile modules, so consistency here would be beneficial.- Line 147:
rmdir()result is returned but failures aren't logged.🛡️ Proposed defensive improvements
private function deleteFolder($folderPath) { + if (!is_dir($folderPath)) { + return false; + } // Open the folder - $files = array_diff(scandir($folderPath), ['.', '..']); + $scanned = scandir($folderPath); + if ($scanned === false) { + return false; + } + $files = array_diff($scanned, ['.', '..']); foreach ($files as $file) { $filePath = $folderPath . DIRECTORY_SEPARATOR . $file; if (is_dir($filePath)) { // Recursively delete subfolders self::deleteFolder($filePath); } else { // Delete file - unlink($filePath); + if (is_file($filePath)) { + unlink($filePath); + } } } // Remove the folder itself return rmdir($folderPath); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@upgrade/upd_2.5.11-to-2.5.12/index.php` around lines 130 - 148, The deleteFolder method should be hardened: first check that scandir($folderPath) did not return false before calling array_diff (e.g., treat false as empty array and return false if folder unreadable), then when iterating ensure $filePath is a directory with is_dir() before recursing into self::deleteFolder($filePath) and use is_file() (or is_link()/is_writable as appropriate) before calling unlink($filePath) to avoid warnings, and finally when calling rmdir($folderPath) detect failure and log or handle the error (do not silently return false) so callers can act on removal failures; update deleteFolder to perform these guards and emit a clear log/error message on rmdir failures.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@upgrade/upd_2.5.11-to-2.5.12/index.php`:
- Around line 130-148: The deleteFolder method should be hardened: first check
that scandir($folderPath) did not return false before calling array_diff (e.g.,
treat false as empty array and return false if folder unreadable), then when
iterating ensure $filePath is a directory with is_dir() before recursing into
self::deleteFolder($filePath) and use is_file() (or is_link()/is_writable as
appropriate) before calling unlink($filePath) to avoid warnings, and finally
when calling rmdir($folderPath) detect failure and log or handle the error (do
not silently return false) so callers can act on removal failures; update
deleteFolder to perform these guards and emit a clear log/error message on rmdir
failures.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 6e89c767-f39e-4c8e-b1ba-238285c99b53
📒 Files selected for processing (2)
htdocs/class/XoopsTokenHandler.phpupgrade/upd_2.5.11-to-2.5.12/index.php
…ags per PSR-12 Addresses SonarCloud quality gate failure (4.9% duplication vs 3% threshold) and PSR-12 cleanup on PR XOOPS#1646: - ModuleScriptConventionsTest.php: Consolidated 6 test methods into 3 — merged noDeprecatedQueryF/noErrorSuppression/noDeprecatedQuoteString into single noForbiddenPatterns using a const array; merged everyFetchHasResultSetGuard/resultSetGuardIncludesInstanceof into fetchCallsHaveProperResultSetGuards. File reduced from 142 to 107 lines. - Removed @Package, @subpackage, @category tags from all 7 source files touched by this PR: XoopsTokenHandler.php, pm/install.php, pm/update.php, profile/update.php, system/update.php, upgrade/index.php, and the test file.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 8 out of 8 changed files in this pull request and generated 2 comments.
Comments suppressed due to low confidence (1)
htdocs/class/XoopsTokenHandler.php:35
- The file/class docblocks removed
@package(and previously@category) tags. Most core classes in htdocs/class still include these tags (for example htdocs/class/criteria.php). If you’re aiming for consistent generated docs and metadata across core classes, consider restoring@package corein the file header and aligning the class-level tags with the existing convention.
* MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.
*
* @copyright (c) 2000-2026 XOOPS Project (https://xoops.org)
* @license GNU GPL 2 (https://www.gnu.org/licenses/gpl-2.0.html)
* @since 2.5.12
*/
declare(strict_types=1);
defined('XOOPS_ROOT_PATH') || exit('Restricted access');
/**
* Generic scoped token handler for XOOPS.
*
* Provides create/verify/revoke/purge for any token-based flow:
* password reset, account activation, email verification, etc.
*
* Recommended scopes: 'lostpass', 'activation', 'emailchange'.
*
* Requires PHP 8.2+ (readonly properties, union return types).
*
* @author XOOPS Team
* @copyright (c) 2000-2026 XOOPS Project (https://xoops.org)
* @license GNU GPL 2 (https://www.gnu.org/licenses/gpl-2.0.html)
* @link https://xoops.org
*/
Comment on lines
+97
to
+102
| // Match if (is_file(...)) { ... unlink(...) ... } pattern | ||
| $guardedUnlinks = preg_match_all( | ||
| '/if\s*\([^)]*\bis_file\s*\([^)]*\)[^)]*\)\s*\{[^}]*\bunlink\s*\(/s', | ||
| $source | ||
| ); | ||
|
|
| @@ -5,7 +5,6 @@ | |||
| * | |||
| * @copyright (c) 2000-2026 XOOPS Project (https://xoops.org) | |||
| * @license GNU GPL 2 (https://www.gnu.org/licenses/gpl-2.0.html) | |||
…lication gate
SonarCloud was still failing at 6.2% duplication (needs ≤ 3%). Collapsed 3 separate test methods (noForbiddenPatterns,
fetchCallsHaveProperResultSetGuards, everyUnlinkIsGuardedByIsFile) into a single scriptFollowsConventions() that runs all checks in one pass per script.
File reduced from 109 to 78 lines. Also broadened the unlink guard regex from the narrow if (is_file(...)) { ... unlink(...) } to
/\bis_file\s*\([^)]*\).*?\bunlink\s*\(/s which accepts both braced and unbraced forms.
|
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #1646 +/- ##
==============================
==============================
☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
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.
…ule update scripts
Summary by CodeRabbit
Bug Fixes
Tests