Guard avatar delete with compensating restore, localize FK strings, add proxy PHPDoc#1651
Merged
Merged
Conversation
…y PHPDoc M-5: Wrap avatar delete (user reset + record delete) in a DB transaction so that a failed delete rolls back the user profile reset, preventing orphaned blank.gif references while the avatar record persists. M-6: Replace hard-coded English FK_CHECKS and DROP TABLE error messages in modulesadmin.php with new _AM_SYSTEM_MODULES_FK_DISABLE, _AM_SYSTEM_MODULES_FK_ENABLE, _AM_SYSTEM_MODULES_DROP_FAIL, and _AM_SYSTEM_MODULES_DROP_OK language constants for i18n consistency. M-7: Add missing @category, @license, and @link PHPDoc tags to the XoopsMySQLDatabaseProxy class docblock, matching the standard format used by other classes in the same file.
WalkthroughUpdates MySQL proxy docblock and query behavior (trims SQL, blocks non-reads on GET); refactors avatar deletion to collect affected UIDs, perform bulk user reset, delete avatar, and run a compensating restore on failure; replaces hard-coded FK/drop messages with localized constants and adds unit tests for these changes. (39 words) Changes
Sequence DiagramsequenceDiagram
participant Admin as Admin Request
participant Logic as Avatar Logic
participant DB as Database
participant Users as User System
Admin->>Logic: Delete avatar (avatar_id, avatar_file)
Logic->>DB: SELECT uid FROM users WHERE user_avatar = avatar_file
DB-->>Logic: Affected UID list
Logic->>Users: UPDATE users SET user_avatar = '' WHERE uid IN (Affected UIDs)
Users-->>Logic: Reset result
alt Reset success
Logic->>DB: DELETE FROM avatars WHERE avatar_id = ...
alt Delete success
DB-->>Logic: Delete OK
Logic-->>Admin: Redirect success
else Delete fails
DB-->>Logic: Error
Logic->>Users: UPDATE users SET user_avatar = previous_file WHERE uid IN (Affected UIDs)
Users-->>Logic: Restore OK
Logic-->>Admin: Redirect with error
end
else Reset fails
Users-->>Logic: Reset Error
Logic-->>Admin: Redirect (no delete attempted)
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 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 improves admin-side robustness and i18n compliance in the XOOPS 2.5.x core by making avatar deletion DB operations atomic, localizing module install/uninstall FK/drop-table error strings, and completing required PHPDoc metadata for the MySQL database proxy class.
Changes:
- Wrap avatar user-avatar reset + avatar record delete in an explicit DB transaction with commit/rollback.
- Add new
_AM_SYSTEM_MODULES_*language constants and use them instead of hard-coded English FK/DROP error messages. - Add missing PHPDoc tags to
XoopsMySQLDatabaseProxy, with accompanying unit tests.
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/unit/htdocs/modules/system/language/english/admin/ModulesadminLanguageTest.php | Adds tests ensuring new modulesadmin language constants exist and hard-coded FK/drop strings are removed. |
| tests/unit/htdocs/modules/system/admin/avatars/AvatarDeleteTransactionTest.php | Adds tests asserting avatar delete flow contains transaction statements and ordering. |
| tests/unit/htdocs/class/database/XoopsMySQLDatabaseProxyDocTest.php | Adds tests verifying required docblock tags exist for XoopsMySQLDatabaseProxy. |
| htdocs/modules/system/language/english/admin/modulesadmin.php | Introduces new _AM_SYSTEM_MODULES_FK_* and _AM_SYSTEM_MODULES_DROP_* language constants. |
| htdocs/modules/system/admin/modulesadmin/modulesadmin.php | Replaces hard-coded FK/drop error strings with localized constants. |
| htdocs/modules/system/admin/avatars/main.php | Wraps avatar reset + delete in START TRANSACTION / COMMIT / ROLLBACK. |
Comment on lines
+60
to
+69
| // START TRANSACTION should appear before the UPDATE ... blank.gif | ||
| $txPos = strpos($this->source, 'START TRANSACTION'); | ||
| $resetPos = strpos($this->source, "SET user_avatar='blank.gif'"); | ||
| $this->assertNotFalse($txPos); | ||
| $this->assertNotFalse($resetPos); | ||
| $this->assertLessThan( | ||
| $resetPos, | ||
| $txPos, | ||
| 'Transaction must start before user avatar reset' | ||
| ); |
There was a problem hiding this comment.
In transactionStartsBeforeUserReset(), the assertLessThan() arguments are reversed. assertLessThan($expected, $actual) currently asserts $resetPos < $txPos, but the intent (and the message) is that the transaction starts before the reset, i.e. $txPos < $resetPos.
Comment on lines
+81
to
+82
| $commitPos, | ||
| $deletePos, |
There was a problem hiding this comment.
In commitComesAfterDelete(), the assertLessThan() arguments are reversed. As written it asserts $commitPos < $deletePos, but the intent is to verify COMMIT comes after the delete call, i.e. $deletePos < $commitPos.
Suggested change
| $commitPos, | |
| $deletePos, | |
| $deletePos, | |
| $commitPos, |
Comment on lines
336
to
345
| // Delete the avatar record | ||
| if (!$avt_handler->delete($avatar)) { | ||
| xoops_cp_header(); | ||
| xoops_error(sprintf(_AM_SYSTEM_AVATAR_FAILDEL, $avatar->getVar('avatar_id'))); | ||
| xoops_cp_footer(); | ||
| exit(); | ||
| if ($success && !$avt_handler->delete($avatar)) { | ||
| $success = false; | ||
| } | ||
| if ($success) { | ||
| $xoopsDB->exec('COMMIT'); | ||
| } else { | ||
| $xoopsDB->exec('ROLLBACK'); | ||
| redirect_header('admin.php?fct=avatars', 2, _AM_SYSTEM_DBERROR); | ||
| } |
There was a problem hiding this comment.
The new rollback path redirects with the generic _AM_SYSTEM_DBERROR, but previously this branch showed a specific _AM_SYSTEM_AVATAR_FAILDEL message (with the avatar id). Consider preserving the more informative failure message (even if you still rollback first) so admins can distinguish delete failures from update/transaction issues.
| $created_tables = []; | ||
| if (!$db->exec('SET FOREIGN_KEY_CHECKS = 0')) { | ||
| $errs[] = 'Failed to disable FOREIGN_KEY_CHECKS before table creation: ' . htmlspecialchars($db->error(), ENT_QUOTES | ENT_HTML5, 'UTF-8'); | ||
| $errs[] = _AM_SYSTEM_MODULES_FK_DISABLE . ': ' . htmlspecialchars($db->error(), ENT_QUOTES | ENT_HTML5, 'UTF-8'); |
There was a problem hiding this comment.
By replacing multiple stage-specific FK failure strings with the single _AM_SYSTEM_MODULES_FK_DISABLE/_AM_SYSTEM_MODULES_FK_ENABLE constants, the error output loses context like whether it happened during table creation, cleanup, or uninstall. Consider appending a localized stage descriptor (or adding separate constants per stage) so admins can still identify where the failure occurred.
Suggested change
| $errs[] = _AM_SYSTEM_MODULES_FK_DISABLE . ': ' . htmlspecialchars($db->error(), ENT_QUOTES | ENT_HTML5, 'UTF-8'); | |
| $errs[] = _AM_SYSTEM_MODULES_FK_DISABLE . ' [install: create tables]: ' . htmlspecialchars($db->error(), ENT_QUOTES | ENT_HTML5, 'UTF-8'); |
Comment on lines
+47
to
+48
| $deletePos, | ||
| $resetPos, |
Comment on lines
+346
to
+350
| } else { | ||
| $xoopsDB->exec('UPDATE ' . $xoopsDB->prefix('users') | ||
| . ' SET user_avatar=' . $xoopsDB->quote($file) . " WHERE user_avatar='blank.gif'"); | ||
| } | ||
| redirect_header('admin.php?fct=avatars', 2, sprintf(_AM_SYSTEM_AVATAR_FAILDEL, $avatar_id)); |
Comment on lines
+321
to
+323
| // Reset user avatars first, then delete the record. | ||
| // NOTE: tables are MyISAM so real transactions are not available; | ||
| // we use a sequential approach with compensating restore instead. |
Comment on lines
+65
to
+66
| $deletePos, | ||
| $abortPos, |
Comment on lines
+319
to
+321
| // Reset user avatars first, then delete the record. | ||
| // NOTE: tables are MyISAM so real transactions are not available; | ||
| // we use a sequential approach with compensating restore instead. |
Comment on lines
+12
to
+65
| /** | ||
| * Tests for XoopsMySQLDatabaseProxy PHPDoc metadata (M-7). | ||
| * | ||
| * Verifies that the class docblock contains all required PSR-12 PHPDoc tags | ||
| * and does not contain legacy tags removed under PSR-12 (@package, @subpackage, @category). | ||
| */ | ||
| #[CoversClass(XoopsMySQLDatabaseProxy::class)] | ||
| class XoopsMySQLDatabaseProxyDocTest extends TestCase | ||
| { | ||
| private string $source; | ||
|
|
||
| protected function setUp(): void | ||
| { | ||
| $this->source = file_get_contents( | ||
| XOOPS_ROOT_PATH . '/class/database/mysqldatabase.php' | ||
| ); | ||
| } | ||
|
|
||
| #[Test] | ||
| public function proxyClassDocblockContainsLicenseTag(): void | ||
| { | ||
| $docblock = $this->extractProxyDocblock(); | ||
| $this->assertStringContainsString('@license', $docblock); | ||
| } | ||
|
|
||
| #[Test] | ||
| public function proxyClassDocblockContainsLinkTag(): void | ||
| { | ||
| $docblock = $this->extractProxyDocblock(); | ||
| $this->assertStringContainsString('@link', $docblock); | ||
| } | ||
|
|
||
| #[Test] | ||
| public function proxyClassDocblockContainsAuthorTag(): void | ||
| { | ||
| $docblock = $this->extractProxyDocblock(); | ||
| $this->assertStringContainsString('@author', $docblock); | ||
| } | ||
|
|
||
| #[Test] | ||
| public function proxyClassDocblockContainsCopyrightTag(): void | ||
| { | ||
| $docblock = $this->extractProxyDocblock(); | ||
| $this->assertStringContainsString('@copyright', $docblock); | ||
| } | ||
|
|
||
| #[Test] | ||
| public function proxyClassDocblockDoesNotContainLegacyTags(): void | ||
| { | ||
| $docblock = $this->extractProxyDocblock(); | ||
| $this->assertStringNotContainsString('@package', $docblock, '@package is not used in PSR-12'); | ||
| $this->assertStringNotContainsString('@subpackage', $docblock, '@subpackage is not used in PSR-12'); | ||
| $this->assertStringNotContainsString('@category', $docblock, '@category is not used in PSR-12'); | ||
| } |
Comment on lines
165
to
170
| // 2.5.12 | ||
| define('_AM_SYSTEM_MODULES_FK_DISABLE', 'Failed to disable foreign key checks'); | ||
| define('_AM_SYSTEM_MODULES_FK_ENABLE', 'Failed to enable foreign key checks'); | ||
| define('_AM_SYSTEM_MODULES_DROP_FAIL', 'Failed to drop table: %s'); | ||
| define('_AM_SYSTEM_MODULES_DROP_OK', 'Dropped table: %s'); | ||
| // 2.5.7 |
Comment on lines
618
to
619
| * @author Kazumi Ono <onokazu@xoops.org> | ||
| * @copyright (c) 2000-2026 XOOPS Project (https://xoops.org) |
Comment on lines
+40
to
+51
| public function userResetHappensBeforeDelete(): void | ||
| { | ||
| $resetPos = strpos($this->source, "SET user_avatar='blank.gif'"); | ||
| $deletePos = strpos($this->source, '$avt_handler->delete($avatar)'); | ||
| $this->assertNotFalse($resetPos); | ||
| $this->assertNotFalse($deletePos); | ||
| $this->assertLessThan( | ||
| $deletePos, | ||
| $resetPos, | ||
| 'User avatar reset must happen before avatar record delete' | ||
| ); | ||
| } |
Comment on lines
+71
to
+84
| #[Test] | ||
| public function capturesAffectedUidsBeforeReset(): void | ||
| { | ||
| // Must collect affected UIDs before the reset to avoid corrupting unrelated users | ||
| $capturePos = strpos($this->source, '$affectedUids'); | ||
| $resetPos = strpos($this->source, "SET user_avatar='blank.gif'"); | ||
| $this->assertNotFalse($capturePos, 'Should capture affected UIDs before reset'); | ||
| $this->assertNotFalse($resetPos); | ||
| $this->assertLessThan( | ||
| $resetPos, | ||
| $capturePos, | ||
| 'Affected UID capture must happen before the avatar reset' | ||
| ); | ||
| } |
| } else { | ||
| $result = $xoopsDB->query('SELECT uid FROM ' . $xoopsDB->prefix('users') | ||
| . ' WHERE user_avatar=' . $xoopsDB->quote($file)); | ||
| if ($xoopsDB->isResultSet($result) && $result instanceof \mysqli_result) { |
Comment on lines
11
to
16
| * | ||
| * @copyright (c) 2000-2026 XOOPS Project (https://xoops.org) | ||
| * @license GNU GPL 2 (https://www.gnu.org/licenses/gpl-2.0.html) | ||
| * @package class | ||
| * @subpackage database | ||
| * @since 1.0.0 | ||
| * @author Kazumi Ono <onokazu@xoops.org> | ||
| * @author Rodney Fulk <redheadedrod@hotmail.com> |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
🤖 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/class/database/mysqldatabase.php`:
- Around line 618-622: The class docblock for the MySQL proxy class in
mysqldatabase.php is missing the required `@category` and `@package` tags; update
the class docblock immediately above the proxy class declaration to include the
`@category` (e.g. XOOPS) and `@package` (e.g. Database or XOOPS_DB) tags alongside
the existing `@author`, `@copyright`, `@license`, and `@link` entries so the docblock
conforms to the project's coding guidelines.
In `@htdocs/modules/system/admin/avatars/main.php`:
- Around line 350-353: The update using $xoopsDB->exec(...) is currently
best-effort and its failure must be handled: check the boolean return of
$xoopsDB->exec when updating user_avatar for $affectedUids, and on failure call
trigger_error() with E_USER_WARNING to warn admins; include only basename($file)
in the message (not the full path) and mention the affected user IDs (from
$affectedUids) to aid troubleshooting. Ensure the check surrounds the call to
$xoopsDB->exec and that the trigger_error text is concise and uses
basename($file) and implode(',', $affectedUids).
In `@tests/unit/htdocs/class/database/XoopsMySQLDatabaseProxyDocTest.php`:
- Around line 59-65: The test proxyClassDocblockDoesNotContainLegacyTags is
currently asserting that `@package`, `@subpackage` and `@category` are not present,
which conflicts with repository standards; update this test to only assert that
`@subpackage` is not present (remove or change the assertions for `@package` and
`@category`) and keep or adjust the expectation via the helper
extractProxyDocblock() so class docblocks are allowed to include `@package` and
`@category` per guidelines.
- Around line 31-65: The test methods (proxyClassDocblockContainsLicenseTag,
proxyClassDocblockContainsLinkTag, proxyClassDocblockContainsAuthorTag,
proxyClassDocblockContainsCopyrightTag,
proxyClassDocblockDoesNotContainLegacyTags) must be renamed to start with "test"
so PHPUnit discovery works across versions; change each method name to
testProxyClassDocblockContainsLicenseTag, testProxyClassDocblockContainsLinkTag,
testProxyClassDocblockContainsAuthorTag,
testProxyClassDocblockContainsCopyrightTag, and
testProxyClassDocblockDoesNotContainLegacyTags respectively, leaving their
bodies and assertions untouched (you can keep or remove the #[Test] attributes
after renaming since the test name is now sufficient).
In
`@tests/unit/htdocs/modules/system/admin/avatars/AvatarDeleteTransactionTest.php`:
- Around line 29-122: Rename the test methods to start with "test" so PHPUnit
9.6/10/11 recognize them: change doesNotUseTransactionKeywords() →
testDoesNotUseTransactionKeywords(), userResetHappensBeforeDelete() →
testUserResetHappensBeforeDelete(), abortsOnResetFailure() →
testAbortsOnResetFailure(), capturesAffectedUidsBeforeReset() →
testCapturesAffectedUidsBeforeReset(), compensatingRestoreUsesExactUids() →
testCompensatingRestoreUsesExactUids(),
usesSpecificErrorMessageOnDeleteFailure() →
testUsesSpecificErrorMessageOnDeleteFailure(), and redirectsAfterFailedDelete()
→ testRedirectsAfterFailedDelete(); update any internal references to these
method names (attributes like #[Test] may remain but ensure method names are
renamed to the corresponding test* identifiers).
In
`@tests/unit/htdocs/modules/system/language/english/admin/ModulesadminLanguageTest.php`:
- Around line 31-99: Rename the attribute-based test methods so their names
start with "test" to satisfy repository PHPUnit conventions; specifically rename
fkEnableConstantIsDefined -> testFkEnableConstantIsDefined,
dropFailConstantIsDefined -> testDropFailConstantIsDefined,
dropFailConstantContainsPlaceholder -> testDropFailConstantContainsPlaceholder,
dropOkConstantIsDefined -> testDropOkConstantIsDefined,
dropOkConstantContainsPlaceholder -> testDropOkConstantContainsPlaceholder, and
modulesadminFileUsesLocalizedFkStrings ->
testModulesadminFileUsesLocalizedFkStrings (and you may remove the #[Test]
attributes after renaming); ensure the renamed methods compile and update any
internal references so tests run under PHPUnit 9.6, 10.5, and 11.x.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 508c436c-c263-4813-8774-7574f1852bef
📒 Files selected for processing (7)
htdocs/class/database/mysqldatabase.phphtdocs/modules/system/admin/avatars/main.phphtdocs/modules/system/admin/modulesadmin/modulesadmin.phphtdocs/modules/system/language/english/admin/modulesadmin.phptests/unit/htdocs/class/database/XoopsMySQLDatabaseProxyDocTest.phptests/unit/htdocs/modules/system/admin/avatars/AvatarDeleteTransactionTest.phptests/unit/htdocs/modules/system/language/english/admin/ModulesadminLanguageTest.php
Comment on lines
+31
to
+65
| public function proxyClassDocblockContainsLicenseTag(): void | ||
| { | ||
| $docblock = $this->extractProxyDocblock(); | ||
| $this->assertStringContainsString('@license', $docblock); | ||
| } | ||
|
|
||
| #[Test] | ||
| public function proxyClassDocblockContainsLinkTag(): void | ||
| { | ||
| $docblock = $this->extractProxyDocblock(); | ||
| $this->assertStringContainsString('@link', $docblock); | ||
| } | ||
|
|
||
| #[Test] | ||
| public function proxyClassDocblockContainsAuthorTag(): void | ||
| { | ||
| $docblock = $this->extractProxyDocblock(); | ||
| $this->assertStringContainsString('@author', $docblock); | ||
| } | ||
|
|
||
| #[Test] | ||
| public function proxyClassDocblockContainsCopyrightTag(): void | ||
| { | ||
| $docblock = $this->extractProxyDocblock(); | ||
| $this->assertStringContainsString('@copyright', $docblock); | ||
| } | ||
|
|
||
| #[Test] | ||
| public function proxyClassDocblockDoesNotContainLegacyTags(): void | ||
| { | ||
| $docblock = $this->extractProxyDocblock(); | ||
| $this->assertStringNotContainsString('@package', $docblock, '@package is not used in PSR-12'); | ||
| $this->assertStringNotContainsString('@subpackage', $docblock, '@subpackage is not used in PSR-12'); | ||
| $this->assertStringNotContainsString('@category', $docblock, '@category is not used in PSR-12'); | ||
| } |
There was a problem hiding this comment.
Rename test methods to test* for required discovery compatibility.
Methods from Line 31 onward should follow the mandated test{MethodName} naming style.
As per coding guidelines "Name test methods test{MethodName} or test{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/class/database/XoopsMySQLDatabaseProxyDocTest.php` around
lines 31 - 65, The test methods (proxyClassDocblockContainsLicenseTag,
proxyClassDocblockContainsLinkTag, proxyClassDocblockContainsAuthorTag,
proxyClassDocblockContainsCopyrightTag,
proxyClassDocblockDoesNotContainLegacyTags) must be renamed to start with "test"
so PHPUnit discovery works across versions; change each method name to
testProxyClassDocblockContainsLicenseTag, testProxyClassDocblockContainsLinkTag,
testProxyClassDocblockContainsAuthorTag,
testProxyClassDocblockContainsCopyrightTag, and
testProxyClassDocblockDoesNotContainLegacyTags respectively, leaving their
bodies and assertions untouched (you can keep or remove the #[Test] attributes
after renaming since the test name is now sufficient).
Comment on lines
+29
to
+122
| public function doesNotUseTransactionKeywords(): void | ||
| { | ||
| // MyISAM tables ignore transactions, so the code must NOT rely on them | ||
| $this->assertStringNotContainsString( | ||
| 'START TRANSACTION', | ||
| $this->source, | ||
| 'MyISAM tables do not support transactions — do not use START TRANSACTION' | ||
| ); | ||
| } | ||
|
|
||
| #[Test] | ||
| public function userResetHappensBeforeDelete(): void | ||
| { | ||
| $resetPos = strpos($this->source, "SET user_avatar='blank.gif'"); | ||
| $deletePos = strpos($this->source, '$avt_handler->delete($avatar)'); | ||
| $this->assertNotFalse($resetPos); | ||
| $this->assertNotFalse($deletePos); | ||
| $this->assertLessThan( | ||
| $deletePos, | ||
| $resetPos, | ||
| 'User avatar reset must happen before avatar record delete' | ||
| ); | ||
| } | ||
|
|
||
| #[Test] | ||
| public function abortsOnResetFailure(): void | ||
| { | ||
| // After a failed reset ($resetOk = false), should redirect before deleting | ||
| $resetOkPos = strpos($this->source, '$resetOk'); | ||
| $this->assertNotFalse($resetOkPos, 'Should use $resetOk flag for reset result'); | ||
|
|
||
| $abortPos = strpos($this->source, "if (!\$resetOk)"); | ||
| $this->assertNotFalse($abortPos, 'Should check $resetOk and abort on failure'); | ||
|
|
||
| $deletePos = strpos($this->source, '$avt_handler->delete($avatar)'); | ||
| $this->assertLessThan( | ||
| $deletePos, | ||
| $abortPos, | ||
| 'Reset-failure abort must come before the delete call' | ||
| ); | ||
| } | ||
|
|
||
| #[Test] | ||
| public function capturesAffectedUidsBeforeReset(): void | ||
| { | ||
| // Must collect affected UIDs before the reset to avoid corrupting unrelated users | ||
| $capturePos = strpos($this->source, '$affectedUids'); | ||
| $resetPos = strpos($this->source, "SET user_avatar='blank.gif'"); | ||
| $this->assertNotFalse($capturePos, 'Should capture affected UIDs before reset'); | ||
| $this->assertNotFalse($resetPos); | ||
| $this->assertLessThan( | ||
| $resetPos, | ||
| $capturePos, | ||
| 'Affected UID capture must happen before the avatar reset' | ||
| ); | ||
| } | ||
|
|
||
| #[Test] | ||
| public function compensatingRestoreUsesExactUids(): void | ||
| { | ||
| // After a failed delete, restore must target exact UIDs, not WHERE user_avatar='blank.gif' | ||
| $deletePos = strpos($this->source, '$avt_handler->delete($avatar)'); | ||
| $this->assertNotFalse($deletePos); | ||
|
|
||
| $afterDelete = substr($this->source, $deletePos, 800); | ||
| $this->assertStringContainsString( | ||
| '$affectedUids', | ||
| $afterDelete, | ||
| 'Compensating restore must use captured $affectedUids, not a broad WHERE clause' | ||
| ); | ||
| $this->assertStringNotContainsString( | ||
| "WHERE user_avatar='blank.gif'", | ||
| $afterDelete, | ||
| 'Restore must NOT use WHERE user_avatar=blank.gif — it would corrupt unrelated users' | ||
| ); | ||
| } | ||
|
|
||
| #[Test] | ||
| public function usesSpecificErrorMessageOnDeleteFailure(): void | ||
| { | ||
| $deletePos = strpos($this->source, '$avt_handler->delete($avatar)'); | ||
| $this->assertNotFalse($deletePos); | ||
|
|
||
| $afterDelete = substr($this->source, $deletePos, 800); | ||
| $this->assertStringContainsString( | ||
| '_AM_SYSTEM_AVATAR_FAILDEL', | ||
| $afterDelete, | ||
| 'Should use specific _AM_SYSTEM_AVATAR_FAILDEL message, not generic _AM_SYSTEM_DBERROR' | ||
| ); | ||
| } | ||
|
|
||
| #[Test] | ||
| public function redirectsAfterFailedDelete(): void | ||
| { |
There was a problem hiding this comment.
Rename test methods to test* to align with repository test standards.
At Line 29 and onward, method names should follow the required test{MethodName} format.
As per coding guidelines "Name test methods test{MethodName} or test{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/modules/system/admin/avatars/AvatarDeleteTransactionTest.php`
around lines 29 - 122, Rename the test methods to start with "test" so PHPUnit
9.6/10/11 recognize them: change doesNotUseTransactionKeywords() →
testDoesNotUseTransactionKeywords(), userResetHappensBeforeDelete() →
testUserResetHappensBeforeDelete(), abortsOnResetFailure() →
testAbortsOnResetFailure(), capturesAffectedUidsBeforeReset() →
testCapturesAffectedUidsBeforeReset(), compensatingRestoreUsesExactUids() →
testCompensatingRestoreUsesExactUids(),
usesSpecificErrorMessageOnDeleteFailure() →
testUsesSpecificErrorMessageOnDeleteFailure(), and redirectsAfterFailedDelete()
→ testRedirectsAfterFailedDelete(); update any internal references to these
method names (attributes like #[Test] may remain but ensure method names are
renamed to the corresponding test* identifiers).
Comment on lines
+31
to
+99
| public function fkDisableConstantIsDefined(): void | ||
| { | ||
| $this->assertTrue(defined('_AM_SYSTEM_MODULES_FK_DISABLE')); | ||
| $this->assertNotEmpty(_AM_SYSTEM_MODULES_FK_DISABLE); | ||
| $this->assertIsString(_AM_SYSTEM_MODULES_FK_DISABLE); | ||
| } | ||
|
|
||
| #[Test] | ||
| public function fkEnableConstantIsDefined(): void | ||
| { | ||
| $this->assertTrue(defined('_AM_SYSTEM_MODULES_FK_ENABLE')); | ||
| $this->assertNotEmpty(_AM_SYSTEM_MODULES_FK_ENABLE); | ||
| $this->assertIsString(_AM_SYSTEM_MODULES_FK_ENABLE); | ||
| } | ||
|
|
||
| #[Test] | ||
| public function dropFailConstantIsDefined(): void | ||
| { | ||
| $this->assertTrue(defined('_AM_SYSTEM_MODULES_DROP_FAIL')); | ||
| $this->assertNotEmpty(_AM_SYSTEM_MODULES_DROP_FAIL); | ||
| $this->assertIsString(_AM_SYSTEM_MODULES_DROP_FAIL); | ||
| } | ||
|
|
||
| #[Test] | ||
| public function dropFailConstantContainsPlaceholder(): void | ||
| { | ||
| $this->assertStringContainsString('%s', _AM_SYSTEM_MODULES_DROP_FAIL); | ||
| } | ||
|
|
||
| #[Test] | ||
| public function dropOkConstantIsDefined(): void | ||
| { | ||
| $this->assertTrue(defined('_AM_SYSTEM_MODULES_DROP_OK')); | ||
| $this->assertNotEmpty(_AM_SYSTEM_MODULES_DROP_OK); | ||
| $this->assertIsString(_AM_SYSTEM_MODULES_DROP_OK); | ||
| } | ||
|
|
||
| #[Test] | ||
| public function dropOkConstantContainsPlaceholder(): void | ||
| { | ||
| $this->assertStringContainsString('%s', _AM_SYSTEM_MODULES_DROP_OK); | ||
| } | ||
|
|
||
| #[Test] | ||
| public function modulesadminFileUsesLocalizedFkStrings(): void | ||
| { | ||
| $source = file_get_contents( | ||
| XOOPS_ROOT_PATH . '/modules/system/admin/modulesadmin/modulesadmin.php' | ||
| ); | ||
| // Should NOT contain the old hard-coded English strings | ||
| $this->assertStringNotContainsString( | ||
| "'Failed to disable FOREIGN_KEY_CHECKS", | ||
| $source, | ||
| 'Hard-coded FK disable string should be replaced with _AM_SYSTEM_MODULES_FK_DISABLE' | ||
| ); | ||
| $this->assertStringNotContainsString( | ||
| "'Failed to restore FOREIGN_KEY_CHECKS", | ||
| $source, | ||
| 'Hard-coded FK restore string should be replaced with _AM_SYSTEM_MODULES_FK_ENABLE' | ||
| ); | ||
| $this->assertStringNotContainsString( | ||
| "'Failed to drop table ", | ||
| $source, | ||
| 'Hard-coded drop table string should be replaced with _AM_SYSTEM_MODULES_DROP_FAIL' | ||
| ); | ||
| // Should contain the constant references | ||
| $this->assertStringContainsString('_AM_SYSTEM_MODULES_FK_DISABLE', $source); | ||
| $this->assertStringContainsString('_AM_SYSTEM_MODULES_FK_ENABLE', $source); | ||
| } |
There was a problem hiding this comment.
Rename test methods to test* to meet repository test conventions.
At Line 31 and following, methods are attribute-based but do not follow the required test{MethodName} naming contract.
As per coding guidelines "Name test methods test{MethodName} or test{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/modules/system/language/english/admin/ModulesadminLanguageTest.php`
around lines 31 - 99, Rename the attribute-based test methods so their names
start with "test" to satisfy repository PHPUnit conventions; specifically rename
fkEnableConstantIsDefined -> testFkEnableConstantIsDefined,
dropFailConstantIsDefined -> testDropFailConstantIsDefined,
dropFailConstantContainsPlaceholder -> testDropFailConstantContainsPlaceholder,
dropOkConstantIsDefined -> testDropOkConstantIsDefined,
dropOkConstantContainsPlaceholder -> testDropOkConstantContainsPlaceholder, and
modulesadminFileUsesLocalizedFkStrings ->
testModulesadminFileUsesLocalizedFkStrings (and you may remove the #[Test]
attributes after renaming); ensure the renamed methods compile and update any
internal references so tests run under PHPUnit 9.6, 10.5, and 11.x.
… restore scope - Add trigger_error() on failed compensating avatar restore in delfileok - Reorder language constants so 2.5.12 section follows 2.5.11 (was misplaced before 2.5.7) - Verified avatar test assertions and proxy docblock test already correct
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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/system/admin/avatars/main.php`:
- Around line 337-341: Flatten the nested conditionals by combining the empty
check and the exec call into a single short-circuit expression: replace the
outer if (!empty($affectedUids)) { if (!$xoopsDB->exec(...)) { $resetOk = false;
}} with a single conditional that only calls $xoopsDB->exec when $affectedUids
is non-empty (e.g., use empty($affectedUids) || $xoopsDB->exec(...) to
short-circuit) and set $resetOk = false when the exec returns falsy; ensure you
reference the same symbols ($affectedUids, $xoopsDB->exec, $resetOk) and
preserve the exact UPDATE query string used previously.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: cee2f124-cf5e-4200-8268-6982f86ca8e7
📒 Files selected for processing (2)
htdocs/modules/system/admin/avatars/main.phphtdocs/modules/system/language/english/admin/modulesadmin.php
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
tests/unit/htdocs/class/database/XoopsMySQLDatabaseProxyDocTest.php (1)
30-72:⚠️ Potential issue | 🟠 MajorRename test methods to
test*to satisfy repository discovery rules.Line 31, Line 38, Line 45, Line 52, Line 59, and Line 67 do not follow the mandated
test{MethodName}naming convention.Proposed fix
- public function proxyClassDocblockContainsLicenseTag(): void + public function testProxyClassDocblockContainsLicenseTag(): void @@ - public function proxyClassDocblockContainsLinkTag(): void + public function testProxyClassDocblockContainsLinkTag(): void @@ - public function proxyClassDocblockContainsAuthorTag(): void + public function testProxyClassDocblockContainsAuthorTag(): void @@ - public function proxyClassDocblockContainsCopyrightTag(): void + public function testProxyClassDocblockContainsCopyrightTag(): void @@ - public function proxyClassDocblockContainsCategoryTag(): void + public function testProxyClassDocblockContainsCategoryTag(): void @@ - public function proxyClassDocblockContainsPackageTag(): void + public function testProxyClassDocblockContainsPackageTag(): void#!/bin/bash set -euo pipefail echo "== PHPUnit version constraints ==" fd '^composer\.json$' --exec jq -r '.["require-dev"]["phpunit/phpunit"] // .require["phpunit/phpunit"] // "phpunit constraint not set"' {} echo echo "== Public test methods not matching test* naming ==" rg -nP '^\s*public function (?!test)[A-Za-z_]\w*\s*\(\)\s*:\s*void' tests/unit/htdocs/class/database/XoopsMySQLDatabaseProxyDocTest.phpAs per coding guidelines "Name test methods test{MethodName} or test{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/class/database/XoopsMySQLDatabaseProxyDocTest.php` around lines 30 - 72, Rename the six public test methods so their names start with "test" to satisfy PHPUnit discovery: change proxyClassDocblockContainsLicenseTag to testProxyClassDocblockContainsLicenseTag, proxyClassDocblockContainsLinkTag to testProxyClassDocblockContainsLinkTag, proxyClassDocblockContainsAuthorTag to testProxyClassDocblockContainsAuthorTag, proxyClassDocblockContainsCopyrightTag to testProxyClassDocblockContainsCopyrightTag, proxyClassDocblockContainsCategoryTag to testProxyClassDocblockContainsCategoryTag, and proxyClassDocblockContainsPackageTag to testProxyClassDocblockContainsPackageTag (keep the #[Test] attribute and method bodies unchanged); this ensures PHPUnit will discover these methods in XoopsMySQLDatabaseProxyDocTest.
🤖 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/system/admin/avatars/main.php`:
- Around line 325-327: Do not trust the POSTed $user_id when computing
$affectedUids for custom avatars; instead of using the request-supplied $user_id
(the block where $affectedUids is set when $avatar->getVar('avatar_type','n')
=== 'C'), query the avatar_user_link table by the current avatar's avatar_id to
build $affectedUids (all user IDs linked to that avatar) and use that result for
reset/restore operations; remove reliance on the POST value in that branch so
only linked users are affected.
In `@tests/unit/htdocs/class/database/XoopsMySQLDatabaseProxyDocTest.php`:
- Around line 23-28: The test's setUp() uses XOOPS_ROOT_PATH in
file_get_contents which couples the unit test to a runtime installation; modify
setUp() in XoopsMySQLDatabaseProxyDocTest to be self-contained by either
defining XOOPS_ROOT_PATH if undefined to point at a local test-fixture directory
or, better, load the fixture via a relative path (e.g. locate a copy of
class/database/mysqldatabase.php inside the tests/fixtures directory) and assign
its contents to $this->source; update references to file_get_contents and
XOOPS_ROOT_PATH in setUp() accordingly so the test no longer relies on external
XOOPS constants or installation.
---
Duplicate comments:
In `@tests/unit/htdocs/class/database/XoopsMySQLDatabaseProxyDocTest.php`:
- Around line 30-72: Rename the six public test methods so their names start
with "test" to satisfy PHPUnit discovery: change
proxyClassDocblockContainsLicenseTag to
testProxyClassDocblockContainsLicenseTag, proxyClassDocblockContainsLinkTag to
testProxyClassDocblockContainsLinkTag, proxyClassDocblockContainsAuthorTag to
testProxyClassDocblockContainsAuthorTag, proxyClassDocblockContainsCopyrightTag
to testProxyClassDocblockContainsCopyrightTag,
proxyClassDocblockContainsCategoryTag to
testProxyClassDocblockContainsCategoryTag, and
proxyClassDocblockContainsPackageTag to testProxyClassDocblockContainsPackageTag
(keep the #[Test] attribute and method bodies unchanged); this ensures PHPUnit
will discover these methods in XoopsMySQLDatabaseProxyDocTest.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 445749ac-7214-499b-9006-965a4ec8e583
📒 Files selected for processing (3)
htdocs/class/database/mysqldatabase.phphtdocs/modules/system/admin/avatars/main.phptests/unit/htdocs/class/database/XoopsMySQLDatabaseProxyDocTest.php
Comment on lines
325
to
327
| if ($user_id > 0 && $avatar->getVar('avatar_type', 'n') === 'C') { | ||
| if (!$xoopsDB->exec('UPDATE ' . $xoopsDB->prefix('users') | ||
| . " SET user_avatar='blank.gif' WHERE uid=" . $user_id)) { | ||
| $resetOk = false; | ||
| } | ||
| $affectedUids = [$user_id]; | ||
| } else { |
There was a problem hiding this comment.
Do not trust POST user_id when computing affected users for custom avatars.
At Line 325-Line 327, $affectedUids is taken from request input. A tampered request can reset/restore the wrong user’s avatar. Resolve affected users from avatar_user_link by avatar_id instead.
🔧 Suggested fix
- if ($user_id > 0 && $avatar->getVar('avatar_type', 'n') === 'C') {
- $affectedUids = [$user_id];
+ if ($avatar->getVar('avatar_type', 'n') === 'C') {
+ $result = $xoopsDB->query(
+ 'SELECT user_id FROM ' . $xoopsDB->prefix('avatar_user_link')
+ . ' WHERE avatar_id=' . (int)$avatar_id
+ );
+ if (!$xoopsDB->isResultSet($result) || !($result instanceof \mysqli_result)) {
+ redirect_header('admin.php?fct=avatars', 2, _AM_SYSTEM_DBERROR);
+ }
+ while ($row = $xoopsDB->fetchArray($result)) {
+ $affectedUids[] = (int)$row['user_id'];
+ }
} else {
$result = $xoopsDB->query('SELECT uid FROM ' . $xoopsDB->prefix('users')
. ' WHERE user_avatar=' . $xoopsDB->quote($file));🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@htdocs/modules/system/admin/avatars/main.php` around lines 325 - 327, Do not
trust the POSTed $user_id when computing $affectedUids for custom avatars;
instead of using the request-supplied $user_id (the block where $affectedUids is
set when $avatar->getVar('avatar_type','n') === 'C'), query the avatar_user_link
table by the current avatar's avatar_id to build $affectedUids (all user IDs
linked to that avatar) and use that result for reset/restore operations; remove
reliance on the POST value in that branch so only linked users are affected.
Comment on lines
+23
to
+28
| protected function setUp(): void | ||
| { | ||
| $this->source = file_get_contents( | ||
| XOOPS_ROOT_PATH . '/class/database/mysqldatabase.php' | ||
| ); | ||
| } |
There was a problem hiding this comment.
setUp() is not unit-test isolated from XOOPS runtime constants.
Line 26 assumes XOOPS_ROOT_PATH exists; this couples the test to installation/runtime bootstrap and can fail in isolated unit environments.
Proposed fix
protected function setUp(): void
{
- $this->source = file_get_contents(
- XOOPS_ROOT_PATH . '/class/database/mysqldatabase.php'
- );
+ $rootPath = defined('XOOPS_ROOT_PATH')
+ ? XOOPS_ROOT_PATH
+ : dirname(__DIR__, 5) . '/htdocs';
+
+ $file = $rootPath . '/class/database/mysqldatabase.php';
+ $contents = file_get_contents($file);
+ $this->assertNotFalse($contents, 'Unable to read mysqldatabase.php for docblock checks');
+ $this->source = $contents;
}As per coding guidelines "tests/**/*.php: Tests must be fully isolated with no XOOPS installation required for unit tests".
📝 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
| protected function setUp(): void | |
| { | |
| $this->source = file_get_contents( | |
| XOOPS_ROOT_PATH . '/class/database/mysqldatabase.php' | |
| ); | |
| } | |
| protected function setUp(): void | |
| { | |
| $rootPath = defined('XOOPS_ROOT_PATH') | |
| ? XOOPS_ROOT_PATH | |
| : dirname(__DIR__, 5) . '/htdocs'; | |
| $file = $rootPath . '/class/database/mysqldatabase.php'; | |
| $contents = file_get_contents($file); | |
| $this->assertNotFalse($contents, 'Unable to read mysqldatabase.php for docblock checks'); | |
| $this->source = $contents; | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tests/unit/htdocs/class/database/XoopsMySQLDatabaseProxyDocTest.php` around
lines 23 - 28, The test's setUp() uses XOOPS_ROOT_PATH in file_get_contents
which couples the unit test to a runtime installation; modify setUp() in
XoopsMySQLDatabaseProxyDocTest to be self-contained by either defining
XOOPS_ROOT_PATH if undefined to point at a local test-fixture directory or,
better, load the fixture via a relative path (e.g. locate a copy of
class/database/mysqldatabase.php inside the tests/fixtures directory) and assign
its contents to $this->source; update references to file_get_contents and
XOOPS_ROOT_PATH in setUp() accordingly so the test no longer relies on external
XOOPS constants or installation.
mambax7
changed the title
Comment on lines
+339
to
+341
| if (!empty($affectedUids) && !$xoopsDB->exec('UPDATE ' . $xoopsDB->prefix('users') | ||
| . " SET user_avatar='blank.gif' WHERE uid IN (" . implode(',', $affectedUids) . ')')) { | ||
| $resetOk = false; |
| $resetOk = false; | ||
| $result = $xoopsDB->query('SELECT uid FROM ' . $xoopsDB->prefix('users') | ||
| . ' WHERE user_avatar=' . $xoopsDB->quote($file)); | ||
| if (!$xoopsDB->isResultSet($result) || !($result instanceof \mysqli_result)) { |
| define('_AM_SYSTEM_MODULES_FK_DISABLE', 'Failed to disable foreign key checks'); | ||
| define('_AM_SYSTEM_MODULES_FK_ENABLE', 'Failed to enable foreign key checks'); | ||
| define('_AM_SYSTEM_MODULES_DROP_FAIL', 'Failed to drop table: %s'); | ||
| define('_AM_SYSTEM_MODULES_DROP_OK', 'Dropped table: %s'); |
| @@ -11,8 +11,6 @@ | |||
| * | |||
| * @copyright (c) 2000-2026 XOOPS Project (https://xoops.org) | |||
| * @license GNU GPL 2 (https://www.gnu.org/licenses/gpl-2.0.html) | |||
… tests to assert absence Addresses Copilot review feedback on PR XOOPS#1651: - mysqldatabase.php: Remove @category Xoops, @Package kernel, @subpackage database from the XoopsMySQLDatabaseProxy class docblock — these are legacy tags not part of PSR-12 - modulesadmin.php: Remove empty @Package and @SInCE tags from the file header (valueless placeholders) - XoopsMySQLDatabaseProxyDocTest.php: Flip the two tag tests from assertStringContainsString to assertStringNotContainsString — tests now verify that legacy @Package and @category tags are absent per PSR-12, not present
|
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (4)
htdocs/class/database/mysqldatabase.php (1)
611-622:⚠️ Potential issue | 🟠 MajorMissing required
@categoryand@packagetags in class docblock.The
XoopsMySQLDatabaseProxyclass docblock is missing the required@categoryand@packagetags. While the PR summary mentions removing these "per PSR-12," the repository's coding guidelines explicitly require them for class docblocks.Current docblock has:
@author,@copyright,@license,@link
Still missing:@category,@packageProposed fix
/** * Proxy connection to a MySQL database with GET-request write protection. * * During GET requests (when allowWebChanges is false), this class restricts * {`@link` query()} to read-only statements (SELECT, SHOW, DESCRIBE, EXPLAIN). * Write operations should use {`@link` exec()} which is not subject to this guard. * + * `@category` Database + * `@package` Xoops\Database * `@author` Kazumi Ono <onokazu@xoops.org> * `@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 */As per coding guidelines: "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 `@htdocs/class/database/mysqldatabase.php` around lines 611 - 622, Add the missing `@category` and `@package` tags to the class docblock for XoopsMySQLDatabaseProxy so it complies with the repository coding guidelines; locate the docblock above the XoopsMySQLDatabaseProxy class (currently containing `@author`, `@copyright`, `@license`, `@link`) and insert appropriate `@category` and `@package` entries (matching the project's existing category/package values) so the block contains `@category`, `@package`, `@author`, `@copyright`, `@license`, and `@link`.tests/unit/htdocs/class/database/XoopsMySQLDatabaseProxyDocTest.php (3)
59-73:⚠️ Potential issue | 🟠 MajorTest assertions contradict repository coding guidelines.
These tests assert that
@packageand@categorytags must be absent from the class docblock, but the coding guidelines explicitly require them:"Class docblocks must include
@category,@package,@author,@copyright,@license, and@linktags"Either:
- The coding guidelines need updating to reflect PSR-12 preferences, or
- These assertions should be inverted to require
@packageand@categoryIf aligning with current guidelines
#[Test] -public function proxyClassDocblockDoesNotContainLegacyPackageTag(): void +public function testProxyClassDocblockContainsPackageTag(): void { $docblock = $this->extractProxyDocblock(); - $this->assertStringNotContainsString('@package', $docblock, - 'Proxy class docblock must not include legacy `@package` tag (PSR-12)'); + $this->assertStringContainsString('@package', $docblock, + 'Proxy class docblock must include `@package` tag per coding guidelines'); } #[Test] -public function proxyClassDocblockDoesNotContainLegacyCategoryTag(): void +public function testProxyClassDocblockContainsCategoryTag(): void { $docblock = $this->extractProxyDocblock(); - $this->assertStringNotContainsString('@category', $docblock, - 'Proxy class docblock must not include legacy `@category` tag (PSR-12)'); + $this->assertStringContainsString('@category', $docblock, + 'Proxy class docblock must include `@category` tag per coding guidelines'); }As per coding guidelines: "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/class/database/XoopsMySQLDatabaseProxyDocTest.php` around lines 59 - 73, The tests proxyClassDocblockDoesNotContainLegacyPackageTag and proxyClassDocblockDoesNotContainLegacyCategoryTag contradict the repo guidelines; update the assertions in those methods to require the tags instead of forbidding them by changing assertStringNotContainsString('@package', $docblock, ...) to assertStringContainsString('@package', $docblock, 'Proxy class docblock must include legacy `@package` tag (per coding guidelines)') and similarly change assertStringNotContainsString('@category', $docblock, ...) to assertStringContainsString('@category', $docblock, 'Proxy class docblock must include legacy `@category` tag (per coding guidelines)') so the tests match the stated requirement; keep the calls to $this->extractProxyDocblock() and only invert the assertion methods and update the failure messages.
24-29:⚠️ Potential issue | 🟠 Major
setUp()relies onXOOPS_ROOT_PATHconstant — breaks test isolation.The test assumes
XOOPS_ROOT_PATHis defined, which couples it to the XOOPS runtime bootstrap. This will fail in isolated unit test environments where XOOPS is not installed.Proposed fix for isolation
protected function setUp(): void { - $this->source = file_get_contents( - XOOPS_ROOT_PATH . '/class/database/mysqldatabase.php' - ); + $rootPath = defined('XOOPS_ROOT_PATH') + ? XOOPS_ROOT_PATH + : dirname(__DIR__, 5) . '/htdocs'; + + $file = $rootPath . '/class/database/mysqldatabase.php'; + $contents = file_get_contents($file); + $this->assertNotFalse($contents, 'Unable to read mysqldatabase.php'); + $this->source = $contents; }As per coding guidelines: "Tests must be fully isolated with no XOOPS installation required for unit tests".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/unit/htdocs/class/database/XoopsMySQLDatabaseProxyDocTest.php` around lines 24 - 29, The setUp() method in XoopsMySQLDatabaseProxyDocTest depends on the XOOPS_ROOT_PATH constant which breaks test isolation; replace usage of XOOPS_ROOT_PATH in setUp() by loading the mysqldatabase.php fixture from a test-local path instead (e.g. compute a path relative to this test file using __DIR__ or a dedicated tests/fixtures directory) and assign its contents to $this->source; update setUp() (and any helper that references XOOPS_ROOT_PATH) to use the local fixture path so the test runs without a XOOPS installation.
31-73:⚠️ Potential issue | 🟠 MajorTest methods must use
test*prefix for PHPUnit 9.6 compatibility.The
#[Test]attribute only works in PHPUnit 10+. Since the guidelines require compatibility with PHPUnit 9.6, 10.5, and 11.x, these test methods will not be discovered in PHPUnit 9.6 without thetestprefix.Rename methods:
proxyClassDocblockContainsLicenseTag→testProxyClassDocblockContainsLicenseTagproxyClassDocblockContainsLinkTag→testProxyClassDocblockContainsLinkTagproxyClassDocblockContainsAuthorTag→testProxyClassDocblockContainsAuthorTagproxyClassDocblockContainsCopyrightTag→testProxyClassDocblockContainsCopyrightTagproxyClassDocblockDoesNotContainLegacyPackageTag→testProxyClassDocblockDoesNotContainLegacyPackageTagproxyClassDocblockDoesNotContainLegacyCategoryTag→testProxyClassDocblockDoesNotContainLegacyCategoryTagYou may keep the
#[Test]attributes for clarity, but thetestprefix is required for cross-version discovery.As per coding guidelines: "Name test methods test{MethodName} or test{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/class/database/XoopsMySQLDatabaseProxyDocTest.php` around lines 31 - 73, The test methods using the PHPUnit attribute #[Test] must be renamed to include the legacy "test" prefix so they will be discovered by PHPUnit 9.6; rename proxyClassDocblockContainsLicenseTag → testProxyClassDocblockContainsLicenseTag, proxyClassDocblockContainsLinkTag → testProxyClassDocblockContainsLinkTag, proxyClassDocblockContainsAuthorTag → testProxyClassDocblockContainsAuthorTag, proxyClassDocblockContainsCopyrightTag → testProxyClassDocblockContainsCopyrightTag, proxyClassDocblockDoesNotContainLegacyPackageTag → testProxyClassDocblockDoesNotContainLegacyPackageTag, and proxyClassDocblockDoesNotContainLegacyCategoryTag → testProxyClassDocblockDoesNotContainLegacyCategoryTag; you may keep the existing #[Test] attributes but ensure the method names are updated (methods shown in this diff: extractProxyDocblock callers) so tests run on PHPUnit 9.6, 10.5 and 11.x.
🤖 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/system/admin/modulesadmin/modulesadmin.php`:
- Around line 122-124: Wrap usages of the language constants
_AM_SYSTEM_MODULES_FK_DISABLE, _AM_SYSTEM_MODULES_FK_ENABLE, and
_AM_SYSTEM_MODULES_DROP_FAIL with defined() fallbacks before adding to $errs (or
other messages) so undefined constants in non-English locales don't emit
warnings; e.g., replace direct uses like _AM_SYSTEM_MODULES_FK_DISABLE with a
ternary that checks defined('_AM_SYSTEM_MODULES_FK_DISABLE') ?
_AM_SYSTEM_MODULES_FK_DISABLE : 'Failed to disable foreign key checks' (and
analogous fallback strings for the other two constants) wherever these symbols
appear in modulesadmin.php.
In `@tests/unit/htdocs/class/database/XoopsMySQLDatabaseProxyDocTest.php`:
- Around line 12-18: Add a proper class-level docblock for the
XoopsMySQLDatabaseProxyDocTest class that includes the required tags: `@category`,
`@package`, `@author`, `@copyright`, `@license`, and `@link`; update the docblock
immediately above the class declaration in XoopsMySQLDatabaseProxyDocTest so it
follows PSR-12 and the project coding guidelines by providing appropriate values
for each tag (replace placeholder values with real project/author/license/link
info).
- Line 1: Add the required XOOPS copyright/header block at the very top of the
file (before the opening <?php) so the file complies with project coding
guidelines; update the header in
tests/unit/htdocs/class/database/XoopsMySQLDatabaseProxyDocTest.php by inserting
the standard XOOPS copyright/header block used across the codebase (matching
other files) and ensure there are no blank lines before the header or before the
opening <?php.
---
Duplicate comments:
In `@htdocs/class/database/mysqldatabase.php`:
- Around line 611-622: Add the missing `@category` and `@package` tags to the class
docblock for XoopsMySQLDatabaseProxy so it complies with the repository coding
guidelines; locate the docblock above the XoopsMySQLDatabaseProxy class
(currently containing `@author`, `@copyright`, `@license`, `@link`) and insert
appropriate `@category` and `@package` entries (matching the project's existing
category/package values) so the block contains `@category`, `@package`, `@author`,
`@copyright`, `@license`, and `@link`.
In `@tests/unit/htdocs/class/database/XoopsMySQLDatabaseProxyDocTest.php`:
- Around line 59-73: The tests proxyClassDocblockDoesNotContainLegacyPackageTag
and proxyClassDocblockDoesNotContainLegacyCategoryTag contradict the repo
guidelines; update the assertions in those methods to require the tags instead
of forbidding them by changing assertStringNotContainsString('@package',
$docblock, ...) to assertStringContainsString('@package', $docblock, 'Proxy
class docblock must include legacy `@package` tag (per coding guidelines)') and
similarly change assertStringNotContainsString('@category', $docblock, ...) to
assertStringContainsString('@category', $docblock, 'Proxy class docblock must
include legacy `@category` tag (per coding guidelines)') so the tests match the
stated requirement; keep the calls to $this->extractProxyDocblock() and only
invert the assertion methods and update the failure messages.
- Around line 24-29: The setUp() method in XoopsMySQLDatabaseProxyDocTest
depends on the XOOPS_ROOT_PATH constant which breaks test isolation; replace
usage of XOOPS_ROOT_PATH in setUp() by loading the mysqldatabase.php fixture
from a test-local path instead (e.g. compute a path relative to this test file
using __DIR__ or a dedicated tests/fixtures directory) and assign its contents
to $this->source; update setUp() (and any helper that references
XOOPS_ROOT_PATH) to use the local fixture path so the test runs without a XOOPS
installation.
- Around line 31-73: The test methods using the PHPUnit attribute #[Test] must
be renamed to include the legacy "test" prefix so they will be discovered by
PHPUnit 9.6; rename proxyClassDocblockContainsLicenseTag →
testProxyClassDocblockContainsLicenseTag, proxyClassDocblockContainsLinkTag →
testProxyClassDocblockContainsLinkTag, proxyClassDocblockContainsAuthorTag →
testProxyClassDocblockContainsAuthorTag, proxyClassDocblockContainsCopyrightTag
→ testProxyClassDocblockContainsCopyrightTag,
proxyClassDocblockDoesNotContainLegacyPackageTag →
testProxyClassDocblockDoesNotContainLegacyPackageTag, and
proxyClassDocblockDoesNotContainLegacyCategoryTag →
testProxyClassDocblockDoesNotContainLegacyCategoryTag; you may keep the existing
#[Test] attributes but ensure the method names are updated (methods shown in
this diff: extractProxyDocblock callers) so tests run on PHPUnit 9.6, 10.5 and
11.x.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 6cbccda1-ad08-4219-98cb-af0981f4ac27
📒 Files selected for processing (3)
htdocs/class/database/mysqldatabase.phphtdocs/modules/system/admin/modulesadmin/modulesadmin.phptests/unit/htdocs/class/database/XoopsMySQLDatabaseProxyDocTest.php
Comment on lines
122
to
124
| if (!$db->exec('SET FOREIGN_KEY_CHECKS = 0')) { | ||
| $errs[] = 'Failed to disable FOREIGN_KEY_CHECKS before table creation: ' . htmlspecialchars($db->error(), ENT_QUOTES | ENT_HTML5, 'UTF-8'); | ||
| $errs[] = _AM_SYSTEM_MODULES_FK_DISABLE . ': ' . htmlspecialchars($db->error(), ENT_QUOTES | ENT_HTML5, 'UTF-8'); | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check if language constants are defined in any other locale directories
echo "=== Searching for locale directories under system/language ==="
fd -t d . htdocs/modules/system/language/ 2>/dev/null || echo "No language directories found"
echo ""
echo "=== Searching for _AM_SYSTEM_MODULES_FK_DISABLE definition across all files ==="
rg -n "_AM_SYSTEM_MODULES_FK_DISABLE" --type php
echo ""
echo "=== Checking if XOOPS has a fallback mechanism for undefined constants ==="
rg -n "defined\s*\(\s*['\"]_AM_SYSTEM" --type php -C2 | head -40Repository: XOOPS/XoopsCore25
Length of output: 6412
Language constants lack fallback protection for non-English locales.
The constants _AM_SYSTEM_MODULES_FK_DISABLE, _AM_SYSTEM_MODULES_FK_ENABLE, and _AM_SYSTEM_MODULES_DROP_FAIL are defined only in the English language file (htdocs/modules/system/language/english/admin/modulesadmin.php). Code at lines 123, 167, 186, 737 (and others) uses these constants without checking if they're defined. Non-English XOOPS installations that have their own locale folders but lack translations for these constants will trigger PHP warnings and display the undefined constant name as a literal string.
XOOPS elsewhere uses defensive defined() checks (e.g., tests/bootstrap.php:627). Apply the same pattern here:
$message = defined('_AM_SYSTEM_MODULES_FK_DISABLE')
? _AM_SYSTEM_MODULES_FK_DISABLE
: 'Failed to disable foreign key checks';Or ensure all supported locales receive these new constants in their respective language files.
The escaping with htmlspecialchars($db->error(), ENT_QUOTES | ENT_HTML5, 'UTF-8') is correct throughout.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@htdocs/modules/system/admin/modulesadmin/modulesadmin.php` around lines 122 -
124, Wrap usages of the language constants _AM_SYSTEM_MODULES_FK_DISABLE,
_AM_SYSTEM_MODULES_FK_ENABLE, and _AM_SYSTEM_MODULES_DROP_FAIL with defined()
fallbacks before adding to $errs (or other messages) so undefined constants in
non-English locales don't emit warnings; e.g., replace direct uses like
_AM_SYSTEM_MODULES_FK_DISABLE with a ternary that checks
defined('_AM_SYSTEM_MODULES_FK_DISABLE') ? _AM_SYSTEM_MODULES_FK_DISABLE :
'Failed to disable foreign key checks' (and analogous fallback strings for the
other two constants) wherever these symbols appear in modulesadmin.php.
| @@ -0,0 +1,94 @@ | |||
| <?php | |||
There was a problem hiding this comment.
Missing XOOPS copyright header block.
Every source file must begin with the XOOPS copyright header block per coding guidelines.
Proposed header
<?php
+/**
+ * Test file for XoopsMySQLDatabaseProxy docblock validation
+ *
+ * You may not change or alter any portion of this comment or credits
+ * of supporting developers from this source code or any supporting source code
+ * which is considered copyrighted (c) material of the original comment or credit authors.
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * 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)
+ */
declare(strict_types=1);As per coding guidelines: "Every source file must begin with the XOOPS copyright header block".
📝 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
| <?php | |
| <?php | |
| /** | |
| * Test file for XoopsMySQLDatabaseProxy docblock validation | |
| * | |
| * You may not change or alter any portion of this comment or credits | |
| * of supporting developers from this source code or any supporting source code | |
| * which is considered copyrighted (c) material of the original comment or credit authors. | |
| * This program is distributed in the hope that it will be useful, | |
| * but WITHOUT ANY WARRANTY; without even the implied warranty of | |
| * 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) | |
| */ | |
| declare(strict_types=1); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tests/unit/htdocs/class/database/XoopsMySQLDatabaseProxyDocTest.php` at line
1, Add the required XOOPS copyright/header block at the very top of the file
(before the opening <?php) so the file complies with project coding guidelines;
update the header in
tests/unit/htdocs/class/database/XoopsMySQLDatabaseProxyDocTest.php by inserting
the standard XOOPS copyright/header block used across the codebase (matching
other files) and ensure there are no blank lines before the header or before the
opening <?php.
Comment on lines
+12
to
+18
| /** | ||
| * Tests for XoopsMySQLDatabaseProxy PHPDoc metadata (M-7). | ||
| * | ||
| * Verifies that the class docblock follows PSR-12 conventions: | ||
| * required tags (@author, @copyright, @license, @link) are present, | ||
| * and legacy tags (@package, @subpackage, @category) are absent. | ||
| */ |
There was a problem hiding this comment.
Test class docblock is missing required tags.
The test class itself should follow the same docblock standards. Missing: @category, @package, @author, @copyright, @license, @link.
Proposed docblock
/**
* Tests for XoopsMySQLDatabaseProxy PHPDoc metadata (M-7).
*
* Verifies that the class docblock follows PSR-12 conventions:
* required tags (`@author`, `@copyright`, `@license`, `@link`) are present,
* and legacy tags (`@package`, `@subpackage`, `@category`) are absent.
+ *
+ * `@category` Test
+ * `@package` Xoops\Test\Database
+ * `@author` XOOPS Development 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
*/As per coding guidelines: "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/class/database/XoopsMySQLDatabaseProxyDocTest.php` around
lines 12 - 18, Add a proper class-level docblock for the
XoopsMySQLDatabaseProxyDocTest class that includes the required tags: `@category`,
`@package`, `@author`, `@copyright`, `@license`, and `@link`; update the docblock
immediately above the class declaration in XoopsMySQLDatabaseProxyDocTest so it
follows PSR-12 and the project coding guidelines by providing appropriate values
for each tag (replace placeholder values with real project/author/license/link
info).
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #1651 +/- ##
==============================
==============================
☔ 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.
Summary
_AM_SYSTEM_MODULES_*language constants and replace 10 hard-coded English FK/DROP error strings in modulesadmin.php for i18n compliance@category,@license,@linkPHPDoc tags toXoopsMySQLDatabaseProxyTest plan
Summary by CodeRabbit
Bug Fixes
Improvements