Remove eval() from PHP blocks, verify Protector lifecycle guards and legacy cleanup#1653
Merged
mambax7 merged 13 commits intoMar 14, 2026
Merged
Conversation
- Replace legacy eval()-based PHP block execution in kernel/block.php with a permanent no-op and deprecation warning. File-based PHP blocks (filename.php|function_name) continue to work as before. - Replace eval()-based dynamic function definitions in all four protector lifecycle files (oninstall, onuninstall, onupdate, notification) with literal function declarations since $mydirname is always 'protector'. - Update existing XoopsBlockPhpBlockTest to match new behavior. - Add EvalRemovalVerificationTest with source-scanning tests confirming eval() removal across all affected files.
WalkthroughReplaces legacy eval-based PHP block execution with exclusive file-based loading, adds protected static getCustomBlocksDir(), detects and deprecates malformed "filename.php|function" pipe content, and adds tests to verify eval removal and related behaviors. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~22 minutes Possibly related PRs
🚥 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)
📝 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 removes eval() execution paths from XOOPS core custom PHP blocks and from Protector module lifecycle/notification callback generation, and adds unit tests that verify the removals via runtime behavior checks and source scanning.
Changes:
- Disable legacy DB-stored PHP block execution by making
executeLegacyBlock()log once and always return an empty string. - Replace
eval()-generated Protector lifecycle/notification callbacks with literal function definitions. - Add/update PHPUnit tests to verify
eval()removal and that legacy PHP blocks no longer execute.
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/unit/htdocs/kernel/XoopsBlockPhpBlockTest.php | Updates PHP-block tests to match “legacy eval removed” behavior. |
| tests/unit/htdocs/kernel/EvalRemovalVerificationTest.php | Adds source-scanning and behavior tests to verify eval() and other legacy patterns are removed. |
| htdocs/xoops_lib/modules/protector/oninstall.php | Replaces eval() dynamic lifecycle callback with hard-coded callback. |
| htdocs/xoops_lib/modules/protector/onuninstall.php | Replaces eval() dynamic lifecycle callback with hard-coded callback. |
| htdocs/xoops_lib/modules/protector/onupdate.php | Replaces eval() dynamic lifecycle callback with hard-coded callback. |
| htdocs/xoops_lib/modules/protector/notification.php | Replaces eval() dynamic notification callback with hard-coded callback. |
| htdocs/kernel/block.php | Permanently disables legacy eval() PHP blocks and logs a migration warning. |
| */ | ||
| function xoops_module_install_protector($module) | ||
| { | ||
| return protector_oninstall_base($module, 'protector'); |
There was a problem hiding this comment.
The lifecycle callback is now hard-coded to xoops_module_install_protector() and passes 'protector' into protector_oninstall_base(). This breaks D3-style cloned installs where the installed module directory name is not protector (the expected callback name is xoops_module_install_{mydirname} and $mydirname is set from basename(__DIR__) in the wrapper htdocs/modules/protector/oninstall.php). If cloned installs are still supported, this needs a different strategy or an explicit deprecation of clones.
Suggested change
| return protector_oninstall_base($module, 'protector'); | |
| $registry = ProtectorRegistry::getInstance(); | |
| $mydirname = $registry->getEntry('mydirname'); | |
| if (empty($mydirname)) { | |
| $mydirname = 'protector'; | |
| } | |
| return protector_oninstall_base($module, $mydirname); |
Comment on lines
+13
to
+24
| if (!function_exists('xoops_module_uninstall_protector')) { | ||
| /** | ||
| * XOOPS module lifecycle callback for protector uninstall. | ||
| * | ||
| * @param XoopsModule $module | ||
| * | ||
| * @return bool | ||
| */ | ||
| function xoops_module_uninstall_protector($module) | ||
| { | ||
| return protector_onuninstall_base($module, 'protector'); | ||
| } |
There was a problem hiding this comment.
The lifecycle callback is now hard-coded to xoops_module_uninstall_protector() and passes 'protector' into protector_onuninstall_base(). This breaks D3-style cloned installs where the installed module directory name is not protector (XOOPS will look for xoops_module_uninstall_{mydirname}, and $mydirname is set from basename(__DIR__) in the wrapper htdocs/modules/protector/onuninstall.php). If cloned installs are still supported, this needs a different strategy or an explicit deprecation of clones.
| */ | ||
| function protector_notify_iteminfo($category, $item_id) | ||
| { | ||
| return protector_notify_base('protector', $category, $item_id); |
There was a problem hiding this comment.
The notification callback is now hard-coded to protector_notify_iteminfo() and always calls protector_notify_base('protector', ...). This breaks D3-style cloned installs where XOOPS expects {mydirname}_notify_iteminfo() and where $mydirname is set from basename(__DIR__) in the wrapper htdocs/modules/protector/notification.php. If cloned installs are still supported, this needs to remain dirname-aware (or cloned installs should be explicitly deprecated/unsupported).
Suggested change
| return protector_notify_base('protector', $category, $item_id); | |
| // Use the module dirname resolved via ProtectorRegistry to support cloned installs. | |
| // Fallback to the base 'protector' dirname if not set. | |
| global $mydirname; | |
| if (empty($mydirname)) { | |
| $mydirname = 'protector'; | |
| } | |
| return protector_notify_base($mydirname, $category, $item_id); |
Comment on lines
+49
to
+76
| // Remove all comments (single-line and multi-line) and strings | ||
| // to check only actual code for eval() | ||
| $lines = explode("\n", $content); | ||
| $executableEvalFound = false; | ||
| foreach ($lines as $lineNum => $line) { | ||
| $trimmed = trim($line); | ||
| // Skip comment-only lines | ||
| if (str_starts_with($trimmed, '//') || str_starts_with($trimmed, '*') || str_starts_with($trimmed, '/*')) { | ||
| continue; | ||
| } | ||
| // Check for eval( in non-comment parts of the line | ||
| $codePart = $trimmed; | ||
| // Strip inline comments | ||
| $commentPos = strpos($codePart, '//'); | ||
| if ($commentPos !== false) { | ||
| $codePart = substr($codePart, 0, $commentPos); | ||
| } | ||
| // Strip string literals (simple approach: check for eval( outside of quotes) | ||
| if (preg_match('/\beval\s*\(/', $codePart)) { | ||
| // Check it's not inside a string | ||
| $beforeEval = substr($codePart, 0, strpos($codePart, 'eval')); | ||
| $singleQuotes = substr_count($beforeEval, "'") - substr_count($beforeEval, "\\'"); | ||
| $doubleQuotes = substr_count($beforeEval, '"') - substr_count($beforeEval, '\\"'); | ||
| if ($singleQuotes % 2 === 0 && $doubleQuotes % 2 === 0) { | ||
| $executableEvalFound = true; | ||
| break; | ||
| } | ||
| } |
There was a problem hiding this comment.
kernelBlockPhpHasNoExecutableEval() tries to ignore comments/strings by doing manual line parsing, but it doesn't handle inline /* ... */ comments, # comments, heredoc/nowdoc, or complex escaping. This can lead to false positives/negatives and makes the security verification brittle. Consider switching this check to token_get_all() and asserting there are no T_EVAL tokens in the token stream (excluding tokens inside T_COMMENT, T_DOC_COMMENT, and string tokens).
Suggested change
| // Remove all comments (single-line and multi-line) and strings | |
| // to check only actual code for eval() | |
| $lines = explode("\n", $content); | |
| $executableEvalFound = false; | |
| foreach ($lines as $lineNum => $line) { | |
| $trimmed = trim($line); | |
| // Skip comment-only lines | |
| if (str_starts_with($trimmed, '//') || str_starts_with($trimmed, '*') || str_starts_with($trimmed, '/*')) { | |
| continue; | |
| } | |
| // Check for eval( in non-comment parts of the line | |
| $codePart = $trimmed; | |
| // Strip inline comments | |
| $commentPos = strpos($codePart, '//'); | |
| if ($commentPos !== false) { | |
| $codePart = substr($codePart, 0, $commentPos); | |
| } | |
| // Strip string literals (simple approach: check for eval( outside of quotes) | |
| if (preg_match('/\beval\s*\(/', $codePart)) { | |
| // Check it's not inside a string | |
| $beforeEval = substr($codePart, 0, strpos($codePart, 'eval')); | |
| $singleQuotes = substr_count($beforeEval, "'") - substr_count($beforeEval, "\\'"); | |
| $doubleQuotes = substr_count($beforeEval, '"') - substr_count($beforeEval, '\\"'); | |
| if ($singleQuotes % 2 === 0 && $doubleQuotes % 2 === 0) { | |
| $executableEvalFound = true; | |
| break; | |
| } | |
| } | |
| $tokens = token_get_all($content); | |
| $executableEvalFound = false; | |
| foreach ($tokens as $token) { | |
| if (!is_array($token)) { | |
| continue; | |
| } | |
| if ($token[0] === T_EVAL) { | |
| $executableEvalFound = true; | |
| break; | |
| } |
Comment on lines
+545
to
+548
| 'PHP block eval() execution has been permanently removed in XOOPS 2.5.12. ' | ||
| . 'Convert block to file-based format (filename.php|function_name in custom_blocks/), ' | ||
| . 'S (Smarty), or H (HTML) type.' | ||
| ); |
There was a problem hiding this comment.
The warning text suggests converting to S (Smarty) blocks, but in getContent() the c_type === 'S' branch is “text sanitizer (smilies enabled)”, not Smarty. This message is likely to mislead admins migrating legacy PHP blocks; consider updating it to reference the actual supported content types (e.g., H/HTML and S/T sanitized text) or the correct terminology used in the blocks UI.
| */ | ||
| function xoops_module_update_protector($module) | ||
| { | ||
| return protector_onupdate_base($module, 'protector'); |
There was a problem hiding this comment.
The lifecycle callback is now hard-coded to xoops_module_update_protector() and passes 'protector' into protector_onupdate_base(). This breaks D3-style cloned installs where the module directory name (and expected callback name/table prefix) is not exactly protector (see htdocs/modules/protector/onupdate.php which sets $mydirname = basename(__DIR__) before including this file). If clone support is still required, this needs an alternative approach (e.g., keep dynamic $mydirname support via per-clone wrapper callbacks, or otherwise explicitly deprecate/disable cloned Protector installs and update docs/tests accordingly).
Suggested change
| return protector_onupdate_base($module, 'protector'); | |
| // Prefer the globally set mydirname (from ProtectorRegistry or wrapper), | |
| // fall back to the module's dirname, and finally to the canonical name. | |
| global $mydirname; | |
| if (!is_string($mydirname) || $mydirname === '') { | |
| if ($module instanceof XoopsModule) { | |
| $mydirname = (string)$module->getVar('dirname'); | |
| } else { | |
| $mydirname = 'protector'; | |
| } | |
| } | |
| return protector_onupdate_base($module, $mydirname); |
…okenizer
- Use ProtectorRegistry mydirname in lifecycle callbacks instead of
hard-coded 'protector', preserving table prefix behavior while
documenting that D3-style cloned installs are no longer supported.
- Fix block.php warning text: S is sanitized text, not Smarty.
- Replace brittle manual eval-scan parsing with token_get_all()/T_EVAL
in EvalRemovalVerificationTest for reliable detection.
- Add assertions verifying D3 deprecation comments and registry usage.
Comment on lines
+545
to
+548
| 'PHP block eval() execution has been permanently removed in XOOPS 2.5.12. ' | ||
| . 'Convert block to file-based format (filename.php|function_name in custom_blocks/), ' | ||
| . 'H (HTML), or S/T (sanitized text) type.' | ||
| ); |
Comment on lines
+206
to
+215
| * Verify that protector/oninstall.php has no eval() and defines the callback literally. | ||
| */ | ||
| #[Test] | ||
| public function protectorOninstallHasNoEval(): void | ||
| { | ||
| $file = XOOPS_ROOT_PATH . '/../xoops_lib/modules/protector/oninstall.php'; | ||
| // Normalize for different path structures | ||
| if (!file_exists($file)) { | ||
| $file = XOOPS_PATH . '/modules/protector/oninstall.php'; | ||
| } |
Comment on lines
+227
to
+236
| $this->assertStringContainsString( | ||
| 'D3-style cloned installs', | ||
| $content, | ||
| 'protector/oninstall.php should document D3 clone deprecation' | ||
| ); | ||
| $this->assertStringContainsString( | ||
| "getEntry('mydirname')", | ||
| $content, | ||
| 'protector/oninstall.php should use registry mydirname, not hard-coded string' | ||
| ); |
Comment on lines
+260
to
+264
| $this->assertStringContainsString( | ||
| 'D3-style cloned installs', | ||
| $content, | ||
| 'protector/onuninstall.php should document D3 clone deprecation' | ||
| ); |
Comment on lines
+288
to
+292
| $this->assertStringContainsString( | ||
| 'D3-style cloned installs', | ||
| $content, | ||
| 'protector/onupdate.php should document D3 clone deprecation' | ||
| ); |
Comment on lines
+316
to
+320
| $this->assertStringContainsString( | ||
| 'D3-style cloned installs', | ||
| $content, | ||
| 'protector/notification.php should document D3 clone deprecation' | ||
| ); |
Comment on lines
746
to
758
| /** | ||
| * Verify that the pipe guard only applies when legacy mode is disabled. | ||
| * | ||
| * Content with "|" gets a pipe-specific extra warning. | ||
| * Content without "|" does NOT get a pipe warning (different branch). | ||
| * This proves the two code paths are distinct. | ||
| * Verify that legacy eval content (non file-based) always returns empty | ||
| * and logs a removal warning regardless of content pattern. | ||
| */ | ||
| #[Test] | ||
| public function pipeGuardAndLegacyPathAreSeparateBranches(): void | ||
| public function legacyEvalContentAlwaysReturnsEmpty(): void | ||
| { | ||
| $logger = \XoopsLogger::getInstance(); | ||
|
|
||
| // Content WITH pipe — should get pipe-specific extra warning | ||
| $extraBefore = count($logger->extra); | ||
| $block = $this->createPhpBlock('file.php|func-name'); | ||
| $block->getContent('S', 'P'); | ||
|
|
||
| $newExtras = array_slice($logger->extra, $extraBefore); | ||
| $pipeMessages = array_column($newExtras, 'msg'); | ||
| $this->assertNotEmpty(array_filter($pipeMessages, fn($m) => str_contains($m, 'contains ".php|"')), | ||
| 'Pipe content should produce pipe-specific warning'); | ||
|
|
||
| // Content WITHOUT pipe — should NOT produce a pipe-specific warning | ||
| $extraBefore2 = count($logger->extra); | ||
| $block2 = $this->createPhpBlock('echo "hello";'); | ||
| $result2 = $block2->getContent('S', 'P'); | ||
|
|
||
| $this->assertSame('', $result2); | ||
| // Content WITHOUT pipe — should still be blocked | ||
| $block = $this->createPhpBlock('echo "hello";'); | ||
| $result = $block->getContent('S', 'P'); | ||
|
|
||
| $newExtras2 = array_slice($logger->extra, $extraBefore2); | ||
| $legacyPipeWarnings = array_filter(array_column($newExtras2, 'msg'), fn($m) => str_contains($m, 'contains ".php|"')); | ||
| $this->assertEmpty($legacyPipeWarnings, 'Non-pipe content should not trigger pipe warning'); | ||
| $this->assertSame('', $result); | ||
| } |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (4)
htdocs/xoops_lib/modules/protector/onupdate.php (1)
1-1:⚠️ Potential issue | 🟡 MinorMissing XOOPS copyright header block.
Per coding guidelines, every PHP source file must begin with the XOOPS copyright header block. This file jumps straight into code.
Suggested fix
<?php +/** + * XOOPS Protector Module - Update Lifecycle Handler + * + * 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) + * `@package` protector + * `@since` 2.5.12 + */ // start hack by TrabisAs per coding guidelines: "Every source file must begin with the XOOPS copyright header block".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@htdocs/xoops_lib/modules/protector/onupdate.php` at line 1, Add the required XOOPS copyright header block at the very top of the file (before the opening <?php) to comply with coding guidelines; update the file htdocs/xoops_lib/modules/protector/onupdate.php by inserting the standard XOOPS header comment block (including license and author lines) so the source begins with the mandated header rather than starting directly with code.htdocs/xoops_lib/modules/protector/notification.php (1)
1-1:⚠️ Potential issue | 🟡 MinorMissing XOOPS copyright header block.
Same issue as onupdate.php — this file needs the standard copyright header.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@htdocs/xoops_lib/modules/protector/notification.php` at line 1, Add the standard XOOPS copyright/header block to the top of notification.php (same header used in onupdate.php) so the file begins with the canonical multi-line XOOPS header comment instead of only "<?php"; ensure the header includes the project name, copyright years, license notice and maintainer/author lines consistent with other module files.htdocs/xoops_lib/modules/protector/oninstall.php (1)
1-1:⚠️ Potential issue | 🟡 MinorMissing XOOPS copyright header block.
Consistent with the other Protector lifecycle files — needs the standard header.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@htdocs/xoops_lib/modules/protector/oninstall.php` at line 1, Add the standard XOOPS copyright header block to the top of the Protector lifecycle file oninstall.php (before the opening <?php) so it matches other Protector lifecycle files; locate an existing lifecycle file in the Protector module (e.g., any other oninstall/onupdate/onuninstall file) and copy its header block verbatim, updating module-specific fields if needed to maintain consistent formatting and metadata.htdocs/xoops_lib/modules/protector/onuninstall.php (1)
1-1:⚠️ Potential issue | 🟡 MinorMissing XOOPS copyright header block.
Same pattern as the other Protector files.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@htdocs/xoops_lib/modules/protector/onuninstall.php` at line 1, Add the standard XOOPS module file header to the top of onuninstall.php (the same copyright/license/author block used in other Protector files) so the file matches project conventions; copy the header block from another Protector module file and place it immediately above the opening <?php tag in onuninstall.php, ensuring it includes the XOOPS copyright line, license reference, author/maintainer info, and any package/module annotations.
🤖 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/kernel/block.php`:
- Around line 529-553: The executeLegacyBlock(string $raw): string method
currently has an intentionally unused parameter $raw which triggers static
analysis; update the method to explicitly acknowledge the unused parameter to
silence tools by either adding a suppression annotation (e.g.,
`@SuppressWarnings`(PHPMD.UnusedFormalParameter) in the docblock) or by adding an
explicit no-op reference to the parameter inside the function (e.g., cast to
void) and update the `@param` docblock to state it is retained for interface
compatibility. Locate executeLegacyBlock and apply one of these fixes so the
intent is documented and static analyzers no longer flag $raw as unused.
---
Outside diff comments:
In `@htdocs/xoops_lib/modules/protector/notification.php`:
- Line 1: Add the standard XOOPS copyright/header block to the top of
notification.php (same header used in onupdate.php) so the file begins with the
canonical multi-line XOOPS header comment instead of only "<?php"; ensure the
header includes the project name, copyright years, license notice and
maintainer/author lines consistent with other module files.
In `@htdocs/xoops_lib/modules/protector/oninstall.php`:
- Line 1: Add the standard XOOPS copyright header block to the top of the
Protector lifecycle file oninstall.php (before the opening <?php) so it matches
other Protector lifecycle files; locate an existing lifecycle file in the
Protector module (e.g., any other oninstall/onupdate/onuninstall file) and copy
its header block verbatim, updating module-specific fields if needed to maintain
consistent formatting and metadata.
In `@htdocs/xoops_lib/modules/protector/onuninstall.php`:
- Line 1: Add the standard XOOPS module file header to the top of
onuninstall.php (the same copyright/license/author block used in other Protector
files) so the file matches project conventions; copy the header block from
another Protector module file and place it immediately above the opening <?php
tag in onuninstall.php, ensuring it includes the XOOPS copyright line, license
reference, author/maintainer info, and any package/module annotations.
In `@htdocs/xoops_lib/modules/protector/onupdate.php`:
- Line 1: Add the required XOOPS copyright header block at the very top of the
file (before the opening <?php) to comply with coding guidelines; update the
file htdocs/xoops_lib/modules/protector/onupdate.php by inserting the standard
XOOPS header comment block (including license and author lines) so the source
begins with the mandated header rather than starting directly with code.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 83d66376-82e0-4a33-858d-92e5fd02c55e
📒 Files selected for processing (7)
htdocs/kernel/block.phphtdocs/xoops_lib/modules/protector/notification.phphtdocs/xoops_lib/modules/protector/oninstall.phphtdocs/xoops_lib/modules/protector/onuninstall.phphtdocs/xoops_lib/modules/protector/onupdate.phptests/unit/htdocs/kernel/EvalRemovalVerificationTest.phptests/unit/htdocs/kernel/XoopsBlockPhpBlockTest.php
Protector lifecycle files (oninstall, onuninstall, onupdate, notification) retain eval() with var_export() for safe dynamic function naming required by D3-style cloned installs. Block docblock corrected: c_type S is sanitized text, not Smarty. Tests rewritten to verify var_export safety and base function existence instead of asserting eval absence in protector files. XoopsBlockPhpBlockTest docblock fixed to match actual assertions.
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 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/kernel/block.php`:
- Around line 542-549: The static $warningLogged flag inside block.php makes the
deprecation message process-global; change it to a request-scoped flag by
removing the static variable and instead using an instance/request-level marker
(e.g. a property like $this->warningLogged or a per-request registry key) so the
check before calling $this->logBlockWarning(...) only suppresses duplicates for
the current request rather than across the whole PHP process; update the check
and assignment for $warningLogged and ensure any new storage is initialized
per-request (or cleared between requests) to preserve one-per-request logging.
In `@tests/unit/htdocs/kernel/EvalRemovalVerificationTest.php`:
- Around line 91-107: Remove the redundant logger initialization inside
legacyPhpBlockReturnsEmptyContent(): the logger is already set up in
setUpBeforeClass(), so delete the conditional block that checks
isset($GLOBALS['xoopsLogger']) and assigns \XoopsLogger::getInstance(); leave
the rest of the test (creating XoopsBlock, setting vars, calling getContent and
asserting) unchanged to avoid duplicate initialization.
- Around line 208-216: The resolveProtectorFile method in
EvalRemovalVerificationTest relies on XOOPS_ROOT_PATH/XOOPS_PATH and will fail
in isolated unit environments; update the test to either (A) treat it as an
integration test by moving/annotating it accordingly, or (B) make
resolveProtectorFile return gracefully when neither path exists and have the
surrounding test use $this->markTestSkipped('Protector files not available') if
the resolved file is missing; locate the resolveProtectorFile method and the
tests that call it and implement the skip-or-mark-as-integration change so unit
runs don't require a real XOOPS installation.
- Around line 1-19: The class-level docblock for EvalRemovalVerificationTest is
missing required coding-guideline tags; update the docblock above the
EvalRemovalVerificationTest class to include `@category`, `@package`, `@author`,
`@copyright`, `@license`, and `@link` tags (keeping the existing description),
ensuring the namespace/kernel context and the CoversClass(XoopsBlock::class)
attribute remain unchanged.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 97bc04bf-ec21-4384-8139-9d596e410fe5
📒 Files selected for processing (7)
htdocs/kernel/block.phphtdocs/xoops_lib/modules/protector/notification.phphtdocs/xoops_lib/modules/protector/oninstall.phphtdocs/xoops_lib/modules/protector/onuninstall.phphtdocs/xoops_lib/modules/protector/onupdate.phptests/unit/htdocs/kernel/EvalRemovalVerificationTest.phptests/unit/htdocs/kernel/XoopsBlockPhpBlockTest.php
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 7 out of 7 changed files in this pull request and generated 2 comments.
Comments suppressed due to low confidence (4)
htdocs/xoops_lib/modules/protector/oninstall.php:18
- This file still defines the lifecycle callback via
eval()(dynamic function name), but the PR title/description says Protector lifecycle eval() was removed/replaced with literal function definitions. Either remove the eval here (e.g., define the fixedxoops_module_install_protectorcallback if clones are no longer supported), or update the PR description/tests to explicitly document that eval remains required for D3-style cloned installs.
// Dynamic function name is required for D3-style cloned installs.
// $mydirname comes from basename(__DIR__) via ProtectorRegistry, not user input.
if (!function_exists('xoops_module_install_' . $mydirname)) {
eval('function xoops_module_install_' . $mydirname . '($module) { return protector_oninstall_base($module, ' . var_export($mydirname, true) . '); }');
}
htdocs/xoops_lib/modules/protector/onupdate.php:18
- This file still uses
eval()to define the update callback function dynamically. That contradicts the PR summary claiming Protector lifecycle eval() was replaced with literal function definitions. Either remove the eval (if cloned installs are not supported) or update the PR description/tests to reflect that eval remains for D3-style clones.
// Dynamic function name is required for D3-style cloned installs.
// $mydirname comes from basename(__DIR__) via ProtectorRegistry, not user input.
if (!function_exists('xoops_module_update_' . $mydirname)) {
eval('function xoops_module_update_' . $mydirname . '($module) { return protector_onupdate_base($module, ' . var_export($mydirname, true) . '); }');
}
htdocs/xoops_lib/modules/protector/onuninstall.php:17
- This file still uses
eval()to define the uninstall callback function dynamically. That conflicts with the PR summary claiming Protector lifecycle eval() was removed. Please either switch to a literal callback (if$mydirnameis alwaysprotector) or update the PR description/tests to state that eval remains required for D3-style clones.
// Dynamic function name is required for D3-style cloned installs.
// $mydirname comes from basename(__DIR__) via ProtectorRegistry, not user input.
if (!function_exists('xoops_module_uninstall_' . $mydirname)) {
eval('function xoops_module_uninstall_' . $mydirname . '($module) { return protector_onuninstall_base($module, ' . var_export($mydirname, true) . '); }');
}
htdocs/xoops_lib/modules/protector/notification.php:17
- This file still defines the notify callback via
eval()with a dynamic function name. The PR title/description indicates eval() was removed from Protector lifecycle files; please reconcile by either removing eval (fixed function name) or documenting/justifying why dynamic eval remains necessary for cloned installs.
// Dynamic function name is required for D3-style cloned installs.
// $mydirname comes from basename(__DIR__) via ProtectorRegistry, not user input.
if (!function_exists($mydirname . '_notify_iteminfo')) {
eval('function ' . $mydirname . '_notify_iteminfo($category, $item_id) { return protector_notify_base(' . var_export($mydirname, true) . ', $category, $item_id); }');
}
Comment on lines
+201
to
+203
| // ========================================================================= | ||
| // M-11c: Protector lifecycle — eval() retained for D3 clone support | ||
| // ========================================================================= |
Comment on lines
+542
to
+549
| static $warningLogged = false; | ||
| if (!$warningLogged) { | ||
| $this->logBlockWarning( | ||
| 'PHP block eval() execution has been permanently removed in XOOPS 2.5.12. ' | ||
| . 'Convert block to file-based format (filename.php|function_name in custom_blocks/), ' | ||
| . 'H (HTML), or S/T (sanitized text) type.' | ||
| ); | ||
| $warningLogged = true; |
- Add @SuppressWarnings for intentionally unused $raw parameter in executeLegacyBlock() (Scrutinizer) - Remove redundant logger initialization in test — already handled by setUpBeforeClass() (CodeRabbit #18) - Skip protector lifecycle tests gracefully when trust-path files are not available in the test environment (CodeRabbit #19) - Add targeted warning for near-miss file-based block content that contains .php| but fails the strict regex (Copilot #20)
Comment on lines
+387
to
+395
| // Detect near-miss: content looks like file-based format but has invalid syntax | ||
| if (strpos($raw, '.php|') !== false) { | ||
| $this->logBlockWarning( | ||
| 'Block content looks like file-based format but has invalid syntax. ' | ||
| . 'Expected: filename.php|function_name (alphanumeric and underscores only).' | ||
| ); | ||
|
|
||
| return ''; | ||
| } |
| if (strpos($raw, '.php|') !== false) { | ||
| $this->logBlockWarning( | ||
| 'Block content looks like file-based format but has invalid syntax. ' | ||
| . 'Expected: filename.php|function_name (alphanumeric and underscores only).' |
Comment on lines
+196
to
+199
| // ========================================================================= | ||
| // M-11c: Protector lifecycle — eval() retained for D3 clone support | ||
| // ========================================================================= | ||
|
|
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
tests/unit/htdocs/kernel/EvalRemovalVerificationTest.php (1)
12-19:⚠️ Potential issue | 🟡 MinorAdd required class-docblock metadata tags.
This class docblock is missing required tags:
@category,@package,@author,@copyright,@license,@link.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/kernel/EvalRemovalVerificationTest.php` around lines 12 - 19, The class docblock for EvalRemovalVerificationTest is missing required metadata tags; update the block above the EvalRemovalVerificationTest class declaration to include `@category`, `@package`, `@author`, `@copyright`, `@license`, and `@link` tags with appropriate values (e.g., project/category and contact/license URLs), ensuring the docblock remains directly above the class EvalRemovalVerificationTest (which covers XoopsBlock::class) so static analysis and coding-guideline checks pass.
🤖 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/kernel/block.php`:
- Around line 554-561: The branch currently only calls
$this->logBlockWarning(...) and doesn't emit a user-level warning; change it to
also (or instead) call trigger_error(...) with E_USER_WARNING so a PHP user
warning is raised for this non-fatal deprecation, build the message using
basename(...) when including any file paths to avoid exposing server paths, and
keep the $warningLogged flag behavior intact (i.e., still set $warningLogged =
true after emitting the warning); locate this in the same block that references
$warningLogged and logBlockWarning to update the behavior.
In `@tests/unit/htdocs/kernel/EvalRemovalVerificationTest.php`:
- Around line 58-278: Rename the PHPUnit test methods so they begin with "test"
to be discoverable by PHPUnit 9.6: change kernelBlockPhpHasNoExecutableEval →
testKernelBlockPhpHasNoExecutableEval, systemBlockPhpHasNoEval →
testSystemBlockPhpHasNoEval, legacyPhpBlockReturnsEmptyContent →
testLegacyPhpBlockReturnsEmptyContent,
xoopsVersionPhpHasNoHtmlTemplateRegistrations →
testXoopsVersionPhpHasNoHtmlTemplateRegistrations, snoopyPhpHasNoShellExecution
→ testSnoopyPhpHasNoShellExecution, zipfilePhpHasNoEval →
testZipfilePhpHasNoEval, artFunctionsIniPhpHasNoEval →
testArtFunctionsIniPhpHasNoEval, protectorOninstallUsesVarExportAndDefinesBase →
testProtectorOninstallUsesVarExportAndDefinesBase,
protectorOnuninstallUsesVarExportAndDefinesBase →
testProtectorOnuninstallUsesVarExportAndDefinesBase,
protectorOnupdateUsesVarExportAndDefinesBase →
testProtectorOnupdateUsesVarExportAndDefinesBase, and
protectorNotificationUsesVarExportAndDefinesBase →
testProtectorNotificationUsesVarExportAndDefinesBase; update each method
declaration (e.g., function kernelBlockPhpHasNoExecutableEval() to function
testKernelBlockPhpHasNoExecutableEval()) so PHPUnit 9.6 discovers them while
leaving the existing assertions and attributes intact.
---
Duplicate comments:
In `@tests/unit/htdocs/kernel/EvalRemovalVerificationTest.php`:
- Around line 12-19: The class docblock for EvalRemovalVerificationTest is
missing required metadata tags; update the block above the
EvalRemovalVerificationTest class declaration to include `@category`, `@package`,
`@author`, `@copyright`, `@license`, and `@link` tags with appropriate values (e.g.,
project/category and contact/license URLs), ensuring the docblock remains
directly above the class EvalRemovalVerificationTest (which covers
XoopsBlock::class) so static analysis and coding-guideline checks pass.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 1a612f8c-050a-46c0-8466-a98dc1757ef3
📒 Files selected for processing (2)
htdocs/kernel/block.phptests/unit/htdocs/kernel/EvalRemovalVerificationTest.php
Comment on lines
+58
to
+278
| #[Test] | ||
| public function kernelBlockPhpHasNoExecutableEval(): void | ||
| { | ||
| $file = XOOPS_ROOT_PATH . '/kernel/block.php'; | ||
| $this->assertFileExists($file); | ||
|
|
||
| $content = file_get_contents($file); | ||
| $this->assertNotFalse($content); | ||
|
|
||
| // Use PHP's tokenizer to reliably detect eval() in executable code. | ||
| // T_EVAL tokens only appear for actual eval() calls, not in comments or strings. | ||
| $this->assertNoEvalTokens($content, 'kernel/block.php should not contain any executable eval() calls'); | ||
| } | ||
|
|
||
| /** | ||
| * Verify that system/class/block.php contains no eval() calls. | ||
| */ | ||
| #[Test] | ||
| public function systemBlockPhpHasNoEval(): void | ||
| { | ||
| $file = XOOPS_ROOT_PATH . '/modules/system/class/block.php'; | ||
| $this->assertFileExists($file); | ||
|
|
||
| $content = file_get_contents($file); | ||
| $this->assertNotFalse($content); | ||
|
|
||
| $this->assertNoEvalTokens($content, 'system/class/block.php should not contain any eval() calls'); | ||
| } | ||
|
|
||
| /** | ||
| * Verify that the legacy block path returns empty content for raw PHP code. | ||
| */ | ||
| #[Test] | ||
| public function legacyPhpBlockReturnsEmptyContent(): void | ||
| { | ||
| require_once XOOPS_ROOT_PATH . '/kernel/block.php'; | ||
|
|
||
| $block = new XoopsBlock(); | ||
| $block->setVar('block_type', 'C'); | ||
| $block->setVar('c_type', 'P'); | ||
| $block->setVar('content', 'echo "This should never execute";'); | ||
|
|
||
| $result = $block->getContent('S', 'P'); | ||
| $this->assertSame('', $result, 'Legacy PHP block content must return empty string'); | ||
| } | ||
|
|
||
| // ========================================================================= | ||
| // M-9: .html template registrations | ||
| // ========================================================================= | ||
|
|
||
| /** | ||
| * Verify that xoops_version.php does not register any .html templates. | ||
| */ | ||
| #[Test] | ||
| public function xoopsVersionPhpHasNoHtmlTemplateRegistrations(): void | ||
| { | ||
| $file = XOOPS_ROOT_PATH . '/modules/system/xoops_version.php'; | ||
| $this->assertFileExists($file); | ||
|
|
||
| $content = file_get_contents($file); | ||
| $this->assertNotFalse($content); | ||
|
|
||
| $this->assertDoesNotMatchRegularExpression( | ||
| '/[\'"]file[\'"]\\s*=>\\s*[\'"][^\'"]*\\.html[\'"]/', | ||
| $content, | ||
| 'xoops_version.php should not register any .html templates' | ||
| ); | ||
| } | ||
|
|
||
| // ========================================================================= | ||
| // M-10: Snoopy shell exec() — already replaced with PHP cURL | ||
| // ========================================================================= | ||
|
|
||
| /** | ||
| * Verify that snoopy.php does not use shell exec(), system(), or passthru(). | ||
| */ | ||
| #[Test] | ||
| public function snoopyPhpHasNoShellExecution(): void | ||
| { | ||
| $file = XOOPS_ROOT_PATH . '/class/snoopy.php'; | ||
| $this->assertFileExists($file); | ||
|
|
||
| $content = file_get_contents($file); | ||
| $this->assertNotFalse($content); | ||
|
|
||
| // Check for shell execution functions (not curl_exec which is PHP cURL) | ||
| $this->assertDoesNotMatchRegularExpression( | ||
| '/\b(shell_exec|system|passthru)\s*\(/', | ||
| $content, | ||
| 'snoopy.php should not use shell execution functions' | ||
| ); | ||
|
|
||
| // Verify it uses PHP cURL extension | ||
| $this->assertStringContainsString( | ||
| 'curl_init', | ||
| $content, | ||
| 'snoopy.php should use PHP cURL extension' | ||
| ); | ||
| } | ||
|
|
||
| // ========================================================================= | ||
| // M-11a: class.zipfile.php eval() removal | ||
| // ========================================================================= | ||
|
|
||
| /** | ||
| * Verify that class.zipfile.php contains no eval() calls. | ||
| */ | ||
| #[Test] | ||
| public function zipfilePhpHasNoEval(): void | ||
| { | ||
| $file = XOOPS_ROOT_PATH . '/class/class.zipfile.php'; | ||
| $this->assertFileExists($file); | ||
|
|
||
| $content = file_get_contents($file); | ||
| $this->assertNotFalse($content); | ||
|
|
||
| $this->assertNoEvalTokens($content, 'class.zipfile.php should not contain eval()'); | ||
| } | ||
|
|
||
| // ========================================================================= | ||
| // M-11b: art/functions.ini.php eval() removal | ||
| // ========================================================================= | ||
|
|
||
| /** | ||
| * Verify that art/functions.ini.php contains no eval() calls. | ||
| */ | ||
| #[Test] | ||
| public function artFunctionsIniPhpHasNoEval(): void | ||
| { | ||
| $file = XOOPS_ROOT_PATH . '/Frameworks/art/functions.ini.php'; | ||
| $this->assertFileExists($file); | ||
|
|
||
| $content = file_get_contents($file); | ||
| $this->assertNotFalse($content); | ||
|
|
||
| $this->assertNoEvalTokens($content, 'art/functions.ini.php should not contain eval()'); | ||
| } | ||
|
|
||
| // ========================================================================= | ||
| // M-11c: Protector lifecycle — eval() retained for D3 clone support | ||
| // ========================================================================= | ||
|
|
||
| /** | ||
| * Resolve a protector trust-path file across different path structures. | ||
| */ | ||
| private function resolveProtectorFile(string $basename): string | ||
| { | ||
| $file = XOOPS_ROOT_PATH . '/../xoops_lib/modules/protector/' . $basename; | ||
| if (!file_exists($file)) { | ||
| $file = XOOPS_PATH . '/modules/protector/' . $basename; | ||
| } | ||
| if (!file_exists($file)) { | ||
| $this->markTestSkipped("Protector trust-path file not found: {$basename}"); | ||
| } | ||
|
|
||
| return $file; | ||
| } | ||
|
|
||
| /** | ||
| * Verify protector lifecycle files use var_export() for safe eval | ||
| * and define the expected base functions. | ||
| */ | ||
| #[Test] | ||
| public function protectorOninstallUsesVarExportAndDefinesBase(): void | ||
| { | ||
| $file = $this->resolveProtectorFile('oninstall.php'); | ||
| $this->assertFileExists($file); | ||
|
|
||
| $content = file_get_contents($file); | ||
| $this->assertNotFalse($content); | ||
|
|
||
| $this->assertStringContainsString('var_export($mydirname', $content, | ||
| 'protector/oninstall.php should use var_export() for safe dirname embedding'); | ||
| $this->assertStringContainsString('protector_oninstall_base', $content, | ||
| 'protector/oninstall.php should define the base install function'); | ||
| } | ||
|
|
||
| #[Test] | ||
| public function protectorOnuninstallUsesVarExportAndDefinesBase(): void | ||
| { | ||
| $file = $this->resolveProtectorFile('onuninstall.php'); | ||
| $this->assertFileExists($file); | ||
|
|
||
| $content = file_get_contents($file); | ||
| $this->assertNotFalse($content); | ||
|
|
||
| $this->assertStringContainsString('var_export($mydirname', $content, | ||
| 'protector/onuninstall.php should use var_export() for safe dirname embedding'); | ||
| $this->assertStringContainsString('protector_onuninstall_base', $content, | ||
| 'protector/onuninstall.php should define the base uninstall function'); | ||
| } | ||
|
|
||
| #[Test] | ||
| public function protectorOnupdateUsesVarExportAndDefinesBase(): void | ||
| { | ||
| $file = $this->resolveProtectorFile('onupdate.php'); | ||
| $this->assertFileExists($file); | ||
|
|
||
| $content = file_get_contents($file); | ||
| $this->assertNotFalse($content); | ||
|
|
||
| $this->assertStringContainsString('var_export($mydirname', $content, | ||
| 'protector/onupdate.php should use var_export() for safe dirname embedding'); | ||
| $this->assertStringContainsString('protector_onupdate_base', $content, | ||
| 'protector/onupdate.php should define the base update function'); | ||
| } | ||
|
|
||
| #[Test] | ||
| public function protectorNotificationUsesVarExportAndDefinesBase(): void | ||
| { | ||
| $file = $this->resolveProtectorFile('notification.php'); | ||
| $this->assertFileExists($file); | ||
|
|
||
| $content = file_get_contents($file); | ||
| $this->assertNotFalse($content); | ||
|
|
||
| $this->assertStringContainsString('var_export($mydirname', $content, | ||
| 'protector/notification.php should use var_export() for safe dirname embedding'); | ||
| $this->assertStringContainsString('protector_notify_base', $content, | ||
| 'protector/notification.php should define the base notify function'); | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "PHPUnit version constraints found in composer files:"
rg -n '"phpunit/phpunit"\s*:' composer.json composer.*.json 2>/dev/null || true
echo
echo "Public methods in EvalRemovalVerificationTest not prefixed with test:"
rg -nP '^\s*public function (?!test)[A-Za-z_]\w*\s*\(' tests/unit/htdocs/kernel/EvalRemovalVerificationTest.php
echo
echo "Attribute-based test markers in file:"
rg -n '#\[Test\]' tests/unit/htdocs/kernel/EvalRemovalVerificationTest.phpRepository: XOOPS/XoopsCore25
Length of output: 1148
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== Full composer.json (first 100 lines) ==="
head -100 composer.json
echo
echo "=== Search for phpunit constraint anywhere in composer.json ==="
cat composer.json | grep -i phpunit || echo "No phpunit found in output above"
echo
echo "=== Test class declaration and imports (first 60 lines) ==="
head -60 tests/unit/htdocs/kernel/EvalRemovalVerificationTest.phpRepository: XOOPS/XoopsCore25
Length of output: 195
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== Locating composer.json files ==="
fd -t f 'composer\.json' . 2>/dev/null | head -20 || echo "No composer.json found"
echo
echo "=== Test class declaration and imports (first 70 lines) ==="
head -70 tests/unit/htdocs/kernel/EvalRemovalVerificationTest.phpRepository: XOOPS/XoopsCore25
Length of output: 3609
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== Repository structure (root level files and dirs) ==="
ls -la | head -30
echo
echo "=== Search for any composer file in repo root ==="
find . -maxdepth 1 -name 'composer*' -type f 2>/dev/null || echo "No composer.* files at root"
echo
echo "=== Check for version or constraint files ==="
ls -la | grep -i -E '(version|require|phpunit|\.git)' || trueRepository: XOOPS/XoopsCore25
Length of output: 1901
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== .github/copilot-instructions.md (relevant sections) ==="
cat .github/copilot-instructions.md | grep -A 5 -B 5 "PHPUnit\|test.*method\|9.6\|10.5\|11" || echo "Pattern not found"
echo
echo "=== Full copilot-instructions.md ==="
cat .github/copilot-instructions.mdRepository: XOOPS/XoopsCore25
Length of output: 6045
Rename test methods to test... prefix for PHPUnit 9.6 compatibility.
All 11 test methods in lines 59–266 use #[Test] attributes with non-test* names. The #[Test] attribute was introduced in PHPUnit 10.0 and is unavailable in PHPUnit 9.6. PHPUnit 9.6 only discovers test methods by the test* naming convention. This violates the repository requirement to support PHPUnit 9.6, 10.5, and 11.x.
Rename each method: kernelBlockPhpHasNoExecutableEval → testKernelBlockPhpHasNoExecutableEval, systemBlockPhpHasNoEval → testSystemBlockPhpHasNoEval, legacyPhpBlockReturnsEmptyContent → testLegacyPhpBlockReturnsEmptyContent, xoopsVersionPhpHasNoHtmlTemplateRegistrations → testXoopsVersionPhpHasNoHtmlTemplateRegistrations, snoopyPhpHasNoShellExecution → testSnoopyPhpHasNoShellExecution, zipfilePhpHasNoEval → testZipfilePhpHasNoEval, artFunctionsIniPhpHasNoEval → testArtFunctionsIniPhpHasNoEval, protectorOninstallUsesVarExportAndDefinesBase → testProtectorOninstallUsesVarExportAndDefinesBase, protectorOnuninstallUsesVarExportAndDefinesBase → testProtectorOnuninstallUsesVarExportAndDefinesBase, protectorOnupdateUsesVarExportAndDefinesBase → testProtectorOnupdateUsesVarExportAndDefinesBase, and protectorNotificationUsesVarExportAndDefinesBase → testProtectorNotificationUsesVarExportAndDefinesBase.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tests/unit/htdocs/kernel/EvalRemovalVerificationTest.php` around lines 58 -
278, Rename the PHPUnit test methods so they begin with "test" to be
discoverable by PHPUnit 9.6: change kernelBlockPhpHasNoExecutableEval →
testKernelBlockPhpHasNoExecutableEval, systemBlockPhpHasNoEval →
testSystemBlockPhpHasNoEval, legacyPhpBlockReturnsEmptyContent →
testLegacyPhpBlockReturnsEmptyContent,
xoopsVersionPhpHasNoHtmlTemplateRegistrations →
testXoopsVersionPhpHasNoHtmlTemplateRegistrations, snoopyPhpHasNoShellExecution
→ testSnoopyPhpHasNoShellExecution, zipfilePhpHasNoEval →
testZipfilePhpHasNoEval, artFunctionsIniPhpHasNoEval →
testArtFunctionsIniPhpHasNoEval, protectorOninstallUsesVarExportAndDefinesBase →
testProtectorOninstallUsesVarExportAndDefinesBase,
protectorOnuninstallUsesVarExportAndDefinesBase →
testProtectorOnuninstallUsesVarExportAndDefinesBase,
protectorOnupdateUsesVarExportAndDefinesBase →
testProtectorOnupdateUsesVarExportAndDefinesBase, and
protectorNotificationUsesVarExportAndDefinesBase →
testProtectorNotificationUsesVarExportAndDefinesBase; update each method
declaration (e.g., function kernelBlockPhpHasNoExecutableEval() to function
testKernelBlockPhpHasNoExecutableEval()) so PHPUnit 9.6 discovers them while
leaving the existing assertions and attributes intact.
…ck tags
- Use stripos on trimmed content for case-insensitive near-miss
detection of malformed file-based block content (Copilot)
- Fix warning text: regex accepts hyphens, not just alphanumeric
and underscores (Copilot)
- Add trigger_error(E_USER_WARNING) alongside logger for both
legacy eval and near-miss paths (CodeRabbit)
- Add standard XOOPS class-level docblock tags to
EvalRemovalVerificationTest (CodeRabbit)
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (1)
tests/unit/htdocs/kernel/EvalRemovalVerificationTest.php (1)
65-285:⚠️ Potential issue | 🟠 MajorRename attribute-only test methods to
test...for PHPUnit 9.6 compatibility.Methods at Line 66 onward are public tests without the
testprefix. This risks non-discovery on PHPUnit 9.6 even though#[Test]exists.#!/bin/bash set -euo pipefail echo "Public methods not prefixed with test* in EvalRemovalVerificationTest:" rg -nP '^\s*public function (?!test)[A-Za-z_]\w*\s*\(' tests/unit/htdocs/kernel/EvalRemovalVerificationTest.php -C1 echo echo "Attribute-based tests in file:" rg -n '#\[Test\]' tests/unit/htdocs/kernel/EvalRemovalVerificationTest.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/kernel/EvalRemovalVerificationTest.php` around lines 65 - 285, Rename all public test methods that rely only on the #[Test] attribute to start with the "test" prefix so PHPUnit 9.6 will discover them; specifically rename kernelBlockPhpHasNoExecutableEval, systemBlockPhpHasNoEval, legacyPhpBlockReturnsEmptyContent, xoopsVersionPhpHasNoHtmlTemplateRegistrations, snoopyPhpHasNoShellExecution, zipfilePhpHasNoEval, artFunctionsIniPhpHasNoEval, protectorOninstallUsesVarExportAndDefinesBase, protectorOnuninstallUsesVarExportAndDefinesBase, protectorOnupdateUsesVarExportAndDefinesBase, and protectorNotificationUsesVarExportAndDefinesBase to testKernelBlockPhpHasNoExecutableEval, testSystemBlockPhpHasNoEval, testLegacyPhpBlockReturnsEmptyContent, testXoopsVersionPhpHasNoHtmlTemplateRegistrations, testSnoopyPhpHasNoShellExecution, testZipfilePhpHasNoEval, testArtFunctionsIniPhpHasNoEval, testProtectorOninstallUsesVarExportAndDefinesBase, testProtectorOnuninstallUsesVarExportAndDefinesBase, testProtectorOnupdateUsesVarExportAndDefinesBase, and testProtectorNotificationUsesVarExportAndDefinesBase respectively; keep the #[Test] attributes and existing assertions unchanged so behavior is identical.
🤖 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/kernel/block.php`:
- Around line 390-392: Update the warning string in block.php that currently
reads 'Expected: filename.php|function_name (alphanumeric, hyphens, and
underscores only).' to accurately reflect the validation rule used for function
names (regex [a-zA-Z_]\w*). Replace it with a clearer message such as:
"Expected: filename.php|function_name (function_name must start with a letter or
underscore and contain only letters, digits, or underscores; hyphens are not
allowed)." Locate and edit the literal in the block content validation code in
htdocs/kernel/block.php to ensure admins see the correct rule.
In `@tests/unit/htdocs/kernel/EvalRemovalVerificationTest.php`:
- Around line 1-4: Add the required XOOPS copyright/header block to the top of
the file so the test file begins with the standard XOOPS header comment before
the "<?php" tag; update tests/unit/htdocs/kernel/EvalRemovalVerificationTest.php
to include the repository-required XOOPS file header block (the standard
multi-line comment used across the project) immediately above the existing
declare(strict_types=1); line.
- Around line 223-285: The current tests
protectorOninstallUsesVarExportAndDefinesBase,
protectorOnuninstallUsesVarExportAndDefinesBase,
protectorOnupdateUsesVarExportAndDefinesBase and
protectorNotificationUsesVarExportAndDefinesBase check for legacy
"var_export($mydirname" instead of verifying eval was removed; update each test
to assert that eval is not present (e.g. replace the
assertStringContainsString('var_export($mydirname', $content) with
assertStringNotContainsString('eval(', $content) or an equivalent check for
absence of eval usage) while keeping the existing assertions that the
corresponding base functions (protector_oninstall_base,
protector_onuninstall_base, protector_onupdate_base, protector_notify_base) are
defined.
---
Duplicate comments:
In `@tests/unit/htdocs/kernel/EvalRemovalVerificationTest.php`:
- Around line 65-285: Rename all public test methods that rely only on the
#[Test] attribute to start with the "test" prefix so PHPUnit 9.6 will discover
them; specifically rename kernelBlockPhpHasNoExecutableEval,
systemBlockPhpHasNoEval, legacyPhpBlockReturnsEmptyContent,
xoopsVersionPhpHasNoHtmlTemplateRegistrations, snoopyPhpHasNoShellExecution,
zipfilePhpHasNoEval, artFunctionsIniPhpHasNoEval,
protectorOninstallUsesVarExportAndDefinesBase,
protectorOnuninstallUsesVarExportAndDefinesBase,
protectorOnupdateUsesVarExportAndDefinesBase, and
protectorNotificationUsesVarExportAndDefinesBase to
testKernelBlockPhpHasNoExecutableEval, testSystemBlockPhpHasNoEval,
testLegacyPhpBlockReturnsEmptyContent,
testXoopsVersionPhpHasNoHtmlTemplateRegistrations,
testSnoopyPhpHasNoShellExecution, testZipfilePhpHasNoEval,
testArtFunctionsIniPhpHasNoEval,
testProtectorOninstallUsesVarExportAndDefinesBase,
testProtectorOnuninstallUsesVarExportAndDefinesBase,
testProtectorOnupdateUsesVarExportAndDefinesBase, and
testProtectorNotificationUsesVarExportAndDefinesBase respectively; keep the
#[Test] attributes and existing assertions unchanged so behavior is identical.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 6de08135-9361-4520-86c7-b9b797f525d4
📒 Files selected for processing (2)
htdocs/kernel/block.phptests/unit/htdocs/kernel/EvalRemovalVerificationTest.php
Comment on lines
+1
to
+4
| <?php | ||
|
|
||
| declare(strict_types=1); | ||
|
|
There was a problem hiding this comment.
Add the required XOOPS file header block at the top.
Line 1 starts directly with <?php and declare(strict_types=1); without the repository-required XOOPS header comment block.
📌 Suggested fix
<?php
+/**
+ * XOOPS Project - https://xoops.org
+ * ...
+ */
declare(strict_types=1);As per coding guidelines: "Every source file must begin with the XOOPS copyright header block".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tests/unit/htdocs/kernel/EvalRemovalVerificationTest.php` around lines 1 - 4,
Add the required XOOPS copyright/header block to the top of the file so the test
file begins with the standard XOOPS header comment before the "<?php" tag;
update tests/unit/htdocs/kernel/EvalRemovalVerificationTest.php to include the
repository-required XOOPS file header block (the standard multi-line comment
used across the project) immediately above the existing declare(strict_types=1);
line.
…ules Warning text now distinguishes that hyphens are valid in the filename part but function_name must be a valid PHP identifier, preventing admins from trying invalid callback names like my-function.
Comment on lines
+387
to
+399
| // Detect near-miss: content looks like file-based format but has invalid syntax | ||
| if (stripos(trim($raw), '.php|') !== false) { | ||
| $this->logBlockWarning( | ||
| 'Block content looks like file-based format but has invalid syntax. ' | ||
| . 'Expected: filename.php|function_name (filename may contain letters, digits, hyphens, ' | ||
| . 'underscores; function_name must be a valid PHP identifier).' | ||
| ); | ||
| trigger_error( | ||
| 'XOOPS block has malformed file-based content — check filename.php|function_name syntax.', | ||
| E_USER_WARNING | ||
| ); | ||
|
|
||
| return ''; |
Comment on lines
14
to
17
| // Dynamic function name is required for D3-style cloned installs. | ||
| // $mydirname comes from basename(__DIR__) via ProtectorRegistry, not user input. | ||
| if (!function_exists('xoops_module_install_' . $mydirname)) { | ||
| eval('function xoops_module_install_' . $mydirname . '($module) { return protector_oninstall_base($module, ' . var_export($mydirname, true) . '); }'); |
Comment on lines
14
to
17
| // Dynamic function name is required for D3-style cloned installs. | ||
| // $mydirname comes from basename(__DIR__) via ProtectorRegistry, not user input. | ||
| if (!function_exists('xoops_module_update_' . $mydirname)) { | ||
| eval('function xoops_module_update_' . $mydirname . '($module) { return protector_onupdate_base($module, ' . var_export($mydirname, true) . '); }'); |
Comment on lines
13
to
16
| // Dynamic function name is required for D3-style cloned installs. | ||
| // $mydirname comes from basename(__DIR__) via ProtectorRegistry, not user input. | ||
| if (!function_exists('xoops_module_uninstall_' . $mydirname)) { | ||
| eval('function xoops_module_uninstall_' . $mydirname . '($module) { return protector_onuninstall_base($module, ' . var_export($mydirname, true) . '); }'); |
Comment on lines
13
to
16
| // Dynamic function name is required for D3-style cloned installs. | ||
| // $mydirname comes from basename(__DIR__) via ProtectorRegistry, not user input. | ||
| if (!function_exists($mydirname . '_notify_iteminfo')) { | ||
| eval('function ' . $mydirname . '_notify_iteminfo($category, $item_id) { return protector_notify_base(' . var_export($mydirname, true) . ', $category, $item_id); }'); |
Comment on lines
+203
to
+285
| // ========================================================================= | ||
| // M-11c: Protector lifecycle — eval() retained for D3 clone support | ||
| // ========================================================================= | ||
|
|
||
| /** | ||
| * Resolve a protector trust-path file across different path structures. | ||
| */ | ||
| private function resolveProtectorFile(string $basename): string | ||
| { | ||
| $file = XOOPS_ROOT_PATH . '/../xoops_lib/modules/protector/' . $basename; | ||
| if (!file_exists($file)) { | ||
| $file = XOOPS_PATH . '/modules/protector/' . $basename; | ||
| } | ||
| if (!file_exists($file)) { | ||
| $this->markTestSkipped("Protector trust-path file not found: {$basename}"); | ||
| } | ||
|
|
||
| return $file; | ||
| } | ||
|
|
||
| /** | ||
| * Verify protector lifecycle files use var_export() for safe eval | ||
| * and define the expected base functions. | ||
| */ | ||
| #[Test] | ||
| public function protectorOninstallUsesVarExportAndDefinesBase(): void | ||
| { | ||
| $file = $this->resolveProtectorFile('oninstall.php'); | ||
| $this->assertFileExists($file); | ||
|
|
||
| $content = file_get_contents($file); | ||
| $this->assertNotFalse($content); | ||
|
|
||
| $this->assertStringContainsString('var_export($mydirname', $content, | ||
| 'protector/oninstall.php should use var_export() for safe dirname embedding'); | ||
| $this->assertStringContainsString('protector_oninstall_base', $content, | ||
| 'protector/oninstall.php should define the base install function'); | ||
| } | ||
|
|
||
| #[Test] | ||
| public function protectorOnuninstallUsesVarExportAndDefinesBase(): void | ||
| { | ||
| $file = $this->resolveProtectorFile('onuninstall.php'); | ||
| $this->assertFileExists($file); | ||
|
|
||
| $content = file_get_contents($file); | ||
| $this->assertNotFalse($content); | ||
|
|
||
| $this->assertStringContainsString('var_export($mydirname', $content, | ||
| 'protector/onuninstall.php should use var_export() for safe dirname embedding'); | ||
| $this->assertStringContainsString('protector_onuninstall_base', $content, | ||
| 'protector/onuninstall.php should define the base uninstall function'); | ||
| } | ||
|
|
||
| #[Test] | ||
| public function protectorOnupdateUsesVarExportAndDefinesBase(): void | ||
| { | ||
| $file = $this->resolveProtectorFile('onupdate.php'); | ||
| $this->assertFileExists($file); | ||
|
|
||
| $content = file_get_contents($file); | ||
| $this->assertNotFalse($content); | ||
|
|
||
| $this->assertStringContainsString('var_export($mydirname', $content, | ||
| 'protector/onupdate.php should use var_export() for safe dirname embedding'); | ||
| $this->assertStringContainsString('protector_onupdate_base', $content, | ||
| 'protector/onupdate.php should define the base update function'); | ||
| } | ||
|
|
||
| #[Test] | ||
| public function protectorNotificationUsesVarExportAndDefinesBase(): void | ||
| { | ||
| $file = $this->resolveProtectorFile('notification.php'); | ||
| $this->assertFileExists($file); | ||
|
|
||
| $content = file_get_contents($file); | ||
| $this->assertNotFalse($content); | ||
|
|
||
| $this->assertStringContainsString('var_export($mydirname', $content, | ||
| 'protector/notification.php should use var_export() for safe dirname embedding'); | ||
| $this->assertStringContainsString('protector_notify_base', $content, | ||
| 'protector/notification.php should define the base notify function'); | ||
| } |
Comment on lines
733
to
736
| /** | ||
| * Verify that content containing a pipe but not matching file-based format | ||
| * is blocked with a pipe-specific warning when legacy mode is disabled. | ||
| * falls to the legacy path and returns empty (eval permanently removed). | ||
| */ |
Replace eval()-based dynamic function definitions in protector oninstall, onuninstall, onupdate, and notification files with literal function definitions. Since $mydirname is always 'protector' in the core distribution, no dynamic naming is needed. Update EvalRemovalVerificationTest to verify eval() absence using token_get_all() T_EVAL detection instead of checking for var_export patterns.
The Protector lifecycle files (oninstall, onuninstall, onupdate, notification) require eval() for D3-style cloned installs where $mydirname varies at runtime (e.g. protector2). Hard-coding function names to _protector broke clone support. Reverted to master's guarded eval() pattern. Tests now verify the guard (function_exists) and registry sourcing rather than asserting eval absence.
mambax7
changed the title
This addresses Copilot review feedback on PR XOOPS#1653: - EvalRemovalVerificationTest.php: Section header said "eval() removed, literal functions used" but the Protector lifecycle files intentionally retain guarded eval() for D3-style cloned installs. Changed to "eval() retained for D3 clone support". - XoopsBlockPhpBlockTest.php: Docblock said malformed pipe content "falls to the legacy path" but it's actually caught by the near-miss regex guard before reaching executeLegacyBlock(). Changed to "caught by the near-miss guard".
| $block->setVar('c_type', 'P'); | ||
| $block->setVar('content', 'echo "This should never execute";'); | ||
|
|
||
| $result = $block->getContent('S', 'P'); |
| ); | ||
| trigger_error( | ||
| 'XOOPS block has malformed file-based content — check filename.php|function_name syntax.', | ||
| E_USER_WARNING |
| ); | ||
| trigger_error( | ||
| 'XOOPS: PHP block eval() has been removed in 2.5.12. Convert to file-based format.', | ||
| E_USER_WARNING |
Comment on lines
741
to
742
| $result = $block->getContent('S', 'P'); | ||
|
|
Comment on lines
+7
to
+26
| use PHPUnit\Framework\Attributes\CoversClass; | ||
| use PHPUnit\Framework\Attributes\Test; | ||
| use PHPUnit\Framework\TestCase; | ||
| use XoopsBlock; | ||
|
|
||
| /** | ||
| * Verification tests for eval() removal and legacy cleanup (2.5.12 RC). | ||
| * | ||
| * These tests scan source files to confirm that eval() calls have been | ||
| * removed from security-critical paths. | ||
| * | ||
| * @category XoopsCore | ||
| * @package Tests\Kernel | ||
| * @author XOOPS Development Team | ||
| * @copyright 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 | ||
| */ | ||
| #[CoversClass(XoopsBlock::class)] | ||
| class EvalRemovalVerificationTest extends TestCase |
Comment on lines
+223
to
+243
| /** | ||
| * Protector lifecycle files intentionally retain eval() for D3-style cloned | ||
| * installs where $mydirname varies at runtime. These tests verify: | ||
| * 1. eval() is guarded by function_exists() to prevent redefinition | ||
| * 2. $mydirname is sourced from ProtectorRegistry (not user input) | ||
| * 3. The base function is defined | ||
| */ | ||
| #[Test] | ||
| public function protectorOninstallUsesGuardedEvalAndDefinesBase(): void | ||
| { | ||
| $file = $this->resolveProtectorFile('oninstall.php'); | ||
| $content = file_get_contents($file); | ||
| $this->assertNotFalse($content); | ||
|
|
||
| $this->assertStringContainsString('function_exists(', $content, | ||
| 'protector/oninstall.php must guard eval() with function_exists()'); | ||
| $this->assertStringContainsString('protector_oninstall_base', $content, | ||
| 'protector/oninstall.php must define the base install function'); | ||
| $this->assertStringContainsString("getEntry('mydirname')", $content, | ||
| 'protector/oninstall.php must source $mydirname from ProtectorRegistry'); | ||
| } |
…or handler Addresses Codex merge-blocking feedback on PR XOOPS#1653: the trigger_error(..., E_USER_WARNING) calls in block.php would cause test failures under strict PHPUnit warning handling. - block.php: Both trigger_error() calls (near-miss malformed content and legacy eval removal) downgraded from E_USER_WARNING to E_USER_DEPRECATED — these are deprecation signals, not runtime errors - XoopsBlockPhpBlockTest.php: Both malformedPipeContentIsBlockedAndReturnsEmpty() and legacyEvalContentAlwaysReturnsEmpty() now install a temporary set_error_handler() to capture and assert the deprecation message - EvalRemovalVerificationTest.php: legacyPhpBlockReturnsEmptyContent() now traps E_USER_DEPRECATED and asserts it fires
Comment on lines
+252
to
+258
| $this->assertStringContainsString('function_exists(', $content, | ||
| 'protector/oninstall.php must guard eval() with function_exists()'); | ||
| $this->assertStringContainsString('protector_oninstall_base', $content, | ||
| 'protector/oninstall.php must define the base install function'); | ||
| $this->assertStringContainsString("getEntry('mydirname')", $content, | ||
| 'protector/oninstall.php must source $mydirname from ProtectorRegistry'); | ||
| } |
Comment on lines
+227
to
+235
| $file = XOOPS_ROOT_PATH . '/../xoops_lib/modules/protector/' . $basename; | ||
| if (!file_exists($file)) { | ||
| $file = XOOPS_PATH . '/modules/protector/' . $basename; | ||
| } | ||
| if (!file_exists($file)) { | ||
| $this->markTestSkipped("Protector trust-path file not found: {$basename}"); | ||
| } | ||
|
|
||
| return $file; |
Comment on lines
+7
to
+27
| use PHPUnit\Framework\Attributes\CoversClass; | ||
| use PHPUnit\Framework\Attributes\Test; | ||
| use PHPUnit\Framework\TestCase; | ||
| use XoopsBlock; | ||
|
|
||
| /** | ||
| * Verification tests for eval() removal and legacy cleanup (2.5.12 RC). | ||
| * | ||
| * These tests scan source files to confirm that eval() calls have been | ||
| * removed from security-critical paths. | ||
| * | ||
| * @category XoopsCore | ||
| * @package Tests\Kernel | ||
| * @author XOOPS Development Team | ||
| * @copyright 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 | ||
| */ | ||
| #[CoversClass(XoopsBlock::class)] | ||
| class EvalRemovalVerificationTest extends TestCase | ||
| { |
Comment on lines
566
to
572
| trigger_error( | ||
| 'XOOPS: PHP block eval() has been removed in 2.5.12. Convert to file-based format.', | ||
| E_USER_DEPRECATED | ||
| ); | ||
| $warningLogged = true; | ||
| } | ||
|
|
Comment on lines
+741
to
+747
| set_error_handler(function (int $errno, string $errstr) use (&$deprecations) { | ||
| if ($errno === E_USER_DEPRECATED) { | ||
| $deprecations[] = $errstr; | ||
| return true; | ||
| } | ||
| return false; | ||
| }); |
Comment on lines
+768
to
+774
| set_error_handler(function (int $errno, string $errstr) use (&$deprecations) { | ||
| if ($errno === E_USER_DEPRECATED) { | ||
| $deprecations[] = $errstr; | ||
| return true; | ||
| } | ||
| return false; | ||
| }); |
| return true; | ||
| } | ||
| return false; | ||
| }); |
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (3)
tests/unit/htdocs/kernel/EvalRemovalVerificationTest.php (3)
1-4:⚠️ Potential issue | 🟡 MinorAdd the required XOOPS source header block at file top.
The file currently begins without the repository-required XOOPS header comment block.
As per coding guidelines: "Every source file must begin with the XOOPS copyright header block".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/unit/htdocs/kernel/EvalRemovalVerificationTest.php` around lines 1 - 4, Add the repository-required XOOPS source header comment block to the top of EvalRemovalVerificationTest.php by inserting the standard XOOPS header comment immediately after the opening <?php and before declare(strict_types=1); — ensure the block matches the project template (copyright, license, author, and link fields) and appears above the namespace/class/test declarations so the file complies with the XOOPS source header requirement.
65-303:⚠️ Potential issue | 🟠 MajorUse
test...method names to satisfy PHPUnit 9.6 compatibility.All test methods are attribute-marked but non-
test...; this conflicts with your declared PHPUnit 9.6/10.5/11 support target.#!/bin/bash set -euo pipefail echo "PHPUnit requirement(s):" rg -n '"phpunit/phpunit"' composer.json composer.*.json 2>/dev/null || true echo echo "Methods with #[Test] but no test* prefix in EvalRemovalVerificationTest.php:" python - <<'PY' import re, pathlib p = pathlib.Path("tests/unit/htdocs/kernel/EvalRemovalVerificationTest.php") s = p.read_text(encoding="utf-8") for m in re.finditer(r'#\[Test\]\s*public function\s+([A-Za-z_]\w*)\s*\(', s, re.M): name = m.group(1) if not name.startswith("test"): line = s.count("\n", 0, m.start()) + 1 print(f"{line}: {name}") PYAs per coding guidelines: "Tests must work across PHPUnit 9.6, 10.5, and 11.x" and "Name test methods test{MethodName} or test{MethodName}{Scenario}".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/unit/htdocs/kernel/EvalRemovalVerificationTest.php` around lines 65 - 303, The PHPUnit attribute-marked methods in EvalRemovalVerificationTest.php do not use the required test* prefix; rename each public function with #[Test] that doesn't start with "test" to a name beginning with "test" (e.g., kernelBlockPhpHasNoExecutableEval -> testKernelBlockPhpHasNoExecutableEval, systemBlockPhpHasNoEval -> testSystemBlockPhpHasNoEval, legacyPhpBlockReturnsEmptyContent -> testLegacyPhpBlockReturnsEmptyContent, xoopsVersionPhpHasNoHtmlTemplateRegistrations -> testXoopsVersionPhpHasNoHtmlTemplateRegistrations, snoopyPhpHasNoShellExecution -> testSnoopyPhpHasNoShellExecution, zipfilePhpHasNoEval -> testZipfilePhpHasNoEval, artFunctionsIniPhpHasNoEval -> testArtFunctionsIniPhpHasNoEval, protectorOninstallUsesGuardedEvalAndDefinesBase -> testProtectorOninstallUsesGuardedEvalAndDefinesBase, protectorOnuninstallUsesGuardedEvalAndDefinesBase -> testProtectorOnuninstallUsesGuardedEvalAndDefinesBase, protectorOnupdateUsesGuardedEvalAndDefinesBase -> testProtectorOnupdateUsesGuardedEvalAndDefinesBase, protectorNotificationUsesGuardedEvalAndDefinesBase -> testProtectorNotificationUsesGuardedEvalAndDefinesBase) so they are compatible with PHPUnit 9.6/10.5/11; keep all attributes, docblocks and assertions unchanged and update any internal references or data providers if they rely on the old names.
227-233:⚠️ Potential issue | 🟠 MajorExplicitly require
tests/bootstrap.phprather than relying on undefined constants.This test uses
XOOPS_ROOT_PATHandXOOPS_PATHconstants (lines 68, 227, 229, etc.) without ensuring they are defined. Unlike other test files in the same directory, this file does not explicitly requiretests/bootstrap.php, which defines these constants. The PHPUnit bootstrap (vendor/autoload.php) does not define XOOPS constants.Add this require at the top of the file after the namespace declaration:
require_once dirname(__DIR__, 4) . '/bootstrap.php';This aligns the test with the isolation requirement ("Tests must be fully isolated with no XOOPS installation required for unit tests") and matches the pattern used by peer test files like
XoopsFormElementTrayTest.php.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/unit/htdocs/kernel/EvalRemovalVerificationTest.php` around lines 227 - 233, This test uses the XOOPS constants XOOPS_ROOT_PATH and XOOPS_PATH but does not ensure they're defined; open tests/unit/htdocs/kernel/EvalRemovalVerificationTest.php and add a require_once for the test bootstrap immediately after the namespace declaration (the same pattern other tests use) so the constants are loaded—specifically insert require_once dirname(__DIR__, 4) . '/bootstrap.php'; right after the namespace line to guarantee XOOPS_ROOT_PATH and XOOPS_PATH are defined before any use in methods like the file-existence checks.
🤖 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/kernel/XoopsBlockPhpBlockTest.php`:
- Around line 755-758: The test in XoopsBlockPhpBlockTest uses order-dependent
checks by asserting the expected token against $deprecations[0]; change these to
order-independent checks by asserting that any element of the $deprecations
array contains the token (e.g., use implode(' ', $deprecations) with
assertStringContainsString, or assertTrue((bool) preg_grep('/malformed/',
$deprecations)) or assertNotEmpty(array_filter($deprecations, fn($d) =>
str_contains($d, 'malformed')))); apply the same fix for the other occurrence
around lines 782-784 so both assertions inspect the whole $deprecations array
rather than index 0.
- Around line 738-739: Rename all PHPUnit test methods that currently rely on
#[Test] so they start with the test prefix (e.g. change
malformedPipeContentIsBlockedAndReturnsEmpty to
testMalformedPipeContentIsBlockedAndReturnsEmpty) so PHPUnit 9.6 detects them;
locate methods such as malformedPipeContentIsBlockedAndReturnsEmpty and the
other 23 methods listed in the comment and prepend "test" to each method name
(preserving camelCase and any existing visibility/return type) ensuring method
signatures (e.g., public function
malformedPipeContentIsBlockedAndReturnsEmpty(): void) become public function
testMalformedPipeContentIsBlockedAndReturnsEmpty(): void.
---
Duplicate comments:
In `@tests/unit/htdocs/kernel/EvalRemovalVerificationTest.php`:
- Around line 1-4: Add the repository-required XOOPS source header comment block
to the top of EvalRemovalVerificationTest.php by inserting the standard XOOPS
header comment immediately after the opening <?php and before
declare(strict_types=1); — ensure the block matches the project template
(copyright, license, author, and link fields) and appears above the
namespace/class/test declarations so the file complies with the XOOPS source
header requirement.
- Around line 65-303: The PHPUnit attribute-marked methods in
EvalRemovalVerificationTest.php do not use the required test* prefix; rename
each public function with #[Test] that doesn't start with "test" to a name
beginning with "test" (e.g., kernelBlockPhpHasNoExecutableEval ->
testKernelBlockPhpHasNoExecutableEval, systemBlockPhpHasNoEval ->
testSystemBlockPhpHasNoEval, legacyPhpBlockReturnsEmptyContent ->
testLegacyPhpBlockReturnsEmptyContent,
xoopsVersionPhpHasNoHtmlTemplateRegistrations ->
testXoopsVersionPhpHasNoHtmlTemplateRegistrations, snoopyPhpHasNoShellExecution
-> testSnoopyPhpHasNoShellExecution, zipfilePhpHasNoEval ->
testZipfilePhpHasNoEval, artFunctionsIniPhpHasNoEval ->
testArtFunctionsIniPhpHasNoEval, protectorOninstallUsesGuardedEvalAndDefinesBase
-> testProtectorOninstallUsesGuardedEvalAndDefinesBase,
protectorOnuninstallUsesGuardedEvalAndDefinesBase ->
testProtectorOnuninstallUsesGuardedEvalAndDefinesBase,
protectorOnupdateUsesGuardedEvalAndDefinesBase ->
testProtectorOnupdateUsesGuardedEvalAndDefinesBase,
protectorNotificationUsesGuardedEvalAndDefinesBase ->
testProtectorNotificationUsesGuardedEvalAndDefinesBase) so they are compatible
with PHPUnit 9.6/10.5/11; keep all attributes, docblocks and assertions
unchanged and update any internal references or data providers if they rely on
the old names.
- Around line 227-233: This test uses the XOOPS constants XOOPS_ROOT_PATH and
XOOPS_PATH but does not ensure they're defined; open
tests/unit/htdocs/kernel/EvalRemovalVerificationTest.php and add a require_once
for the test bootstrap immediately after the namespace declaration (the same
pattern other tests use) so the constants are loaded—specifically insert
require_once dirname(__DIR__, 4) . '/bootstrap.php'; right after the namespace
line to guarantee XOOPS_ROOT_PATH and XOOPS_PATH are defined before any use in
methods like the file-existence checks.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: a9585f1d-6941-4add-999e-5d1909fb0a96
📒 Files selected for processing (3)
htdocs/kernel/block.phptests/unit/htdocs/kernel/EvalRemovalVerificationTest.phptests/unit/htdocs/kernel/XoopsBlockPhpBlockTest.php
Comment on lines
+738
to
739
| public function malformedPipeContentIsBlockedAndReturnsEmpty(): void | ||
| { |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "Check PHPUnit requirement(s):"
rg -n '"phpunit/phpunit"' composer.json composer.*.json 2>/dev/null || true
echo
echo "Methods with #[Test] but no test* prefix in this file:"
python - <<'PY'
import re, pathlib
p = pathlib.Path("tests/unit/htdocs/kernel/XoopsBlockPhpBlockTest.php")
s = p.read_text(encoding="utf-8")
for m in re.finditer(r'#\[Test\]\s*public function\s+([A-Za-z_]\w*)\s*\(', s, re.M):
name = m.group(1)
if not name.startswith("test"):
line = s.count("\n", 0, m.start()) + 1
print(f"{line}: {name}")
PYRepository: XOOPS/XoopsCore25
Length of output: 1242
🏁 Script executed:
cat composer.json | grep -A 5 -B 5 "phpunit"Repository: XOOPS/XoopsCore25
Length of output: 106
🌐 Web query:
PHPUnit 9.6 #[Test] attribute discovery without test prefix
💡 Result:
PHPUnit 9.6 does not support discovering tests via the PHP 8 attribute #[Test]. In 9.6, a method is only considered a test if it either:
- is prefixed with
test, or - has the PHPDoc annotation
/**@test*/(so you can drop thetestprefix that way). [1]
If you specifically want #[Test] to mark methods as tests without the test prefix, you need to upgrade to PHPUnit 10+, where the PHPUnit\Framework\Attributes\Test attribute exists for exactly that purpose. [2]
PHPUnit 9.6 example (no test prefix):
/**
* `@test`
*/
public function it_calculates_totals(): void
{
$this->assertTrue(true);
}PHPUnit 10+ example (no test prefix):
use PHPUnit\Framework\Attributes\Test;
#[Test]
public function it_calculates_totals(): void
{
$this->assertTrue(true);
}Sources: [1] [2]
🏁 Script executed:
find . -maxdepth 2 -name "composer.json" -o -name "composer.lock" | head -5Repository: XOOPS/XoopsCore25
Length of output: 43
🏁 Script executed:
head -50 tests/unit/htdocs/kernel/XoopsBlockPhpBlockTest.phpRepository: XOOPS/XoopsCore25
Length of output: 1305
🏁 Script executed:
tail -20 tests/unit/htdocs/kernel/XoopsBlockPhpBlockTest.phpRepository: XOOPS/XoopsCore25
Length of output: 765
Rename all test methods to use test* prefix for PHPUnit 9.6 compatibility.
PHPUnit 9.6 does not support the #[Test] attribute for test discovery; it requires either the test* method name prefix or @test PHPDoc annotation. Your file currently has 24 methods marked with #[Test] but lacking the test* prefix, making them invisible to PHPUnit 9.6—none of these tests will run under that version. Since your codebase targets PHPUnit 9.6, 10.5, and 11.x compatibility, rename all affected methods to follow the test{MethodName} convention.
This includes lines 738-739 and 765-766, but also 22 other methods throughout the file:
173, 193, 214, 235, 267, 283, 419, 432, 444, 460, 495, 513, 529, 549, 558, 567, 576, 592, 617, 650, 675, 707, 727.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tests/unit/htdocs/kernel/XoopsBlockPhpBlockTest.php` around lines 738 - 739,
Rename all PHPUnit test methods that currently rely on #[Test] so they start
with the test prefix (e.g. change malformedPipeContentIsBlockedAndReturnsEmpty
to testMalformedPipeContentIsBlockedAndReturnsEmpty) so PHPUnit 9.6 detects
them; locate methods such as malformedPipeContentIsBlockedAndReturnsEmpty and
the other 23 methods listed in the comment and prepend "test" to each method
name (preserving camelCase and any existing visibility/return type) ensuring
method signatures (e.g., public function
malformedPipeContentIsBlockedAndReturnsEmpty(): void) become public function
testMalformedPipeContentIsBlockedAndReturnsEmpty(): void.
… and require_once Addresses Codex high-priority finding and Copilot review feedback on PR XOOPS#1653: 1. block.php: Remove static $warningLogged guard from executeLegacyBlock() — deprecation now fires every call, eliminating order-dependent test failures where an earlier test consumed the one-time signal 2. Test error handlers: All set_error_handler() calls now pass E_USER_DEPRECATED as the second argument to avoid masking unrelated warnings/notices during test execution 3. EvalRemovalVerificationTest.php: - Add require_once XOOPS_ROOT_PATH . '/kernel/block.php' at file scope so #[CoversClass(XoopsBlock::class)] resolves correctly - resolveProtectorFile() now prefers XOOPS_TRUST_PATH over relative ../xoops_lib/ path - New assertHasEvalTokens() helper verifies Protector lifecycle files actually contain T_EVAL tokens (catches accidental removal) - Removed legacy @category/@Package tags per PSR-12 4. XoopsBlockPhpBlockTest.php: Deprecation assertions use array_filter() to match any captured message (not just $deprecations[0]), avoiding order-dependency All 56 tests pass (133 assertions) on PHP 8.4.
| return $file; | ||
| } | ||
|
|
||
| $this->markTestSkipped("Protector trust-path file not found: {$basename}"); |
Comment on lines
+276
to
+284
| $this->assertHasEvalTokens($content, | ||
| 'protector/oninstall.php must contain eval() for D3 clone dynamic function names'); | ||
| $this->assertStringContainsString('function_exists(', $content, | ||
| 'protector/oninstall.php must guard eval() with function_exists()'); | ||
| $this->assertStringContainsString('protector_oninstall_base', $content, | ||
| 'protector/oninstall.php must define the base install function'); | ||
| $this->assertStringContainsString("getEntry('mydirname')", $content, | ||
| 'protector/oninstall.php must source $mydirname from ProtectorRegistry'); | ||
| } |
Comment on lines
+387
to
+400
| // Detect near-miss: content looks like file-based format but has invalid syntax | ||
| if (preg_match('/\.php\s*\|/i', $raw)) { | ||
| $this->logBlockWarning( | ||
| 'Block content looks like file-based format but has invalid syntax. ' | ||
| . 'Expected: filename.php|function_name (filename may contain letters, digits, hyphens, ' | ||
| . 'underscores; function_name must be a valid PHP identifier).' | ||
| ); | ||
| trigger_error( | ||
| 'XOOPS block has malformed file-based content — check filename.php|function_name syntax.', | ||
| E_USER_DEPRECATED | ||
| ); | ||
|
|
||
| return ''; | ||
| } |
Comment on lines
559
to
568
| $this->logBlockWarning( | ||
| 'PHP block eval() execution has been permanently removed in XOOPS 2.5.12. ' | ||
| . 'Convert block to file-based format (filename.php|function_name in custom_blocks/), ' | ||
| . 'H (HTML), or S/T (sanitized text) type.' | ||
| ); | ||
| trigger_error( | ||
| 'XOOPS: PHP block eval() has been removed in 2.5.12. Convert to file-based format.', | ||
| E_USER_DEPRECATED | ||
| ); | ||
|
|
…after markTestSkipped Addresses two Copilot comments on commit 00c1d8c: - block.php: Re-add static guard on logBlockWarning() only (prevents flooding XoopsLogger when a site has many legacy blocks), while keeping trigger_error(E_USER_DEPRECATED) on every call so error handlers and tests can always detect it - EvalRemovalVerificationTest.php: Add explicit return '' after markTestSkipped() — the method throws internally so it's unreachable, but satisfies the declared string return type for static analysis
|
Comment on lines
+569
to
+573
| // Always emit deprecation so error handlers and tests can detect it | ||
| trigger_error( | ||
| 'XOOPS: PHP block eval() has been removed in 2.5.12. Convert to file-based format.', | ||
| E_USER_DEPRECATED | ||
| ); |
There was a problem hiding this comment.
♻️ Duplicate comments (3)
tests/unit/htdocs/kernel/EvalRemovalVerificationTest.php (2)
1-24:⚠️ Potential issue | 🟡 MinorAdd required XOOPS file header and complete class docblock tags.
This new file is missing the repository-required XOOPS header block, and the class docblock is missing required tags (
@category,@package).As per coding guidelines: "Every source file must begin with the XOOPS copyright header block" and "Class docblocks must include
@category,@package,@author,@copyright,@license, and@linktags".🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/unit/htdocs/kernel/EvalRemovalVerificationTest.php` around lines 1 - 24, Add the required XOOPS file header block at the top of tests/unit/htdocs/kernel/EvalRemovalVerificationTest.php using the project's standard copyright/header template, and update the class docblock for the EvalRemovalVerificationTest class to include the missing `@category` and `@package` tags (while keeping existing `@author`, `@copyright`, `@license`, and `@link` tags); ensure the header and docblock follow the repository formatting conventions.
81-337:⚠️ Potential issue | 🟠 MajorRename test methods to
test...to satisfy PHPUnit 9.6 compatibility.The class currently uses
#[Test]with non-test*method names across multiple methods, which conflicts with the repository’s cross-version test requirement.#!/bin/bash set -euo pipefail rg -nP '^\s*public function (?!test)[A-Za-z_]\w*\s*\(' tests/unit/htdocs/kernel/EvalRemovalVerificationTest.phpAs per coding guidelines: "Tests must work across PHPUnit 9.6, 10.5, and 11.x" and "Name test methods test{MethodName} or test{MethodName}{Scenario}".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/unit/htdocs/kernel/EvalRemovalVerificationTest.php` around lines 81 - 337, Rename the public test methods in EvalRemovalVerificationTest.php so their names start with "test" to satisfy PHPUnit 9.6 (e.g., kernelBlockPhpHasNoExecutableEval → testKernelBlockPhpHasNoExecutableEval, systemBlockPhpHasNoEval → testSystemBlockPhpHasNoEval, legacyPhpBlockReturnsEmptyContent → testLegacyPhpBlockReturnsEmptyContent, xoopsVersionPhpHasNoHtmlTemplateRegistrations → testXoopsVersionPhpHasNoHtmlTemplateRegistrations, snoopyPhpHasNoShellExecution → testSnoopyPhpHasNoShellExecution, zipfilePhpHasNoEval → testZipfilePhpHasNoEval, artFunctionsIniPhpHasNoEval → testArtFunctionsIniPhpHasNoEval, protectorOninstallUsesGuardedEvalAndDefinesBase → testProtectorOninstallUsesGuardedEvalAndDefinesBase, protectorOnuninstallUsesGuardedEvalAndDefinesBase → testProtectorOnuninstallUsesGuardedEvalAndDefinesBase, protectorOnupdateUsesGuardedEvalAndDefinesBase → testProtectorOnupdateUsesGuardedEvalAndDefinesBase, protectorNotificationUsesGuardedEvalAndDefinesBase → testProtectorNotificationUsesGuardedEvalAndDefinesBase), leave the #[Test] attributes intact, update any internal references to those method names if present, and run the provided regex/grep check to confirm no public function names remain that don't begin with "test".tests/unit/htdocs/kernel/XoopsBlockPhpBlockTest.php (1)
737-765:⚠️ Potential issue | 🟠 MajorRename these test methods to
test...for PHPUnit 9.6 compatibility.Line 738 and Line 765 still rely on
#[Test]with non-test*names. That conflicts with the repo’s cross-version test requirement.✅ Minimal fix
- public function malformedPipeContentIsBlockedAndReturnsEmpty(): void + public function testMalformedPipeContentIsBlockedAndReturnsEmpty(): void @@ - public function legacyEvalContentAlwaysReturnsEmpty(): void + public function testLegacyEvalContentAlwaysReturnsEmpty(): void#!/bin/bash set -euo pipefail rg -nPU '#\[Test\]\s*\R\s*public function (?!test)[A-Za-z_]\w*\s*\(' tests/unit/htdocs/kernel/XoopsBlockPhpBlockTest.phpAs per coding guidelines: "Tests must work across PHPUnit 9.6, 10.5, and 11.x" and "Name test methods test{MethodName} or test{MethodName}{Scenario}".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/unit/htdocs/kernel/XoopsBlockPhpBlockTest.php` around lines 737 - 765, Rename the two PHPUnit test methods so their names begin with "test" for PHPUnit 9.6 compatibility: change malformedPipeContentIsBlockedAndReturnsEmpty to testMalformedPipeContentIsBlockedAndReturnsEmpty and legacyEvalContentAlwaysReturnsEmpty to testLegacyEvalContentAlwaysReturnsEmpty; keep the existing #[Test] attribute if present and ensure all internal references (e.g., calls to createPhpBlock and getContent inside those methods) remain unchanged.
🤖 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/kernel/EvalRemovalVerificationTest.php`:
- Around line 1-24: Add the required XOOPS file header block at the top of
tests/unit/htdocs/kernel/EvalRemovalVerificationTest.php using the project's
standard copyright/header template, and update the class docblock for the
EvalRemovalVerificationTest class to include the missing `@category` and `@package`
tags (while keeping existing `@author`, `@copyright`, `@license`, and `@link` tags);
ensure the header and docblock follow the repository formatting conventions.
- Around line 81-337: Rename the public test methods in
EvalRemovalVerificationTest.php so their names start with "test" to satisfy
PHPUnit 9.6 (e.g., kernelBlockPhpHasNoExecutableEval →
testKernelBlockPhpHasNoExecutableEval, systemBlockPhpHasNoEval →
testSystemBlockPhpHasNoEval, legacyPhpBlockReturnsEmptyContent →
testLegacyPhpBlockReturnsEmptyContent,
xoopsVersionPhpHasNoHtmlTemplateRegistrations →
testXoopsVersionPhpHasNoHtmlTemplateRegistrations, snoopyPhpHasNoShellExecution
→ testSnoopyPhpHasNoShellExecution, zipfilePhpHasNoEval →
testZipfilePhpHasNoEval, artFunctionsIniPhpHasNoEval →
testArtFunctionsIniPhpHasNoEval, protectorOninstallUsesGuardedEvalAndDefinesBase
→ testProtectorOninstallUsesGuardedEvalAndDefinesBase,
protectorOnuninstallUsesGuardedEvalAndDefinesBase →
testProtectorOnuninstallUsesGuardedEvalAndDefinesBase,
protectorOnupdateUsesGuardedEvalAndDefinesBase →
testProtectorOnupdateUsesGuardedEvalAndDefinesBase,
protectorNotificationUsesGuardedEvalAndDefinesBase →
testProtectorNotificationUsesGuardedEvalAndDefinesBase), leave the #[Test]
attributes intact, update any internal references to those method names if
present, and run the provided regex/grep check to confirm no public function
names remain that don't begin with "test".
In `@tests/unit/htdocs/kernel/XoopsBlockPhpBlockTest.php`:
- Around line 737-765: Rename the two PHPUnit test methods so their names begin
with "test" for PHPUnit 9.6 compatibility: change
malformedPipeContentIsBlockedAndReturnsEmpty to
testMalformedPipeContentIsBlockedAndReturnsEmpty and
legacyEvalContentAlwaysReturnsEmpty to testLegacyEvalContentAlwaysReturnsEmpty;
keep the existing #[Test] attribute if present and ensure all internal
references (e.g., calls to createPhpBlock and getContent inside those methods)
remain unchanged.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 64d0462b-ce23-48c7-8989-af6c9a437ea8
📒 Files selected for processing (3)
htdocs/kernel/block.phptests/unit/htdocs/kernel/EvalRemovalVerificationTest.phptests/unit/htdocs/kernel/XoopsBlockPhpBlockTest.php
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #1653 +/- ##
==============================
==============================
☔ 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
eval()execution —executeLegacyBlock()inkernel/block.phpnow logs a deprecation warning and returns empty string. File-based PHP blocks (custom_blocks/*.php) are preservedoninstall.php,onupdate.php,onuninstall.php,notification.php) intentionally retain guardedeval()for D3-style cloned installs where$mydirnamevaries at runtime. Tests verify:function_exists()guard present,$mydirnamesourced fromProtectorRegistry, base function defined.htmltemplates already deregistered, Snoopy already uses PHP cURL extension, zipfile useschr(hexdec()), art framework uses direct static callTest plan
Summary by CodeRabbit
Bug Fixes
Refactor
Tests