Feature/change order for all modules#1677
Conversation
WalkthroughAdds a new boolean Changes
Sequence Diagram(s)mermaid Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Pull request overview
This PR decouples module ordering from menu visibility by introducing a dedicated show_in_menu flag, allowing hidden modules to be re-ordered via drag-and-drop while preserving existing visibility behavior during upgrades.
Changes:
- Add
show_in_menucolumn to themodulestable (schema + migration) and wire it intoXoopsModule. - Update modules admin UI/actions to toggle
show_in_menuinstead of overloadingweight, and allow ordering for all modules in the sortable list. - Update menu/module listing logic (templates + main menu block) to use
show_in_menu.
Reviewed changes
Copilot reviewed 9 out of 9 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| htdocs/modules/system/xoops_version.php | Bumps system module version to trigger the update/migration path. |
| htdocs/modules/system/themes/dark/modules/system/admin/system_modules.tpl | Switch menu-visibility icon logic from weight to show_in_menu. |
| htdocs/modules/system/templates/admin/system_modules.tpl | Same as above for the default admin template. |
| htdocs/modules/system/include/update.php | Adds migration to create/populate show_in_menu and reassign contiguous weights. |
| htdocs/modules/system/blocks/system_blocks.php | Main menu block now filters by show_in_menu instead of weight > 0. |
| htdocs/modules/system/admin/modulesadmin/modulesadmin.php | Ensures new installs set show_in_menu on module creation. |
| htdocs/modules/system/admin/modulesadmin/main.php | Enables ordering for hidden modules and toggles show_in_menu on menu visibility action. |
| htdocs/kernel/module.php | Adds show_in_menu var and persists it in module insert/update SQL. |
| htdocs/install/sql/mysql.structure.sql | Adds show_in_menu to fresh-install schema. |
| if (version_compare((string) $prev_version, '2.1.10', '<')) { | ||
| if (!system_modules_add_menu_visibility($module)) { | ||
| $ret = false; | ||
| } | ||
| } |
There was a problem hiding this comment.
The update gate uses PHP version_compare((string)$prev_version, '2.1.10', '<'), but module versions in this codebase commonly include suffixes like -Stable, -beta, etc. XOOPS already provides XoopsModule::versionCompare() which normalizes by trimming and stripping the suffix after - (and has unit tests for this behavior). Using raw version_compare() here can cause this migration to re-run (or be skipped) when only the suffix changes (e.g. 2.1.10-beta → 2.1.10-Stable), potentially overwriting user-adjusted weights/show_in_menu. Use $module->versionCompare($prev_version, '2.1.10', '<') (or normalize similarly) for the update condition.
Module weight was overloaded as both sort position and menu visibility flag (weight=0 meant hidden), preventing hidden modules from being reordered via drag-and-drop. Adds show_in_menu column to decouple the two concerns, with migration that preserves existing visibility and assigns contiguous weights to all non-system modules.
mambax7
force-pushed
the
feature/Change_order_for_all_modules
branch
from
2bc1c39 to
0c3f618
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@htdocs/kernel/module.php`:
- Around line 866-868: XoopsModuleHandler::insert() currently always includes
the show_in_menu column in its INSERT/UPDATE SQL, which breaks upgrades on
schemas that lack that column; change insert() to detect whether the modules
table has the show_in_menu column (or whether the schema version is >= the
version that adds it) and only include show_in_menu in the SQL when that column
exists, otherwise omit show_in_menu from the field list and values; update the
INSERT and UPDATE code paths (the SQL sprintf calls building the queries) to
conditionally add the field/value pair based on the detection so
xoops_module_update() and installs remain backward-compatible.
In `@htdocs/modules/system/blocks/system_blocks.php`:
- Around line 122-125: The query adds a Criteria on 'show_in_menu' immediately
which breaks pre-2.1.10 installs where that column doesn't exist; instead detect
whether the schema/column is present before adding new Criteria('show_in_menu',
1) and, if it is absent, add the legacy predicate (weight > 0) to $criteria
(e.g. new Criteria('weight', 0, '>')) so $module_handler->getObjects($criteria,
true) works during the upgrade window; locate the logic around $criteria and the
call to $module_handler->getObjects and conditionally add the appropriate
Criteria based on a schema/version or column-exists check.
In `@htdocs/modules/system/include/update.php`:
- Around line 35-39: Flatten the nested if by combining the version check and
function call with short-circuit evaluation: when version_compare((string)
$prev_version, '2.1.10', '<') is true, call
system_modules_add_menu_visibility($module) and if that returns false set $ret =
false; update the block around version_compare and
system_modules_add_menu_visibility so the function is only invoked when the
version check passes but without a nested if, keeping $prev_version, $module and
$ret usage unchanged.
- Around line 80-97: The UPDATE that migrates show_in_menu currently runs
unconditionally and may overwrite manual changes; modify the logic around
$columnExists so that the ALTER TABLE block (where $sql adds show_in_menu) also
performs the data migration — move the "UPDATE {$tableName} SET show_in_menu =
CASE WHEN weight > 0 THEN 1 ELSE 0 END" and its error handling inside the if
(!$columnExists) block after the ALTER succeeds, keeping the same $xoopsDB->exec
error checks and $module->setErrors calls to ensure the migration only runs when
the column was newly added.
- Around line 108-118: The loop in update.php that uses
$xoopsDB->fetchArray($result) and issues one UPDATE per row via
$xoopsDB->exec($sql) should be replaced with a single SQL statement that assigns
incremental weights (for example using a MySQL user variable like `@w`) so the
operation is executed in one atomic query instead of N updates; modify the code
that currently initializes $weight and increments it inside the while to instead
build and execute one UPDATE ... JOIN (or UPDATE with a subselect that uses `@w`)
ordered as needed, and on failure still call $module->setErrors('Unable to
reassign module weights during migration.') and return false to preserve
existing error handling.
🪄 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: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: b7de5ca1-cfb8-4b11-9a90-b4b19aafe27c
📒 Files selected for processing (9)
htdocs/install/sql/mysql.structure.sqlhtdocs/kernel/module.phphtdocs/modules/system/admin/modulesadmin/main.phphtdocs/modules/system/admin/modulesadmin/modulesadmin.phphtdocs/modules/system/blocks/system_blocks.phphtdocs/modules/system/include/update.phphtdocs/modules/system/templates/admin/system_modules.tplhtdocs/modules/system/themes/dark/modules/system/admin/system_modules.tplhtdocs/modules/system/xoops_version.php
| $columnExists = false !== $xoopsDB->fetchArray($result); | ||
| if (!$columnExists) { | ||
| $sql = "ALTER TABLE {$tableName} ADD show_in_menu tinyint(1) unsigned NOT NULL default '1' AFTER weight"; | ||
| if (!$xoopsDB->exec($sql)) { | ||
| xoops_error($xoopsDB->error() . '<br>' . $sql); | ||
| $module->setErrors('Unable to add show_in_menu to the modules table.'); | ||
|
|
||
| return false; | ||
| } | ||
| } | ||
|
|
||
| $sql = "UPDATE {$tableName} SET show_in_menu = CASE WHEN weight > 0 THEN 1 ELSE 0 END"; | ||
| if (!$xoopsDB->exec($sql)) { | ||
| xoops_error($xoopsDB->error() . '<br>' . $sql); | ||
| $module->setErrors('Unable to migrate module menu visibility.'); | ||
|
|
||
| return false; | ||
| } |
There was a problem hiding this comment.
Data migration runs unconditionally — risk of overwriting admin changes on re-run.
The UPDATE at line 91 executes regardless of whether the column was just added or already existed. If an upgrade partially completes (column added but version not recorded), a subsequent run will overwrite any manual show_in_menu adjustments.
Move the data migration inside the column-addition block to ensure it only runs once.
🐛 Proposed fix
$columnExists = false !== $xoopsDB->fetchArray($result);
if (!$columnExists) {
$sql = "ALTER TABLE {$tableName} ADD show_in_menu tinyint(1) unsigned NOT NULL default '1' AFTER weight";
if (!$xoopsDB->exec($sql)) {
xoops_error($xoopsDB->error() . '<br>' . $sql);
$module->setErrors('Unable to add show_in_menu to the modules table.');
return false;
}
- }
- $sql = "UPDATE {$tableName} SET show_in_menu = CASE WHEN weight > 0 THEN 1 ELSE 0 END";
- if (!$xoopsDB->exec($sql)) {
- xoops_error($xoopsDB->error() . '<br>' . $sql);
- $module->setErrors('Unable to migrate module menu visibility.');
+ // Only migrate data when column is newly added
+ $sql = "UPDATE {$tableName} SET show_in_menu = CASE WHEN weight > 0 THEN 1 ELSE 0 END";
+ if (!$xoopsDB->exec($sql)) {
+ xoops_error($xoopsDB->error() . '<br>' . $sql);
+ $module->setErrors('Unable to migrate module menu visibility.');
- return false;
+ return false;
+ }
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@htdocs/modules/system/include/update.php` around lines 80 - 97, The UPDATE
that migrates show_in_menu currently runs unconditionally and may overwrite
manual changes; modify the logic around $columnExists so that the ALTER TABLE
block (where $sql adds show_in_menu) also performs the data migration — move the
"UPDATE {$tableName} SET show_in_menu = CASE WHEN weight > 0 THEN 1 ELSE 0 END"
and its error handling inside the if (!$columnExists) block after the ALTER
succeeds, keeping the same $xoopsDB->exec error checks and $module->setErrors
calls to ensure the migration only runs when the column was newly added.
| $weight = 1; | ||
| while (false !== ($row = $xoopsDB->fetchArray($result))) { | ||
| $sql = sprintf('UPDATE %s SET weight = %u WHERE mid = %u', $tableName, $weight, (int) $row['mid']); | ||
| if (!$xoopsDB->exec($sql)) { | ||
| xoops_error($xoopsDB->error() . '<br>' . $sql); | ||
| $module->setErrors('Unable to reassign module weights during migration.'); | ||
|
|
||
| return false; | ||
| } | ||
| ++$weight; | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
N+1 query pattern in weight reassignment — consider single-query approach.
The loop issues one UPDATE per module. While acceptable for typical module counts, a single query using a MySQL user variable is more efficient and atomic.
♻️ Optional: Single-query weight assignment
- $weight = 1;
- while (false !== ($row = $xoopsDB->fetchArray($result))) {
- $sql = sprintf('UPDATE %s SET weight = %u WHERE mid = %u', $tableName, $weight, (int) $row['mid']);
- if (!$xoopsDB->exec($sql)) {
- xoops_error($xoopsDB->error() . '<br>' . $sql);
- $module->setErrors('Unable to reassign module weights during migration.');
-
- return false;
- }
- ++$weight;
+ // Atomic weight reassignment via user variable
+ $sql = "SET `@w` = 0";
+ if (!$xoopsDB->exec($sql)) {
+ xoops_error($xoopsDB->error() . '<br>' . $sql);
+ $module->setErrors('Unable to initialize weight counter.');
+ return false;
+ }
+
+ $sql = "UPDATE {$tableName} SET weight = (`@w` := `@w` + 1) WHERE dirname <> 'system' ORDER BY weight ASC, mid ASC";
+ if (!$xoopsDB->exec($sql)) {
+ xoops_error($xoopsDB->error() . '<br>' . $sql);
+ $module->setErrors('Unable to reassign module weights during migration.');
+ return false;
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@htdocs/modules/system/include/update.php` around lines 108 - 118, The loop in
update.php that uses $xoopsDB->fetchArray($result) and issues one UPDATE per row
via $xoopsDB->exec($sql) should be replaced with a single SQL statement that
assigns incremental weights (for example using a MySQL user variable like `@w`) so
the operation is executed in one atomic query instead of N updates; modify the
code that currently initializes $weight and increments it inside the while to
instead build and execute one UPDATE ... JOIN (or UPDATE with a subselect that
uses `@w`) ordered as needed, and on failure still call $module->setErrors('Unable
to reassign module weights during migration.') and return false to preserve
existing error handling.
| } | ||
|
|
||
| $sql = "UPDATE {$tableName} SET show_in_menu = CASE WHEN weight > 0 THEN 1 ELSE 0 END"; | ||
| if (!$xoopsDB->exec($sql)) { | ||
| xoops_error($xoopsDB->error() . '<br>' . $sql); | ||
| $module->setErrors('Unable to migrate module menu visibility.'); | ||
|
|
||
| return false; |
There was a problem hiding this comment.
The migration backfills show_in_menu unconditionally. If the column already exists (e.g., partially completed upgrade retry, or a site that already has the column), this will overwrite any existing menu visibility choices based on the current weight values. Consider only setting show_in_menu when the column is newly added (or when it’s still uninitialized), so rerunning the update is idempotent and won’t reset visibility.
| } | |
| $sql = "UPDATE {$tableName} SET show_in_menu = CASE WHEN weight > 0 THEN 1 ELSE 0 END"; | |
| if (!$xoopsDB->exec($sql)) { | |
| xoops_error($xoopsDB->error() . '<br>' . $sql); | |
| $module->setErrors('Unable to migrate module menu visibility.'); | |
| return false; | |
| $sql = "UPDATE {$tableName} SET show_in_menu = CASE WHEN weight > 0 THEN 1 ELSE 0 END"; | |
| if (!$xoopsDB->exec($sql)) { | |
| xoops_error($xoopsDB->error() . '<br>' . $sql); | |
| $module->setErrors('Unable to migrate module menu visibility.'); | |
| return false; | |
| } |
| $module = $module_handler->get($module_id); | ||
| $old = $module->getVar('weight'); | ||
| // Set value | ||
| $module->setVar('weight', !$old); | ||
| $old = (int) $module->getVar('show_in_menu'); | ||
| $module->setVar('show_in_menu', $old ? 0 : 1); | ||
| if (!$module_handler->insert($module)) { |
There was a problem hiding this comment.
There are existing unit tests (tests/unit/htdocs/modules/system/admin/modulesadmin/MainControllerTest.php) that assert display_in_menu toggles weight. With this change switching to show_in_menu, those tests need to be updated and should add coverage asserting show_in_menu is toggled instead.
| foreach (Request::getArray('mod', [], 'POST') as $order) { | ||
| if ($order > 0) { | ||
| $module = $module_handler->get($order); | ||
| //Change order only for visible modules | ||
| if ($module->getVar('weight') != 0) { | ||
| $module->setVar('weight', $i); | ||
| if (!$module_handler->insert($module)) { | ||
| $error = true; | ||
| } | ||
| ++$i; | ||
| $module->setVar('weight', $i); | ||
| if (!$module_handler->insert($module)) { |
There was a problem hiding this comment.
There are existing unit tests (tests/unit/htdocs/modules/system/admin/modulesadmin/MainControllerTest.php) that assert the order operation only reorders visible modules (weight != 0). Since this endpoint now reorders all modules, those tests should be updated to match the new behavior and to cover that hidden modules are included in the weight reassignment.
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
htdocs/kernel/module.php (1)
49-71:⚠️ Potential issue | 🟠 MajorKeep legacy rows from defaulting to visible before the migration runs.
show_in_menudefaults to1, so whenget(),getByDirname(), orgetObjects()reads from a pre-2.1.10modulestable the missing column never overwrites that value. Legacy modules that were hidden viaweight = 0are then exposed asshow_in_menu = 1in the admin list/templates until the updater backfills the column. Please deriveshow_in_menufromweight > 0whenever the fetched row does not contain the new field.🔁 Suggested read-side fallback
private function applyLegacyShowInMenuFallback(XoopsModule $module, array $row): void { if (!array_key_exists('show_in_menu', $row)) { $module->setVar('show_in_menu', !empty($row['weight']) ? 1 : 0); } }Call this right after each
assignVars($myrow)inXoopsModuleHandler::get(),XoopsModuleHandler::getByDirname(), andXoopsModuleHandler::getObjects().As per coding guidelines,
htdocs/kernel/**/*.php: Core kernel classes. Pay special attention to security: input filtering, output escaping, SQL parameterization, and proper use of XOOPS database API. Check for backward compatibility.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@htdocs/kernel/module.php` around lines 49 - 71, The module visibility defaults to 1 and causes legacy rows without a show_in_menu column to appear visible; implement a read-side fallback: add a private helper (e.g., applyLegacyShowInMenuFallback(XoopsModule $module, array $row): void) in XoopsModuleHandler that checks if array_key_exists('show_in_menu', $row) is false and then calls $module->setVar('show_in_menu', !empty($row['weight']) ? 1 : 0); and invoke this helper immediately after each assignVars($myrow) call in XoopsModuleHandler::get(), XoopsModuleHandler::getByDirname(), and XoopsModuleHandler::getObjects() so legacy modules derive visibility from weight > 0 until the migration backfills the column.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@htdocs/modules/system/admin/modulesadmin/main.php`:
- Around line 194-198: Guard against XoopsModuleHandler::get() returning false
before dereferencing $module: after calling $module = $module_handler->get($mid)
check that $module !== false (or is_object($module)) and if not, set $error =
true and continue the loop so you don't call $module->setVar() or
$module->getVar() or attempt $module_handler->insert(); apply the same
validation to the other occurrence around lines 277-279 so unknown/tampered IDs
are skipped and earlier updates are not left half-committed.
- Around line 194-198: The module state mutations (reordering via
$module->setVar('weight', $i) with $module_handler->insert($module) and the
display toggle via $module->setVar('display_in_menu', ...) / insert) must be
protected by XOOPS security token checks; before performing these mutations call
$GLOBALS['xoopsSecurity']->check() and abort/return an error (or redirect) when
it fails, and ensure the AJAX endpoints expect and validate the token (and
return a clear error) so only requests with a valid token can reach the
$module_handler->insert(...) paths.
In `@htdocs/modules/system/include/update.php`:
- Around line 91-118: Wrap the DML backfill and renumbering in a DB transaction:
after the new column exists, start a transaction (e.g. $xoopsDB->exec('START
TRANSACTION')), then perform the first UPDATE that sets show_in_menu and the
loop that updates weight (the SQL at the UPDATE {$tableName} SET show_in_menu...
and the loop using $xoopsDB->fetchArray/$xoopsDB->exec to UPDATE weight). On any
failure use $xoopsDB->exec('ROLLBACK') before returning false (preserve existing
xoops_error and $module->setErrors calls), and if the entire sequence succeeds
commit with $xoopsDB->exec('COMMIT') before returning; this ensures the backfill
and renumbering are atomic.
- Around line 35-39: The migration gate currently uses PHP's version_compare on
the raw $prev_version string which may include suffixes; replace that check to
use XoopsModule::versionCompare so versions like "2.1.10-Stable" are normalized
before comparing (i.e., change the condition that now calls
version_compare((string) $prev_version, '2.1.10', '<') to use
XoopsModule::versionCompare($prev_version, '2.1.10', '<') around the block that
calls system_modules_add_menu_visibility($module) so the 2.1.10 migration runs
only for the intended versions).
---
Outside diff comments:
In `@htdocs/kernel/module.php`:
- Around line 49-71: The module visibility defaults to 1 and causes legacy rows
without a show_in_menu column to appear visible; implement a read-side fallback:
add a private helper (e.g., applyLegacyShowInMenuFallback(XoopsModule $module,
array $row): void) in XoopsModuleHandler that checks if
array_key_exists('show_in_menu', $row) is false and then calls
$module->setVar('show_in_menu', !empty($row['weight']) ? 1 : 0); and invoke this
helper immediately after each assignVars($myrow) call in
XoopsModuleHandler::get(), XoopsModuleHandler::getByDirname(), and
XoopsModuleHandler::getObjects() so legacy modules derive visibility from weight
> 0 until the migration backfills the column.
🪄 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: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: d65955d9-b062-49ba-9124-27e6e74f3000
📒 Files selected for processing (9)
htdocs/install/sql/mysql.structure.sqlhtdocs/kernel/module.phphtdocs/modules/system/admin/modulesadmin/main.phphtdocs/modules/system/admin/modulesadmin/modulesadmin.phphtdocs/modules/system/blocks/system_blocks.phphtdocs/modules/system/include/update.phphtdocs/modules/system/templates/admin/system_modules.tplhtdocs/modules/system/themes/dark/modules/system/admin/system_modules.tplhtdocs/modules/system/xoops_version.php
| $module->setVar('weight', $i); | ||
| if (!$module_handler->insert($module)) { | ||
| $error = true; | ||
| } | ||
| ++$i; |
There was a problem hiding this comment.
Validate the module lookup before calling setVar() or getVar().
XoopsModuleHandler::get() returns false on an unknown ID. Both paths dereference it immediately, so a stale or tampered request becomes a fatal error; in the reorder loop that can leave earlier weight updates committed before the crash.
🛡️ Suggested guard
foreach (Request::getArray('mod', [], 'POST') as $order) {
if ($order > 0) {
$module = $module_handler->get($order);
+ if (!$module instanceof XoopsModule) {
+ $error = true;
+ break;
+ }
$module->setVar('weight', $i);
if (!$module_handler->insert($module)) {
$error = true;
+ break;
}
++$i;
}
}if ($module_id > 0) {
$module = $module_handler->get($module_id);
+ if (!$module instanceof XoopsModule) {
+ $error = true;
+ break;
+ }
$old = (int) $module->getVar('show_in_menu');
$module->setVar('show_in_menu', $old ? 0 : 1);Also applies to: 277-279
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@htdocs/modules/system/admin/modulesadmin/main.php` around lines 194 - 198,
Guard against XoopsModuleHandler::get() returning false before dereferencing
$module: after calling $module = $module_handler->get($mid) check that $module
!== false (or is_object($module)) and if not, set $error = true and continue the
loop so you don't call $module->setVar() or $module->getVar() or attempt
$module_handler->insert(); apply the same validation to the other occurrence
around lines 277-279 so unknown/tampered IDs are skipped and earlier updates are
not left half-committed.
Protect these AJAX mutations with a XOOPS security token.
order and display_in_menu both persist module state, but neither branch is covered by the $GLOBALS['xoopsSecurity']->check() whitelist above. That leaves module reordering and menu-visibility toggles CSRFable against any authenticated admin session.
Also applies to: 270-279
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@htdocs/modules/system/admin/modulesadmin/main.php` around lines 194 - 198,
The module state mutations (reordering via $module->setVar('weight', $i) with
$module_handler->insert($module) and the display toggle via
$module->setVar('display_in_menu', ...) / insert) must be protected by XOOPS
security token checks; before performing these mutations call
$GLOBALS['xoopsSecurity']->check() and abort/return an error (or redirect) when
it fails, and ensure the AJAX endpoints expect and validate the token (and
return a clear error) so only requests with a valid token can reach the
$module_handler->insert(...) paths.
| if (version_compare((string) $prev_version, '2.1.10', '<')) { | ||
| if (!system_modules_add_menu_visibility($module)) { | ||
| $ret = false; | ||
| } | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
sed -n '24,39p' htdocs/modules/system/include/update.php
sed -n '835,896p' htdocs/modules/system/admin/modulesadmin/modulesadmin.php
sed -n '1,35p' htdocs/kernel/object.php
sed -n '196,215p' htdocs/kernel/module.phpRepository: XOOPS/XoopsCore25
Length of output: 5904
Use XoopsModule::versionCompare() for the 2.1.10 migration gate.
Line 35 uses PHP's version_compare() on the raw version string, but module versions stored in $module->getVar('version') may include suffixes like -Stable. The XoopsModule::versionCompare() method already normalizes these suffixes before comparison. Without that normalization, the migration gate can run or skip on the wrong versions, which is risky because the migration rewrites menu visibility.
Proposed fix
- if (version_compare((string) $prev_version, '2.1.10', '<')) {
+ if ($module->versionCompare($prev_version, '2.1.10', '<')) {
if (!system_modules_add_menu_visibility($module)) {
$ret = false;
}
}🧰 Tools
🪛 GitHub Check: SonarCloud Code Analysis
[warning] 36-36: Merge this if statement with the enclosing one.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@htdocs/modules/system/include/update.php` around lines 35 - 39, The migration
gate currently uses PHP's version_compare on the raw $prev_version string which
may include suffixes; replace that check to use XoopsModule::versionCompare so
versions like "2.1.10-Stable" are normalized before comparing (i.e., change the
condition that now calls version_compare((string) $prev_version, '2.1.10', '<')
to use XoopsModule::versionCompare($prev_version, '2.1.10', '<') around the
block that calls system_modules_add_menu_visibility($module) so the 2.1.10
migration runs only for the intended versions).
| $sql = "UPDATE {$tableName} SET show_in_menu = CASE WHEN weight > 0 THEN 1 ELSE 0 END"; | ||
| if (!$xoopsDB->exec($sql)) { | ||
| xoops_error($xoopsDB->error() . '<br>' . $sql); | ||
| $module->setErrors('Unable to migrate module menu visibility.'); | ||
|
|
||
| return false; | ||
| } | ||
|
|
||
| $sql = "SELECT mid FROM {$tableName} WHERE dirname <> 'system' ORDER BY weight ASC, mid ASC"; | ||
| $result = $xoopsDB->query($sql); | ||
| if (!$xoopsDB->isResultSet($result) || !($result instanceof \mysqli_result)) { | ||
| xoops_error($xoopsDB->error() . '<br>' . $sql); | ||
| $module->setErrors('Unable to load modules for weight migration.'); | ||
|
|
||
| return false; | ||
| } | ||
|
|
||
| $weight = 1; | ||
| while (false !== ($row = $xoopsDB->fetchArray($result))) { | ||
| $sql = sprintf('UPDATE %s SET weight = %u WHERE mid = %u', $tableName, $weight, (int) $row['mid']); | ||
| if (!$xoopsDB->exec($sql)) { | ||
| xoops_error($xoopsDB->error() . '<br>' . $sql); | ||
| $module->setErrors('Unable to reassign module weights during migration.'); | ||
|
|
||
| return false; | ||
| } | ||
| ++$weight; | ||
| } |
There was a problem hiding this comment.
Make the DML phase of this migration atomic.
After the schema check, Lines 91-117 backfill show_in_menu and then renumber weights one row at a time. If a later UPDATE fails, the function returns with a partially migrated modules table, and the next retry will sort by intermediate weights instead of the original order this helper says it preserves. Start a transaction once the column exists and roll back the Line 91 backfill plus the renumbering loop on failure.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@htdocs/modules/system/include/update.php` around lines 91 - 118, Wrap the DML
backfill and renumbering in a DB transaction: after the new column exists, start
a transaction (e.g. $xoopsDB->exec('START TRANSACTION')), then perform the first
UPDATE that sets show_in_menu and the loop that updates weight (the SQL at the
UPDATE {$tableName} SET show_in_menu... and the loop using
$xoopsDB->fetchArray/$xoopsDB->exec to UPDATE weight). On any failure use
$xoopsDB->exec('ROLLBACK') before returning false (preserve existing xoops_error
and $module->setErrors calls), and if the entire sequence succeeds commit with
$xoopsDB->exec('COMMIT') before returning; this ensures the backfill and
renumbering are atomic.
kernel/module.php insert() fails on upgrade from pre-2.1.10 schemas because it writes show_in_menu before the migration adds the column. Added cached column detection so INSERT/UPDATE omit show_in_menu when it doesn't exist yet. Same fallback in system_blocks.php menu query. Updated two tests to match the new toggle and ordering logic.
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## master #1677 +/- ##
=============================================
+ Coverage 0 19.16% +19.16%
- Complexity 0 7461 +7461
=============================================
Files 0 620 +620
Lines 0 39150 +39150
=============================================
+ Hits 0 7502 +7502
- Misses 0 31648 +31648 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Pre-2.1.10 schemas lack show_in_menu, so loaded modules defaulted to visible. Added assignVars override to derive from weight > 0 when column absent. Moved column detection to resettable class static so the migration's ALTER TABLE is picked up within the same request.
|
| $db = \XoopsDatabaseFactory::getDatabaseConnection(); | ||
| $colCheck = $db->query("SHOW COLUMNS FROM " . $db->prefix('modules') . " LIKE 'show_in_menu'"); | ||
| if ($db->isResultSet($colCheck) && ($colCheck instanceof \mysqli_result) && false !== $db->fetchArray($colCheck)) { |
There was a problem hiding this comment.
This block runs a SHOW COLUMNS probe on every call to b_system_main_show(), which adds an extra schema query to each page render where the block appears. Cache the column-existence result (e.g. via a static variable) or reuse the per-request cached check already implemented in XoopsModuleHandler, so the schema probe happens at most once per request.
| $db = \XoopsDatabaseFactory::getDatabaseConnection(); | |
| $colCheck = $db->query("SHOW COLUMNS FROM " . $db->prefix('modules') . " LIKE 'show_in_menu'"); | |
| if ($db->isResultSet($colCheck) && ($colCheck instanceof \mysqli_result) && false !== $db->fetchArray($colCheck)) { | |
| static $hasShowInMenu = null; | |
| if (null === $hasShowInMenu) { | |
| $db = \XoopsDatabaseFactory::getDatabaseConnection(); | |
| $colCheck = $db->query("SHOW COLUMNS FROM " . $db->prefix('modules') . " LIKE 'show_in_menu'"); | |
| $hasShowInMenu = $db->isResultSet($colCheck) | |
| && ($colCheck instanceof \mysqli_result) | |
| && false !== $db->fetchArray($colCheck); | |
| } | |
| if ($hasShowInMenu) { |
| parent::assignVars($var_arr); | ||
| // Pre-2.1.10 schemas lack show_in_menu; derive from legacy weight convention | ||
| if (is_array($var_arr) && !array_key_exists('show_in_menu', $var_arr)) { | ||
| $this->assignVar('show_in_menu', ($this->getVar('weight') > 0) ? 1 : 0); | ||
| } |
There was a problem hiding this comment.
New backward-compat behavior is introduced here (deriving show_in_menu when the DB row lacks the column). There are existing unit tests for XoopsModule, but none validate this derivation (or the related handler insert() behavior that conditionally includes show_in_menu based on schema). Add unit tests covering assignVars() with/without show_in_menu in the input row, and ensuring inserts/updates don’t reference the column when it’s absent.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (3)
htdocs/modules/system/include/update.php (3)
110-120:⚠️ Potential issue | 🟡 MinorConsider wrapping DML operations in a transaction for atomicity.
The weight reassignment loop performs N individual
UPDATEstatements. If any fails mid-way, themodulestable will be left in an inconsistent state with partially assigned weights. This could cause unpredictable ordering on retry.🛡️ Proposed fix: Add transaction wrapper
+ $xoopsDB->exec('START TRANSACTION'); + $sql = "UPDATE {$tableName} SET show_in_menu = CASE WHEN weight > 0 THEN 1 ELSE 0 END"; if (!$xoopsDB->exec($sql)) { xoops_error($xoopsDB->error() . '<br>' . $sql); $module->setErrors('Unable to migrate module menu visibility.'); + $xoopsDB->exec('ROLLBACK'); return false; } $sql = "SELECT mid FROM {$tableName} WHERE dirname <> 'system' ORDER BY weight ASC, mid ASC"; $result = $xoopsDB->query($sql); if (!$xoopsDB->isResultSet($result) || !($result instanceof \mysqli_result)) { xoops_error($xoopsDB->error() . '<br>' . $sql); $module->setErrors('Unable to load modules for weight migration.'); + $xoopsDB->exec('ROLLBACK'); return false; } $weight = 1; while (false !== ($row = $xoopsDB->fetchArray($result))) { $sql = sprintf('UPDATE %s SET weight = %u WHERE mid = %u', $tableName, $weight, (int) $row['mid']); if (!$xoopsDB->exec($sql)) { xoops_error($xoopsDB->error() . '<br>' . $sql); $module->setErrors('Unable to reassign module weights during migration.'); + $xoopsDB->exec('ROLLBACK'); return false; } ++$weight; } + $xoopsDB->exec('COMMIT'); + return true;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@htdocs/modules/system/include/update.php` around lines 110 - 120, Wrap the weight reassignment loop in a database transaction to ensure atomicity: before the loop call the DB begin transaction method on $xoopsDB, run the loop that updates $tableName using $xoopsDB->exec($sql) as currently done, and if any exec fails call rollback and return false (preserving the existing xoops_error and $module->setErrors), otherwise commit the transaction after the loop completes; use $xoopsDB methods for begin/commit/rollback where available to locate and wrap the existing code around the while (false !== ($row = $xoopsDB->fetchArray($result))) { ... } block.
35-39:⚠️ Potential issue | 🟡 MinorUse
XoopsModule::versionCompare()for version string normalization.Module versions may include suffixes like
-Stableor-RC1. The nativeversion_compare()doesn't normalize these, butXoopsModule::versionCompare()does (see lines 210-230 inhtdocs/kernel/module.php). This could cause the migration to run or skip incorrectly.🔧 Proposed fix
- if (version_compare((string) $prev_version, '2.1.10', '<')) { + if ($module->versionCompare($prev_version, '2.1.10', '<')) { if (!system_modules_add_menu_visibility($module)) { $ret = false; } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@htdocs/modules/system/include/update.php` around lines 35 - 39, The version check uses PHP's version_compare which doesn't normalize suffixes; update the check to call XoopsModule::versionCompare($prev_version, '2.1.10', '<') (the static normalizing comparator in htdocs/kernel/module.php) instead of version_compare, so the migration correctly handles versions with suffixes; keep the existing conditional body (the call to system_modules_add_menu_visibility($module) and setting $ret = false) intact.
93-99:⚠️ Potential issue | 🟠 MajorData migration runs unconditionally — risk of overwriting admin changes on re-run.
The
UPDATEat line 93-98 executes regardless of whether the column was just added or already existed. If an upgrade partially completes (column added but version not recorded), a subsequent run will resetshow_in_menuvalues based onweight, overwriting any manual admin adjustments.Move the data migration inside the column-addition block to ensure it only runs once when the column is first created.
🐛 Proposed fix
$columnExists = false !== $xoopsDB->fetchArray($result); if (!$columnExists) { $sql = "ALTER TABLE {$tableName} ADD show_in_menu tinyint(1) unsigned NOT NULL default '1' AFTER weight"; if (!$xoopsDB->exec($sql)) { xoops_error($xoopsDB->error() . '<br>' . $sql); $module->setErrors('Unable to add show_in_menu to the modules table.'); return false; } // Reset cached column check so insert() picks up the new column XoopsModuleHandler::resetShowInMenuCache(); - } - $sql = "UPDATE {$tableName} SET show_in_menu = CASE WHEN weight > 0 THEN 1 ELSE 0 END"; - if (!$xoopsDB->exec($sql)) { - xoops_error($xoopsDB->error() . '<br>' . $sql); - $module->setErrors('Unable to migrate module menu visibility.'); + // Only migrate data when column is newly added + $sql = "UPDATE {$tableName} SET show_in_menu = CASE WHEN weight > 0 THEN 1 ELSE 0 END"; + if (!$xoopsDB->exec($sql)) { + xoops_error($xoopsDB->error() . '<br>' . $sql); + $module->setErrors('Unable to migrate module menu visibility.'); - return false; + return false; + } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@htdocs/modules/system/include/update.php` around lines 93 - 99, The migration UPDATE ($sql = "UPDATE {$tableName} SET show_in_menu = CASE WHEN weight > 0 THEN 1 ELSE 0 END"; plus the xoopsDB->exec($sql), xoops_error(...) and $module->setErrors(...) handling) must be moved inside the block that actually creates/adds the show_in_menu column so it only runs when the column is newly added; locate the column-addition branch (the code that performs ALTER TABLE / add column) and paste this UPDATE and its error handling there, removing the unconditional execution so re-runs won't overwrite admin changes.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@htdocs/modules/system/blocks/system_blocks.php`:
- Around line 124-131: Replace the local SHOW COLUMNS query in system_blocks.php
with the centralized schema check in XoopsModuleHandler: make the existing
private static method hasShowInMenuColumn() in class XoopsModuleHandler public
static (or add a small public wrapper like hasShowInMenuColumnExists()) and then
call XoopsModuleHandler::hasShowInMenuColumn() from system_blocks.php; based on
its boolean result keep the existing behavior (if true -> $criteria->add(new
Criteria('show_in_menu', 1)); else -> $criteria->add(new Criteria('weight', 0,
'>'))), and remove the redundant $db->query / SHOW COLUMNS logic.
---
Duplicate comments:
In `@htdocs/modules/system/include/update.php`:
- Around line 110-120: Wrap the weight reassignment loop in a database
transaction to ensure atomicity: before the loop call the DB begin transaction
method on $xoopsDB, run the loop that updates $tableName using
$xoopsDB->exec($sql) as currently done, and if any exec fails call rollback and
return false (preserving the existing xoops_error and $module->setErrors),
otherwise commit the transaction after the loop completes; use $xoopsDB methods
for begin/commit/rollback where available to locate and wrap the existing code
around the while (false !== ($row = $xoopsDB->fetchArray($result))) { ... }
block.
- Around line 35-39: The version check uses PHP's version_compare which doesn't
normalize suffixes; update the check to call
XoopsModule::versionCompare($prev_version, '2.1.10', '<') (the static
normalizing comparator in htdocs/kernel/module.php) instead of version_compare,
so the migration correctly handles versions with suffixes; keep the existing
conditional body (the call to system_modules_add_menu_visibility($module) and
setting $ret = false) intact.
- Around line 93-99: The migration UPDATE ($sql = "UPDATE {$tableName} SET
show_in_menu = CASE WHEN weight > 0 THEN 1 ELSE 0 END"; plus the
xoopsDB->exec($sql), xoops_error(...) and $module->setErrors(...) handling) must
be moved inside the block that actually creates/adds the show_in_menu column so
it only runs when the column is newly added; locate the column-addition branch
(the code that performs ALTER TABLE / add column) and paste this UPDATE and its
error handling there, removing the unconditional execution so re-runs won't
overwrite admin changes.
🪄 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: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: e3e344d0-d2d7-46e3-8721-ccb80670ce62
📒 Files selected for processing (4)
htdocs/kernel/module.phphtdocs/modules/system/blocks/system_blocks.phphtdocs/modules/system/include/update.phptests/unit/htdocs/modules/system/admin/modulesadmin/MainControllerTest.php
| // show_in_menu may not exist on pre-2.1.10 schemas; fall back to weight > 0 | ||
| $db = \XoopsDatabaseFactory::getDatabaseConnection(); | ||
| $colCheck = $db->query("SHOW COLUMNS FROM " . $db->prefix('modules') . " LIKE 'show_in_menu'"); | ||
| if ($db->isResultSet($colCheck) && ($colCheck instanceof \mysqli_result) && false !== $db->fetchArray($colCheck)) { | ||
| $criteria->add(new Criteria('show_in_menu', 1)); | ||
| } else { | ||
| $criteria->add(new Criteria('weight', 0, '>')); | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Redundant schema check duplicates logic from XoopsModuleHandler.
This code performs a SHOW COLUMNS query on every page load where the main menu block renders. The identical column-detection logic already exists in XoopsModuleHandler::hasShowInMenuColumn() (see htdocs/kernel/module.php lines 869-877) with per-request caching.
Since hasShowInMenuColumn() is private static, consider either:
- Making it
public staticso callers like this block can reuse it, or - Adding a public wrapper method to
XoopsModuleHandler
This would eliminate the redundant database round-trip and centralize the schema detection.
♻️ Proposed approach: Add public accessor to XoopsModuleHandler
In htdocs/kernel/module.php:
+ /**
+ * Public accessor for show_in_menu column existence check.
+ *
+ * `@return` bool
+ */
+ public static function hasShowInMenu(): bool
+ {
+ $db = \XoopsDatabaseFactory::getDatabaseConnection();
+ return self::hasShowInMenuColumn($db);
+ }Then in this file:
- // show_in_menu may not exist on pre-2.1.10 schemas; fall back to weight > 0
- $db = \XoopsDatabaseFactory::getDatabaseConnection();
- $colCheck = $db->query("SHOW COLUMNS FROM " . $db->prefix('modules') . " LIKE 'show_in_menu'");
- if ($db->isResultSet($colCheck) && ($colCheck instanceof \mysqli_result) && false !== $db->fetchArray($colCheck)) {
+ // show_in_menu may not exist on pre-2.1.10 schemas; fall back to weight > 0
+ /** `@var` XoopsModuleHandler $module_handler */
+ if (XoopsModuleHandler::hasShowInMenu()) {
$criteria->add(new Criteria('show_in_menu', 1));
} else {
$criteria->add(new Criteria('weight', 0, '>'));
}🧰 Tools
🪛 PHPMD (2.15.0)
[error] 125-125: Avoid using static access to class '\XoopsDatabaseFactory' in method 'b_system_main_show'. (undefined)
(StaticAccess)
[warning] 125-125: Avoid variables with short names like $db. Configured minimum length is 3. (undefined)
(ShortVariable)
[error] 128-128: Missing class import via use statement (line '128', column '28'). (undefined)
(MissingImport)
[error] 129-131: The method b_system_main_show uses an else expression. Else clauses are basically not necessary and you can simplify the code by not using them. (undefined)
(ElseExpression)
[error] 130-130: Missing class import via use statement (line '130', column '28'). (undefined)
(MissingImport)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@htdocs/modules/system/blocks/system_blocks.php` around lines 124 - 131,
Replace the local SHOW COLUMNS query in system_blocks.php with the centralized
schema check in XoopsModuleHandler: make the existing private static method
hasShowInMenuColumn() in class XoopsModuleHandler public static (or add a small
public wrapper like hasShowInMenuColumnExists()) and then call
XoopsModuleHandler::hasShowInMenuColumn() from system_blocks.php; based on its
boolean result keep the existing behavior (if true -> $criteria->add(new
Criteria('show_in_menu', 1)); else -> $criteria->add(new Criteria('weight', 0,
'>'))), and remove the redundant $db->query / SHOW COLUMNS logic.
#812
Module weight was overloaded as both sort position and menu visibility
flag (weight=0 meant hidden), preventing hidden modules from being
reordered via drag-and-drop. Adds show_in_menu column to decouple
the two concerns, with migration that preserves existing visibility
and assigns contiguous weights to all non-system modules.
Summary by CodeRabbit
New Features
Chores
Bug Fixes