updated creation code to generate module with no errors from copilot#173
updated creation code to generate module with no errors from copilot#173ggoffy merged 7 commits intoXoopsModules25x:masterfrom
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughProject-wide addition of strict PHP scalar/nullable type hints, docblock cleanups, and minor control-flow hardenings; several private methods changed visibility; file/dir operations and cloning gained runtime error checks; many code-/template-generators and renderers were refactored; module metadata and platform requirements were bumped. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
✨ Finishing Touches🧪 Generate unit tests (beta)
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Note
Due to the large number of review comments, Critical severity comments were prioritized as inline comments.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (15)
class/Files/Language/LanguageHelp.php (1)
115-118:⚠️ Potential issue | 🟠 MajorDuplicate file creation when language is not English.
$languageis assigned from$GLOBALS['xoopsConfig']['language']on line 85. When the language is not'english', the file is created atlanguage/{$language}/help(line 116), then immediately created again at the same path (line 118) since both reference the same value.The intent appears to be: always create an English fallback, plus create in the current language if different. Line 118 should use the hardcoded
'english'path instead.Proposed fix
if ('english' !== $language) { $this->create($moduleDirname, 'language/' . $language . '/help', $filename, $content, \_AM_MODULEBUILDER_FILE_CREATED, \_AM_MODULEBUILDER_FILE_NOTCREATED); } - $this->create($moduleDirname, 'language/' . $GLOBALS['xoopsConfig']['language'] . '/help', $filename, $content, \_AM_MODULEBUILDER_FILE_CREATED, \_AM_MODULEBUILDER_FILE_NOTCREATED); + $this->create($moduleDirname, 'language/english/help', $filename, $content, \_AM_MODULEBUILDER_FILE_CREATED, \_AM_MODULEBUILDER_FILE_NOTCREATED);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@class/Files/Language/LanguageHelp.php` around lines 115 - 118, The code in LanguageHelp.php calls $this->create twice for the same language when $language equals $GLOBALS['xoopsConfig']['language']; change the second call so it always writes the English fallback instead of reusing $GLOBALS['xoopsConfig']['language': locate the two create() calls (the first creating 'language/' . $language . '/help' and the second currently using 'language/' . $GLOBALS['xoopsConfig']['language'] . '/help') and update the second call to use the hardcoded 'language/english/help' path so you always create the English fallback and avoid duplicate creation when the current language is not English.class/Common/Migrate.php (1)
61-74:⚠️ Potential issue | 🟡 MinorUnused private method should be removed or made accessible.
The
convertIPAddressesmethod is only called within commented-out example code (lines 111-112). Since it'sprivate, it cannot be invoked by subclasses or external code. Consider one of the following:
- Remove the method if it's no longer needed
- Change visibility to
protectedif it's meant to be called by users extending this class- Uncomment the calls in
preSyncActions()if the IP conversion functionality is required🧹 Option 1: Remove the method if unused
- /** - * Change integer IPv4 column to varchar IPv6 capable - * - * `@param` string $tableName table to convert - * `@param` string $columnName column with IP address - */ - private function convertIPAddresses(string $tableName, string $columnName): void - { - if ($this->tableHandler->useTable($tableName)) { - $attributes = $this->tableHandler->getColumnAttributes($tableName, $columnName); - if (false !== \mb_strpos($attributes, ' int(')) { - if (false === \mb_strpos($attributes, 'unsigned')) { - $this->tableHandler->alterColumn($tableName, $columnName, " bigint(16) NOT NULL DEFAULT '0' "); - $this->tableHandler->update($tableName, [$columnName => "4294967296 + $columnName"], "WHERE $columnName < 0", false); - } - $this->tableHandler->alterColumn($tableName, $columnName, " varchar(45) NOT NULL DEFAULT '' "); - $this->tableHandler->update($tableName, [$columnName => "INET_NTOA($columnName)"], '', false); - } - } - }♻️ Option 2: Make it protected for subclass usage
- private function convertIPAddresses(string $tableName, string $columnName): void + protected function convertIPAddresses(string $tableName, string $columnName): void
Static analysis tool PHPMD flagged this method: "Avoid unused private methods such as 'convertIPAddresses'."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@class/Common/Migrate.php` around lines 61 - 74, The private method convertIPAddresses is unused and flagged by PHPMD; either remove convertIPAddresses if IP conversion is no longer needed, or change its visibility to protected so subclasses can call it, and if the migration behavior is required, uncomment and use the convertIPAddresses(...) calls in preSyncActions() to invoke it; update any unit tests or documentation accordingly to reflect removal or the new protected API.class/Files/Templates/User/Defstyle/MoreFiles.php (1)
61-67:⚠️ Potential issue | 🟡 MinorDocblock parameter order is inconsistent with the method signature.
Line 63/64 documents
$filenamebefore$folder, but Line 67 declares$folderbefore$filename. Please align the docblock to avoid API confusion.Suggested docblock fix
/** * `@public` function write * `@param` $module - * `@param` string $filename * `@param` $folder + * `@param` string $filename * `@param` $extension */🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@class/Files/Templates/User/Defstyle/MoreFiles.php` around lines 61 - 67, The docblock for the public method write() lists parameters in a different order than the method signature; update the `@param` lines to match the signature order (module, $folder, string $filename, $extension) so the docblock reflects write($module, $folder, string $filename, $extension): void; ensure each `@param` uses the correct variable name and types to match the method declaration.class/Common/DirectoryChecker.php (2)
136-144:⚠️ Potential issue | 🟠 MajorUndefined variables if POST parameters are missing.
$pathand$redirectare used without guaranteed initialization. If the corresponding POST vars don't exist, these variables remain undefined, causing runtime errors on lines 143-144.🛡️ Proposed fix with defaults or early return
case 'createdir': - if (\Xmf\Request::hasVar('path', 'POST')) { - $path = $_POST['path']; - } - if (\Xmf\Request::hasVar('redirect', 'POST')) { - $redirect = $_POST['redirect']; - } + if (!\Xmf\Request::hasVar('path', 'POST') || !\Xmf\Request::hasVar('redirect', 'POST')) { + break; + } + $path = $_POST['path']; + $redirect = $_POST['redirect']; $msg = DirectoryChecker::createDirectory($path) ? \constant('CO_' . $moduleDirNameUpper . '_' . 'DC_DIRCREATED') : \constant('CO_' . $moduleDirNameUpper . '_' . 'DC_DIRNOTCREATED'); \redirect_header($redirect, 2, $msg . ': ' . $path); break;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@class/Common/DirectoryChecker.php` around lines 136 - 144, In the 'createdir' case, $path and $redirect may be undefined because you only read $_POST when \Xmf\Request::hasVar('path','POST') / 'redirect' is true; update the handler to ensure $path and $redirect are always initialized (either assign safe defaults like empty string or a known safe redirect target, or return/redirect early if required POST params are missing) before calling DirectoryChecker::createDirectory($path) and \redirect_header($redirect,...). Locate the block handling the 'createdir' case and adjust initialization logic around the \Xmf\Request::hasVar checks so $path and $redirect are deterministically set or you bail out with an explanatory message.
153-157:⚠️ Potential issue | 🔴 CriticalType mismatch:
$modefrom POST is a string, butsetDirectoryPermissions()expectsint.With
strict_types=1, passing a string$_POST['mode']to a method expectingint $modewill throw aTypeError. Additionally, if the POST variable is missing,$modewill be undefined.🐛 Proposed fix to cast mode and provide default
if (\Xmf\Request::hasVar('mode', 'POST')) { $mode = $_POST['mode']; } - $msg = DirectoryChecker::setDirectoryPermissions($path, $mode) ? \constant('CO_' . $moduleDirNameUpper . '_' . 'DC_PERMSET') : \constant('CO_' . $moduleDirNameUpper . '_' . 'DC_PERMNOTSET'); + $msg = DirectoryChecker::setDirectoryPermissions($path, (int)$mode) ? \constant('CO_' . $moduleDirNameUpper . '_' . 'DC_PERMSET') : \constant('CO_' . $moduleDirNameUpper . '_' . 'DC_PERMNOTSET');🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@class/Common/DirectoryChecker.php` around lines 153 - 157, The code reads $_POST['mode'] as a string and may leave $mode undefined, but DirectoryChecker::setDirectoryPermissions(..., int $mode) requires an integer; change the retrieval to always produce an int (and a safe default) before calling setDirectoryPermissions — e.g. replace the \Xmf\Request::hasVar/$_POST logic with a single integer retrieval/cast (or use \Xmf\Request::getInt('mode', 'POST', <default>)) so $mode is defined as an int and then pass that $mode to DirectoryChecker::setDirectoryPermissions and to the redirect message.class/Files/CreateXoopsCode.php (1)
649-663:⚠️ Potential issue | 🟡 Minor
$methodparameter should accept request method strings but is typed asbool.The
Intbranch interpolates$methodintoRequest::getInt(..., '{$method}'), which expects values like'POST'or'GET'. Typing it asboolallows onlytrueorfalse—the former would generate'1'as the request method (invalid), and the latter omits the parameter. The type should bestring|falseto support the intended API.🔧 Suggested fix
- public function getXcXoopsRequest(string $left = '', string $var1 = '', string $var2 = '', string $type = 'String', bool $method = false, string $t = '') + public function getXcXoopsRequest(string $left = '', string $var1 = '', string $var2 = '', string $type = 'String', string|false $method = false, string $t = '')🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@class/Files/CreateXoopsCode.php` around lines 649 - 663, Update the getXcXoopsRequest signature to type $method as string|false (instead of bool) so it can accept request method names like 'POST'/'GET'; keep the default false. In the Int branch ensure the existing check (false !== $method) remains and that when true you interpolate the method as a string in the generated code (i.e., produce Request::getInt(..., 'POST') ), so change the function declaration from getXcXoopsRequest(..., bool $method = false, ...) to getXcXoopsRequest(..., string|false $method = false, ...) and leave the conditional and interpolation logic intact.class/Files/CreatePhpCode.php (1)
479-507:⚠️ Potential issue | 🟠 MajorFix
$defaultparameter type ingetPhpCodeCaseSwitch()andgetXcSwitch().
getPhpCodeCaseSwitch()concatenates$defaultdirectly into generated PHP code (line 503-505). Typing it asboolbreaks the API since boolean values become1or empty string when stringified, not valid code. The parameter should acceptstring|falseto allow legitimate code strings. The same issue exists inCreateXoopsCode::getXcSwitch()which forwards this parameter.Additionally,
getPhpCodeHtmlentities()andgetPhpCodeHtmlspecialchars()type their flag parameters asboolbut concatenate them into PHP function calls that expectintconstants (likeENT_QUOTES,ENT_COMPAT). These flag parameters should be typed asint|falseinstead.🔧 Suggested fixes
- public function getPhpCodeCaseSwitch(array $cases = [], bool $defaultAfterCase = false, bool $default = false, string $t = '', bool $isConst = false) + public function getPhpCodeCaseSwitch(array $cases = [], bool $defaultAfterCase = false, string|false $default = false, string $t = '', bool $isConst = false)In CreateXoopsCode.php:
- public function getXcSwitch(string $op = '', array $cases = [], bool $defaultAfterCase = false, bool $default = false, string $t = '', bool $isString = true, bool $isConst = false) + public function getXcSwitch(string $op = '', array $cases = [], bool $defaultAfterCase = false, string|false $default = false, string $t = '', bool $isString = true, bool $isConst = false)In CreatePhpCode.php:
- public function getPhpCodeHtmlentities($entitiesVar, bool $entitiesQuote = false) + public function getPhpCodeHtmlentities($entitiesVar, int|false $entitiesQuote = false)- public function getPhpCodeHtmlspecialchars($specialVar, bool $specialQuote = false) + public function getPhpCodeHtmlspecialchars($specialVar, int|false $specialQuote = false)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@class/Files/CreatePhpCode.php` around lines 479 - 507, Update the parameter types so string code can be injected instead of boolean: change getPhpCodeCaseSwitch() signature to accept string|false for $default (instead of bool) and ensure getXcSwitch() in CreateXoopsCode forwards that same string|false type; likewise change getPhpCodeHtmlentities() and getPhpCodeHtmlspecialchars() flag parameters from bool to int|false so the flags (ENT_* constants) are concatenated correctly into the generated calls; adjust docblocks and any callers that pass the $default or flag values to use string (or false) and int (or false) respectively to avoid boolean stringification.class/Files/Templates/User/Defstyle/Categories.php (1)
98-103:⚠️ Potential issue | 🟠 MajorUse the Categories helpers here (current calls target AdminPages helpers).
Inside
getTemplatesUserCategoriesTable, the method calls don’t match this class’s own helper methods. This likely causes wrong template composition and explains why the localThead/Tbodyhelpers are reported unused.Proposed fix
- $table = $this->getTemplatesAdminPagesTableThead($tableName, $language); - $table .= $this->getTemplatesAdminPagesTableTBody($moduleDirname, $tableName, $tableSoleName, $language); + $table = $this->getTemplatesUserCategoriesThead($tableName, $language); + $table .= $this->getTemplatesUserCategoriesTbody($moduleDirname, $tableName, $tableSoleName);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@class/Files/Templates/User/Defstyle/Categories.php` around lines 98 - 103, The method getTemplatesUserCategoriesTable currently calls the admin helpers getTemplatesAdminPagesTableThead and getTemplatesAdminPagesTableTBody, causing wrong composition; update getTemplatesUserCategoriesTable to call this class's user-specific helpers (e.g., getTemplatesUserCategoriesTableThead and getTemplatesUserCategoriesTableTBody) with the same parameters ($tableName, $language) and ($moduleDirname, $tableName, $tableSoleName, $language) respectively so the local Thead/Tbody helpers are used.class/Building.php (1)
53-57:⚠️ Potential issue | 🟠 MajorKeep
getForm()accepting action URLs.Line 53 declares
$actionasbool, but line 57 reassigns it as a string from\Xmf\Request::getString()and passes it to\XoopsThemeFormconstructor on line 60. Withstrict_types=1enabled in this file, any caller passing a custom action path as a string will trigger aTypeError. The public method signature must accept bothbool(default behavior) andstring(custom actions).💡 Compatibility fix
- public function getForm(bool $action = false) + public function getForm(string|bool $action = false)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@class/Building.php` around lines 53 - 57, The getForm signature currently restricts $action to bool causing TypeError when a string action is used; change the parameter to accept both types (e.g., public function getForm(string|bool $action = false) or remove the type hint) and update the method docblock accordingly so callers can pass the string from \Xmf\Request::getString() and it can be forwarded to \XoopsThemeForm without type errors.class/Files/Templates/User/Defstyle/PagesList.php (1)
71-78:⚠️ Potential issue | 🟡 MinorFix the
write()docblock argument order.The signature is
($module, $table, $tables, string $filename), but the docblock still documents$filenamebefore$tables. That public API mismatch is now more confusing because$filenameis typed.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@class/Files/Templates/User/Defstyle/PagesList.php` around lines 71 - 78, The docblock for the public method write() is out of order relative to its signature; update the `@param` annotations to match the method signature write($module, $table, $tables, string $filename) by ordering parameters as $module, $table, $tables, $filename and ensure the type for $filename is string (and adjust any other param types/descriptions if needed) so the docblock and the write() signature align.class/Files/CreateClone.php (2)
65-84:⚠️ Potential issue | 🟠 MajorAdd error handling for file operations to prevent silent failures.
The
cloneFile()method ignores return values fromcopy(),file_get_contents(), andfile_put_contents(). When these operations fail, the method succeeds silently, leaving the destination file missing or incomplete. Additionally, iffile_get_contents()returnsfalseon line 74, passing it tostr_replace()on line 75 produces unexpected behavior.🛠️ Proposed fix
if (\in_array(\mb_strtolower(\pathinfo($src_file, PATHINFO_EXTENSION)), $noChangeExtensions)) { // image - \copy($src_file, $dst_file); + if (!\copy($src_file, $dst_file)) { + throw new \RuntimeException("Unable to copy {$src_file} to {$dst_file}"); + } } else { // file, read it and replace text - $content = file_get_contents($src_file); + $content = \file_get_contents($src_file); + if (false === $content) { + throw new \RuntimeException("Unable to read source file: {$src_file}"); + } $content = \str_replace($patKeys, $patValues, $content); //check file name whether it contains replace code $path_parts = \pathinfo($dst_file); $path = $path_parts['dirname']; $file = $path_parts['basename']; $dst_file = $path . '/' . \str_replace($patKeys, $patValues, $file); - file_put_contents($dst_file, $content); + if (false === \file_put_contents($dst_file, $content)) { + throw new \RuntimeException("Unable to write destination file: {$dst_file}"); + } } } else { - \copy($src_file, $dst_file); + if (!\copy($src_file, $dst_file)) { + throw new \RuntimeException("Unable to copy {$src_file} to {$dst_file}"); + } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@class/Files/CreateClone.php` around lines 65 - 84, The cloneFile method currently ignores failures from copy(), file_get_contents(), and file_put_contents(), and may call str_replace() on a false value; update cloneFile to check return values and raise/return an error when operations fail: after any \copy($src_file, $dst_file) check its boolean result and throw a RuntimeException (include $src_file and $dst_file in the message); when reading content use $content = file_get_contents($src_file) and if $content === false throw a RuntimeException before calling \str_replace; after file_put_contents($dst_file, $content) verify the return value !== false and throw if it failed; optionally ensure the target directory (derived from pathinfo($dst_file)) exists and is writable before writing and include these checks in the same cloneFile function to avoid silent failures.
38-55:⚠️ Potential issue | 🟠 MajorHandle directory-open failures and use explicit
readdir()comparison.
opendir()can returnfalse, andwhile ($file = readdir($dir))stops prematurely if any filename is the string"0"(which evaluates falsey in PHP). Either case leaves the clone incomplete or fails silently.🛠️ Proposed fix
- $dir = \opendir($src_path); + $dir = \opendir($src_path); + if (false === $dir) { + throw new \RuntimeException("Unable to open source directory: {$src_path}"); + } // Make the destination directory if not exist - @\mkdir($dst_path); + if (!\is_dir($dst_path) && !@\mkdir($dst_path) && !\is_dir($dst_path)) { + throw new \RuntimeException("Unable to create destination directory: {$dst_path}"); + } // Loop through the files in source directory - while ($file = \readdir($dir)) { + while (false !== ($file = \readdir($dir))) {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@class/Files/CreateClone.php` around lines 38 - 55, The cloneFileFolder function currently assumes opendir($src_path) succeeds and uses a non-strict while ($file = readdir($dir)) which breaks on filenames equal to "0"; update cloneFileFolder to check opendir’s return (store into $dir and if $dir === false throw or log an error and return) and change the loop to a strict comparison while (false !== ($file = readdir($dir))) to ensure "0" is handled; also ensure `@mkdir` result is checked (or create directory with proper error handling) and call closedir($dir) only when $dir is a valid resource.class/Modules.php (1)
97-107:⚠️ Potential issue | 🔴 CriticalAdd the required
: mixedreturn type to__call().Once you declare parameter types on the magic method
__call, PHP 8.3 requires the complete signature including the return type. Missing: mixedcauses a fatal compatibility error.Proposed fix
- public function __call(string $method, array $args) + public function __call(string $method, array $args): mixed🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@class/Modules.php` around lines 97 - 107, The magic method __call currently declares parameter types but lacks the required return type causing PHP 8.3 compatibility errors; update the method signature for __call to include the return type : mixed so it matches PHP's required complete signature (modify the public function __call(string $method, array $args) declaration to return mixed) and ensure the body still returns $this->getVar($method, $arg) accordingly.class/Files/User/UserPrint.php (1)
135-158:⚠️ Potential issue | 🔴 CriticalRead publish/expiry fields from the loaded object, not the handler.
The generated guards still call
getVar()on\${$tableName}Handlerforpublished/expired/date/timefields. Those are row fields, so modules with one of these columns will fatal before the object validation path can help. Move those checks below the\${$tableName}Objfetch or switch them to\${$tableName}Obj->getVar(...).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@class/Files/User/UserPrint.php` around lines 135 - 158, The guard checks for publish/expiry/date/time currently call getVar() on \${$tableName}Handler (in the block with mb_strpos checks), which causes a fatal for row fields; move those checks to operate on the loaded object instead: either relocate the entire mb_strpos-based condition blocks so they execute after the call to $this->xc->getXcHandlerGet(...)/$this->pc->getPhpCodeIsobject(...) that produces \${$tableName}Obj, or change the conditions to use \${$tableName}Obj->getVar(...) rather than \${$tableName}Handler->getVar(...). Ensure you keep the same redirectHeader/redirectError behavior and use the same pc->getPhpCodeConditions(...) and getSimpleString('exit();') calls but referencing \${$tableName}Obj where appropriate.class/Files/CreateStructure.php (1)
182-196:⚠️ Potential issue | 🟠 MajorFix the inverted
isDirEmpty()contract.This helper returns
trueas soon as it finds any child entry, so its name and behavior are currently opposite.\opendir()is also unchecked, which turns an unreadable path into a runtime error on\readdir().Proposed fix
- public function isDirEmpty(string $dir) + public function isDirEmpty(string $dir): bool { - $content = []; - $handle = \opendir($dir); - while (false !== ($entry = \readdir($handle))) { - if ('.' !== $entry && '..' !== $entry) { - $content[] = $entry; - } - } - \closedir($handle); - if (\count($content) > 0) { - return true; - } - - return false; + $handle = \opendir($dir); + if (false === $handle) { + throw new \RuntimeException(\sprintf('Directory "%s" cannot be opened', $dir)); + } + while (false !== ($entry = \readdir($handle))) { + if ('.' !== $entry && '..' !== $entry) { + \closedir($handle); + return false; + } + } + \closedir($handle); + return true; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@class/Files/CreateStructure.php` around lines 182 - 196, The isDirEmpty method currently returns true when it finds any entry (inverted logic) and calls \opendir() without checking for failure; change it so that it returns true only when the directory has no entries. Specifically, in isDirEmpty(string $dir) check if $handle = \opendir($dir) is false and handle that case (e.g., return true or false according to project policy — choose return true to treat unreadable paths as empty), then iterate with \readdir and on finding the first non '.'/ '..' entry immediately return false (not true); after closing the handle return true. This fixes both the inverted contract and the unchecked opendir call.
🟠 Major comments (22)
class/Files/CreateCodeIndents.php-13-13 (1)
13-13:⚠️ Potential issue | 🟠 MajorGuard against negative
$tabsbeforestr_repeatto avoid runtimeValueError.At Line 13,
str_repeat(' ', $tabs)will throw on negative$tabsin PHP, which is a behavioral regression from the previous loop-based implementation.Proposed fix
function nl(int $tabs = 0) { + if ($tabs < 0) { + $tabs = 0; + } return "\n" . str_repeat(' ', $tabs); }#!/bin/bash # Check for potential negative arguments passed to nl() rg -nP --type=php '\bnl\s*\(\s*-\d+' -C2 # Inspect nl() call sites for non-literal values that may become negative rg -nP --type=php '\bnl\s*\(' -C2🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@class/Files/CreateCodeIndents.php` at line 13, The nl() implementation in CreateCodeIndents.php calls str_repeat(' ', $tabs) and will throw a ValueError for negative $tabs; guard against that by normalizing $tabs before calling str_repeat (e.g., cast to int and clamp with max(0, $tabs) or early-return the newline if $tabs is negative) so nl() never passes a negative count to str_repeat; update the function that returns "\n" . str_repeat(' ', $tabs) to sanitize $tabs and keep the rest of the behavior unchanged.class/Files/admin/AdminAbout.php-96-96 (1)
96-96:⚠️ Potential issue | 🟠 MajorEscape generated PayPal value before embedding into generated PHP.
Line 96 injects
$moduleDonationsdirectly into a single-quoted PHP literal. If the value contains', generated code can break (or allow code injection into generated output).🛠️ Safer generation patch
- $content .= $this->getSimpleString("\$adminObject->setPaypal('$moduleDonations');"); + $moduleDonationsLiteral = var_export((string)$moduleDonations, true); + $content .= $this->getSimpleString("\$adminObject->setPaypal({$moduleDonationsLiteral});");🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@class/Files/admin/AdminAbout.php` at line 96, The generated PHP currently injects $moduleDonations directly into a single-quoted literal which breaks if it contains a single-quote or backslash; change the generation to produce a safe PHP string literal (e.g. use var_export($moduleDonations, true) or explicitly escape quotes/backslashes) before embedding into the call built by getSimpleString so the resulting line becomes a valid, escaped PHP literal for \$adminObject->setPaypal(...); locate the construction that uses getSimpleString in AdminAbout.php and replace the direct '$moduleDonations' insertion with a safe-escaped/var_exported representation.class/Files/Classes/ClassHandlerFiles.php-101-101 (1)
101-101:⚠️ Potential issue | 🟠 MajorPotential null passed to
stringparameter.
$fieldIdand$fieldMainare initialized asnullat lines 338-339 and may remainnullif their conditions aren't met:
$fieldIdis only assigned if$f == 0ANDtable_autoincrement == 1(line 344-345)$fieldMainis only assigned if a field hasfield_main == 1(line 347-348)Passing
nullto thestring-typed parameters at line 101 will cause aTypeErrorin PHP 8+.Proposed fix: Use nullable types
- private function getClassObjectHandler(string $moduleDirname, $table, string $fieldId, string $fieldMain, $fieldParentId, $fieldElement) + private function getClassObjectHandler(string $moduleDirname, $table, ?string $fieldId, ?string $fieldMain, $fieldParentId, $fieldElement)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@class/Files/Classes/ClassHandlerFiles.php` at line 101, The method signature for getClassObjectHandler currently types $fieldId and $fieldMain as string but those variables can remain null earlier in the code; change the signature of getClassObjectHandler to accept nullable strings (e.g., ?string $fieldId, ?string $fieldMain) and update any internal logic to handle null values safely (or coerce to empty string before use) so callers that pass null won't trigger a TypeError; reference the function name getClassObjectHandler and the parameters $fieldId and $fieldMain when making this change and verify/update any call sites that assume non-null inputs.class/Helper.php-45-45 (1)
45-45: 🛠️ Refactor suggestion | 🟠 MajorAdd matching type hint to constructor for consistency.
The
getInstancemethod enforces abooltype hint, but the constructor at line 33 still accepts any type for$debug. This creates an inconsistent contract where the static factory is stricter than the constructor it invokes. Since both are part of the singleton initialization contract, they should have matching signatures.♻️ Proposed fix
- public function __construct($debug = false) + public function __construct(bool $debug = false)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@class/Helper.php` at line 45, The constructor signature for Helper is missing a bool type hint for $debug while getInstance(bool $debug = false) enforces it; update the Helper::__construct to accept bool $debug (e.g., change parameter to bool $debug = false) so both the static factory getInstance and the instance constructor have matching type contracts, and update any constructor docblock accordingly.class/Files/Sql/SqlFile.php-113-113 (1)
113-113:⚠️ Potential issue | 🟠 MajorCast
$fieldsNumbtointat line 152 to preventTypeErrorunder strict types.With
declare(strict_types=1)enabled, theint $fieldsNumbparameter requirement at line 113 is enforced strictly. SincegetVar('table_nbfields')is an external XOOPS method that may return a string rather than an integer, the call at line 195 will throw aTypeErrorif the value is not explicitly cast.Suggested fix
- $fieldsNumb = $tables[$t]->getVar('table_nbfields'); + $fieldsNumb = (int)$tables[$t]->getVar('table_nbfields');🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@class/Files/Sql/SqlFile.php` at line 113, The getHeadDatabaseTable method's $fieldsNumb parameter (function getHeadDatabaseTable) can receive a non-integer from getVar('table_nbfields'), so cast the value to int at the call site before passing it (e.g., replace usages of getVar('table_nbfields') with (int) getVar('table_nbfields') where getHeadDatabaseTable is invoked) to avoid a TypeError under strict types; ensure any other calls that forward table_nbfields also perform the explicit (int) cast.class/Common/VersionChecks.php-47-50 (1)
47-50:⚠️ Potential issue | 🟠 MajorAdd missing comparison operator to versionCompare call (Line 47).
The method call is missing the
'<'operator, causing incorrect version validation. WhencurrentVer > requiredVer, the method returns1(truthy), incorrectly setting an error for valid higher versions. Use explicit'<'operator to match the pattern used elsewhere in this file (lines 77, 134) and in the reference template.Suggested fix
- if ($module->versionCompare($currentVer, $requiredVer)) { + if ($module->versionCompare($currentVer, $requiredVer, '<')) { $success = false; $module->setErrors(\sprintf(\constant('CO_' . $moduleDirNameUpper . '_ERROR_BAD_XOOPS'), $requiredVer, $currentVer)); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@class/Common/VersionChecks.php` around lines 47 - 50, The call to module->versionCompare is missing the comparison operator argument, causing higher currentVer to be treated as an error; update the version check in VersionChecks.php to call module->versionCompare($currentVer, $requiredVer, '<') so it only flags versions where currentVer is less than requiredVer and keep the existing error construction using moduleDirNameUpper and setErrors unchanged.class/Form/FormRadio.php-34-35 (1)
34-35:⚠️ Potential issue | 🟠 MajorAdd explicit string normalization before HTML escaping to prevent TypeError with strict_types and PHP 8.0+.
With
declare(strict_types=1), the empty string checks at lines 44 and 47 are insufficient. A loose comparison'' != $valueevaluates to true when$valueisnull, causinghtmlspecialchars(null, ENT_QUOTES)to be called, which raisesTypeErrorin PHP 8.0+. Reintroduce explicit (string) casts to ensure non-string values are converted before reachinghtmlspecialchars().Proposed fix
- $ele_title = $this->getTitle(); - $ele_value = $this->getValue(); + $ele_title = (string)$this->getTitle(); + $ele_value = (string)$this->getValue();🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@class/Form/FormRadio.php` around lines 34 - 35, The empty-string checks before calling htmlspecialchars() can still pass non-string values (e.g. null) under strict_types and cause a TypeError; update the handling of $ele_title and $ele_value (from getTitle() and getValue()) to normalize to strings before HTML-escaping by explicitly casting to (string) or using a string coalesce so that htmlspecialchars() always receives a string (apply this normalization where htmlspecialchars(...) is invoked for title/value in FormRadio).class/Utility.php-74-82 (1)
74-82:⚠️ Potential issue | 🟠 MajorUse defensive check for
XOOPS_LICENSE_KEYconstant (Line 79) to handle incomplete XOOPS installations.
XOOPS_LICENSE_KEYis a framework constant generated during XOOPS installation and stored in/include/license.php. While it should be available at runtime in a properly installed XOOPS system, if the license key file is missing or not included during bootstrap, this code will trigger a fatal error in PHP 8+. Add a defensive fallback:Suggested fix
- \XOOPS_LICENSE_KEY, + \defined('XOOPS_LICENSE_KEY') ? \XOOPS_LICENSE_KEY : '',🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@class/Utility.php` around lines 74 - 82, The code in Utility.php that formats $donationform (inside the block checking if $key == 2) directly uses the XOOPS_LICENSE_KEY constant which can be undefined in incomplete installations; update that sprintf argument to use a defensive fallback (e.g., use defined('XOOPS_LICENSE_KEY') ? XOOPS_LICENSE_KEY : '' or a configurable default) so the string interpolation never triggers an undefined constant/fatal error; locate the $donationform[$key] sprintf call in the Utility class and replace the direct XOOPS_LICENSE_KEY reference with the conditional fallback.class/Common/TestdataButtons.php-44-44 (1)
44-44:⚠️ Potential issue | 🟠 MajorPHPStan configuration missing Xmf class discovery.
At Line 44,
\Xmf\Module\Admincannot be resolved by PHPStan because the config lacks autoload or stub configuration for the external Xmf dependency. While Xmf classes are used throughout the codebase (Xmf\Request, Xmf\Yaml, Xmf\Module\Helper, etc.), PHPStan has no mechanism to discover them. Update phpstan.neon to include Xmf class discovery (via autoload, bootstrapFiles, or stub configuration) to prevent analysis failures if PHPStan is enforced in CI.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@class/Common/TestdataButtons.php` at line 44, PHPStan cannot resolve the external Xmf classes (e.g., \Xmf\Module\Admin referenced in loadButtonConfig) because phpstan.neon lacks autoload/bootstrap/stub configuration for the Xmf library; update phpstan.neon to register Xmf class discovery by adding an appropriate autoload/bootstrap entry (bootstrapFiles or autoload_files) or pointing to a generated stub file for the Xmf namespace so PHPStan can find classes like Xmf\Request, Xmf\Yaml, Xmf\Module\Helper and resolve loadButtonConfig’s type hints during analysis.class/Common/FileChecker.php-147-158 (1)
147-158:⚠️ Potential issue | 🟠 MajorUndefined variables if POST parameters are missing.
Same issue as in
DirectoryChecker.php:$original_file_path,$file_path, and$redirectare conditionally set but always used. This will cause runtime errors when POST vars are missing.🛡️ Proposed fix with early validation
if ($op == 'copyfile') { - if (\Xmf\Request::hasVar('original_file_path', 'POST')) { - $original_file_path = $_POST['original_file_path']; - } - if (\Xmf\Request::hasVar('file_path', 'POST')) { - $file_path = $_POST['file_path']; - } - if (\Xmf\Request::hasVar('redirect', 'POST')) { - $redirect = $_POST['redirect']; - } + if (!\Xmf\Request::hasVar('original_file_path', 'POST') + || !\Xmf\Request::hasVar('file_path', 'POST') + || !\Xmf\Request::hasVar('redirect', 'POST')) { + return; + } + $original_file_path = $_POST['original_file_path']; + $file_path = $_POST['file_path']; + $redirect = $_POST['redirect']; $msg = FileChecker::copyFile($original_file_path, $file_path) ? \constant('CO_' . $moduleDirNameUpper . '_' . 'FC_FILECOPIED') : \constant('CO_' . $moduleDirNameUpper . '_' . 'FC_FILENOTCOPIED'); \redirect_header($redirect, 2, $msg . ': ' . $file_path); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@class/Common/FileChecker.php` around lines 147 - 158, The branch handling op == 'copyfile' in class FileChecker.php uses $original_file_path, $file_path and $redirect without guaranteeing they exist, causing undefined variable errors; update the handler to validate required POST params before use (use \Xmf\Request::hasVar or getString/getRaw to read originals), return or redirect with a clear error if original_file_path or file_path are missing, and ensure $redirect has a safe default before calling FileChecker::copyFile and redirect_header; reference the op == 'copyfile' block, variables $original_file_path, $file_path, $redirect, and the FileChecker::copyFile call when making the fix.class/Common/FileChecker.php-45-45 (1)
45-45:⚠️ Potential issue | 🟠 MajorRequired parameter after optional parameter is deprecated in PHP 8.0+.
string $redirectFileis required but comes afterstring $original_file_path = nullwhich is optional. This causes a deprecation notice and may lead to unexpected behavior. Additionally, the check on line 52if (null === $redirectFile)can never be true since$redirectFileis typed asstring.🐛 Proposed fix: make redirectFile nullable or reorder parameters
- public static function getFileStatus(string $file_path, string $original_file_path = null, string $redirectFile) + public static function getFileStatus(string $file_path, ?string $original_file_path = null, ?string $redirectFile = null)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@class/Common/FileChecker.php` at line 45, The function signature getFileStatus currently declares a required string $redirectFile after an optional $original_file_path which triggers a PHP deprecation; change the signature to accept a nullable redirect (e.g. ?string $redirectFile = null) or reorder parameters so the required param is first — I recommend making $redirectFile nullable to minimize callsite changes; update the internal null check (the existing if (null === $redirectFile)) to be valid with the nullable type and audit usages/call sites of getFileStatus to handle a null $redirectFile accordingly (look for getFileStatus, $redirectFile, and any callers in the codebase).class/Files/CreatePhpCode.php-797-800 (1)
797-800:⚠️ Potential issue | 🟠 MajorThe parameter type should accept flag constants and expressions, not just boolean values.
The
booltype hint for$entitiesQuoteand$specialQuoteprevents callers from passing the flags thathtmlentities()andhtmlspecialchars()require (e.g.,ENT_QUOTES,ENT_COMPAT). Since these functions generate PHP code strings, the parameters should acceptint(for flag constants) andstring(for flag expressions), notbool.🔧 Suggested fix
- public function getPhpCodeHtmlentities($entitiesVar, bool $entitiesQuote = false) + public function getPhpCodeHtmlentities($entitiesVar, int|string|false $entitiesQuote = false) { $entitiesVar = (false !== $entitiesQuote) ? $entitiesVar . ', ' . $entitiesQuote : $entitiesVar; return "htmlentities({$entitiesVar})"; } - public function getPhpCodeHtmlspecialchars($specialVar, bool $specialQuote = false) + public function getPhpCodeHtmlspecialchars($specialVar, int|string|false $specialQuote = false) { $specialVar = (false !== $specialQuote) ? $specialVar . ', ' . $specialQuote : $specialVar; return "htmlspecialchars({$specialVar})"; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@class/Files/CreatePhpCode.php` around lines 797 - 800, Change the type for the quote/flags parameters from bool to allow flag constants or expressions: update getPhpCodeHtmlentities($entitiesVar, bool $entitiesQuote = false) to accept int|string|false (e.g. getPhpCodeHtmlentities($entitiesVar, int|string|false $entitiesQuote = false)) and do the same for getPhpCodeSpecialchars($specialVar, bool $specialQuote = false). Keep the existing concatenation logic (append ', ' . $entitiesQuote / $specialQuote only when not false) so callers can pass integer flag constants (ENT_QUOTES) or string expressions without being blocked by a bool type hint.class/LogoGenerator.php-31-31 (1)
31-31:⚠️ Potential issue | 🟠 MajorGuard the new
stringhelper againstfalsereturns.Line 41 passes
LogoGenerator::createLogo()intophpFunction(), butcreateLogo()returnsfalse|string. Withstrict_types=1, the newstring $valparameter turns those existing failure paths (lines 60, 65, 80) into aTypeError.🐛 Proposed fix
if (\function_exists($myfunction)) { $ret = \XoopsModules\Modulebuilder\LogoGenerator::createLogo($_GET['iconName'], $_GET['caption']); - phpFunction($ret); + if (false !== $ret) { + phpFunction($ret); + } } else { echo 'Method Not Exist'; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@class/LogoGenerator.php` at line 31, The new phpFunction(string $val = '') signature is incompatible with LogoGenerator::createLogo() which can return false|string, causing a TypeError; change phpFunction to accept the same union (string|false $val = '') or loosen the type (mixed) and then explicitly guard/normalize the false case inside phpFunction (e.g., return early or convert false to an empty string) so existing failure paths from LogoGenerator::createLogo() no longer throw when passed into phpFunction.class/Files/CreateArchitecture.php-589-595 (1)
589-595:⚠️ Potential issue | 🟠 MajorThis still collapses multiple comment tables down to one.
Lines 589-593 keep overwriting
$table, so the comment callbacks andcomment_functions.phpbelow are generated against only the last table with comments enabled. Either enforce a single comment-enabled table here or makeIncludeComments/IncludeCommentFunctionsdispatch per table.💡 Minimal guard if only one table should support comments
- foreach (\array_keys($tables) as $t) { - if (1 == (int)$tables[$t]->getVar('table_comments')) { - $table = $tables[$t]; - } - } + $commentTables = []; + foreach ($tables as $candidateTable) { + if (1 === (int)$candidateTable->getVar('table_comments')) { + $commentTables[] = $candidateTable; + } + } + if (1 !== \count($commentTables)) { + throw new \RuntimeException('Exactly one table with comments must be selected.'); + } + $table = $commentTables[0];🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@class/Files/CreateArchitecture.php` around lines 589 - 595, The loop currently overwrites $table so only the last comment-enabled table is used; update CreateArchitecture.php to either enforce a single comment table by collecting tables with getVar('table_comments')==1, and if more than one exists throw or choose the first before calling $includeComments->write($module, $table) and renderCommentsIncludes, or (preferred) change the logic to iterate the collected comment tables and call $includeComments->write($module, $table) and $ret[] = $includeComments->renderCommentsIncludes($module, 'comment_edit') for each table so IncludeComments and IncludeCommentFunctions are dispatched per table rather than only for the last one.class/Files/User/UserSearch.php-129-135 (1)
129-135:⚠️ Potential issue | 🟠 Major
getUserSearch()now generates a no-op search body.Line 134 only emits
$keywords = [];, andrender()has no other search handling between the header and footer. That leaves the generatedsearch.phpwithout any request parsing or result generation, so this generator now produces a dead search endpoint.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@class/Files/User/UserSearch.php` around lines 129 - 135, getUserSearch() currently returns only "$keywords = [];" which produces a no-op search page; replace that stub with a real search handler: parse incoming request parameters (e.g., 'q' or 'keywords' from $_GET/$_POST), sanitize and populate the $keywords array, run the application search/query (e.g., call the existing user search/query method such as User::search or $this->searchUsers with $keywords), capture the results into a variable used by render(), and ensure the generated body assigns the results so render() can output them between the header and footer.class/Files/Templates/User/Defstyle/PagesItem.php-251-267 (1)
251-267:⚠️ Potential issue | 🟠 MajorFooter images disappear for
FIELD_ELE_IMAGELISTandFIELD_ELE_UPLOADIMAGE.Lines 257 and 266 comment out the only append to
$retFoot, so footer fields of those types now render nothing even though$imgis still built. If those field types are allowed infield_tfoot, this is a user-visible regression in the generated item template.Suggested fix
- //$retElem .= $this->hc->getHtmlSpan($img, 'block-pie justify', "\t"); + $retFoot .= $this->hc->getHtmlSpan($img, 'block-pie justify', "\t"); ... - //$retElem .= $this->hc->getHtmlSpan($img, 'block-pie justify',"\t"); + $retFoot .= $this->hc->getHtmlSpan($img, 'block-pie justify', "\t");🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@class/Files/Templates/User/Defstyle/PagesItem.php` around lines 251 - 267, The footer image cases (Constants::FIELD_ELE_IMAGELIST and Constants::FIELD_ELE_UPLOADIMAGE) build $img but the lines that append it to $retFoot are commented out, removing footer output; restore the append by calling $retFoot .= $this->hc->getHtmlSpan($img, 'block-pie justify', "\t") (or the existing span format used elsewhere) inside both case blocks before unset($img) so $retFoot receives the generated image; reference the case blocks, $retFoot, $img, and getHtmlSpan/$this->hc to locate where to re-add the lines.class/SplClassLoader.php-84-86 (1)
84-86:⚠️ Potential issue | 🟠 MajorKeep
setIncludePath()nullable.
_includePathis nullable throughout the class: the constructor acceptsnull(line 55),getIncludePath()declares return typestring|null(line 92), andloadClass()explicitly checksif (null !== $this->_includePath)(line 152). TighteningsetIncludePath()to requirestringremoves the public way to restore null and breaks API consistency.Suggested fix
- public function setIncludePath(string $includePath): void + public function setIncludePath(?string $includePath): void🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@class/SplClassLoader.php` around lines 84 - 86, The setIncludePath method currently requires a non-null string but _includePath is treated as nullable elsewhere (constructor accepts null, getIncludePath returns ?string, loadClass checks for null); update setIncludePath to accept a nullable string (change its parameter to ?string) and assign it directly to $this->_includePath so callers can restore null and maintain API consistency with __construct, getIncludePath, and loadClass.class/Files/Templates/Admin/TemplatesAdminPages.php-250-250 (1)
250-250:⚠️ Potential issue | 🟠 MajorInconsistency: CSS class change needs alignment across all templates.
The table class change from Bootstrap-style
'table table-bordered'to'outer'appears only in this file. Other template generators still use'table table-bordered'(TemplatesAdminBroken.php, UserPrint.php), and the custom'outer'CSS class is not defined anywhere in the codebase. This inconsistency may break table styling or indicates an incomplete migration.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@class/Files/Templates/Admin/TemplatesAdminPages.php` at line 250, The return call using $this->hc->getHtmlTable($tbody, 'outer', "\t") introduces an undefined CSS class "outer" and diverges from other generators; either revert the second argument to the existing Bootstrap classes used elsewhere (e.g., "table table-bordered") in TemplatesAdminPages.php to match TemplatesAdminBroken.php and UserPrint.php, or add a concrete CSS definition for .outer in the global stylesheet; update the call site in TemplatesAdminPages.php (the $this->hc->getHtmlTable invocation) and ensure any other template generators remain consistent with the chosen approach.class/Files/User/UserPdf.php-203-206 (1)
203-206:⚠️ Potential issue | 🟠 MajorDon't overwrite the record-derived PDF subject.
pdfData['subject']is populated from the row above, but this block unconditionally replaces it with the site slogan. If the slogan is only a fallback, guard this assignment withempty($pdfData['subject']).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@class/Files/User/UserPdf.php` around lines 203 - 206, The code unconditionally replaces the record-derived $pdfData['subject'] via the call to $this->xc->getXcEqualsOperator(" $pdfData['subject'] ", "$GLOBALS['xoopsConfig']['slogan']"); change this to only assign the slogan when the subject is empty: wrap the assignment in an empty($pdfData['subject']) guard (or the equivalent conditional) so the existing subject from the record is preserved; update the block in the UserPdf class where the $ret lines are built (the lines calling getXcEqualsOperator for 'subject') to use that conditional check.class/Files/User/UserPdf.php-124-126 (1)
124-126:⚠️ Potential issue | 🟠 MajorRemove the leaked
wgtestmbtemplate variable.This generator now emits
wgtestmb_upload_urlfor every module. That bakes a specific sample module name into all generated PDF pages instead of using the current$moduleDirname.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@class/Files/User/UserPdf.php` around lines 124 - 126, Replace the hardcoded template key 'wgtestmb_upload_url' emitted in the getSimpleString call so it uses the current module name instead of the sample module; in the line that calls $this->cf->getSimpleString("\$pdfTpl->assign('wgtestmb_upload_url', \\" . strtoupper($moduleDirname) . '_UPLOAD_URL);'); change the first argument to build the assign key from $moduleDirname (e.g. use strtolower($moduleDirname) . '_upload_url' or similar consistent casing) so the template variable is generated dynamically rather than always 'wgtestmb_upload_url'.class/Files/CreateStructure.php-265-272 (1)
265-272:⚠️ Potential issue | 🟠 MajorFail fast when a copy operation fails.
Ignoring
\copy()here lets generation continue with a missing file and no exception.Proposed fix
- \copy($fromFile, $fname); + if (!\copy($fromFile, $fname)) { + throw new \RuntimeException(\sprintf('Failed to copy "%s" to "%s"', $fromFile, $fname)); + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@class/Files/CreateStructure.php` around lines 265 - 272, The setCopy method currently ignores the boolean result of \copy() which can allow generation to continue with missing files; update setCopy to check the return value of \copy($fromFile, $fname) and if it returns false throw a descriptive exception (e.g., RuntimeException) including the source ($fromFile) and destination ($fname) so callers fail fast; keep the existing directory creation/chmod logic (makeDir and chmod) but ensure any failures that prevent copying are reported via the thrown exception from setCopy.class/Devtools.php-504-505 (1)
504-505:⚠️ Potential issue | 🟠 MajorFix type signature for custom action URL support in these form helpers.
With
strict_types=1enabled, these four methods declare$actionasboolbut internally treat it as a string—passing it to\XoopsSimpleFormas a form action URL. This type mismatch prevents callers from passing custom action URLs, even though the parameter logic supports them via thestring|falseunion.Update all four method signatures:
Proposed fix
- public static function getFormModulesFq(bool $action = false) + public static function getFormModulesFq(string|false $action = false) - public static function getFormModulesCl(bool $action = false) + public static function getFormModulesCl(string|false $action = false) - public static function getFormModulesTab(bool $action = false) + public static function getFormModulesTab(string|false $action = false) - public static function getFormModulesRemovePrefix($dst_path, bool $action = false) + public static function getFormModulesRemovePrefix($dst_path, string|false $action = false)Also applies to: 533-534, 562-563, 591-592
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@class/Devtools.php` around lines 504 - 505, The method getFormModulesFq (and the three sibling form helper methods noted in the comment) declare the $action parameter as bool but treat it as a form action URL; change each method signature to accept string|false $action = false (i.e. replace bool $action = false with string|false $action = false) and update any corresponding PHPDoc/type hints so callers can pass a custom action URL while preserving the default false behavior when omitted.
There was a problem hiding this comment.
Actionable comments posted: 10
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (7)
class/Files/CreateStructure.php (1)
199-217:⚠️ Potential issue | 🟡 MinorPHPDoc visibility mismatch:
@publicbut method isprivate.The PHPDoc comment says
@public function addFolderPathbut the method is declared asprivate. This is misleading and should be corrected.Proposed fix
/** - * `@public` function addFolderPath + * `@private` function addFolderPath * * `@param` string $folderName * `@param` bool|string $fileName * `@return` string */ private function addFolderPath(string $folderName, $fileName = false)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@class/Files/CreateStructure.php` around lines 199 - 217, The PHPDoc for addFolderPath incorrectly states "@public" while the method is declared private; update the docblock for the addFolderPath method in CreateStructure (function addFolderPath) so the visibility annotation matches (change "@public" to "@private" or remove the visibility tag), and ensure the summary and param/return annotations remain accurate for the private method.class/Files/CreateClone.php (1)
40-55:⚠️ Potential issue | 🟠 MajorUnchecked
opendir()return value creates inconsistent error handling.
opendir()at line 41 can returnfalseon failure, but the code proceeds to use$dirin thewhileloop andclosedir()without checking. This is inconsistent with the new exception-throwing pattern added incloneFile().Proposed fix
{ // open the source directory - $dir = \opendir($src_path); + $dir = \opendir($src_path); + if (false === $dir) { + throw new \RuntimeException("Unable to open source directory: {$src_path}"); + } // Make the destination directory if not exist - @\mkdir($dst_path); + if (!\is_dir($dst_path) && !\mkdir($dst_path, 0755) && !\is_dir($dst_path)) { + throw new \RuntimeException("Unable to create destination directory: {$dst_path}"); + } // Loop through the files in source directory🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@class/Files/CreateClone.php` around lines 40 - 55, The opendir() call in CreateClone::cloneFileFolder (the variable $dir) is not checked for failure before being used and closed; update cloneFileFolder to verify $dir !== false (or is_resource($dir)) right after calling \opendir($src_path) and throw an exception (matching the pattern used in cloneFile) with a clear message including $src_path if opendir fails, and only enter the readdir loop and call \closedir($dir) when $dir is a valid resource to avoid using a false value.class/Modules.php (1)
363-393:⚠️ Potential issue | 🟠 MajorRemove unused private static method
createLogo(lines 363-393).This method is unreachable—it's
private staticwith no internal calls within the class. Other publiccreateLogoimplementations exist inLogo.phpandLogoGenerator.phpand are properly invoked; this one is dead code and should be removed.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@class/Modules.php` around lines 363 - 393, Delete the unused private static method createLogo from the Modules class: remove the entire private static function createLogo($logoIcon, string $moduleDirname) { ... } block (lines shown in the diff) since it is dead code and duplicates implementations in Logo.php and LogoGenerator.php; after removal, search for any references to Modules::createLogo or createLogo within the project to confirm no callers remain and run tests/lint to ensure no regressions.class/Common/DirectoryChecker.php (2)
102-107:⚠️ Potential issue | 🔴 CriticalFix the inverted success condition in
createDirectory().Line 107 negates
mkdir()/is_dir(), so a successful create returnsfalseand some failures can returntrue. Thecreatedirflow will report the wrong outcome to admins.Suggested fix
- return \is_dir($target) || (self::createDirectory(\dirname($target), $mode) && !\mkdir($target, $mode) && !\is_dir($target)); + return \is_dir($target) + || ( + self::createDirectory(\dirname($target), $mode) + && (\mkdir($target, $mode) || \is_dir($target)) + );🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@class/Common/DirectoryChecker.php` around lines 102 - 107, The createDirectory() return logic is inverted: it negates mkdir()/is_dir() so successful directory creation yields false; fix by changing the compound condition after the recursive call so it returns true when mkdir succeeds or the directory exists (race-safe). Specifically update the return expression in createDirectory() to use self::createDirectory(dirname($target), $mode) && (mkdir($target, $mode) || is_dir($target)) (preserving the initial is_dir($target) short-circuit) so successful creation returns true.
145-156:⚠️ Potential issue | 🟠 MajorFail fast when
setdirpermreceives incomplete or malformed POST data.Lines 146-155 only assign
$path,$redirect, and$modeconditionally, but Line 155 always uses them. A partial request will trigger notices, and non-scalar input can still leak into the chmod path.Suggested fix
case 'setdirperm': - if (\Xmf\Request::hasVar('path', 'POST')) { - $path = $_POST['path']; - } - if (\Xmf\Request::hasVar('redirect', 'POST')) { - $redirect = $_POST['redirect']; - } - if (\Xmf\Request::hasVar('mode', 'POST')) { - $mode = $_POST['mode']; - } - $msg = DirectoryChecker::setDirectoryPermissions($path, (int)$mode) ? \constant('CO_' . $moduleDirNameUpper . '_' . 'DC_PERMSET') : \constant('CO_' . $moduleDirNameUpper . '_' . 'DC_PERMNOTSET'); + if ( + !isset($_POST['path'], $_POST['redirect'], $_POST['mode']) + || !\is_string($_POST['path']) + || !\is_string($_POST['redirect']) + || !\is_scalar($_POST['mode']) + ) { + break; + } + $path = $_POST['path']; + $redirect = $_POST['redirect']; + $mode = (int)$_POST['mode']; + $msg = DirectoryChecker::setDirectoryPermissions($path, $mode) ? \constant('CO_' . $moduleDirNameUpper . '_' . 'DC_PERMSET') : \constant('CO_' . $moduleDirNameUpper . '_' . 'DC_PERMNOTSET');🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@class/Common/DirectoryChecker.php` around lines 145 - 156, The case 'setdirperm' block uses $path, $mode, and $redirect without guaranteeing valid POST input; add a fail-fast validation: require all three POST params present and scalar (use \Xmf\Request::hasVar and \Xmf\Request::getString/getInt or equivalent) and if any are missing or non-scalar immediately call \redirect_header with a clear error message (use CO_{moduleDirNameUpper}_DC_PERMNOTSET or a new error constant) to avoid notices and unsafe values being passed to DirectoryChecker::setDirectoryPermissions; ensure $mode is cast to int and $path/$redirect are sanitized/strings before calling DirectoryChecker::setDirectoryPermissions and redirect_header.class/Common/TableChecker.php (1)
74-80:⚠️ Potential issue | 🔴 CriticalDon’t overwrite
$this->resultin the create-table branch.Line 79 replaces the result array with the query result, so the next
$this->result[]append will blow up as soon as update mode tries to create a missing table.Suggested fix
- if ($this->result = $GLOBALS['xoopsDB']->queryF($sql)) { + if ($GLOBALS['xoopsDB']->queryF($sql)) {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@class/Common/TableChecker.php` around lines 74 - 80, The bug is that in the create-table branch you overwrite the results array by assigning $this->result = $GLOBALS['xoopsDB']->queryF($sql); — change this to capture the query result in a temporary/local variable (e.g. $res = $GLOBALS['xoopsDB']->queryF($sql)) and then append the creation message to $this->result (e.g. $this->result[] = 'Table created:'.$table) so the results array is preserved; update any subsequent checks that expect the query resource to use the temporary variable instead of $this->result.class/Files/Templates/User/Defstyle/PagesItem.php (1)
73-78:⚠️ Potential issue | 🟡 MinorCorrect the PHPDoc parameter order in
write()method.Lines 73-76 incorrectly document the parameters as
($module, $table, $filename, $tables), but the actual signature at line 78 is($module, $table, $tables, string $filename). Update the PHPDoc to match the implemented parameter order.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@class/Files/Templates/User/Defstyle/PagesItem.php` around lines 73 - 78, The PHPDoc for the write() method has parameters listed in the wrong order; update the docblock above public function write($module, $table, $tables, string $filename): void so the `@param` tags match the actual signature order (module, table, tables, filename) and ensure types/comments align with each parameter name referenced in the write() method.
🧹 Nitpick comments (10)
class/Files/CreateStructure.php (3)
155-163: Uncheckedchmod()return value.Moving
chmod($dname, 0755)outside the conditional is reasonable, butchmod()can fail and returnsfalseon error. Given the new exception-throwing pattern in this PR, this should also be checked for consistency. However, this may be acceptable if permission changes are non-critical.Optional: Add error check for consistency
- chmod($dname, 0755); + if (!\chmod($dname, 0755)) { + // Log warning or throw if permissions are critical + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@class/Files/CreateStructure.php` around lines 155 - 163, The isDir method currently calls chmod($dname, 0755) without checking its return; update isDir to verify chmod succeeded and throw a RuntimeException (similar to the mkdir error handling) when chmod returns false, referencing the same directory name in the message; locate the isDir function and add a conditional around chmod that throws \RuntimeException(sprintf('Failed to change permissions for "%s"', $dname)) if chmod($dname, 0755) === false.
175-197: Good refactor with exception handling and early return.The
isDirEmptymethod now properly handlesopendir()failure by throwing an exception and uses early return when a non-dot entry is found. This is cleaner and more efficient than scanning all entries.Note: The return type
boolis documented in PHPDoc but not declared in the method signature. Consider adding: boolfor consistency with the PR's type-hinting improvements.Optional: Add return type declaration
- public function isDirEmpty(string $dir) + public function isDirEmpty(string $dir): bool🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@class/Files/CreateStructure.php` around lines 175 - 197, The isDirEmpty method currently documents a bool return but lacks a declared return type; update the method signature for isDirEmpty(string $dir) to declare a boolean return (add : bool) so the signature and PHPDoc match the PR's type-hinting improvements, and run static checks to ensure no callers rely on mixed/missing return typing (method name: isDirEmpty, parameter: $dir).
265-275: Uncaught exceptions fromsetCopy()propagate through callers without error handling.The explicit check for
copy()failure withRuntimeExceptionimproves error visibility. The methodscopyFile()andmakeDirAndCopyFile()inCreateStructure.phpare called throughoutsetBaseFoldersFiles()without try/catch blocks. Since these methods invokesetCopy()directly or indirectly, the newRuntimeExceptionwill propagate uncaught up the call chain. The public caller inadmin/building.php(line 117) also lacks exception handling, allowing failures to bubble up to the application level. Consider wrapping these file operations with try/catch or allowing intentional exception propagation if that is the desired behavior.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@class/Files/CreateStructure.php` around lines 265 - 275, setCopy now throws a RuntimeException when copy() fails but callers like copyFile and makeDirAndCopyFile (used by setBaseFoldersFiles) and the public caller in admin/building.php don't catch it, so failures will bubble to the app; update callers to handle the exception: wrap calls to setCopy (and the higher-level copyFile/makeDirAndCopyFile invocations) in try/catch blocks that log a clear error and either recover or rethrow a more specific exception, or alternatively document/propagate the exception intentionally and add a top-level try/catch in admin/building.php around the setBaseFoldersFiles call to present a user-friendly error instead of an uncaught exception.class/Files/CreateClone.php (1)
71-93: Good addition of filesystem error handling; consider a dedicated exception class for better granularity.The explicit checks for
copy(),file_get_contents(), andfile_put_contents()failures withRuntimeExceptionthrows improve error visibility. However, SonarCloud flags the use of genericRuntimeException.For better error handling granularity and to satisfy static analysis, consider creating a dedicated
FilesystemExceptionclass within theXoopsModules\Modulebuilder\Filesnamespace. This allows callers to catch filesystem-specific errors distinctly.Additionally, the callers in
CreateArchitecture.php(setFilesToBuilding,setCommonFiles,CopyRatingFiles) invoke these methods without try/catch blocks—theseRuntimeExceptions will propagate uncaught and may cause unexpected termination during module generation. Wrapping calls or declaring throws in the calling methods would improve robustness.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@class/Files/CreateClone.php` around lines 71 - 93, Replace the generic RuntimeException throws in CreateClone (the file-copy/replace blocks that call copy(), file_get_contents(), file_put_contents()) with a dedicated FilesystemException class in the XoopsModules\Modulebuilder\Files namespace; create that exception (extends \RuntimeException or a more specific SPL exception) and throw it with the same messages from the CreateClone methods. Then update the callers in CreateArchitecture.php (the methods setFilesToBuilding, setCommonFiles, CopyRatingFiles) to wrap their calls to the CreateClone routines in try/catch blocks that catch XoopsModules\Modulebuilder\Files\FilesystemException and handle or log the error (or rethrow) instead of allowing uncaught RuntimeException to propagate. Ensure all references import or fully-qualify the new FilesystemException type.class/Files/Templates/User/Defstyle/Categories.php (2)
98-142: This table path is not on the render flow.
getTemplatesUserCategoriesTable()only feeds the privategetTemplatesUserCategories()helper, butrender()still assembles the output withgetTemplatesUserCategoriesPanel(). If the table layout is the intended result, wirerender()to that path; otherwise these changes stay in dead private code. (raw.githubusercontent.com)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@class/Files/Templates/User/Defstyle/Categories.php` around lines 98 - 142, The new table-building helper getTemplatesUserCategoriesTable(...) is never used by render() (render() still calls getTemplatesUserCategoriesPanel(...) and getTemplatesUserCategories(...)), so your table code is dead; either update render() to call getTemplatesUserCategoriesTable(...) where the table layout should appear (replace the getTemplatesUserCategoriesPanel/getTemplatesUserCategories call with getTemplatesUserCategoriesTable and pass the same $moduleDirname, $tableName, $tableSoleName, $language args) or remove the unused private methods (getTemplatesUserCategoriesTable, getTemplatesUserCategoriesThead, getTemplatesUserCategoriesTbody) to avoid dead code. Ensure you keep parameter names and return types consistent with existing render() expectations when wiring the new method.
85-85: Promote these string contracts to native return types.The private builders and
render()still depend on PHPDoc-onlystringcontracts. Sincerender()just returnsFiles\CreateFile::renderFile(), which already produces a string result, adding native: stringreturns here would make this typing pass enforceable at runtime. Please just confirm no broader interface expects a looserrender()signature before tightening the public method. (raw.githubusercontent.com)Also applies to: 98-98, 113-113, 131-131, 181-181, 202-202, 209-211
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@class/Files/Templates/User/Defstyle/Categories.php` at line 85, Add native string return types to the private builder methods (e.g., getTemplatesUserCategoriesHeader) and to render() so PHP enforces the string contract at runtime; update each method signature to append ": string" (for all private builders referenced at lines 85, 98, 113, 131, 181, 202, 209-211) and keep their bodies unchanged. Before changing the public render() signature, confirm it is not required by any external interface or parent class (render() currently returns Files\CreateFile::renderFile() which produces a string), then add ": string" to render() as well. Ensure all updated methods still return a string in every code path.class/Files/CreateXoopsCode.php (1)
1033-1053: Drop the unused$tableNameparameter fromgetXcXoopsConfirm().The method body does not read it anymore, so every caller now passes dead state and this helper keeps carrying an avoidable warning.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@class/Files/CreateXoopsCode.php` around lines 1033 - 1053, Remove the unused $tableName parameter from the getXcXoopsConfirm method signature and any accompanying docblock in the CreateXoopsCode class; update the function declaration for getXcXoopsConfirm($language, $fieldId, $fieldMain, string $options = 'delete', string $t = '') (keeping the same default values/order for remaining params) and then update all callers to stop passing the extra tableName argument so calls match the new signature; ensure any static/instance invocation sites and related tests are updated to avoid warnings.class/Files/User/UserPages.php (1)
169-169: Use$tconsistently for breadcrumb indentation.These helpers now accept an indent parameter, but the breadcrumb calls still hardcode
"\t\t". Threading$tthrough here would keep the generated output aligned the same way everywhere.Also applies to: 416-416, 439-439, 466-466, 497-497, 529-529
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@class/Files/User/UserPages.php` at line 169, The breadcrumb calls currently pass a hardcoded indent string "\t\t" to uxC->getUserBreadcrumbs; update these to use the existing indentation variable $t instead so breadcrumb output remains consistently indented—replace occurrences like $this->uxc->getUserBreadcrumbs($language, $tableName, 'list', '', "\t\t") with passing $t (e.g., $this->uxc->getUserBreadcrumbs($language, $tableName, 'list', '', $t)); apply the same change for the other getUserBreadcrumbs calls in this class (the ones at the other similar call sites) to ensure consistent use of $t across all breadcrumb invocations.class/Files/Blocks/BlocksFiles.php (1)
95-305: SplitgetBlocksShow()into a few focused builders.This method is now carrying status discovery, sort-case assembly, block-field mapping, and template assignment in one place. Extracting those branches into helpers would make follow-up generator changes safer and should address the current cognitive-complexity hotspot.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@class/Files/Blocks/BlocksFiles.php` around lines 95 - 305, The getBlocksShow function is doing too much (status discovery, sort-case assembly, block-field mapping, template/config assignment); split it into small focused helper methods and call them from getBlocksShow: extract status criteria logic into a method like buildStatusCriteria($fields, $tablePermissions, $critName) that returns added criteria lines or null; extract the switch/case construction into buildSortCases($fields, $tableName, $tableFieldname, $fieldId, $critName) returning the $contSwitch/$contElse string; extract the foreach block-field mapping into buildBlockFieldMapping($fields, $tableName, $fieldId, $ucfTableName, $lenName, $moduleDirname, $utilityVar) returning $contentForeach; and extract template/config assigns into assignBlockTemplateVars($func, $moduleDirname, $stuModuleDirname) which appends the XOOPS tpl assigns and return value; update getBlocksShow to compute only inputs (fields, names, flags), call these helpers and concatenate their returned strings into $func, preserving existing calls to Helper::getInstance(), Utility, criteria limit/handler lines, and the final return.class/Files/Templates/User/Defstyle/PagesItem.php (1)
119-143: Add a dedicated filter parameter togetSmartySingleVar()andgetSmartyDoubleVar()helpers instead of tunneling filters through the default value.These call sites pass filter chains like
''|escape:'html'and''|escape:'htmlattr'as the$defaultparameter, which the helpers concatenate directly into the template syntax. This works, but it couples escaping to a parameter intended for default values, creating brittle code that relies on string formatting rather than explicit API design. A dedicated$filteror$escapeparameter in both helper methods would make this intent clear and prevent future misuse.Also applies to: lines 172–206, 227–265
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@class/Files/Templates/User/Defstyle/PagesItem.php` around lines 119 - 143, The helpers getSmartySingleVar(...) and getSmartyDoubleVar(...) are misusing the $default parameter to pass filter chains like "''|escape:'html'"—add a dedicated $filter (or $escape) parameter to both helper signatures, update their internal template string construction to append the filter only from that new parameter (not from $default), and then update every call in this diff (e.g., the calls that currently pass "''|escape:'html'" or "''|escape:'htmlattr'") to supply the filter via the new argument while keeping $default for real default values; ensure all call sites (including the other similar blocks in this file) are updated to the new parameter order or use named args to avoid breaking callers.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@class/Files/admin/AdminPages.php`:
- Around line 183-185: The code uses a non-existent language constant
THEREARENO_{$stuTableName} which will cause an undefined constant at runtime;
update the generated template assignment in the call to
$this->xc->getXcXoopsTplAssign (the $condElse binding) to use the existing
THEREARENT_{$stuTableName} constant name, or alternatively ensure the admin
language generator writes THEREARENO_* into language/english/admin.php; change
the string in the $condElse construction (or adjust the language generator) so
the constant name matches the language file.
- Around line 351-356: The fallback that sets $saved{$ucfFieldId} to
${$tableName}Obj->getNewInsertedId{$ucfTableName}() must not be used for tables
with manual keys; update the code that builds $contentInsert (around
$saved{$ucfFieldId} and the call to getNewInsertedId{$ucfTableName}()) to only
use getNewInsertedId... when the table's autoincrement flag is true, otherwise
obtain the newly inserted identifier by reading it back from the entity/object
after the insert (via the object's ID getter) and use that value for the
permission save and uploader redirect paths; in short, gate the
getNewInsertedId... fallback on the table_autoincrement property or replace it
with reading the identifier from ${$tableName}Obj after insert.
In `@class/Files/Blocks/BlocksFiles.php`:
- Around line 159-163: The current criteria uses STATUS_OFFLINE with a '>'
comparator so any status above offline is included; change it to explicitly
target the approved state by replacing the constant lookup
getXcGetConstants('STATUS_OFFLINE') with getXcGetConstants('STATUS_APPROVED')
and update the getXcCriteria call to use an equality comparator (instead of
"'>'") so $fieldStatus is compared directly to the approved constant; keep using
the same variables ($fieldStatus, $critName, $func) and the
getXcCriteria/getXcCriteriaAdd calls so only the constant and comparator logic
are changed.
In `@class/Files/Classes/ClassFiles.php`:
- Line 348: The method getPermissionsInForm was changed to require string
$fieldId but getClassObject may leave $fieldId null when table_autoincrement is
false, causing a TypeError; change the signature back to allow null (use ?string
$fieldId) or explicitly handle the null case inside getPermissionsInForm by
deriving the identifier independently of table_autoincrement (or calling the
logic from getClassObject that computes a key) so permission-enabled tables with
manual keys no longer throw under strict_types=1; reference getPermissionsInForm
and getClassObject and the table_autoincrement logic when making the fix.
In `@class/Files/Language/LanguageHelp.php`:
- Around line 34-45: Remove the dead $ld property docblock and the redundant
constructor: delete the orphaned "@var" docblock, the commented-out "private $ld
= null;" and the entire "__construct" method that only calls
"parent::__construct()", and also remove the commented "//$this->ld =
LanguageDefines::getInstance();" line; this cleans up the unused $ld symbol and
the unnecessary constructor (no other references to $this->ld or
LanguageDefines::getInstance() exist in this file).
- Around line 115-118: The current logic in LanguageHelp (methods create() and
renderFile()) overwrites created-file state when creating two help files: call
renderFile() immediately after each create() so each create's status is captured
(i.e., call create(...); renderFile(...); then if needed collect/append the
results), or change create() to return a boolean/result object and update the
caller to call renderFile() per result; specifically update the block that
invokes create() for 'language/{lang}/help' and 'language/english/help' to
either invoke renderFile() right after each create() and accumulate
messages/statuses, or refactor create() to return operation outcomes and use
those to render both results so localized failures are not masked by the English
creation.
In `@class/Files/Templates/User/Defstyle/PagesList.php`:
- Around line 280-283: The render() method's PHPDoc says it returns string but
the method lacks a native return type; update the method signature for the
render() function to declare a native return type of string (i.e., change its
declaration to public function render(): string) so it matches the PHPDoc and
the value returned from renderFile(); ensure the method still returns the
renderFile() result and run tests/static analysis to confirm no callers expect a
different signature.
In `@class/Files/User/UserPdf.php`:
- Around line 147-160: The generated code always emits a table-scoped view check
via getXcCheckRight() (stored in $gperm) and wraps it in getPhpCodeConditions(),
which breaks PDFs for tables with table_permissions == 0; update the caller to
pass the table_permissions flag into getUserPdfTcpdf(), and inside
getUserPdfTcpdf() skip the entire permission-check block (the lines building
$condIf/$condElse, $gperm and the getPhpCodeConditions(...) that uses $noPerm)
when $table_permissions is false/0 so no {module}_view_{table} right is emitted
for public tables.
In `@class/Files/User/UserRate.php`:
- Around line 247-258: The generated SQL assembled in class UserRate (the $sql
string built around the SET clause) leaves a trailing comma before the WHERE,
causing invalid SQL; update the assembly so the last SET assignment (the part
using $fieldRatings) does not append a comma—i.e., remove the comma from the
string built by the getSimpleString call that contains 't.' . $fieldRatings . '
= COALESCE(r.avg_rating, 0)' or change the concatenation logic to only add
commas between assignments (build an array of SET assignments and join with ',
'), and ensure the subsequent WHERE fragment is concatenated without being
preceded by an extraneous comma.
In `@class/Modules.php`:
- Around line 384-385: The x-coordinate $spaceBorder is a float but
imagefttext() requires an int; in the code near the image generation where
$spaceBorder is computed and imagefttext($imageModule, 8.5, 0, $spaceBorder, 45,
$textColor, $font, \ucfirst($moduleDirname)) is called, cast $spaceBorder to an
int (e.g. (int)$spaceBorder or intval($spaceBorder)) before passing it to
imagefttext to avoid a TypeError when strict_types=1 is enabled.
---
Outside diff comments:
In `@class/Common/DirectoryChecker.php`:
- Around line 102-107: The createDirectory() return logic is inverted: it
negates mkdir()/is_dir() so successful directory creation yields false; fix by
changing the compound condition after the recursive call so it returns true when
mkdir succeeds or the directory exists (race-safe). Specifically update the
return expression in createDirectory() to use
self::createDirectory(dirname($target), $mode) && (mkdir($target, $mode) ||
is_dir($target)) (preserving the initial is_dir($target) short-circuit) so
successful creation returns true.
- Around line 145-156: The case 'setdirperm' block uses $path, $mode, and
$redirect without guaranteeing valid POST input; add a fail-fast validation:
require all three POST params present and scalar (use \Xmf\Request::hasVar and
\Xmf\Request::getString/getInt or equivalent) and if any are missing or
non-scalar immediately call \redirect_header with a clear error message (use
CO_{moduleDirNameUpper}_DC_PERMNOTSET or a new error constant) to avoid notices
and unsafe values being passed to DirectoryChecker::setDirectoryPermissions;
ensure $mode is cast to int and $path/$redirect are sanitized/strings before
calling DirectoryChecker::setDirectoryPermissions and redirect_header.
In `@class/Common/TableChecker.php`:
- Around line 74-80: The bug is that in the create-table branch you overwrite
the results array by assigning $this->result =
$GLOBALS['xoopsDB']->queryF($sql); — change this to capture the query result in
a temporary/local variable (e.g. $res = $GLOBALS['xoopsDB']->queryF($sql)) and
then append the creation message to $this->result (e.g. $this->result[] = 'Table
created:'.$table) so the results array is preserved; update any subsequent
checks that expect the query resource to use the temporary variable instead of
$this->result.
In `@class/Files/CreateClone.php`:
- Around line 40-55: The opendir() call in CreateClone::cloneFileFolder (the
variable $dir) is not checked for failure before being used and closed; update
cloneFileFolder to verify $dir !== false (or is_resource($dir)) right after
calling \opendir($src_path) and throw an exception (matching the pattern used in
cloneFile) with a clear message including $src_path if opendir fails, and only
enter the readdir loop and call \closedir($dir) when $dir is a valid resource to
avoid using a false value.
In `@class/Files/CreateStructure.php`:
- Around line 199-217: The PHPDoc for addFolderPath incorrectly states "@public"
while the method is declared private; update the docblock for the addFolderPath
method in CreateStructure (function addFolderPath) so the visibility annotation
matches (change "@public" to "@private" or remove the visibility tag), and
ensure the summary and param/return annotations remain accurate for the private
method.
In `@class/Files/Templates/User/Defstyle/PagesItem.php`:
- Around line 73-78: The PHPDoc for the write() method has parameters listed in
the wrong order; update the docblock above public function write($module,
$table, $tables, string $filename): void so the `@param` tags match the actual
signature order (module, table, tables, filename) and ensure types/comments
align with each parameter name referenced in the write() method.
In `@class/Modules.php`:
- Around line 363-393: Delete the unused private static method createLogo from
the Modules class: remove the entire private static function
createLogo($logoIcon, string $moduleDirname) { ... } block (lines shown in the
diff) since it is dead code and duplicates implementations in Logo.php and
LogoGenerator.php; after removal, search for any references to
Modules::createLogo or createLogo within the project to confirm no callers
remain and run tests/lint to ensure no regressions.
---
Nitpick comments:
In `@class/Files/Blocks/BlocksFiles.php`:
- Around line 95-305: The getBlocksShow function is doing too much (status
discovery, sort-case assembly, block-field mapping, template/config assignment);
split it into small focused helper methods and call them from getBlocksShow:
extract status criteria logic into a method like buildStatusCriteria($fields,
$tablePermissions, $critName) that returns added criteria lines or null; extract
the switch/case construction into buildSortCases($fields, $tableName,
$tableFieldname, $fieldId, $critName) returning the $contSwitch/$contElse
string; extract the foreach block-field mapping into
buildBlockFieldMapping($fields, $tableName, $fieldId, $ucfTableName, $lenName,
$moduleDirname, $utilityVar) returning $contentForeach; and extract
template/config assigns into assignBlockTemplateVars($func, $moduleDirname,
$stuModuleDirname) which appends the XOOPS tpl assigns and return value; update
getBlocksShow to compute only inputs (fields, names, flags), call these helpers
and concatenate their returned strings into $func, preserving existing calls to
Helper::getInstance(), Utility, criteria limit/handler lines, and the final
return.
In `@class/Files/CreateClone.php`:
- Around line 71-93: Replace the generic RuntimeException throws in CreateClone
(the file-copy/replace blocks that call copy(), file_get_contents(),
file_put_contents()) with a dedicated FilesystemException class in the
XoopsModules\Modulebuilder\Files namespace; create that exception (extends
\RuntimeException or a more specific SPL exception) and throw it with the same
messages from the CreateClone methods. Then update the callers in
CreateArchitecture.php (the methods setFilesToBuilding, setCommonFiles,
CopyRatingFiles) to wrap their calls to the CreateClone routines in try/catch
blocks that catch XoopsModules\Modulebuilder\Files\FilesystemException and
handle or log the error (or rethrow) instead of allowing uncaught
RuntimeException to propagate. Ensure all references import or fully-qualify the
new FilesystemException type.
In `@class/Files/CreateStructure.php`:
- Around line 155-163: The isDir method currently calls chmod($dname, 0755)
without checking its return; update isDir to verify chmod succeeded and throw a
RuntimeException (similar to the mkdir error handling) when chmod returns false,
referencing the same directory name in the message; locate the isDir function
and add a conditional around chmod that throws \RuntimeException(sprintf('Failed
to change permissions for "%s"', $dname)) if chmod($dname, 0755) === false.
- Around line 175-197: The isDirEmpty method currently documents a bool return
but lacks a declared return type; update the method signature for
isDirEmpty(string $dir) to declare a boolean return (add : bool) so the
signature and PHPDoc match the PR's type-hinting improvements, and run static
checks to ensure no callers rely on mixed/missing return typing (method name:
isDirEmpty, parameter: $dir).
- Around line 265-275: setCopy now throws a RuntimeException when copy() fails
but callers like copyFile and makeDirAndCopyFile (used by setBaseFoldersFiles)
and the public caller in admin/building.php don't catch it, so failures will
bubble to the app; update callers to handle the exception: wrap calls to setCopy
(and the higher-level copyFile/makeDirAndCopyFile invocations) in try/catch
blocks that log a clear error and either recover or rethrow a more specific
exception, or alternatively document/propagate the exception intentionally and
add a top-level try/catch in admin/building.php around the setBaseFoldersFiles
call to present a user-friendly error instead of an uncaught exception.
In `@class/Files/CreateXoopsCode.php`:
- Around line 1033-1053: Remove the unused $tableName parameter from the
getXcXoopsConfirm method signature and any accompanying docblock in the
CreateXoopsCode class; update the function declaration for
getXcXoopsConfirm($language, $fieldId, $fieldMain, string $options = 'delete',
string $t = '') (keeping the same default values/order for remaining params) and
then update all callers to stop passing the extra tableName argument so calls
match the new signature; ensure any static/instance invocation sites and related
tests are updated to avoid warnings.
In `@class/Files/Templates/User/Defstyle/Categories.php`:
- Around line 98-142: The new table-building helper
getTemplatesUserCategoriesTable(...) is never used by render() (render() still
calls getTemplatesUserCategoriesPanel(...) and getTemplatesUserCategories(...)),
so your table code is dead; either update render() to call
getTemplatesUserCategoriesTable(...) where the table layout should appear
(replace the getTemplatesUserCategoriesPanel/getTemplatesUserCategories call
with getTemplatesUserCategoriesTable and pass the same $moduleDirname,
$tableName, $tableSoleName, $language args) or remove the unused private methods
(getTemplatesUserCategoriesTable, getTemplatesUserCategoriesThead,
getTemplatesUserCategoriesTbody) to avoid dead code. Ensure you keep parameter
names and return types consistent with existing render() expectations when
wiring the new method.
- Line 85: Add native string return types to the private builder methods (e.g.,
getTemplatesUserCategoriesHeader) and to render() so PHP enforces the string
contract at runtime; update each method signature to append ": string" (for all
private builders referenced at lines 85, 98, 113, 131, 181, 202, 209-211) and
keep their bodies unchanged. Before changing the public render() signature,
confirm it is not required by any external interface or parent class (render()
currently returns Files\CreateFile::renderFile() which produces a string), then
add ": string" to render() as well. Ensure all updated methods still return a
string in every code path.
In `@class/Files/Templates/User/Defstyle/PagesItem.php`:
- Around line 119-143: The helpers getSmartySingleVar(...) and
getSmartyDoubleVar(...) are misusing the $default parameter to pass filter
chains like "''|escape:'html'"—add a dedicated $filter (or $escape) parameter to
both helper signatures, update their internal template string construction to
append the filter only from that new parameter (not from $default), and then
update every call in this diff (e.g., the calls that currently pass
"''|escape:'html'" or "''|escape:'htmlattr'") to supply the filter via the new
argument while keeping $default for real default values; ensure all call sites
(including the other similar blocks in this file) are updated to the new
parameter order or use named args to avoid breaking callers.
In `@class/Files/User/UserPages.php`:
- Line 169: The breadcrumb calls currently pass a hardcoded indent string "\t\t"
to uxC->getUserBreadcrumbs; update these to use the existing indentation
variable $t instead so breadcrumb output remains consistently indented—replace
occurrences like $this->uxc->getUserBreadcrumbs($language, $tableName, 'list',
'', "\t\t") with passing $t (e.g., $this->uxc->getUserBreadcrumbs($language,
$tableName, 'list', '', $t)); apply the same change for the other
getUserBreadcrumbs calls in this class (the ones at the other similar call
sites) to ensure consistent use of $t across all breadcrumb invocations.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 4a2e360c-5328-4026-a58d-286e5a76348d
📒 Files selected for processing (32)
class/Building.phpclass/Common/DirectoryChecker.phpclass/Common/FileChecker.phpclass/Common/Migrate.phpclass/Common/TableChecker.phpclass/Devtools.phpclass/Files/Admin/AdminBroken.phpclass/Files/Admin/AdminPermissions.phpclass/Files/Blocks/BlocksFiles.phpclass/Files/Classes/ClassFiles.phpclass/Files/CreateClone.phpclass/Files/CreatePhpCode.phpclass/Files/CreateStructure.phpclass/Files/CreateXoopsCode.phpclass/Files/Includes/IncludeNotifications.phpclass/Files/Language/LanguageHelp.phpclass/Files/Sql/SqlFile.phpclass/Files/Templates/Admin/TemplatesAdminBroken.phpclass/Files/Templates/User/Defstyle/Categories.phpclass/Files/Templates/User/Defstyle/MoreFiles.phpclass/Files/Templates/User/Defstyle/PagesItem.phpclass/Files/Templates/User/Defstyle/PagesList.phpclass/Files/User/UserIndex.phpclass/Files/User/UserPages.phpclass/Files/User/UserPdf.phpclass/Files/User/UserRate.phpclass/Files/admin/AdminPages.phpclass/Helper.phpclass/LogoGenerator.phpclass/Modules.phpclass/SplClassLoader.phpclass/Utility.php
✅ Files skipped from review due to trivial changes (2)
- class/Files/Sql/SqlFile.php
- class/Files/Templates/User/Defstyle/MoreFiles.php
🚧 Files skipped from review as they are similar to previous changes (9)
- class/Utility.php
- class/Helper.php
- class/LogoGenerator.php
- class/Building.php
- class/SplClassLoader.php
- class/Files/User/UserIndex.php
- class/Common/FileChecker.php
- class/Devtools.php
- class/Files/CreatePhpCode.php
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
class/Files/Blocks/BlocksFiles.php (1)
153-163:⚠️ Potential issue | 🟠 MajorApply the approval filter even when table permissions are off.
Because Line 153 still gates the status-field lookup on
$tablePermissions, a table that usesFIELD_ELE_SELECTSTATUSwithout group-permission support never gets theSTATUS_APPROVEDcriteria. Public blocks can still render submitted/broken rows in that case.🔧 Suggested fix
- if (1 == $tablePermissions) { - foreach ($fields as $field) { - if ($field->getVar('field_element') == Constants::FIELD_ELE_SELECTSTATUS) { - $fieldStatus = $field->getVar('field_name'); - } - } - if ('' !== $fieldStatus) { - $constant = $this->xc->getXcGetConstants('STATUS_APPROVED'); - $crit = $this->xc->getXcCriteria('', "'{$fieldStatus}'", $constant, "'='", true); - $func .= $this->pc->getPhpCodeCommentLine("Criteria for status field",'',"\t"); - $func .= $this->xc->getXcCriteriaAdd($critName, $crit, "\t"); - $func .= $this->pc->getPhpCodeBlankLine(); - } + foreach ($fields as $field) { + if ($field->getVar('field_element') == Constants::FIELD_ELE_SELECTSTATUS) { + $fieldStatus = $field->getVar('field_name'); + } + } + if ('' !== $fieldStatus) { + $constant = $this->xc->getXcGetConstants('STATUS_APPROVED'); + $crit = $this->xc->getXcCriteria('', "'{$fieldStatus}'", $constant, "'='", true); + $func .= $this->pc->getPhpCodeCommentLine("Criteria for status field",'',"\t"); + $func .= $this->xc->getXcCriteriaAdd($critName, $crit, "\t"); + $func .= $this->pc->getPhpCodeBlankLine(); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@class/Files/Blocks/BlocksFiles.php` around lines 153 - 163, The status-field detection and approval-criteria generation are incorrectly nested inside the if (1 == $tablePermissions) block; move the loop that finds FIELD_ELE_SELECTSTATUS (iterating $fields and checking $field->getVar('field_element') == Constants::FIELD_ELE_SELECTSTATUS and setting $fieldStatus) and the subsequent generation of $constant = $this->xc->getXcGetConstants('STATUS_APPROVED'), $crit = $this->xc->getXcCriteria(...), and $this->xc->getXcCriteriaAdd($critName, $crit, "\t") so they execute regardless of $tablePermissions value (leaving any permission-specific code inside the original block), ensuring $fieldStatus and $crit are computed and appended to $func even when table permissions are off.class/Files/CreateSmartyCode.php (1)
149-180:⚠️ Potential issue | 🟠 MajorUpdate escape mode to use
htmlinstead of unsupportedhtmlattrmode.The new
$escapeparameter now allows callers to emit|escape:'htmlattr', buthtmlattris not a valid Smarty escape mode according to official documentation. The correct mode for escaping HTML attribute values ishtml(e.g.,|escape:'html').This impacts 22 call sites across multiple template files that currently pass
'htmlattr'. These must be updated to use'html', or the helpers should validate and reject unknown escape modes.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@class/Files/CreateSmartyCode.php` around lines 149 - 180, The helper functions getSmartySingleVar and getSmartyDoubleVar currently allow callers to emit an unsupported Smarty escape mode 'htmlattr'; update these methods to enforce or translate escape modes so callers use 'html' for HTML attribute escaping: either map incoming $escape value 'htmlattr' to 'html' before appending (e.g., if ($escape === 'htmlattr') $escape = 'html'), or validate $escape against the set of supported Smarty modes and throw/reject on unknown values; then update the appended escape fragment construction in both getSmartySingleVar and getSmartyDoubleVar to use the validated/mapped $escape so templates that pass 'htmlattr' will use 'html' instead.
♻️ Duplicate comments (1)
class/Files/Language/LanguageHelp.php (1)
110-115:⚠️ Potential issue | 🟠 MajorLocalized file creation result is still masked by the subsequent English file creation.
The previous review concern remains unaddressed: when
$language !== 'english',create()is called twice (lines 111 and 113), but only onerenderFile()call at line 115 reports the result. Sincecreate()mutates instance state andrenderFile()reads that state, the localized file creation status (success or failure) is overwritten and never reported.Consider calling
renderFile()after eachcreate()and accumulating results:+ $result = ''; if ('english' !== $language) { $this->create($moduleDirname, 'language/' . $language . '/help', $filename, $content, \_AM_MODULEBUILDER_FILE_CREATED, \_AM_MODULEBUILDER_FILE_NOTCREATED); + $result .= $this->renderFile(); } $this->create($moduleDirname, 'language/english/help', $filename, $content, \_AM_MODULEBUILDER_FILE_CREATED, \_AM_MODULEBUILDER_FILE_NOTCREATED); + $result .= $this->renderFile(); - return $this->renderFile(); + return $result;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@class/Files/Language/LanguageHelp.php` around lines 110 - 115, The localized create() call result is being overwritten by the subsequent English create() because renderFile() reads mutated instance state; update LanguageHelp so you call renderFile() immediately after each create() (i.e., after create(... 'language/' . $language . '/help' ...) and again after create(... 'language/english/help' ...)), collect/merge those two results (or status messages) and return the combined outcome instead of a single renderFile() call; alternatively, change create() to return an explicit result value and use those return values to build the final report before returning from the method.
🧹 Nitpick comments (2)
class/Files/Templates/User/Defstyle/Breadcrumbs.php (1)
84-89: Missing return type declaration onrender()method.The docblock declares
@return string, but the method signature lacks an explicit return type. For consistency with the stricter typing introduced inwrite(): void, add: stringto the method signature.♻️ Suggested fix
/** * `@public` function render * * `@return` string */ - public function render() + public function render(): string🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@class/Files/Templates/User/Defstyle/Breadcrumbs.php` around lines 84 - 89, The render() method in Breadcrumbs.php lacks an explicit return type even though its docblock says it returns a string; update the method signature public function render() to public function render(): string and ensure all return paths in the render() implementation return a string (or cast/convert non-string returns) so the signature and docblock remain consistent with strict typing.class/Files/CreateXoopsCode.php (1)
184-185: Make$languagea required parameter on these date error handlers.Both
getXcSetVarTextDateSelect()andgetXcSetVarDateTime()build{$language}INVALID_DATEfor error messages, but the$languageparameter defaults to''. This silently produces unlocalizedINVALID_DATEstrings if called with fewer than 5 arguments. Remove the default value to make the parameter mandatory.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@class/Files/CreateXoopsCode.php` around lines 184 - 185, The language parameter should be required to avoid empty unlocalized error keys: change the signatures of getXcSetVarTextDateSelect(...) and getXcSetVarDateTime(...) to remove the default value for $language (i.e. make the parameter mandatory instead of string $language = ''), update any internal references expecting $language to be non-empty, and adjust callers so they pass an explicit language argument when invoking these methods.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@class/Common/DirectoryChecker.php`:
- Around line 144-145: The Request::getString()/getInt() calls in class
Common\DirectoryChecker (e.g., the $path and $redirect retrieval and the later
int/string calls around where getInt/getString are used) pass 'POST' as the
default value instead of the source hash; update those calls to use the correct
signature by supplying an explicit default ('' for strings, 0 for ints) as the
second argument and 'POST' as the third argument (e.g., getString('name', '',
'POST') or getInt('count', 0, 'POST')) so the values are read strictly from
$_POST.
In `@class/Files/CreateXoopsCode.php`:
- Around line 184-198: getXcSetVarTextDateSelect currently treats
DateTime::createFromFormat(...) === false as the only invalid-case; update the
generated logic to also detect silently normalized/invalid dates by calling
DateTime::getLastErrors() after createFromFormat and treating any returned
error_count or warning_count > 0 as invalid. Concretely, where the code builds
the value and the subsequent condition (see getXcSetVarTextDateSelect, the
$value using DateTime::createFromFormat and the pc->getPhpCodeConditions /
getXcEqualsOperator logic), add or replace the single false-check with a
combined check that fails if the creation returned false OR
DateTime::getLastErrors() reports errors/warnings, and route that failure path
to the same error/form-rendering block (reuse getXcXoopsTplAssign('error', ...),
getXcGetForm and break handling) before calling getXcSetVarObj.
In `@class/Files/Language/LanguageHelp.php`:
- Around line 58-67: The write method lacks a type hint for its $module
parameter; update the method signature of write to type-hint $module as
Modulebuilder\Modules to match validation in setModule (inherited from
CreateAbstractClass) so the parameter and call $this->setModule($module) are
type-safe and consistent with $filename and the void return type.
In `@class/Files/Templates/User/Defstyle/Breadcrumbs.php`:
- Around line 101-102: The language constant prefix used when building $langHome
is incorrect: update the call to the Smarty helper getSmartyConst in
Breadcrumbs.php so it uses the XOOPS main/user-side prefix instead of the admin
one (replace the '_MA_' prefix with '_MD_'); locate the line that constructs
$langHome via $this->sc->getSmartyConst('_MA_' . $stuModuleDirname, '_HOME') and
change the prefix to '_MD_' so the generated constant matches the module
language file conventions for user-facing breadcrumbs.
In `@class/Files/User/UserPdf.php`:
- Around line 148-164: The variable $ret is only set inside the if (1 ===
$tablePermissions) block but later used unconditionally with $ret .=
$this->pc->getPhpCodeCommentLine('Set defaults'); — initialize $ret before the
conditional (e.g., set $ret = '' immediately prior to the if (1 ===
$tablePermissions) { line) so subsequent concatenations (references to $ret,
including uses after the permission check) never operate on an undefined
variable; you can also alternatively ensure both branches assign $ret, but the
simplest fix is to initialize $ret to an empty string before the if.
---
Outside diff comments:
In `@class/Files/Blocks/BlocksFiles.php`:
- Around line 153-163: The status-field detection and approval-criteria
generation are incorrectly nested inside the if (1 == $tablePermissions) block;
move the loop that finds FIELD_ELE_SELECTSTATUS (iterating $fields and checking
$field->getVar('field_element') == Constants::FIELD_ELE_SELECTSTATUS and setting
$fieldStatus) and the subsequent generation of $constant =
$this->xc->getXcGetConstants('STATUS_APPROVED'), $crit =
$this->xc->getXcCriteria(...), and $this->xc->getXcCriteriaAdd($critName, $crit,
"\t") so they execute regardless of $tablePermissions value (leaving any
permission-specific code inside the original block), ensuring $fieldStatus and
$crit are computed and appended to $func even when table permissions are off.
In `@class/Files/CreateSmartyCode.php`:
- Around line 149-180: The helper functions getSmartySingleVar and
getSmartyDoubleVar currently allow callers to emit an unsupported Smarty escape
mode 'htmlattr'; update these methods to enforce or translate escape modes so
callers use 'html' for HTML attribute escaping: either map incoming $escape
value 'htmlattr' to 'html' before appending (e.g., if ($escape === 'htmlattr')
$escape = 'html'), or validate $escape against the set of supported Smarty modes
and throw/reject on unknown values; then update the appended escape fragment
construction in both getSmartySingleVar and getSmartyDoubleVar to use the
validated/mapped $escape so templates that pass 'htmlattr' will use 'html'
instead.
---
Duplicate comments:
In `@class/Files/Language/LanguageHelp.php`:
- Around line 110-115: The localized create() call result is being overwritten
by the subsequent English create() because renderFile() reads mutated instance
state; update LanguageHelp so you call renderFile() immediately after each
create() (i.e., after create(... 'language/' . $language . '/help' ...) and
again after create(... 'language/english/help' ...)), collect/merge those two
results (or status messages) and return the combined outcome instead of a single
renderFile() call; alternatively, change create() to return an explicit result
value and use those return values to build the final report before returning
from the method.
---
Nitpick comments:
In `@class/Files/CreateXoopsCode.php`:
- Around line 184-185: The language parameter should be required to avoid empty
unlocalized error keys: change the signatures of getXcSetVarTextDateSelect(...)
and getXcSetVarDateTime(...) to remove the default value for $language (i.e.
make the parameter mandatory instead of string $language = ''), update any
internal references expecting $language to be non-empty, and adjust callers so
they pass an explicit language argument when invoking these methods.
In `@class/Files/Templates/User/Defstyle/Breadcrumbs.php`:
- Around line 84-89: The render() method in Breadcrumbs.php lacks an explicit
return type even though its docblock says it returns a string; update the method
signature public function render() to public function render(): string and
ensure all return paths in the render() implementation return a string (or
cast/convert non-string returns) so the signature and docblock remain consistent
with strict typing.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 65aeb2b5-d583-4637-8e0e-af099f7e166d
📒 Files selected for processing (21)
class/Common/DirectoryChecker.phpclass/Common/TableChecker.phpclass/Files/Blocks/BlocksFiles.phpclass/Files/Classes/ClassFiles.phpclass/Files/CreateClone.phpclass/Files/CreateSmartyCode.phpclass/Files/CreateStructure.phpclass/Files/CreateXoopsCode.phpclass/Files/Language/LanguageHelp.phpclass/Files/Templates/Admin/TemplatesAdminBroken.phpclass/Files/Templates/Admin/TemplatesAdminPages.phpclass/Files/Templates/Blocks/Defstyle/TemplatesBlocks.phpclass/Files/Templates/User/Defstyle/Breadcrumbs.phpclass/Files/Templates/User/Defstyle/Categories.phpclass/Files/Templates/User/Defstyle/PagesItem.phpclass/Files/Templates/User/Defstyle/PagesList.phpclass/Files/User/UserPages.phpclass/Files/User/UserPdf.phpclass/Files/User/UserPrint.phpclass/Files/User/UserRate.phpclass/Modules.php
✅ Files skipped from review due to trivial changes (1)
- class/Files/Templates/User/Defstyle/PagesList.php
🚧 Files skipped from review as they are similar to previous changes (7)
- class/Common/TableChecker.php
- class/Files/CreateStructure.php
- class/Files/Classes/ClassFiles.php
- class/Files/User/UserPrint.php
- class/Files/Templates/Blocks/Defstyle/TemplatesBlocks.php
- class/Files/User/UserRate.php
- class/Files/CreateClone.php
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
class/Common/DirectoryChecker.php (1)
144-145:⚠️ Potential issue | 🟠 MajorFix
Xmf\Requestargument usage to enforce POST-only reads.Line 144, Line 145, Line 153, Line 154, and Line 155 still read from default request source because
'POST'is either passed in the wrong position or omitted. This reintroduces the same issue previously flagged.Required patch
- $path = \Xmf\Request::getString('path', 'POST'); - $redirect = \Xmf\Request::getString('redirect', 'POST'); + $path = \Xmf\Request::getString('path', '', 'POST'); + $redirect = \Xmf\Request::getString('redirect', '', 'POST'); @@ - $path = \Xmf\Request::getString('path'); - $redirect = \Xmf\Request::getString('redirect'); - $mode = \Xmf\Request::getInt('mode'); + $path = \Xmf\Request::getString('path', '', 'POST'); + $redirect = \Xmf\Request::getString('redirect', '', 'POST'); + $mode = \Xmf\Request::getInt('mode', 0, 'POST');What are the exact parameter signatures of XOOPS xmf `Xmf\Request::getString()` and `Xmf\Request::getInt()`, and which argument must be set to `'POST'` to force POST-only input?Also applies to: 153-155
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@class/Common/DirectoryChecker.php` around lines 144 - 145, The getString/getInt calls are not forcing POST-only because the 'POST' source is in the wrong position or missing; update each Xmf\Request::getString(...) and Xmf\Request::getInt(...) invocation in DirectoryChecker (notably the calls that set $path, $redirect and the ones at lines ~153-155) to pass 'POST' as the third argument (the source parameter) so the functions read only from POST; locate and change the signatures used in DirectoryChecker.php accordingly.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@class/Files/Language/LanguageHelp.php`:
- Line 113: The exception message in class LanguageHelp currently hardcodes
"help.tpl" which can be misleading; update the RuntimeException thrown (in the
method that creates the help file inside LanguageHelp) to include the actual
$filename variable rather than the literal "help.tpl" so failures report the
correct filename (use $filename in the exception text).
---
Duplicate comments:
In `@class/Common/DirectoryChecker.php`:
- Around line 144-145: The getString/getInt calls are not forcing POST-only
because the 'POST' source is in the wrong position or missing; update each
Xmf\Request::getString(...) and Xmf\Request::getInt(...) invocation in
DirectoryChecker (notably the calls that set $path, $redirect and the ones at
lines ~153-155) to pass 'POST' as the third argument (the source parameter) so
the functions read only from POST; locate and change the signatures used in
DirectoryChecker.php accordingly.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 345d8b84-3467-4a5d-992b-60c9a2d127ce
📒 Files selected for processing (6)
class/Common/DirectoryChecker.phpclass/Files/CreateXoopsCode.phpclass/Files/Language/LanguageHelp.phpclass/Files/Templates/User/Defstyle/Breadcrumbs.phpclass/Files/User/UserPdf.phpclass/Files/admin/AdminPages.php
✅ Files skipped from review due to trivial changes (1)
- class/Files/User/UserPdf.php
🚧 Files skipped from review as they are similar to previous changes (2)
- class/Files/Templates/User/Defstyle/Breadcrumbs.php
- class/Files/CreateXoopsCode.php
There was a problem hiding this comment.
♻️ Duplicate comments (1)
class/Common/DirectoryChecker.php (1)
155-155:⚠️ Potential issue | 🔴 Critical
getInt()is called with an invalid default type—pass0instead of''.The second argument to
\Xmf\Request::getInt()must be an integer (the default value), but''(empty string) is passed. This will cause unexpected behavior or type coercion issues.Signature:
getInt($name, $default = 0, $hash = 'default')Proposed fix
- $mode = \Xmf\Request::getInt('mode','', 'POST'); + $mode = \Xmf\Request::getInt('mode', 0, 'POST');🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@class/Common/DirectoryChecker.php` at line 155, The call to \Xmf\Request::getInt in DirectoryChecker.php uses an invalid default '' for the second parameter; replace the empty string default with the integer 0 so the signature getInt($name, $default = 0, $hash = 'default') is honored—update the $mode assignment (where \Xmf\Request::getInt('mode', '', 'POST') is used) to pass 0 as the default.
🧹 Nitpick comments (2)
class/Files/Language/LanguageHelp.php (2)
58-63: Update docblock to include the$moduletype.The method signature correctly specifies
Modulebuilder\Modules $module, but the docblock at line 60 lacks the type. For consistency with the type hint:/** * `@public` function write - * `@param` $module + * `@param` Modulebuilder\Modules $module * `@param` string $filename */🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@class/Files/Language/LanguageHelp.php` around lines 58 - 63, Update the docblock for the write method in LanguageHelp.php to include the correct type for the $module parameter (Modulebuilder\Modules) so it matches the method signature; specifically modify the docblock above public function write(Modulebuilder\Modules $module, string $filename): void to add a `@param` Modulebuilder\Modules $module entry and keep the existing `@param` string $filename doc.
69-74: Add return type hint to match docblock.The docblock at lines 71-72 declares
@return string, but the method signature at line 74 lacks the corresponding return type hint. For type safety and consistency with the typing improvements elsewhere in this PR:- public function render() + public function render(): string🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@class/Files/Language/LanguageHelp.php` around lines 69 - 74, The render method in class LanguageHelp lacks a return type despite the docblock declaring `@return` string; update the method signature public function render() to public function render(): string and ensure all return paths in LanguageHelp::render return a string (adjust any null/other-type returns to string or cast/convert as needed).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@class/Common/DirectoryChecker.php`:
- Line 155: The call to \Xmf\Request::getInt in DirectoryChecker.php uses an
invalid default '' for the second parameter; replace the empty string default
with the integer 0 so the signature getInt($name, $default = 0, $hash =
'default') is honored—update the $mode assignment (where
\Xmf\Request::getInt('mode', '', 'POST') is used) to pass 0 as the default.
---
Nitpick comments:
In `@class/Files/Language/LanguageHelp.php`:
- Around line 58-63: Update the docblock for the write method in
LanguageHelp.php to include the correct type for the $module parameter
(Modulebuilder\Modules) so it matches the method signature; specifically modify
the docblock above public function write(Modulebuilder\Modules $module, string
$filename): void to add a `@param` Modulebuilder\Modules $module entry and keep
the existing `@param` string $filename doc.
- Around line 69-74: The render method in class LanguageHelp lacks a return type
despite the docblock declaring `@return` string; update the method signature
public function render() to public function render(): string and ensure all
return paths in LanguageHelp::render return a string (adjust any null/other-type
returns to string or cast/convert as needed).
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 9c1bef51-86e2-4a28-8249-2593a7020674
📒 Files selected for processing (2)
class/Common/DirectoryChecker.phpclass/Files/Language/LanguageHelp.php
|
1 participant
Summary by CodeRabbit
Requirements Update
Templates & UI
Exports & Feeds
Stability & Reliability
Code Quality