Add unit tests for module admin controller and confirm template#1632
Merged
Conversation
Static analysis tests for main.php (54 tests) covering security checks, operation routing, input sanitization, and all CRUD operations. Template tests for system_modules_confirm.tpl (39 tests) covering structure, Smarty syntax, hidden fields, and regression checks. Based on CodeRabbit suggestions from PR XOOPS#1631, adapted for correct path resolution, XOOPS Smarty delimiters, and aligned source formatting. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
|
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. 📒 Files selected for processing (1)
WalkthroughAdds comprehensive PHPUnit tests for the system modules admin controller and its confirmation template, and introduces a SourceFileTestTrait to centralize locating and loading source files. ModulesAdminTest is updated to use the new trait for source loading instead of manual path resolution. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~30 minutes 🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![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 `@tests/unit/htdocs/modules/system/admin/modulesadmin/MainControllerTest.php`:
- Around line 889-900: The test method testFileHasValidPhpSyntax should
defensively handle missing exec() or unavailable PHP binary: check
function_exists('exec') and that PHP_BINARY is defined and executable
(is_executable(PHP_BINARY)) before calling exec(), and if either check fails
call $this->markTestSkipped with an explanatory message; when calling exec, use
escapeshellarg($this->filePath) with PHP_BINARY . ' -l' to run the syntax check
and keep the existing assertions on $returnCode and $output.
- Around line 19-25: Add the required docblock tags to the test class docblock
for MainControllerTest: update the class-level PHPDoc above class
MainControllerTest to include `@category`, `@package`, `@author`, `@copyright`,
`@license`, and `@link` entries (matching project conventions and values used
elsewhere in test files), ensuring the docblock format and tag order follow the
repository's coding guidelines.
- Around line 1128-1159: The extractOperationSection function contains nested
conditionals; refactor it to use early returns to satisfy PHPMD and improve
readability: keep the preg_match check (return '' if not matched), compute
$startPos, then compute $nextCase and $nextBreak and determine $endPos via a
sequence of simple if/return checks (e.g. if both false return '' or handle each
branch with an early return instead of nested if/elses), finally return
substr($this->sourceCode, $startPos, $endPos - $startPos); retain the same
behavior (including adding 6 for "break;") and reference the same symbol name
extractOperationSection to locate the change.
In
`@tests/unit/htdocs/modules/system/templates/SystemModulesConfirmTemplateTest.php`:
- Around line 19-25: Add the required PHPDoc tags to the class docblock for
SystemModulesConfirmTemplateTest: include `@category`, `@package`, `@author`,
`@copyright`, `@license`, and `@link`, placing them in the docblock immediately above
the class declaration (SystemModulesConfirmTemplateTest) to comply with the
project's coding guidelines; ensure each tag has appropriate values
(project/category name for `@category/`@package, author name/email for `@author`,
copyright holder and year for `@copyright`, SPDX or full text for `@license`, and a
relevant repository or project URL for `@link`).
- Around line 647-667: The testFooterUsesProperColspan method currently uses an
if/else; refactor to use an early-return to satisfy PHPMD by checking the
single-column case first: after computing $columnCount (via preg_match_all on
$this->templateContent), if $columnCount <= 1 perform the
assertStringNotContainsString('colspan="3"', $this->templateContent) and return;
otherwise perform the assertStringContainsString('colspan="'.$columnCount.'"',
$this->templateContent). Keep the same messages and variables but remove the
else block to flatten the control flow.
- Around line 748-764: Rename the misleading test method
testHiddenFieldsUseProperValueEscaping to something like
testHiddenFieldsUseProperValueSyntax (and update the docblock comment) because
the test only asserts the presence of Smarty variable syntax, not actual HTML
escaping; also update the assertion failure messages ('Should escape oldname in
hidden field value' and 'Should escape newname in hidden field value') to
reflect "Should use proper value syntax" (or similar) and leave the assertions
that check for 'value="<{$row.oldname}>"' and 'value="<{$row.newname}>"' against
$this->templateContent unchanged.
ℹ️ Review info
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (2)
tests/unit/htdocs/modules/system/admin/modulesadmin/MainControllerTest.phptests/unit/htdocs/modules/system/templates/SystemModulesConfirmTemplateTest.php
Comment on lines
+889
to
+900
| public function testFileHasValidPhpSyntax(): void | ||
| { | ||
| $output = []; | ||
| $returnCode = 0; | ||
| exec('php -l ' . escapeshellarg($this->filePath) . ' 2>&1', $output, $returnCode); | ||
|
|
||
| $this->assertEquals( | ||
| 0, | ||
| $returnCode, | ||
| 'PHP syntax check failed: ' . implode("\n", $output) | ||
| ); | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Consider defensive handling for exec() availability.
The exec() function may be disabled in certain PHP configurations (e.g., disable_functions in php.ini), and the php binary path isn't guaranteed in all CI environments. Adding a guard prevents unexpected failures.
🛡️ Proposed defensive check
public function testFileHasValidPhpSyntax(): void
{
+ if (!function_exists('exec') || !is_callable('exec')) {
+ $this->markTestSkipped('exec() is not available in this environment');
+ }
+
$output = [];
$returnCode = 0;
exec('php -l ' . escapeshellarg($this->filePath) . ' 2>&1', $output, $returnCode);
$this->assertEquals(
0,
$returnCode,
'PHP syntax check failed: ' . implode("\n", $output)
);
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tests/unit/htdocs/modules/system/admin/modulesadmin/MainControllerTest.php`
around lines 889 - 900, The test method testFileHasValidPhpSyntax should
defensively handle missing exec() or unavailable PHP binary: check
function_exists('exec') and that PHP_BINARY is defined and executable
(is_executable(PHP_BINARY)) before calling exec(), and if either check fails
call $this->markTestSkipped with an explanatory message; when calling exec, use
escapeshellarg($this->filePath) with PHP_BINARY . ' -l' to run the syntax check
and keep the existing assertions on $returnCode and $output.
Comment on lines
+748
to
+764
| /** | ||
| * Verify that hidden fields use proper value escaping. | ||
| */ | ||
| public function testHiddenFieldsUseProperValueEscaping(): void | ||
| { | ||
| $this->assertStringContainsString( | ||
| 'value="<{$row.oldname}>"', | ||
| $this->templateContent, | ||
| 'Should escape oldname in hidden field value' | ||
| ); | ||
|
|
||
| $this->assertStringContainsString( | ||
| 'value="<{$row.newname}>"', | ||
| $this->templateContent, | ||
| 'Should escape newname in hidden field value' | ||
| ); | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Test name may be misleading regarding "escaping".
The test verifies that the template uses Smarty variable syntax <{$row.oldname}>, but Smarty doesn't inherently escape output—that depends on the Smarty configuration (escape_html modifier or security policy). The test name and assertion messages suggest escaping is validated, which isn't quite accurate.
Consider renaming to testHiddenFieldsUseProperValueSyntax or similar for clarity.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@tests/unit/htdocs/modules/system/templates/SystemModulesConfirmTemplateTest.php`
around lines 748 - 764, Rename the misleading test method
testHiddenFieldsUseProperValueEscaping to something like
testHiddenFieldsUseProperValueSyntax (and update the docblock comment) because
the test only asserts the presence of Smarty variable syntax, not actual HTML
escaping; also update the assertion failure messages ('Should escape oldname in
hidden field value' and 'Should escape newname in hidden field value') to
reflect "Should use proper value syntax" (or similar) and leave the assertions
that check for 'value="<{$row.oldname}>"' and 'value="<{$row.newname}>"' against
$this->templateContent unchanged.
There was a problem hiding this comment.
Pull request overview
This PR adds comprehensive static analysis unit tests for the XOOPS module administration system. The tests use source code inspection to verify controller logic and template structure without requiring a database or full XOOPS bootstrap, enabling fast and isolated testing.
Changes:
- Add 54 unit tests for
modules/system/admin/modulesadmin/main.phpcovering security, operation routing, input sanitization, and all CRUD operations - Add 39 unit tests for
system_modules_confirm.tplcovering template structure, Smarty syntax, form controls, and security token handling - Implement adaptive test logic that works both pre- and post-PR #1631 (which removed Action/Order columns)
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| tests/unit/htdocs/modules/system/admin/modulesadmin/MainControllerTest.php | Comprehensive static analysis tests for module admin controller including permission checks, security token validation, operation routing, input sanitization, and all module operations (list, install, update, uninstall, confirm, submit) |
| tests/unit/htdocs/modules/system/templates/SystemModulesConfirmTemplateTest.php | Static analysis tests for confirmation template including HTML structure, Smarty syntax validation, form controls, security token handling, table structure, and adaptive colspan validation for PR #1631 compatibility |
…ication The path resolution and file loading logic was duplicated across ModulesAdminTest, MainControllerTest, and SystemModulesConfirmTemplateTest. Extract into a reusable trait to drop below the 3% duplication threshold. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 5
♻️ Duplicate comments (3)
tests/unit/htdocs/modules/system/admin/modulesadmin/MainControllerTest.php (2)
22-28:⚠️ Potential issue | 🟡 MinorAdd required class PHPDoc metadata tags.
The class-level docblock still misses mandatory tags required by repo standards.
📝 Suggested docblock update
/** * Tests for main.php module administration controller * * These tests verify the behavior of the module administration interface * including operation routing, security token validation, parameter handling, * and data preparation for templates. + * + * `@category` Test + * `@package` Tests\Unit\System\ModulesAdmin + * `@author` XOOPS Development Team + * `@copyright` 2000-2026 XOOPS Project (https://xoops.org) + * `@license` GNU GPL 2.0 or later (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 `@tests/unit/htdocs/modules/system/admin/modulesadmin/MainControllerTest.php` around lines 22 - 28, The class docblock for MainControllerTest is missing required PHPDoc metadata; update the class-level comment above class MainControllerTest to include the mandatory tags `@category`, `@package`, `@author`, `@copyright`, `@license`, and `@link` with appropriate values (matching project conventions), ensuring the docblock sits immediately above the "class MainControllerTest" declaration.
869-874:⚠️ Potential issue | 🟡 MinorGuard syntax-lint execution for restricted environments.
exec('php -l ...')is unguarded; environments with disabledexecor non-executable PHP binary will fail this test for infra reasons, not code correctness.🛡️ Suggested guard
public function testFileHasValidPhpSyntax(): void { + if (!function_exists('exec') || !is_callable('exec')) { + $this->markTestSkipped('exec() is not available in this environment'); + } + if (!defined('PHP_BINARY') || !is_executable(PHP_BINARY)) { + $this->markTestSkipped('PHP_BINARY is not available or not executable'); + } + $output = []; $returnCode = 0; - exec('php -l ' . escapeshellarg($this->filePath) . ' 2>&1', $output, $returnCode); + exec(PHP_BINARY . ' -l ' . escapeshellarg($this->filePath) . ' 2>&1', $output, $returnCode);Use this read-only check to verify the current method lacks those guards:
#!/bin/bash rg -n -C3 'function testFileHasValidPhpSyntax|exec\(|function_exists\(\x27exec\x27\)|PHP_BINARY|is_executable' tests/unit/htdocs/modules/system/admin/modulesadmin/MainControllerTest.phpExpected result:
exec('php -l ...')appears, while guard checks forexec()/PHP_BINARYare absent.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/unit/htdocs/modules/system/admin/modulesadmin/MainControllerTest.php` around lines 869 - 874, The testFileHasValidPhpSyntax method directly calls exec('php -l ' . escapeshellarg($this->filePath)) which can fail in restricted CI/containers; guard the execution by checking function_exists('exec') and is_executable(PHP_BINARY) (or existence of PHP_BINARY) before running exec, and call $this->markTestSkipped with a clear message when the checks fail; locate the exec invocation in testFileHasValidPhpSyntax and add the pre-checks there (using PHP_BINARY, function_exists('exec'), and is_executable) to avoid false failures.tests/unit/htdocs/modules/system/templates/SystemModulesConfirmTemplateTest.php (1)
22-27:⚠️ Potential issue | 🟡 MinorAdd the required class PHPDoc metadata tags.
The class docblock is missing required tags (
@category,@author,@link, etc.), which is still a repo rule violation.📝 Suggested docblock update
/** * Tests for system_modules_confirm.tpl template * * These tests verify the template structure, Smarty syntax, variable usage, * and security token handling in the module confirmation template. + * + * `@category` Test + * `@package` Tests\Unit\System\Templates + * `@author` XOOPS Development Team + * `@copyright` 2000-2026 XOOPS Project (https://xoops.org) + * `@license` GNU GPL 2.0 or later (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 `@tests/unit/htdocs/modules/system/templates/SystemModulesConfirmTemplateTest.php` around lines 22 - 27, Add a complete PHPDoc block above the SystemModulesConfirmTemplateTest class that includes the required tags: `@category`, `@package`, `@author`, `@copyright`, `@license`, and `@link`; ensure the class-level docblock sits immediately before the class declaration for SystemModulesConfirmTemplateTest and fills each tag with appropriate values (project/category names, author name/email, copyright holder and year, license identifier and URL, and a project or repo link) to satisfy the repository docblock rules.
🤖 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/modules/system/admin/modulesadmin/MainControllerTest.php`:
- Around line 737-747: The second assertion in MainControllerTest duplicates the
first and doesn't validate a separate toggle path; update the second
assertStringContainsString on $displaySection to assert the distinct
block-toggle output instead of repeating "setVar('isactive', !\$old)". Replace
the redundant assertion with one that checks for the block-specific toggle
string (e.g. "setVar('blockactive', !\$old)" or the exact template fragment
emitted for the block toggle) using assertStringContainsString($needle,
$displaySection) so both toggle behaviors are verified.
- Around line 783-793: The test asserts currently expect raw-superglobal checks
in the generated $orderSection (e.g., "if (isset(\$_POST['mod']))" and "foreach
(\$_POST['mod'] as \$order)"); update the assertions to instead look for secure
input handling (for example usage of Xmf\Request::getVar('mod', ...) or
Xmf\FilterInput::clean(...)) and that the code iterates the filtered variable
(not $_POST directly). Locate references to $orderSection and any other similar
blocks (also those around lines 947-964) and change the assertions to assert
presence of Xmf\Request::getVar or Xmf\FilterInput::clean and iteration over the
resulting variable name rather than raw $_POST access.
In `@tests/unit/htdocs/modules/system/SourceFileTestTrait.php`:
- Around line 48-54: The candidate path generation loop that iterates $level and
appends $base . '/' . $relativePath (and variant without 'htdocs/') must be made
boundary-safe: compute realpath($base) and realpath($repoRoot) (or __DIR__
anchored repo root) and only add candidate entries when realpath($base) exists
and is a descendant of the repo root to prevent climbing above the repository;
also clamp the traversal by reducing the max $level to a repo-relevant depth
(don’t exceed the repo root) and apply realpath() to the final candidate before
pushing to $candidates so all path resolution uses canonical paths. Ensure the
same changes are applied to the similar block around lines 57-63.
- Around line 69-70: file_get_contents($this->filePath) can return false and
assigning it directly to the typed property $this->sourceContent will cause a
TypeError before the assertion runs; instead, read into a temporary variable
(e.g. $content = file_get_contents($this->filePath)), assert it is not false
(use $this->assertNotFalse($content, '...') or fail with a clear message), then
cast/assign the validated string to $this->sourceContent so the typed property
only ever receives a string.
In
`@tests/unit/htdocs/modules/system/templates/SystemModulesConfirmTemplateTest.php`:
- Around line 117-120: The code slices template branches using substr() based on
strpos() results without checking for false; update the extraction around
$modifsStart/$elsePos by first asserting both strpos() calls returned !== false
(e.g., check $modifsStart !== false and $elsePos !== false) before calling
substr(), and throw or fail the test with a clear message if either is false;
apply the same guarding pattern to the other branch-extraction block that uses
strpos() and substr() (the block around the later $modifsStart/$elsePos
variables at the other extraction) so no substr() is invoked with a false
offset.
---
Duplicate comments:
In `@tests/unit/htdocs/modules/system/admin/modulesadmin/MainControllerTest.php`:
- Around line 22-28: The class docblock for MainControllerTest is missing
required PHPDoc metadata; update the class-level comment above class
MainControllerTest to include the mandatory tags `@category`, `@package`, `@author`,
`@copyright`, `@license`, and `@link` with appropriate values (matching project
conventions), ensuring the docblock sits immediately above the "class
MainControllerTest" declaration.
- Around line 869-874: The testFileHasValidPhpSyntax method directly calls
exec('php -l ' . escapeshellarg($this->filePath)) which can fail in restricted
CI/containers; guard the execution by checking function_exists('exec') and
is_executable(PHP_BINARY) (or existence of PHP_BINARY) before running exec, and
call $this->markTestSkipped with a clear message when the checks fail; locate
the exec invocation in testFileHasValidPhpSyntax and add the pre-checks there
(using PHP_BINARY, function_exists('exec'), and is_executable) to avoid false
failures.
In
`@tests/unit/htdocs/modules/system/templates/SystemModulesConfirmTemplateTest.php`:
- Around line 22-27: Add a complete PHPDoc block above the
SystemModulesConfirmTemplateTest class that includes the required tags:
`@category`, `@package`, `@author`, `@copyright`, `@license`, and `@link`; ensure the
class-level docblock sits immediately before the class declaration for
SystemModulesConfirmTemplateTest and fills each tag with appropriate values
(project/category names, author name/email, copyright holder and year, license
identifier and URL, and a project or repo link) to satisfy the repository
docblock rules.
ℹ️ Review info
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (4)
tests/unit/htdocs/modules/system/ModulesAdminTest.phptests/unit/htdocs/modules/system/SourceFileTestTrait.phptests/unit/htdocs/modules/system/admin/modulesadmin/MainControllerTest.phptests/unit/htdocs/modules/system/templates/SystemModulesConfirmTemplateTest.php
Comment on lines
+783
to
+793
| $this->assertStringContainsString( | ||
| "if (isset(\$_POST['mod']))", | ||
| $orderSection, | ||
| 'Should check for mod POST data' | ||
| ); | ||
|
|
||
| $this->assertStringContainsString( | ||
| "foreach (\$_POST['mod'] as \$order)", | ||
| $orderSection, | ||
| 'Should iterate over module order array' | ||
| ); |
There was a problem hiding this comment.
These assertions lock in insecure input-handling patterns.
The tests currently enforce raw $_POST checks and variable-variable extraction patterns, which makes secure refactoring harder and conflicts with the project’s input-filtering rules.
As per coding guidelines: "All user input must be filtered using Xmf\Request::getVar() or Xmf\FilterInput::clean() — never access $_GET, $_POST, or $_REQUEST directly."
Also applies to: 947-964
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tests/unit/htdocs/modules/system/admin/modulesadmin/MainControllerTest.php`
around lines 783 - 793, The test asserts currently expect raw-superglobal checks
in the generated $orderSection (e.g., "if (isset(\$_POST['mod']))" and "foreach
(\$_POST['mod'] as \$order)"); update the assertions to instead look for secure
input handling (for example usage of Xmf\Request::getVar('mod', ...) or
Xmf\FilterInput::clean(...)) and that the code iterates the filtered variable
(not $_POST directly). Locate references to $orderSection and any other similar
blocks (also those around lines 947-964) and change the assertions to assert
presence of Xmf\Request::getVar or Xmf\FilterInput::clean and iteration over the
resulting variable name rather than raw $_POST access.
Comment on lines
+117
to
+120
| $modifsStart = strpos($this->templateContent, '{if isset($modifs_mods)}'); | ||
| $elsePos = strpos($this->templateContent, '{else}', $modifsStart); | ||
| $modifsBranch = substr($this->templateContent, $modifsStart, $elsePos - $modifsStart); | ||
|
|
There was a problem hiding this comment.
Guard strpos() results before slicing template branches.
Both branch-extraction paths compute substr() offsets from strpos() results without fully asserting they are not false. Add explicit assertions first to avoid misleading failures.
🧪 Suggested hardening
$modifsStart = strpos($this->templateContent, '{if isset($modifs_mods)}');
+ $this->assertNotFalse($modifsStart, 'Should have modifs_mods conditional start');
$elsePos = strpos($this->templateContent, '{else}', $modifsStart);
+ $this->assertNotFalse($elsePos, 'Should have else branch for modifs_mods conditional');
$modifsBranch = substr($this->templateContent, $modifsStart, $elsePos - $modifsStart);
@@
$elsePos = strpos($this->templateContent, '{else}', $formEnd);
$this->assertNotFalse($elsePos, 'Should have else branch after form');
$endifPos = strpos($this->templateContent, '{/if}', $elsePos);
+ $this->assertNotFalse($endifPos, 'Should have closing {/if} after else branch');
$elseBranch = substr($this->templateContent, $elsePos, $endifPos - $elsePos);Also applies to: 151-152
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@tests/unit/htdocs/modules/system/templates/SystemModulesConfirmTemplateTest.php`
around lines 117 - 120, The code slices template branches using substr() based
on strpos() results without checking for false; update the extraction around
$modifsStart/$elsePos by first asserting both strpos() calls returned !== false
(e.g., check $modifsStart !== false and $elsePos !== false) before calling
substr(), and throw or fail the test with a clear message if either is false;
apply the same guarding pattern to the other branch-extraction block that uses
strpos() and substr() (the block around the later $modifsStart/$elsePos
variables at the other extraction) so no substr() is invoked with a false
offset.
Comment on lines
18
to
+20
|
|
||
| require_once __DIR__ . '/SourceFileTestTrait.php'; | ||
| use Tests\Unit\System\SourceFileTestTrait; |
There was a problem hiding this comment.
use import statements must appear before any non-declare/namespace statements. Here require_once precedes use Tests\Unit\System\SourceFileTestTrait;, which will cause a PHP parse error. Move the require_once below the use statements (but still before the class definition), or drop the use import and reference the trait with a fully-qualified name inside the class.
Suggested change
| require_once __DIR__ . '/SourceFileTestTrait.php'; | |
| use Tests\Unit\System\SourceFileTestTrait; | |
| use Tests\Unit\System\SourceFileTestTrait; | |
| require_once __DIR__ . '/SourceFileTestTrait.php'; |
Comment on lines
+18
to
+20
|
|
||
| require_once dirname(__DIR__) . '/SourceFileTestTrait.php'; | ||
| use Tests\Unit\System\SourceFileTestTrait; |
There was a problem hiding this comment.
use import statements must appear before any non-declare/namespace statements. Here require_once precedes use Tests\Unit\System\SourceFileTestTrait;, which will cause a PHP parse error. Move the require_once below the use statements (but still before the class definition), or drop the use import and reference the trait with a fully-qualified name inside the class.
Suggested change
| require_once dirname(__DIR__) . '/SourceFileTestTrait.php'; | |
| use Tests\Unit\System\SourceFileTestTrait; | |
| use Tests\Unit\System\SourceFileTestTrait; | |
| require_once dirname(__DIR__) . '/SourceFileTestTrait.php'; |
| public function testIncludesSystemHeader(): void | ||
| { | ||
| $this->assertStringContainsString( | ||
| '{include file="db:system_header.tpl"}', |
There was a problem hiding this comment.
These assertions use {...} Smarty tags, but the actual system_modules_confirm.tpl uses XOOPS <{...}> delimiters (e.g. <{include ...}>, <{if ...}>). As written, tests like testIncludesSystemHeader() will fail. Update expected strings/regexes to match the template’s <{...}> syntax (or make the tests accept both syntaxes consistently).
Suggested change
| '{include file="db:system_header.tpl"}', | |
| '<{include file="db:system_header.tpl"}>', |
Comment on lines
+18
to
+20
|
|
||
| require_once dirname(__DIR__, 2) . '/SourceFileTestTrait.php'; | ||
| use Tests\Unit\System\SourceFileTestTrait; |
There was a problem hiding this comment.
use import statements must appear before any non-declare/namespace statements. Here require_once precedes use Tests\Unit\System\SourceFileTestTrait;, which will cause a PHP parse error. Move the require_once below the use statements (but still before the class definition), or drop the use import and reference the trait with a fully-qualified name inside the class.
Suggested change
| require_once dirname(__DIR__, 2) . '/SourceFileTestTrait.php'; | |
| use Tests\Unit\System\SourceFileTestTrait; | |
| use Tests\Unit\System\SourceFileTestTrait; | |
| require_once dirname(__DIR__, 2) . '/SourceFileTestTrait.php'; |
- Bound path resolution to repo root (.git) to prevent directory traversal - Initialize trait properties with defaults to avoid uninitialized state - Add PHPDoc class tags (@category, @author, @copyright, @license, @link) - Guard exec() call, simplify extractOperationSection with match expression - Remove duplicate assertion, add strpos guards before substr - Rename legacy POST test with security warning docblock - Fix method name typo, use early return pattern, rename escaping test Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
♻️ Duplicate comments (1)
tests/unit/htdocs/modules/system/SourceFileTestTrait.php (1)
82-83:⚠️ Potential issue | 🟡 MinorHandle
file_get_contents()failure before assigning to typedstringproperty.
file_get_contents()returnsstring|false. Assigningfalsedirectly to thestring-typed$sourceContentproperty will throw aTypeErrorin PHP 8.0+ before your assertion executes.🛠️ Suggested fix
- $this->sourceContent = file_get_contents($this->filePath); + $content = file_get_contents($this->filePath); + $this->assertNotFalse($content, 'Failed to read source file: ' . basename($this->filePath)); + $this->sourceContent = $content; $this->assertNotEmpty($this->sourceContent, 'Source file should not be empty');🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/unit/htdocs/modules/system/SourceFileTestTrait.php` around lines 82 - 83, file_get_contents($this->filePath) can return false and assigning it directly to the string-typed property $sourceContent will cause a TypeError before the assertion runs; change the code to first capture the result into an untyped local (e.g. $content = file_get_contents($this->filePath)), check that $content !== false (and if false call $this->fail or an assertion with a clear message that includes $this->filePath), then assign the validated string to $this->sourceContent and proceed to $this->assertNotEmpty($this->sourceContent, 'Source file should not be empty').
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@tests/unit/htdocs/modules/system/SourceFileTestTrait.php`:
- Around line 82-83: file_get_contents($this->filePath) can return false and
assigning it directly to the string-typed property $sourceContent will cause a
TypeError before the assertion runs; change the code to first capture the result
into an untyped local (e.g. $content = file_get_contents($this->filePath)),
check that $content !== false (and if false call $this->fail or an assertion
with a clear message that includes $this->filePath), then assign the validated
string to $this->sourceContent and proceed to
$this->assertNotEmpty($this->sourceContent, 'Source file should not be empty').
ℹ️ Review info
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (3)
tests/unit/htdocs/modules/system/SourceFileTestTrait.phptests/unit/htdocs/modules/system/admin/modulesadmin/MainControllerTest.phptests/unit/htdocs/modules/system/templates/SystemModulesConfirmTemplateTest.php
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
|
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #1632 +/- ##
==============================
==============================
☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
This was referenced Mar 7, 2026
This was referenced Mar 30, 2026
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
modules/system/admin/modulesadmin/main.php— covers security/access control, operation routing, input sanitization, confirm/submit/list/install/update/uninstall/display operations, and edge casessystem_modules_confirm.tpl— covers template structure, Smarty syntax, conditional logic, hidden fields, form controls, result display, table structure, and regression checksBoth use static code analysis (source file inspection) so they run without database or XOOPS bootstrap.
Based on CodeRabbit-generated tests from PR #1631, adapted for:
dirname()path resolution for the project layout<{...}>Smarty delimiters$old =style)Test plan
tests/unit/htdocs/modules/system/pass together🤖 Generated with Claude Code
Summary by CodeRabbit