Closed
Conversation
…arent. On ne peut pas modifier ou supprimer un élément désactivé
…ur il y avait une erreur.
|
Warning Rate limit exceeded
⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughAdds a menu management subsystem: kernel classes for categories/items, admin CRUD and AJAX reordering/toggle UI, theme integration to load/render menus (including inbox unread count), DB migration and config, frontend multilevel menu support, templates, assets, and language entries. Changes
Sequence Diagram(s)sequenceDiagram
participant Theme as Theme Init
participant Loader as loadMenus()
participant DB as Database
participant Tree as SystemMenusTree
participant Template as Template Engine
Theme->>Loader: xoInit() calls loadMenus()
Loader->>DB: Query active categories
DB-->>Loader: categories
loop per category
Loader->>DB: Query items for category
DB-->>Loader: items
Loader->>Tree: build hierarchical structure
Tree-->>Loader: nested items
Loader->>DB: fetch group permissions
DB-->>Loader: permissions
end
Loader->>Template: assign xoMenuCategories (+affixes)
Template->>Template: nav-menu.tpl calls renderMenu() recursively
Template-->>Theme: rendered navbar HTML
sequenceDiagram
participant Admin as Admin User
participant Browser as Browser
participant JS as menus.js
participant AdminPHP as admin/menus/main.php
participant DB as Database
Admin->>Browser: Click reorder or toggle
Browser->>JS: event handler
JS->>AdminPHP: AJAX POST (saveorder / toggle) with token
AdminPHP->>DB: update category/item state or positions
AdminPHP->>DB: cascade updates to descendants (if toggle)
AdminPHP-->>JS: JSON response (success, updated IDs, new token)
JS->>Browser: update UI badges/locks and token
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
📝 Coding Plan
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Pull request overview
Adds a new “Menus” management feature in the System module, including admin UI for managing menu categories/items and frontend rendering via theme-provided navigation data (with multilevel dropdown support).
Changes:
- Introduces admin-side menus manager (templates, language strings, JS/CSS, and admin controller).
- Adds new kernel handlers/objects for
menuscategoryandmenusitems, plus theme initialization that loads menus with permission filtering. - Updates xswatch5 navbar template to render menus dynamically and adds shared multilevel dropdown assets.
Reviewed changes
Copilot reviewed 23 out of 27 changed files in this pull request and generated 15 comments.
Show a summary per file
| File | Description |
|---|---|
| htdocs/themes/xswatch5/tpl/nav-menu.tpl | Switches navbar rendering to dynamic categories/items and supports nested dropdowns. |
| htdocs/themes/xswatch5/style.css | Whitespace-only cleanup around page nav CSS. |
| htdocs/modules/system/xoops_version.php | Bumps system module version/credits and registers system_menus.tpl admin template. |
| htdocs/modules/system/themes/cpadmin/theme.tpl | Whitespace-only cleanup in admin theme template. |
| htdocs/modules/system/themes/cpadmin/cpadmin.php | Whitespace-only cleanup in admin theme PHP. |
| htdocs/modules/system/templates/admin/system_menus.tpl | New admin template for listing/editing categories and viewing items (with toggle UI + sortable). |
| htdocs/modules/system/language/english/modinfo.php | Adds modinfo constant for “active menus” preference. |
| htdocs/modules/system/language/english/menus/menus.dist.php | Adds a distribution language file placeholder for menu constants. |
| htdocs/modules/system/language/english/menus/index.php | Adds 404 index guard for the new language directory. |
| htdocs/modules/system/language/english/menu.php | Formatting-only change. |
| htdocs/modules/system/language/english/admin/menus.php | New English language strings for the menus admin feature + default menu constants. |
| htdocs/modules/system/language/english/admin.php | Adds System admin labels/desc for the new Menus section. |
| htdocs/modules/system/js/multilevelmenu.js | New shared JS to handle nested dropdown submenu toggling. |
| htdocs/modules/system/js/menus.js | New admin JS for sortable categories and active-state toggles with token refresh. |
| htdocs/modules/system/include/update.php | Adds DB schema creation + seed data + permissions/config insertion during module update. |
| htdocs/modules/system/css/multilevelmenu.css | New shared CSS for nested dropdown positioning/hover behavior. |
| htdocs/modules/system/css/menus.css | New admin CSS for sortable cards and disabled/inactive styling. |
| htdocs/modules/system/constants.php | Adds XOOPS_SYSTEM_MENUS constant. |
| htdocs/modules/system/class/SystemMenusTree.php | New tree helper intended to build hierarchical menu item lists. |
| htdocs/modules/system/admin/modulesadmin/modulesadmin.php | Adds menuscategory / menusitems to reserved tables list during install. |
| htdocs/modules/system/admin/menus/xoops_version.php | Registers the “menus” admin section metadata (name, icon, adminpath). |
| htdocs/modules/system/admin/menus/main.php | New admin controller implementing list/add/edit/delete and AJAX endpoints (order/toggle). |
| htdocs/modules/system/admin/menus/index.php | Adds 404 index guard for the new admin directory. |
| htdocs/modules/system/admin/menu.php | Adds Menus link to the System admin menu. |
| htdocs/kernel/menusitems.php | New kernel object + handler for menu items, including forms and constant resolution. |
| htdocs/kernel/menuscategory.php | New kernel object + handler for menu categories, including forms and constant resolution. |
| htdocs/class/theme.php | Loads menu categories/items (with permissions) into Smarty for theme navigation; injects shared multilevel assets. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+62
to
+67
| protected function addTree($fieldName, $productid, $key, $prefix_orig, $prefix_curr = '', $level = 1) | ||
| { | ||
| if ($key > 0) { | ||
| if (($productid == $this->tree[$key]['obj']->getVar('nomenclature_productid')) || $productid == 0) { | ||
| $value = $this->tree[$key]['obj']->getVar('nomenclature_id'); | ||
| $name = $prefix_curr . ' ' . $this->tree[$key]['obj']->getVar($fieldName); |
There was a problem hiding this comment.
SystemMenusTree::addTree() is still using copy/pasted field names (e.g., nomenclature_productid, nomenclature_id) that don’t exist on menu objects. This will prevent the tree from building correctly and can trigger undefined index/var behavior. Update this helper to use the menu fields/IDs (e.g., use the object’s items_id / items_pid, and remove the productid/nomenclature_* filtering entirely) or replace this class with XoopsObjectTree recursion.
Comment on lines
+11
to
+18
| document.addEventListener('DOMContentLoaded', function() { | ||
| document.querySelectorAll('.dropdown-submenu > a').forEach(function(el) { | ||
| el.addEventListener('click', function (e) { | ||
| e.preventDefault(); | ||
| e.stopPropagation(); | ||
| var sub = this.nextElementSibling; | ||
| if (sub) sub.classList.toggle('show'); | ||
| }); |
There was a problem hiding this comment.
multilevelmenu.js always calls preventDefault() for .dropdown-submenu > a. If a submenu parent has both children and a real URL, clicking it will never navigate to that URL (it will only toggle the submenu). Consider only preventing default when href is #/empty, or provide a separate toggle element so the link remains usable.
Comment on lines
+406
to
+408
| // load menu categories and their nested items so themes can render navigation | ||
| $this->template->assign('xoMenuCategories', $this->loadMenus()); | ||
|
|
There was a problem hiding this comment.
loadMenus() runs on every request during theme initialization and always does at least a SHOW TABLES LIKE ... query. This adds DB overhead site-wide and ignores the newly added active_menus config toggle. Consider gating this behind a config check (and/or caching the loaded menu structure per user groups/language) so pages that don’t need menus don’t pay the query cost.
Comment on lines
+34
to
+37
| }).fail(function(jqXHR, textStatus, errorThrown){ | ||
| console.error('Ajax error:', textStatus, errorThrown, jqXHR.responseText); | ||
| alert('Ajax error (voir console)'); | ||
| }); |
There was a problem hiding this comment.
The AJAX failure alert is partially in French (voir console) and isn’t localized, which makes the admin UI inconsistent for non-French users. Prefer using a language-provided message (similar to window.XOOPS_MENUS.messages) or at least an English default.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 16
Note
Due to the large number of review comments, Critical, Major 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 (1)
htdocs/modules/system/themes/cpadmin/theme.tpl (1)
138-161:⚠️ Potential issue | 🟠 MajorClose each preference leaf item unconditionally.
XoopsGuiCpadmin::getPreferences()supplies many$opentries without anoptionskey. In that case neither conditional emits</li>, so the sidebar treeview is rendered with malformed nested list markup.Suggested fix
<li class="nav-item"> <a href="<{$op.link}>" class="nav-link"> <i class="nav-icon <{$op.icon}>"></i> <p> - <{$op.title}><{if !empty($op.options) && $op.options != 0}><i class="fas fa-angle-left right"></i><{/if}> + <{$op.title}><{if !empty($op.options)}><i class="fas fa-angle-left right"></i><{/if}> </p> </a> - <{if !empty($op.options) && $op.options == 0}></li><{/if}> <{if !empty($op.options)}> <ul class="nav nav-treeview"> <{foreach item=sub from=$op.options|default:null}> <li class="nav-item"> <a href="<{$sub.link}>" class="nav-link"> @@ </li> <{/foreach}> </ul> <{/if}> - <{if !empty($op.options) && $op.options != 0}></li><{/if}> + </li>🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@htdocs/modules/system/themes/cpadmin/theme.tpl` around lines 138 - 161, The template emits malformed nested lists because some $op entries lack an options key and the closing </li> is only output conditionally; update theme.tpl so each preference <li> for items from $pref.menu ($op) is always closed: move or add the closing </li> outside/after the block that renders the optional subtree so that regardless of $op.options being unset, null, 0, or an array the <li> opened for $op is properly closed after rendering its possible <ul> (reference the foreach over $pref.menu, the $op.options checks and the inner foreach over $op.options).
🟡 Minor comments (5)
htdocs/modules/system/js/multilevelmenu.js-16-17 (1)
16-17:⚠️ Potential issue | 🟡 MinorMissing CSS rule for
.showclass may break click-based toggling.The JavaScript toggles the
.showclass on the submenu element, butmultilevelmenu.cssonly defines hover-based visibility (.dropdown-submenu:hover > .dropdown-menu { display: block; }). There's no corresponding.dropdown-menu.show { display: block; }rule.This means:
- Hover works on desktop (CSS handles it)
- Click toggle won't visually work unless the theme includes Bootstrap or a similar framework that provides the
.showruleConsider adding the missing rule to
multilevelmenu.css:.dropdown-submenu > .dropdown-menu.show { display: block; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@htdocs/modules/system/js/multilevelmenu.js` around lines 16 - 17, The click handler toggles the 'show' class on the submenu (see usage of this.nextElementSibling and sub.classList.toggle('show') in multilevelmenu.js) but multilevelmenu.css lacks a rule to make a '.dropdown-menu' visible when it has the 'show' class; add a CSS rule targeting '.dropdown-submenu > .dropdown-menu.show' that sets the menu to display (e.g. display: block) so click-based toggling works even without Bootstrap.htdocs/modules/system/templates/admin/system_menus.tpl-107-107 (1)
107-107:⚠️ Potential issue | 🟡 MinorMissing quotes around string literal in Smarty comparison.
The comparison
$op|default:'' == viewcatshould have quotes aroundviewcatto ensure proper string comparison:🐛 Proposed fix
-<{if $op|default:'' == viewcat}> +<{if $op|default:'' == 'viewcat'}>🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@htdocs/modules/system/templates/admin/system_menus.tpl` at line 107, The Smarty conditional is comparing $op|default:'' to an unquoted identifier (viewcat); change the comparison to use a string literal by quoting viewcat so the condition reads like $op|default:'' == 'viewcat' (locate the conditional in the template where <{if $op|default:'' == viewcat}> appears and update it to include the quotes).htdocs/modules/system/css/multilevelmenu.css-30-33 (1)
30-33:⚠️ Potential issue | 🟡 MinorAdd
.showclass rule for JavaScript-toggled visibility.The hover rule works for desktop users, but the companion JavaScript (
multilevelmenu.js) toggles a.showclass for click-based interaction. Without a corresponding CSS rule, the click behavior won't display the menu.🛠️ Proposed fix
/* show on hover for desktop users (optional) */ .dropdown-submenu:hover > .dropdown-menu { display: block; } +/* show when toggled via JavaScript (click/touch) */ +.dropdown-submenu > .dropdown-menu.show { + display: block; +}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@htdocs/modules/system/css/multilevelmenu.css` around lines 30 - 33, The hover rule only shows submenus for pointer hover; update the CSS to also show submenus when JavaScript toggles the .show class by adding a rule that mirrors the hover behavior (e.g., target .dropdown-submenu.show > .dropdown-menu or the .dropdown-menu.show variant used by multilevelmenu.js) so clicks reveal the menu; modify multilevelmenu.css to include this .show-based selector alongside the existing .dropdown-submenu:hover > .dropdown-menu rule to ensure JS-toggled visibility works.htdocs/class/theme.php-490-491 (1)
490-491:⚠️ Potential issue | 🟡 MinorFix inconsistent
setSort()usage.The
setSort()call includes a sort direction ('items_position ASC, items_title'), which is inconsistent with the codebase pattern. Throughout the codebase,setSort()is used with only column names, andsetOrder()handles the sort direction. Change tosetSort('items_position, items_title')to follow the established pattern where column names and directions are separated.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@htdocs/class/theme.php` around lines 490 - 491, Change the inconsistent sort specification on the criteria object: update the call to crit->setSort so it only lists column names (remove the "ASC" from 'items_position ASC, items_title') and rely on the existing crit->setOrder('ASC') call to supply direction; locate the crit variable usage around setSort() / setOrder() and replace the argument to setSort with 'items_position, items_title' so column names and sort direction are handled separately.htdocs/modules/system/language/english/admin/menus.php-13-21 (1)
13-21:⚠️ Potential issue | 🟡 MinorClean up the new English strings before shipping.
_AM_SYSTEM_MENUS_NAV_TIPScloses the list with<ul>instead of</ul>,_AM_SYSTEM_MENUS_ERROR_NOITEMis ungrammatical, and_AM_SYSTEM_MENUS_SUREDELITEMstill contains the French wordavec. These strings surface directly in the admin UI/dialogs.Suggested wording fix
-<ul>'); +</ul>'); @@ -define('_AM_SYSTEM_MENUS_ERROR_NOITEM', 'There are no submenu item.'); +define('_AM_SYSTEM_MENUS_ERROR_NOITEM', 'There are no submenu items.'); @@ -define('_AM_SYSTEM_MENUS_SUREDELITEM', 'Are you sure you want to delete this submenu item "%s"? avec the following submenu items?'); +define('_AM_SYSTEM_MENUS_SUREDELITEM', 'Are you sure you want to delete this submenu item "%s" and its submenu items?');Also applies to: 38-54
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@htdocs/modules/system/language/english/admin/menus.php` around lines 13 - 21, Fix the English copy and HTML in the menu language constants: correct the closing tag in _AM_SYSTEM_MENUS_NAV_TIPS from "<ul>" to "</ul>" and proofread/replace ungrammatical phrases in _AM_SYSTEM_MENUS_ERROR_NOITEM and _AM_SYSTEM_MENUS_SUREDELITEM (remove the French word "avec" and replace with appropriate English such as "with" or restructure the sentence), and apply the same cleanup to the other related menu constants in this file (the block around the other menu strings) so all admin UI/dialog messages are grammatically correct and use proper HTML.
🧹 Nitpick comments (4)
htdocs/modules/system/include/update.php (1)
58-77: INSERT statements use hardcoded auto-increment IDs.Using explicit ID values (e.g.,
VALUES (1, ...),VALUES (2, ...)) can cause issues if the auto-increment counter is not reset, or if data already exists. This also makes the foreign key relationships fragile if IDs don't match expectations.Consider using
NULLfor auto-increment columns and capturingLAST_INSERT_ID()when needed for foreign key references, or ensure the inserts are properly guarded against duplicates.🤖 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 58 - 77, The INSERTs in update.php into tables named via $xoopsDB->prefix('_menuscategory') and $xoopsDB->prefix('_menusitems') use hardcoded ID values (VALUES (1,...), (2,...)) which will break auto-increment and FK integrity; change those INSERTs to omit the id column or use NULL for the auto-increment column so the DB assigns IDs, and where code later relies on specific IDs capture the inserted id via LAST_INSERT_ID() (or use SELECT to find existing rows) instead of hardcoding; also add guards (e.g., EXISTS checks) before inserting to avoid duplicate rows when the update runs twice.htdocs/themes/xswatch5/tpl/nav-menu.tpl (1)
9-9: Remove the unused inbox-count assignment.
$unread_countis not referenced anywhere below, andrenderMenuAffix()already resolves<{xoInboxCount}>in menu prefixes/suffixes before render.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@htdocs/themes/xswatch5/tpl/nav-menu.tpl` at line 9, Remove the unused assignment call that sets unread_count: delete the "<{xoInboxCount assign='unread_count'}>" invocation in nav-menu.tpl because $unread_count is never used and renderMenuAffix already resolves <{xoInboxCount}> for menu prefixes/suffixes; ensure no other code relies on the unread_count variable and run a quick template render to confirm menus still display inbox counts via renderMenuAffix.htdocs/modules/system/js/menus.js (1)
163-170: Variable shadowing: inneridshadows outer scope variable.The
idparameter in the forEach callback shadows the outeridvariable declared at line 138. While this works correctly in this context, it can lead to confusion and potential bugs during maintenance.♻️ Proposed fix - rename inner variable
// if server sent list of updated children, adjust them as well if (response.updated && Array.isArray(response.updated)) { - response.updated.forEach(function(id) { - var $child = $('.item-active-toggle[data-id="' + id + '"]'); + response.updated.forEach(function(childId) { + var $child = $('.item-active-toggle[data-id="' + childId + '"]'); if ($child.length) { updateBadge($child, active); updateRowState($child, active); } }); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@htdocs/modules/system/js/menus.js` around lines 163 - 170, The callback in the response.updated.forEach uses a parameter named id which shadows the outer-scope id; rename the forEach parameter (e.g., to updatedId or childId) wherever referenced inside the callback so the selection $('.item-active-toggle[data-id="' + updatedId + '"]'), and calls to updateBadge(...) and updateRowState(...), use the new name; ensure no other references expect the old name and run tests to confirm behavior of updateBadge and updateRowState remains unchanged.htdocs/modules/system/language/english/menus/menus.dist.php (1)
6-6: Empty constant name may cause issues.The constant
MENUS_without a suffix and with an empty value appears to be an incomplete placeholder. If this file is meant to be a template, consider either removing this line or providing a meaningful example constant likedefine('MENUS_EXAMPLE', 'Example text');with a comment explaining its purpose.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@htdocs/modules/system/language/english/menus/menus.dist.php` at line 6, The define('MENUS_', '') line defines an empty/incomplete constant (MENUS_) and should be fixed: either remove this stray placeholder or replace it with a meaningful example constant (e.g., define('MENUS_EXAMPLE', 'Example text')) and add a brief comment explaining its purpose so maintainers know it’s a template/example; update occurrences of MENUS_ if any to the new name or remove them if unused.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@htdocs/class/theme.php`:
- Line 460: $groups may be an integer when not logged in (XOOPS_GROUP_ANONYMOUS)
but later code (e.g. $gpermHandler->getItemIds()) expects an array; ensure
$groups is always an array by replacing the assignment with logic that yields an
array of group IDs (use $GLOBALS['xoopsUser']->getGroups() when object,
otherwise wrap XOOPS_GROUP_ANONYMOUS in an array or cast to (array)); update the
variable used by $gpermHandler->getItemIds() accordingly so it always receives
an array.
In `@htdocs/kernel/menuscategory.php`:
- Around line 48-49: The XoopsMenusCategory model declares a persistent field
via $this->initVar('dohtml', XOBJ_DTYPE_INT, 1, false) that is not present in
the menuscategory table schema; fix by either adding a matching column
definition (e.g. dohtml INT default 1) to the CREATE TABLE for menuscategory in
the migration/update script (include it in the SQL and any schema version bump)
or remove the initVar('dohtml',...) line from the XoopsMenusCategory class so
the model and database schema remain consistent; pick one approach and ensure
any related code that reads/writes dohtml is updated accordingly.
In `@htdocs/kernel/menusitems.php`:
- Around line 159-165: The parent selector query currently adds a
Criteria('items_active', 1) which hides inactive parent items and can remove the
edited item's current parent (items_pid) from the selection; remove that
active-only filter or alter the Criteria so it includes the current item's
parent (items_pid) even if inactive when building $criteria for
$menusitemsHandler->getall($criteria); update the query that constructs
$criteria (the CriteriaCompo used with items_cid and items_active) so it either
omits the items_active criterion entirely or ORs it with items_pid == current
item's parent ID to ensure existing parents remain selectable.
- Around line 50-51: The model declares a persistent field via
$this->initVar('dohtml', XOBJ_DTYPE_INT, 1, false) in the XoopsMenusItems model
but the menusitems CREATE TABLE in htdocs/modules/system/include/update.php
lacks that column; either add a matching dohtml column to the menusitems table
definition (integer, default 1) in the CREATE TABLE/ALTER statements in
include/update.php or remove the $this->initVar('dohtml', ...) line from the
XoopsMenusItems class so the model matches the schema; locate the initVar call
in the XoopsMenusItems class and the CREATE TABLE for menusitems in
include/update.php and apply one consistent change.
In `@htdocs/modules/system/admin/menus/main.php`:
- Around line 118-122: Several places call handler->get($id) (e.g.
$menuscategoryHandler->get($category_id), $menusitemsHandler->get($item_id)) and
immediately dereference $obj (calling $obj->getFormCat(), $obj->getVar(),
$menuscategoryHandler->delete($obj), etc.); update each such site to guard the
result from get() (use is_object($obj) or null-check) and handle the “not found”
case by returning a clean admin error/redirect instead of continuing, ensuring
every occurrence mentioned in the review (lines around the get calls at the
various branches) validates $obj before calling methods like getFormCat(),
getVar(), or delete().
- Around line 68-69: saveorder is currently renumbering category_position
starting at 1 for every paginated page; change the saveorder flow to account for
the current pagination offset by including the current $start (from the criteria
using setStart/setLimit) in the AJAX payload and then incrementing saved
positions starting at $start + 1 (instead of 1), or alternatively detect when
pagination is active and disable the drag-sort UI. Update the server-side
saveorder handler to accept and apply the offset, and update the client-side JS
that sends the reorder AJAX (the request that triggers saveorder) to include the
offset so reordering on page N continues numbering from $start+1; also apply the
same fix to the other affected blocks referenced (around lines 90-92 and
304-330).
- Around line 221-222: The code currently uses Request::getInt('category_id')
when loading descendants and building the redirect, which can be wrong; after
loading the item object ($obj) derive the category id from the stored field
($obj->getVar('items_cid')) and overwrite $category_id with that value before
calling getAllChild() and building the redirect target; ensure you only do this
when $obj is valid (fallback to the request value if $obj is null). Apply the
same change to the other branches that call getAllChild() and build the redirect
(the blocks that also reference $obj, getAllChild(), items_cid and
$category_id).
- Around line 526-528: When toggling an item's active state in main.php, prevent
re-enabling an item if its owning category is inactive: before allowing $new = 1
(i.e. when $obj->getVar('category_active') would be set to 1), load the item's
category via $menuscategoryHandler (use the item's category id / owning category
field on $obj) and check that that category's category_active is true; if the
category is inactive, keep $new = 0 (or return an error/flash message) and do
not call insert with a re-enabled state. Apply the same guard to the other
toggle branch referenced around lines 626-645 so any direct POST cannot bypass
the category_active invariant.
- Around line 420-428: Validate the submitted parent before calling
$obj->setVar('items_pid', ...) by loading the parent item for the
Request::getInt('items_pid', 0) value and rejecting it if it doesn't exist, if
its items_cid differs from the current Request::getInt('items_cid', 0) (or
$obj->getVar('items_cid')), or if the parent is a descendant (or the same item)
of the current item: walk the parent chain from the submitted parent upward
(using the menu item model/DAO used elsewhere in this file) and if you encounter
the current item id mark an error (append to $error_message) instead of setting
items_pid; keep the existing items_pid==0 allowance. Ensure you reference the
same model/DAO methods used by other menu operations to load parents and
traverse ancestors so checks are consistent with the tree implementation.
In `@htdocs/modules/system/admin/modulesadmin/modulesadmin.php`:
- Around line 71-72: The uninstall routine is missing the new core tables from
its reservation list; update the $reservedTables array inside the
xoops_module_uninstall() function to include 'menuscategory' and 'menusitems' so
the module uninstall will not drop those tables — locate the
xoops_module_uninstall() function and add those two table names to its
$reservedTables (and any similar reservation logic) to mirror the install-time
list.
In `@htdocs/modules/system/class/SystemMenusTree.php`:
- Around line 62-66: The addTree method currently hardcodes 'nomenclature_id'
and 'nomenclature_productid' on $this->tree[$key]['obj']->getVar(...), which
breaks listTree['id'] and causes invalid field accesses for menus; replace those
literals with the configured tree key names (use the class' configured key
properties — e.g. the tree keys holder like $this->treeKeys['id'] and
$this->treeKeys['productid'] or the equivalent property that holds configured
field names) when calling getVar and when assigning $value so the id and product
filter use the configured field names, and avoid calling getVar('product') when
$productid == 0 by first checking the filter or by fetching the product field
into a variable only when needed.
In `@htdocs/modules/system/include/update.php`:
- Around line 54-55: The CREATE TABLE SQL uses unprefixed table names in the
FOREIGN KEY clauses (menuscategory, menusitems) which will fail on prefixed
databases; update the FOREIGN KEY references in the SQL string to use the
application's table-prefix helper (e.g. $table_prefix . 'menuscategory' /
$table_prefix . 'menusitems' or $xoopsDB->prefix('menuscategory') /
$xoopsDB->prefix('menusitems') consistent with the surrounding code) so both
FOREIGN KEY (items_cid) REFERENCES ... and FOREIGN KEY (items_pid) REFERENCES
... point to the correctly prefixed table names.
- Around line 28-129: The migration currently runs many queries via
$xoopsDB->query() without checks and will fail if re-run; update the block that
creates and seeds the menus tables (the SQL strings that reference
_menuscategory and _menusitems and all following INSERTs into those tables,
_group_permission and _config) to: (1) use CREATE TABLE IF NOT EXISTS for the
two CREATE TABLE statements; (2) wrap the whole sequence in a database
transaction (START TRANSACTION / COMMIT / ROLLBACK via $xoopsDB->query or the DB
wrapper’s transaction API) so all inserts are atomic; and (3) after every
$xoopsDB->query() invocation check the return value and on failure log the error
(use $xoopsDB->error() or trigger_error with context mentioning the failing SQL)
and perform a rollback and abort the upgrade to prevent partial state.
- Around line 31-41: The CREATE TABLE SQL is using $xoopsDB->prefix('') .
"_menuscategory" which produces malformed names; replace uses of
$xoopsDB->prefix('') . "_menuscategory" (and any other occurrences of
$xoopsDB->prefix('') concatenated with table suffixes) with the proper call
$xoopsDB->prefix('menuscategory') so the database prefix is applied to the full
table name (do this for all similar table creation / query lines in update.php
that use $xoopsDB->prefix('')).
In `@htdocs/themes/xswatch5/tpl/nav-menu.tpl`:
- Around line 17-18: When rendering the anchor(s) in nav-menu.tpl that use
$item.target (the menu link templates that output target="<{$item.target}>"),
ensure that when $item.target == "_blank" you also add rel="noopener
noreferrer"; update both anchor occurrences that render href="...">
target="<{$item.target}>" (including the one that adds role/data-bs-toggle for
$item.children) so they conditionally include rel="noopener noreferrer" whenever
$item.target equals "_blank".
- Around line 17-18: The anchor currently doubles as a dropdown toggle
(data-bs-toggle="dropdown") which prevents navigation; change the markup in
nav-menu.tpl so the link element uses href="$item.url" (or '#' when empty) and
does NOT get data-bs-toggle, and when $item.children is present render a
separate dropdown toggle control (e.g., a button with class "dropdown-toggle",
data-bs-toggle="dropdown" and aria-expanded="false") next to the anchor;
preserve $item.target, $item.prefix/$item.title/$item.suffix on the anchor and
only attach the Bootstrap dropdown attributes to the separate toggle element so
parent URLs remain navigable.
---
Outside diff comments:
In `@htdocs/modules/system/themes/cpadmin/theme.tpl`:
- Around line 138-161: The template emits malformed nested lists because some
$op entries lack an options key and the closing </li> is only output
conditionally; update theme.tpl so each preference <li> for items from
$pref.menu ($op) is always closed: move or add the closing </li> outside/after
the block that renders the optional subtree so that regardless of $op.options
being unset, null, 0, or an array the <li> opened for $op is properly closed
after rendering its possible <ul> (reference the foreach over $pref.menu, the
$op.options checks and the inner foreach over $op.options).
---
Minor comments:
In `@htdocs/class/theme.php`:
- Around line 490-491: Change the inconsistent sort specification on the
criteria object: update the call to crit->setSort so it only lists column names
(remove the "ASC" from 'items_position ASC, items_title') and rely on the
existing crit->setOrder('ASC') call to supply direction; locate the crit
variable usage around setSort() / setOrder() and replace the argument to setSort
with 'items_position, items_title' so column names and sort direction are
handled separately.
In `@htdocs/modules/system/css/multilevelmenu.css`:
- Around line 30-33: The hover rule only shows submenus for pointer hover;
update the CSS to also show submenus when JavaScript toggles the .show class by
adding a rule that mirrors the hover behavior (e.g., target
.dropdown-submenu.show > .dropdown-menu or the .dropdown-menu.show variant used
by multilevelmenu.js) so clicks reveal the menu; modify multilevelmenu.css to
include this .show-based selector alongside the existing .dropdown-submenu:hover
> .dropdown-menu rule to ensure JS-toggled visibility works.
In `@htdocs/modules/system/js/multilevelmenu.js`:
- Around line 16-17: The click handler toggles the 'show' class on the submenu
(see usage of this.nextElementSibling and sub.classList.toggle('show') in
multilevelmenu.js) but multilevelmenu.css lacks a rule to make a
'.dropdown-menu' visible when it has the 'show' class; add a CSS rule targeting
'.dropdown-submenu > .dropdown-menu.show' that sets the menu to display (e.g.
display: block) so click-based toggling works even without Bootstrap.
In `@htdocs/modules/system/language/english/admin/menus.php`:
- Around line 13-21: Fix the English copy and HTML in the menu language
constants: correct the closing tag in _AM_SYSTEM_MENUS_NAV_TIPS from "<ul>" to
"</ul>" and proofread/replace ungrammatical phrases in
_AM_SYSTEM_MENUS_ERROR_NOITEM and _AM_SYSTEM_MENUS_SUREDELITEM (remove the
French word "avec" and replace with appropriate English such as "with" or
restructure the sentence), and apply the same cleanup to the other related menu
constants in this file (the block around the other menu strings) so all admin
UI/dialog messages are grammatically correct and use proper HTML.
In `@htdocs/modules/system/templates/admin/system_menus.tpl`:
- Line 107: The Smarty conditional is comparing $op|default:'' to an unquoted
identifier (viewcat); change the comparison to use a string literal by quoting
viewcat so the condition reads like $op|default:'' == 'viewcat' (locate the
conditional in the template where <{if $op|default:'' == viewcat}> appears and
update it to include the quotes).
---
Nitpick comments:
In `@htdocs/modules/system/include/update.php`:
- Around line 58-77: The INSERTs in update.php into tables named via
$xoopsDB->prefix('_menuscategory') and $xoopsDB->prefix('_menusitems') use
hardcoded ID values (VALUES (1,...), (2,...)) which will break auto-increment
and FK integrity; change those INSERTs to omit the id column or use NULL for the
auto-increment column so the DB assigns IDs, and where code later relies on
specific IDs capture the inserted id via LAST_INSERT_ID() (or use SELECT to find
existing rows) instead of hardcoding; also add guards (e.g., EXISTS checks)
before inserting to avoid duplicate rows when the update runs twice.
In `@htdocs/modules/system/js/menus.js`:
- Around line 163-170: The callback in the response.updated.forEach uses a
parameter named id which shadows the outer-scope id; rename the forEach
parameter (e.g., to updatedId or childId) wherever referenced inside the
callback so the selection $('.item-active-toggle[data-id="' + updatedId + '"]'),
and calls to updateBadge(...) and updateRowState(...), use the new name; ensure
no other references expect the old name and run tests to confirm behavior of
updateBadge and updateRowState remains unchanged.
In `@htdocs/modules/system/language/english/menus/menus.dist.php`:
- Line 6: The define('MENUS_', '') line defines an empty/incomplete constant
(MENUS_) and should be fixed: either remove this stray placeholder or replace it
with a meaningful example constant (e.g., define('MENUS_EXAMPLE', 'Example
text')) and add a brief comment explaining its purpose so maintainers know it’s
a template/example; update occurrences of MENUS_ if any to the new name or
remove them if unused.
In `@htdocs/themes/xswatch5/tpl/nav-menu.tpl`:
- Line 9: Remove the unused assignment call that sets unread_count: delete the
"<{xoInboxCount assign='unread_count'}>" invocation in nav-menu.tpl because
$unread_count is never used and renderMenuAffix already resolves
<{xoInboxCount}> for menu prefixes/suffixes; ensure no other code relies on the
unread_count variable and run a quick template render to confirm menus still
display inbox counts via renderMenuAffix.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 7f04bad0-8f3d-4afe-901f-0678fbeda018
📒 Files selected for processing (27)
htdocs/class/theme.phphtdocs/kernel/menuscategory.phphtdocs/kernel/menusitems.phphtdocs/modules/system/admin/menu.phphtdocs/modules/system/admin/menus/index.phphtdocs/modules/system/admin/menus/main.phphtdocs/modules/system/admin/menus/xoops_version.phphtdocs/modules/system/admin/modulesadmin/modulesadmin.phphtdocs/modules/system/class/SystemMenusTree.phphtdocs/modules/system/constants.phphtdocs/modules/system/css/menus.csshtdocs/modules/system/css/multilevelmenu.csshtdocs/modules/system/include/update.phphtdocs/modules/system/js/menus.jshtdocs/modules/system/js/multilevelmenu.jshtdocs/modules/system/language/english/admin.phphtdocs/modules/system/language/english/admin/menus.phphtdocs/modules/system/language/english/menu.phphtdocs/modules/system/language/english/menus/index.phphtdocs/modules/system/language/english/menus/menus.dist.phphtdocs/modules/system/language/english/modinfo.phphtdocs/modules/system/templates/admin/system_menus.tplhtdocs/modules/system/themes/cpadmin/cpadmin.phphtdocs/modules/system/themes/cpadmin/theme.tplhtdocs/modules/system/xoops_version.phphtdocs/themes/xswatch5/style.csshtdocs/themes/xswatch5/tpl/nav-menu.tpl
| $viewPermissionCat = []; | ||
| $helper = Xmf\Module\Helper::getHelper('system'); | ||
| $moduleHandler = $helper->getModule(); | ||
| $groups = is_object($GLOBALS['xoopsUser']) ? $GLOBALS['xoopsUser']->getGroups() : XOOPS_GROUP_ANONYMOUS; |
There was a problem hiding this comment.
Type mismatch: $groups may be integer when array is expected.
When the user is not logged in, $groups is assigned XOOPS_GROUP_ANONYMOUS (an integer constant), but $gpermHandler->getItemIds() typically expects an array of group IDs.
🐛 Proposed fix
-$groups = is_object($GLOBALS['xoopsUser']) ? $GLOBALS['xoopsUser']->getGroups() : XOOPS_GROUP_ANONYMOUS;
+$groups = is_object($GLOBALS['xoopsUser']) ? $GLOBALS['xoopsUser']->getGroups() : [XOOPS_GROUP_ANONYMOUS];📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| $groups = is_object($GLOBALS['xoopsUser']) ? $GLOBALS['xoopsUser']->getGroups() : XOOPS_GROUP_ANONYMOUS; | |
| $groups = is_object($GLOBALS['xoopsUser']) ? $GLOBALS['xoopsUser']->getGroups() : [XOOPS_GROUP_ANONYMOUS]; |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@htdocs/class/theme.php` at line 460, $groups may be an integer when not
logged in (XOOPS_GROUP_ANONYMOUS) but later code (e.g.
$gpermHandler->getItemIds()) expects an array; ensure $groups is always an array
by replacing the assignment with logic that yields an array of group IDs (use
$GLOBALS['xoopsUser']->getGroups() when object, otherwise wrap
XOOPS_GROUP_ANONYMOUS in an array or cast to (array)); update the variable used
by $gpermHandler->getItemIds() accordingly so it always receives an array.
Comment on lines
+48
to
+49
| // use html | ||
| $this->initVar('dohtml', XOBJ_DTYPE_INT, 1, false); |
There was a problem hiding this comment.
Keep the object definition aligned with the table schema.
XoopsMenusCategory declares a persistent dohtml field here, but the menuscategory CREATE TABLE in htdocs/modules/system/include/update.php does not have that column. Either add the column in the migration or remove this initVar, otherwise the model and schema diverge.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@htdocs/kernel/menuscategory.php` around lines 48 - 49, The XoopsMenusCategory
model declares a persistent field via $this->initVar('dohtml', XOBJ_DTYPE_INT,
1, false) that is not present in the menuscategory table schema; fix by either
adding a matching column definition (e.g. dohtml INT default 1) to the CREATE
TABLE for menuscategory in the migration/update script (include it in the SQL
and any schema version bump) or remove the initVar('dohtml',...) line from the
XoopsMenusCategory class so the model and database schema remain consistent;
pick one approach and ensure any related code that reads/writes dohtml is
updated accordingly.
Comment on lines
+50
to
+51
| // use html | ||
| $this->initVar('dohtml', XOBJ_DTYPE_INT, 1, false); |
There was a problem hiding this comment.
Keep the item model aligned with the table schema.
XoopsMenusItems declares a persistent dohtml field here, but the menusitems CREATE TABLE in htdocs/modules/system/include/update.php does not define that column. Either add the column there or drop this initVar.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@htdocs/kernel/menusitems.php` around lines 50 - 51, The model declares a
persistent field via $this->initVar('dohtml', XOBJ_DTYPE_INT, 1, false) in the
XoopsMenusItems model but the menusitems CREATE TABLE in
htdocs/modules/system/include/update.php lacks that column; either add a
matching dohtml column to the menusitems table definition (integer, default 1)
in the CREATE TABLE/ALTER statements in include/update.php or remove the
$this->initVar('dohtml', ...) line from the XoopsMenusItems class so the model
matches the schema; locate the initVar call in the XoopsMenusItems class and the
CREATE TABLE for menusitems in include/update.php and apply one consistent
change.
Comment on lines
+159
to
+165
| $criteria = new CriteriaCompo(); | ||
| $criteria->add(new Criteria('items_cid', $category_id)); | ||
| $criteria->add(new Criteria('items_active', 1)); | ||
| $criteria->setSort('items_position ASC, items_title'); | ||
| $criteria->setOrder('ASC'); | ||
| $menusitemsHandler = xoops_getHandler('menusitems'); | ||
| $item_arr = $menusitemsHandler->getall($criteria); |
There was a problem hiding this comment.
Do not hide inactive parents from the parent selector.
Filtering the tree to items_active = 1 hides valid existing parents. If you edit a child whose current parent is inactive, the current items_pid is no longer selectable and the save path can silently reparent the item.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@htdocs/kernel/menusitems.php` around lines 159 - 165, The parent selector
query currently adds a Criteria('items_active', 1) which hides inactive parent
items and can remove the edited item's current parent (items_pid) from the
selection; remove that active-only filter or alter the Criteria so it includes
the current item's parent (items_pid) even if inactive when building $criteria
for $menusitemsHandler->getall($criteria); update the query that constructs
$criteria (the CriteriaCompo used with items_cid and items_active) so it either
omits the items_active criterion entirely or ORs it with items_pid == current
item's parent ID to ensure existing parents remain selectable.
Comment on lines
+68
to
+69
| $criteria->setStart($start); | ||
| $criteria->setLimit($nb_limit); |
There was a problem hiding this comment.
Don't renumber paginated categories from 1.
The list view is paginated, but saveorder always rewrites category_position starting at 1. Reordering page 2 will reuse positions already owned by page 1, so the persisted order becomes unstable. Either include the current offset in the AJAX payload and start from $start + 1, or disable drag sorting when pagination is active.
Also applies to: 90-92, 304-330
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@htdocs/modules/system/admin/menus/main.php` around lines 68 - 69, saveorder
is currently renumbering category_position starting at 1 for every paginated
page; change the saveorder flow to account for the current pagination offset by
including the current $start (from the criteria using setStart/setLimit) in the
AJAX payload and then incrementing saved positions starting at $start + 1
(instead of 1), or alternatively detect when pagination is active and disable
the drag-sort UI. Update the server-side saveorder handler to accept and apply
the offset, and update the client-side JS that sends the reorder AJAX (the
request that triggers saveorder) to include the offset so reordering on page N
continues numbering from $start+1; also apply the same fix to the other affected
blocks referenced (around lines 90-92 and 304-330).
Comment on lines
+28
to
+129
| global $xoopsDB; | ||
| if ($prev_version < '2.2.0-Stable') { | ||
| //$db = XoopsDatabaseFactory::getDatabaseConnection(); | ||
| $sql = "CREATE TABLE " . $xoopsDB->prefix('') . "_menuscategory ( | ||
| category_id INT AUTO_INCREMENT PRIMARY KEY, | ||
| category_title VARCHAR(100) NOT NULL, | ||
| category_prefix VARCHAR(100) NOT NULL, | ||
| category_suffix VARCHAR(100) NOT NULL, | ||
| category_url VARCHAR(255) NULL, | ||
| category_target TINYINT(1) DEFAULT 0, | ||
| category_position INT DEFAULT 0, | ||
| category_protected INT DEFAULT 0, | ||
| category_active TINYINT(1) DEFAULT 1);"; | ||
| $xoopsDB->query($sql); | ||
| $sql = "CREATE TABLE " . $xoopsDB->prefix('') . "_menusitems ( | ||
| items_id INT AUTO_INCREMENT PRIMARY KEY, | ||
| items_pid INT NULL, | ||
| items_cid INT NULL, | ||
| items_title VARCHAR(100) NOT NULL, | ||
| items_prefix VARCHAR(100) NOT NULL, | ||
| items_suffix VARCHAR(100) NOT NULL, | ||
| items_url VARCHAR(255) NULL, | ||
| items_target TINYINT(1) DEFAULT 0, | ||
| items_position INT DEFAULT 0, | ||
| items_protected INT DEFAULT 0, | ||
| items_active TINYINT(1) DEFAULT 1, | ||
| FOREIGN KEY (items_cid) REFERENCES menuscategory(category_id) ON DELETE CASCADE, | ||
| FOREIGN KEY (items_pid) REFERENCES menusitems(items_id) ON DELETE CASCADE);"; | ||
| // add default data | ||
| $xoopsDB->query($sql); | ||
| $sql = "INSERT INTO " . $xoopsDB->prefix('') . "_menuscategory VALUES (1, 'MENUS_HOME', '<span class=\"fa fa-home\" ></span>', '', '/', 0, 0, 1, 1)"; | ||
| $xoopsDB->query($sql); | ||
| $sql = "INSERT INTO " . $xoopsDB->prefix('') . "_menuscategory VALUES (2, 'MENUS_ADMIN', '<span class=\"fa fa-wrench fa-fw\" ></span>', '', 'admin.php', 0, 10, 1, 1)"; | ||
| $xoopsDB->query($sql); | ||
| $sql = "INSERT INTO " . $xoopsDB->prefix('') . "_menuscategory VALUES (3, 'MENUS_ACCOUNT', '<span class=\"fa fa-user fa-fw\" ></span>', '', '', 0, 20, 1, 1)"; | ||
| $xoopsDB->query($sql); | ||
| $sql = "INSERT INTO " . $xoopsDB->prefix('') . "_menusitems VALUES (1, 0, 3, 'MENUS_ACCOUNT_EDIT', '<span class=\"fa fa-edit fa-fw\" ></span>', '', 'user.php', 0, 1, 1, 1)"; | ||
| $xoopsDB->query($sql); | ||
| $sql = "INSERT INTO " . $xoopsDB->prefix('') . "_menusitems VALUES (2, 0, 3, 'MENUS_ACCOUNT_LOGIN', '<span class=\"fa fa-sign-in fa-fw\" ></span>', '', 'user.php', 0, 2, 1, 1)"; | ||
| $xoopsDB->query($sql); | ||
| $sql = "INSERT INTO " . $xoopsDB->prefix('') . "_menusitems VALUES (3, 0, 3, 'MENUS_ACCOUNT_REGISTER', '<span class=\"fa fa-sign-in fa-fw\" ></span>', '', 'register.php', 0, 2, 1, 1)"; | ||
| $xoopsDB->query($sql); | ||
| $sql = "INSERT INTO " . $xoopsDB->prefix('') . "_menusitems VALUES (4, 0, 3, 'MENUS_ACCOUNT_MESSAGES', '<span class=\"fa fa-solid fa-envelope fa-fw\" ></span>', '<span class=\"badge bg-primary rounded-pill\"><{xoInboxCount}></span>', 'viewpmsg.php', 0, 3, 1, 1)"; | ||
| $xoopsDB->query($sql); | ||
| $sql = "INSERT INTO " . $xoopsDB->prefix('') . "_menusitems VALUES (5, 0, 3, 'MENUS_ACCOUNT_NOTIFICATIONS', '<span class=\"fa fa-info-circle fa-fw\" ></span>', '', 'notifications.php', 0, 4, 1, 1)"; | ||
| $xoopsDB->query($sql); | ||
| $sql = "INSERT INTO " . $xoopsDB->prefix('') . "_menusitems VALUES (6, 0, 3, 'MENUS_ACCOUNT_TOOLBAR', '<span class=\"fa-solid fa-screwdriver-wrench fa-fw\" ></span>', '<span id=\"xswatch-toolbar-ind\"></span>', 'javascript:xswatchToolbarToggle();', 0, 5, 1, 1)"; | ||
| $xoopsDB->query($sql); | ||
| $sql = "INSERT INTO " . $xoopsDB->prefix('') . "_menusitems VALUES (7, 0, 3, 'MENUS_ACCOUNT_LOGOUT', '<span class=\"fa fa-sign-out fa-fw\" ></span>', '', 'user.php?op=logout', 0, 5, 1, 1)"; | ||
| $xoopsDB->query($sql); | ||
| // add permissions for category and items | ||
| // MENUS_HOME | ||
| $sql = "INSERT INTO " . $xoopsDB->prefix('') . "_group_permission VALUES (NULL, 1, 1, 1, 'menus_category_view')"; | ||
| $xoopsDB->query($sql); | ||
| $sql = "INSERT INTO " . $xoopsDB->prefix('') . "_group_permission VALUES (NULL, 2, 1, 1, 'menus_category_view')"; | ||
| $xoopsDB->query($sql); | ||
| $sql = "INSERT INTO " . $xoopsDB->prefix('') . "_group_permission VALUES (NULL, 3, 1, 1, 'menus_category_view')"; | ||
| $xoopsDB->query($sql); | ||
| // MENUS_ADMIN | ||
| $sql = "INSERT INTO " . $xoopsDB->prefix('') . "_group_permission VALUES (NULL, 1, 2, 1, 'menus_category_view')"; | ||
| $xoopsDB->query($sql); | ||
| // MENUS_ACCOUNT | ||
| $sql = "INSERT INTO " . $xoopsDB->prefix('') . "_group_permission VALUES (NULL, 1, 3, 1, 'menus_category_view')"; | ||
| $xoopsDB->query($sql); | ||
| $sql = "INSERT INTO " . $xoopsDB->prefix('') . "_group_permission VALUES (NULL, 2, 3, 1, 'menus_category_view')"; | ||
| $xoopsDB->query($sql); | ||
| $sql = "INSERT INTO " . $xoopsDB->prefix('') . "_group_permission VALUES (NULL, 3, 3, 1, 'menus_category_view')"; | ||
| $xoopsDB->query($sql); | ||
| // MENUS_ACCOUNT_EDIT | ||
| $sql = "INSERT INTO " . $xoopsDB->prefix('') . "_group_permission VALUES (NULL, 1, 1, 1, 'menus_items_view')"; | ||
| $xoopsDB->query($sql); | ||
| $sql = "INSERT INTO " . $xoopsDB->prefix('') . "_group_permission VALUES (NULL, 2, 1, 1, 'menus_items_view')"; | ||
| $xoopsDB->query($sql); | ||
| // MENUS_ACCOUNT_LOGIN | ||
| $sql = "INSERT INTO " . $xoopsDB->prefix('') . "_group_permission VALUES (NULL, 3, 2, 1, 'menus_items_view')"; | ||
| $xoopsDB->query($sql); | ||
| // MENUS_ACCOUNT_REGISTER | ||
| $sql = "INSERT INTO " . $xoopsDB->prefix('') . "_group_permission VALUES (NULL, 3, 3, 1, 'menus_items_view')"; | ||
| $xoopsDB->query($sql); | ||
| // MENUS_ACCOUNT_MESSAGES | ||
| $sql = "INSERT INTO " . $xoopsDB->prefix('') . "_group_permission VALUES (NULL, 1, 4, 1, 'menus_items_view')"; | ||
| $xoopsDB->query($sql); | ||
| $sql = "INSERT INTO " . $xoopsDB->prefix('') . "_group_permission VALUES (NULL, 2, 4, 1, 'menus_items_view')"; | ||
| $xoopsDB->query($sql); | ||
| // MENUS_ACCOUNT_NOTIFICATIONS | ||
| $sql = "INSERT INTO " . $xoopsDB->prefix('') . "_group_permission VALUES (NULL, 1, 5, 1, 'menus_items_view')"; | ||
| $xoopsDB->query($sql); | ||
| $sql = "INSERT INTO " . $xoopsDB->prefix('') . "_group_permission VALUES (NULL, 2, 5, 1, 'menus_items_view')"; | ||
| $xoopsDB->query($sql); | ||
| // MENUS_ACCOUNT_TOOLBAR | ||
| $sql = "INSERT INTO " . $xoopsDB->prefix('') . "_group_permission VALUES (NULL, 1, 6, 1, 'menus_items_view')"; | ||
| $xoopsDB->query($sql); | ||
| // MENUS_ACCOUNT_LOGOUT | ||
| $sql = "INSERT INTO " . $xoopsDB->prefix('') . "_group_permission VALUES (NULL, 1, 7, 1, 'menus_items_view')"; | ||
| $xoopsDB->query($sql); | ||
| $sql = "INSERT INTO " . $xoopsDB->prefix('') . "_group_permission VALUES (NULL, 2, 7, 1, 'menus_items_view')"; | ||
| $xoopsDB->query($sql); | ||
|
|
||
| // Add config for active menus | ||
| $sql = "INSERT INTO " . $xoopsDB->prefix('') . "_config VALUES (NULL, 1, 0, 'active_menus', '_MI_SYSTEM_PREFERENCE_ACTIVE_MENUS', '1', '', 'hidden', 'int', 21)"; | ||
| $xoopsDB->query($sql); | ||
| } |
There was a problem hiding this comment.
Migration lacks error handling and idempotency.
Several robustness concerns:
-
No error handling: If
CREATE TABLEfails, subsequentINSERTstatements will still execute and fail silently. Check return values from$xoopsDB->query(). -
Not idempotent: If migration runs twice (e.g., failed upgrade retry),
CREATE TABLEwill fail on existing tables. Consider usingCREATE TABLE IF NOT EXISTS. -
No transaction: Multiple related statements should ideally be wrapped in a transaction to ensure atomic migration.
🛡️ Suggested improvement pattern
if ($prev_version < '2.2.0-Stable') {
+ // Check if table already exists to make migration idempotent
+ $sql = "SHOW TABLES LIKE '" . $xoopsDB->prefix('menuscategory') . "'";
+ $result = $xoopsDB->query($sql);
+ if ($xoopsDB->isResultSet($result) && $xoopsDB->getRowsNum($result) > 0) {
+ // Tables already exist, skip creation
+ } else {
- $sql = "CREATE TABLE " . $xoopsDB->prefix('') . "_menuscategory (
+ $sql = "CREATE TABLE IF NOT EXISTS " . $xoopsDB->prefix('menuscategory') . " (
...
+ if (!$xoopsDB->query($sql)) {
+ $module->setErrors('Failed to create menuscategory table');
+ 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 28 - 129, The
migration currently runs many queries via $xoopsDB->query() without checks and
will fail if re-run; update the block that creates and seeds the menus tables
(the SQL strings that reference _menuscategory and _menusitems and all following
INSERTs into those tables, _group_permission and _config) to: (1) use CREATE
TABLE IF NOT EXISTS for the two CREATE TABLE statements; (2) wrap the whole
sequence in a database transaction (START TRANSACTION / COMMIT / ROLLBACK via
$xoopsDB->query or the DB wrapper’s transaction API) so all inserts are atomic;
and (3) after every $xoopsDB->query() invocation check the return value and on
failure log the error (use $xoopsDB->error() or trigger_error with context
mentioning the failing SQL) and perform a rollback and abort the upgrade to
prevent partial state.
Comment on lines
+31
to
+41
| $sql = "CREATE TABLE " . $xoopsDB->prefix('') . "_menuscategory ( | ||
| category_id INT AUTO_INCREMENT PRIMARY KEY, | ||
| category_title VARCHAR(100) NOT NULL, | ||
| category_prefix VARCHAR(100) NOT NULL, | ||
| category_suffix VARCHAR(100) NOT NULL, | ||
| category_url VARCHAR(255) NULL, | ||
| category_target TINYINT(1) DEFAULT 0, | ||
| category_position INT DEFAULT 0, | ||
| category_protected INT DEFAULT 0, | ||
| category_active TINYINT(1) DEFAULT 1);"; | ||
| $xoopsDB->query($sql); |
There was a problem hiding this comment.
Incorrect table prefix usage creates malformed table names.
The pattern $xoopsDB->prefix('') . "_menuscategory" is non-standard. prefix('') returns only the prefix (e.g., xoops), resulting in table names like xoops_menuscategory but with an extra underscore after the prefix. The standard XOOPS pattern is $xoopsDB->prefix('menuscategory').
🛠️ Proposed fix
- $sql = "CREATE TABLE " . $xoopsDB->prefix('') . "_menuscategory (
+ $sql = "CREATE TABLE " . $xoopsDB->prefix('menuscategory') . " (Apply the same fix to all occurrences of $xoopsDB->prefix('') throughout this migration.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| $sql = "CREATE TABLE " . $xoopsDB->prefix('') . "_menuscategory ( | |
| category_id INT AUTO_INCREMENT PRIMARY KEY, | |
| category_title VARCHAR(100) NOT NULL, | |
| category_prefix VARCHAR(100) NOT NULL, | |
| category_suffix VARCHAR(100) NOT NULL, | |
| category_url VARCHAR(255) NULL, | |
| category_target TINYINT(1) DEFAULT 0, | |
| category_position INT DEFAULT 0, | |
| category_protected INT DEFAULT 0, | |
| category_active TINYINT(1) DEFAULT 1);"; | |
| $xoopsDB->query($sql); | |
| $sql = "CREATE TABLE " . $xoopsDB->prefix('menuscategory') . " ( | |
| category_id INT AUTO_INCREMENT PRIMARY KEY, | |
| category_title VARCHAR(100) NOT NULL, | |
| category_prefix VARCHAR(100) NOT NULL, | |
| category_suffix VARCHAR(100) NOT NULL, | |
| category_url VARCHAR(255) NULL, | |
| category_target TINYINT(1) DEFAULT 0, | |
| category_position INT DEFAULT 0, | |
| category_protected INT DEFAULT 0, | |
| category_active TINYINT(1) DEFAULT 1);"; | |
| $xoopsDB->query($sql); |
🤖 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 31 - 41, The CREATE
TABLE SQL is using $xoopsDB->prefix('') . "_menuscategory" which produces
malformed names; replace uses of $xoopsDB->prefix('') . "_menuscategory" (and
any other occurrences of $xoopsDB->prefix('') concatenated with table suffixes)
with the proper call $xoopsDB->prefix('menuscategory') so the database prefix is
applied to the full table name (do this for all similar table creation / query
lines in update.php that use $xoopsDB->prefix('')).
Comment on lines
+54
to
+55
| FOREIGN KEY (items_cid) REFERENCES menuscategory(category_id) ON DELETE CASCADE, | ||
| FOREIGN KEY (items_pid) REFERENCES menusitems(items_id) ON DELETE CASCADE);"; |
There was a problem hiding this comment.
Foreign key references will fail due to missing table prefix.
The foreign key constraints reference unqualified table names (menuscategory, menusitems) instead of the prefixed table names. This will cause the CREATE TABLE statement to fail on MySQL.
🐛 Proposed fix
- FOREIGN KEY (items_cid) REFERENCES menuscategory(category_id) ON DELETE CASCADE,
- FOREIGN KEY (items_pid) REFERENCES menusitems(items_id) ON DELETE CASCADE);";
+ FOREIGN KEY (items_cid) REFERENCES " . $xoopsDB->prefix('menuscategory') . "(category_id) ON DELETE CASCADE,
+ FOREIGN KEY (items_pid) REFERENCES " . $xoopsDB->prefix('menusitems') . "(items_id) ON DELETE CASCADE);";📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| FOREIGN KEY (items_cid) REFERENCES menuscategory(category_id) ON DELETE CASCADE, | |
| FOREIGN KEY (items_pid) REFERENCES menusitems(items_id) ON DELETE CASCADE);"; | |
| FOREIGN KEY (items_cid) REFERENCES " . $xoopsDB->prefix('menuscategory') . "(category_id) ON DELETE CASCADE, | |
| FOREIGN KEY (items_pid) REFERENCES " . $xoopsDB->prefix('menusitems') . "(items_id) ON DELETE CASCADE);"; |
🤖 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 54 - 55, The CREATE
TABLE SQL uses unprefixed table names in the FOREIGN KEY clauses (menuscategory,
menusitems) which will fail on prefixed databases; update the FOREIGN KEY
references in the SQL string to use the application's table-prefix helper (e.g.
$table_prefix . 'menuscategory' / $table_prefix . 'menusitems' or
$xoopsDB->prefix('menuscategory') / $xoopsDB->prefix('menusitems') consistent
with the surrounding code) so both FOREIGN KEY (items_cid) REFERENCES ... and
FOREIGN KEY (items_pid) REFERENCES ... point to the correctly prefixed table
names.
Comment on lines
+17
to
+18
| href="<{if $item.url neq ''}><{$item.url|escape}><{else}>#<{/if}>"<{if $item.children}> role="button" data-bs-toggle="dropdown" aria-expanded="false"<{/if}> target="<{$item.target}>"> | ||
| <{$item.prefix}> <{$item.title|escape}> <{$item.suffix}> |
There was a problem hiding this comment.
Add rel when a menu link opens in a new tab.
Both anchors can emit target="_blank" for admin-configured URLs, but they omit rel="noopener noreferrer", which leaves the opener window exposed to tabnabbing.
Suggested fix
- href="<{if $item.url neq ''}><{$item.url|escape}><{else}>#<{/if}>"<{if $item.children}> role="button" data-bs-toggle="dropdown" aria-expanded="false"<{/if}> target="<{$item.target}>">
+ href="<{if $item.url neq ''}><{$item.url|escape}><{else}>#<{/if}>"<{if $item.children}> role="button" data-bs-toggle="dropdown" aria-expanded="false"<{/if}> target="<{$item.target}>"<{if $item.target eq '_blank'}> rel="noopener noreferrer"<{/if}>>
...
- <{if $cat.items}>role="button" data-bs-toggle="dropdown" aria-expanded="false"<{/if}> target="<{$cat.category_target}>">
+ <{if $cat.items}>role="button" data-bs-toggle="dropdown" aria-expanded="false"<{/if}> target="<{$cat.category_target}>"<{if $cat.category_target eq '_blank'}> rel="noopener noreferrer"<{/if}>>Also applies to: 30-33
🧰 Tools
🪛 smarty-lint (0.3.3)
[warn] 17-17: Use "!==" instead of "neq".
(eqeqeq)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@htdocs/themes/xswatch5/tpl/nav-menu.tpl` around lines 17 - 18, When rendering
the anchor(s) in nav-menu.tpl that use $item.target (the menu link templates
that output target="<{$item.target}>"), ensure that when $item.target ==
"_blank" you also add rel="noopener noreferrer"; update both anchor occurrences
that render href="..."> target="<{$item.target}>" (including the one that adds
role/data-bs-toggle for $item.children) so they conditionally include
rel="noopener noreferrer" whenever $item.target equals "_blank".
🧩 Analysis chain
🌐 Web query:
In Bootstrap 5, does clicking an withdata-bs-toggle="dropdown"prevent navigation to itshref?
💡 Result:
Yes. In Bootstrap 5, the Dropdown Data API click handler always calls event.preventDefault() for elements matching [data-bs-toggle="dropdown"], so clicking an <a data-bs-toggle="dropdown" href="…"> will not navigate to its href (it will just toggle the dropdown). [1]
Bootstrap’s own examples therefore typically use href="#" on dropdown-toggle links. [2]
Sources
[1] Bootstrap v5.3.3 dropdown.js (Data API click handler calls event.preventDefault()) (github.com)
[2] Bootstrap Dropdown docs examples (<a ... href="#" ... data-bs-toggle="dropdown">) (getbootstrap.com)
Citations:
- 1: https://github.com/twbs/bootstrap/blob/v5.3.3/js/src/dropdown.js
- 2: https://getbootstrap.com/docs/5.0/components/dropdowns/?utm_source=openai
Parent menu URLs are unreachable when menu items also function as dropdown toggles.
In Bootstrap 5, the Dropdown Data API click handler always calls event.preventDefault() on elements with data-bs-toggle="dropdown", preventing navigation to the href attribute. These anchors will only toggle the dropdown menu and never navigate to item.url or category_url.
Current code snippet (lines 17-18, also applies to 30-32)
href="<{if $item.url neq ''}><{$item.url|escape}><{else}>#<{/if}>"<{if $item.children}> role="button" data-bs-toggle="dropdown" aria-expanded="false"<{/if}> target="<{$item.target}>">
<{$item.prefix}> <{$item.title|escape}> <{$item.suffix}>
🧰 Tools
🪛 smarty-lint (0.3.3)
[warn] 17-17: Use "!==" instead of "neq".
(eqeqeq)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@htdocs/themes/xswatch5/tpl/nav-menu.tpl` around lines 17 - 18, The anchor
currently doubles as a dropdown toggle (data-bs-toggle="dropdown") which
prevents navigation; change the markup in nav-menu.tpl so the link element uses
href="$item.url" (or '#' when empty) and does NOT get data-bs-toggle, and when
$item.children is present render a separate dropdown toggle control (e.g., a
button with class "dropdown-toggle", data-bs-toggle="dropdown" and
aria-expanded="false") next to the anchor; preserve $item.target,
$item.prefix/$item.title/$item.suffix on the anchor and only attach the
Bootstrap dropdown attributes to the separate toggle element so parent URLs
remain navigable.
good Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (7)
htdocs/themes/xswatch5/tpl/nav-menu.tpl (2)
16-16:⚠️ Potential issue | 🟠 MajorParent links are still non-navigable when used as dropdown toggles.
At Line 16 and Line 31, the same
<a>is both a URL link and a Bootstrap dropdown toggle (data-bs-toggle="dropdown"), so parent URL navigation is blocked.Also applies to: 30-31
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@htdocs/themes/xswatch5/tpl/nav-menu.tpl` at line 16, The anchor used for parent items currently has both an href and data-bs-toggle="dropdown", which prevents navigation; change the markup so the <a> with href and target (the parent link rendered by the nav-menu.tpl item) does not carry data-bs-toggle or role="button", and instead render a separate sibling control (a <button> or <a> with no href) that holds data-bs-toggle="dropdown", aria-expanded and the dropdown-toggle class to open the submenu; update the template code around the anchor that references $item.url/$item.target and the conditional that adds data-bs-toggle so the toggle is a distinct element (preserve existing classes and aria labels for accessibility).
16-16:⚠️ Potential issue | 🟠 MajorAdd
relfor external-tab targets.When
target="_blank"is rendered,rel="noopener noreferrer"is still missing on both anchors (Line 16 and Line 31).Suggested fix
- href="<{if $item.url neq ''}><{$item.url|escape}><{else}>#<{/if}>"<{if $item.children}> role="button" data-bs-toggle="dropdown" aria-expanded="false"<{/if}> target="<{$item.target}>"> + href="<{if $item.url neq ''}><{$item.url|escape}><{else}>#<{/if}>"<{if $item.children}> role="button" data-bs-toggle="dropdown" aria-expanded="false"<{/if}> target="<{$item.target}>"<{if $item.target eq '_blank'}> rel="noopener noreferrer"<{/if}>> - <{if $cat.items}>role="button" data-bs-toggle="dropdown" aria-expanded="false"<{/if}> target="<{$cat.category_target}>"> + <{if $cat.items}>role="button" data-bs-toggle="dropdown" aria-expanded="false"<{/if}> target="<{$cat.category_target}>"<{if $cat.category_target eq '_blank'}> rel="noopener noreferrer"<{/if}>>Also applies to: 31-31
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@htdocs/themes/xswatch5/tpl/nav-menu.tpl` at line 16, The anchor rendering in nav-menu.tpl omits rel attributes for external tabs: when $item.target renders "_blank" (the anchor that uses href with the conditional and the second anchor around line 31), add rel="noopener noreferrer" conditionally; update the two anchor templates that reference $item.url/$item.target so that if $item.target eq "_blank" the anchor includes rel="noopener noreferrer" (keep current target handling and dropdown attributes intact).htdocs/modules/system/admin/menus/main.php (5)
422-427:⚠️ Potential issue | 🔴 CriticalReject descendant and cross-category parents in
saveitem.Current validation only blocks
items_pid == items_id. It still allows setting parent to a descendant or to an item in another category, which can corrupt the tree.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@htdocs/modules/system/admin/menus/main.php` around lines 422 - 427, The current validation only prevents choosing the item itself as parent; update the saveitem logic (around Request::getInt('items_pid', 0), $itempid, $id and $obj->setVar('items_pid', ...)) to also reject a parent that is in a different category or a descendant: when $itempid != 0, load the parent item record by $itempid, verify the parent's category matches the current item's category field (e.g. items_mid or category identifier) and then walk the parent chain (follow items_pid repeatedly) to ensure none of the ancestors equal $id; if either check fails, append the same error and do not call $obj->setVar('items_pid', $itempid).
120-122:⚠️ Potential issue | 🔴 CriticalGuard every
get()result before dereferencing.Several branches still dereference
$objimmediately afterhandler->get(...)without checking object existence. A stale/tampered ID can still return non-object and cause admin fatals.Also applies to: 134-136, 185-186, 232-238, 352-355, 415-417, 482-487
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@htdocs/modules/system/admin/menus/main.php` around lines 120 - 122, The code dereferences the return of $menuscategoryHandler->get(...) (stored in $obj) without checking it's a valid object, causing possible fatal errors; update each occurrence (e.g., where $obj is used with ->getFormCat() or ->getForm()) to first verify the result is an object (or not null) and handle the failure path (log an error, set an error message, redirect, or skip rendering the form) before calling methods on $obj; apply this guard to every location that calls $menuscategoryHandler->get(...) and immediately uses $obj (including uses of getFormCat() and getForm()) so dereferencing is safe.
222-222:⚠️ Potential issue | 🟠 MajorUse the item’s stored category for descendant delete flow.
delitemstill trusts requestcategory_idfor tree loading and redirect logic. Use$obj->getVar('items_cid')after loading the item to avoid orphaned descendants/permissions when request data is wrong.Also applies to: 249-253, 265-269, 259-259
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@htdocs/modules/system/admin/menus/main.php` at line 222, In delitem flow the code currently trusts Request::getInt('category_id', 0) (assigned to $category_id) for tree loading and redirecting, which can cause orphaned descendants/permissions if the request value is wrong; after loading the item ($obj via the item loader used in delitem), replace usage of $category_id (and any subsequent Request::getInt('category_id') uses at the other spots) with the item's stored category via $obj->getVar('items_cid') for tree operations and redirects so the deletion/descendant handling always uses the authoritative category from the persisted item.
68-69:⚠️ Potential issue | 🟠 Major
saveorderstill renumbers from 1 regardless of page offset.With pagination active, reordering on later pages can overwrite positions from earlier pages. Include pagination offset in payload and start numbering from
start + 1.Also applies to: 316-331
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@htdocs/modules/system/admin/menus/main.php` around lines 68 - 69, The saveorder renumbering ignores pagination offset and always starts at 1; update the saveorder handler (function saveorder) to accept and use the current page offset (e.g., $start or an 'offset'/'start' value carried in the payload) and when assigning new positions start numbering from ($start + 1) instead of 1; ensure the client-side payload includes that offset when calling saveorder and adjust any loops that set position values so they add the offset before persisting (also apply the same fix to the other renumbering block referenced around the second occurrence).
628-646:⚠️ Potential issue | 🟠 MajorBlock item reactivation when owning category is inactive.
The ancestor check does not cover root items (
items_pid = 0). A direct request can still reactivate an item under an inactive category.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@htdocs/modules/system/admin/menus/main.php` around lines 628 - 646, The ancestor-check loop can skip validating the immediate parent when items_pid === 0, allowing reactivation under an inactive category; update the traversal that starts from $parentId = (int)$obj->getVar('items_pid') to fetch and inspect the parent object before deciding to break (use a do/while or pre-check pattern), always checking (int)$parentObj->getVar('items_active') and emitting the same JSON error (using _AM_SYSTEM_MENUS_ERROR_PARENTINACTIVE and the xoopsSecurity token) when a parent is inactive; use $menusitemsHandler->get to obtain the parent object and ensure the logic covers the case where get(0) returns an object or where the immediate parent must still be validated even if its pid is 0.
🤖 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/menus/main.php`:
- Around line 59-60: The code calls $xoBreadCrumb->addLink and
$xoBreadCrumb->addTips using the _AM_SYSTEM_MENUS_NAV_MAIN and
_AM_SYSTEM_MENUS_NAV_TIPS constants before the language file is loaded;
explicitly call xoops_loadLanguage('admin/menus', 'system') immediately before
those lines so the constants are defined (this avoids relying on the later
menuscategoryHandler constructor to implicitly load the file).
---
Duplicate comments:
In `@htdocs/modules/system/admin/menus/main.php`:
- Around line 422-427: The current validation only prevents choosing the item
itself as parent; update the saveitem logic (around Request::getInt('items_pid',
0), $itempid, $id and $obj->setVar('items_pid', ...)) to also reject a parent
that is in a different category or a descendant: when $itempid != 0, load the
parent item record by $itempid, verify the parent's category matches the current
item's category field (e.g. items_mid or category identifier) and then walk the
parent chain (follow items_pid repeatedly) to ensure none of the ancestors equal
$id; if either check fails, append the same error and do not call
$obj->setVar('items_pid', $itempid).
- Around line 120-122: The code dereferences the return of
$menuscategoryHandler->get(...) (stored in $obj) without checking it's a valid
object, causing possible fatal errors; update each occurrence (e.g., where $obj
is used with ->getFormCat() or ->getForm()) to first verify the result is an
object (or not null) and handle the failure path (log an error, set an error
message, redirect, or skip rendering the form) before calling methods on $obj;
apply this guard to every location that calls $menuscategoryHandler->get(...)
and immediately uses $obj (including uses of getFormCat() and getForm()) so
dereferencing is safe.
- Line 222: In delitem flow the code currently trusts
Request::getInt('category_id', 0) (assigned to $category_id) for tree loading
and redirecting, which can cause orphaned descendants/permissions if the request
value is wrong; after loading the item ($obj via the item loader used in
delitem), replace usage of $category_id (and any subsequent
Request::getInt('category_id') uses at the other spots) with the item's stored
category via $obj->getVar('items_cid') for tree operations and redirects so the
deletion/descendant handling always uses the authoritative category from the
persisted item.
- Around line 68-69: The saveorder renumbering ignores pagination offset and
always starts at 1; update the saveorder handler (function saveorder) to accept
and use the current page offset (e.g., $start or an 'offset'/'start' value
carried in the payload) and when assigning new positions start numbering from
($start + 1) instead of 1; ensure the client-side payload includes that offset
when calling saveorder and adjust any loops that set position values so they add
the offset before persisting (also apply the same fix to the other renumbering
block referenced around the second occurrence).
- Around line 628-646: The ancestor-check loop can skip validating the immediate
parent when items_pid === 0, allowing reactivation under an inactive category;
update the traversal that starts from $parentId = (int)$obj->getVar('items_pid')
to fetch and inspect the parent object before deciding to break (use a do/while
or pre-check pattern), always checking (int)$parentObj->getVar('items_active')
and emitting the same JSON error (using _AM_SYSTEM_MENUS_ERROR_PARENTINACTIVE
and the xoopsSecurity token) when a parent is inactive; use
$menusitemsHandler->get to obtain the parent object and ensure the logic covers
the case where get(0) returns an object or where the immediate parent must still
be validated even if its pid is 0.
In `@htdocs/themes/xswatch5/tpl/nav-menu.tpl`:
- Line 16: The anchor used for parent items currently has both an href and
data-bs-toggle="dropdown", which prevents navigation; change the markup so the
<a> with href and target (the parent link rendered by the nav-menu.tpl item)
does not carry data-bs-toggle or role="button", and instead render a separate
sibling control (a <button> or <a> with no href) that holds
data-bs-toggle="dropdown", aria-expanded and the dropdown-toggle class to open
the submenu; update the template code around the anchor that references
$item.url/$item.target and the conditional that adds data-bs-toggle so the
toggle is a distinct element (preserve existing classes and aria labels for
accessibility).
- Line 16: The anchor rendering in nav-menu.tpl omits rel attributes for
external tabs: when $item.target renders "_blank" (the anchor that uses href
with the conditional and the second anchor around line 31), add rel="noopener
noreferrer" conditionally; update the two anchor templates that reference
$item.url/$item.target so that if $item.target eq "_blank" the anchor includes
rel="noopener noreferrer" (keep current target handling and dropdown attributes
intact).
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 8462ce85-9d7e-468d-9ebb-4c175a5e8166
📒 Files selected for processing (3)
htdocs/modules/system/admin/menus/main.phphtdocs/modules/system/language/english/admin/menus.phphtdocs/themes/xswatch5/tpl/nav-menu.tpl
Comment on lines
+59
to
+60
| $xoBreadCrumb->addLink(_AM_SYSTEM_MENUS_NAV_MAIN, system_adminVersion('menus', 'adminpath')); | ||
| $xoBreadCrumb->addTips(sprintf(_AM_SYSTEM_MENUS_NAV_TIPS, $GLOBALS['xoopsConfig']['language'])); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify whether admin/menus language file is explicitly loaded anywhere
rg -n --type=php "xoops_loadLanguage\\(|admin/menus\\.php|_AM_SYSTEM_MENUS_NAV_MAIN|_AM_SYSTEM_MENUS_NAV_TIPS"Repository: XOOPS/XoopsCore27
Length of output: 10285
🏁 Script executed:
# Read the top of the main.php file to check all includes
head -80 htdocs/modules/system/admin/menus/main.phpRepository: XOOPS/XoopsCore27
Length of output: 3246
🏁 Script executed:
# Check if the header file (admin_header.php or system header) loads the menus language file
cat -n htdocs/modules/system/admin/admin_header.php | head -50Repository: XOOPS/XoopsCore27
Length of output: 137
🏁 Script executed:
# Check if system admin common or initialization files load admin/menus language
rg -B5 -A5 "admin/menus" htdocs/modules/system/Repository: XOOPS/XoopsCore27
Length of output: 43
🏁 Script executed:
# Find and check the definition of xoops_cp_header
rg -n "function xoops_cp_header" htdocs/Repository: XOOPS/XoopsCore27
Length of output: 122
🏁 Script executed:
# Check kernel/menuscategory.php to see when it loads the menus language file
sed -n '50,75p' htdocs/kernel/menuscategory.phpRepository: XOOPS/XoopsCore27
Length of output: 1213
🏁 Script executed:
# Check kernel/menusitems.php to see when it loads the menus language file
sed -n '50,75p' htdocs/kernel/menusitems.phpRepository: XOOPS/XoopsCore27
Length of output: 1197
🏁 Script executed:
# Verify the exact execution order in the list case
sed -n '55,75p' htdocs/modules/system/admin/menus/main.php | cat -nRepository: XOOPS/XoopsCore27
Length of output: 1208
Explicitly load admin/menus.php before using _AM_SYSTEM_MENUS_* constants.
Lines 59–60 use _AM_SYSTEM_MENUS_NAV_MAIN and _AM_SYSTEM_MENUS_NAV_TIPS constants before the language file is loaded. The menuscategoryHandler instantiation on line 66 triggers loading via kernel class constructors, but this creates an implicit dependency. Add an explicit xoops_loadLanguage('admin/menus', 'system'); call before line 59 to prevent undefined-constant notices and ensure the constants are always available.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@htdocs/modules/system/admin/menus/main.php` around lines 59 - 60, The code
calls $xoBreadCrumb->addLink and $xoBreadCrumb->addTips using the
_AM_SYSTEM_MENUS_NAV_MAIN and _AM_SYSTEM_MENUS_NAV_TIPS constants before the
language file is loaded; explicitly call xoops_loadLanguage('admin/menus',
'system') immediately before those lines so the constants are defined (this
avoids relying on the later menuscategoryHandler constructor to implicitly load
the file).
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 23 out of 27 changed files in this pull request and generated 7 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+448
to
+453
| try { | ||
| // Tenter une requête pour vérifier l'existence | ||
| $sql = "SHOW TABLES LIKE '" . $GLOBALS['xoopsDB']->prefix('menuscategory') . "'"; | ||
| $result = $GLOBALS['xoopsDB']->queryF($sql); | ||
| $tableExists = $GLOBALS['xoopsDB']->getRowsNum($result) > 0; | ||
| } catch (Exception $e) { |
Comment on lines
+556
to
+569
| // first update all direct items of category | ||
| $critCat = new Criteria('items_cid', $category_id); | ||
| $allItems = $menusitemsHandler->getAll($critCat); | ||
| foreach ($allItems as $itm) { | ||
| $idtmp = $itm->getVar('items_id'); | ||
| if ((int)$itm->getVar('items_active') !== $new) { | ||
| $itm->setVar('items_active', $new); | ||
| if ($menusitemsHandler->insert($itm, true)) { | ||
| $updatedItems[] = $idtmp; | ||
| } | ||
| } | ||
| // propagate to children of this item | ||
| $recursiveUpdate($menusitemsHandler, $idtmp, $new, $updatedItems); | ||
| } |
Comment on lines
+661
to
+676
| function propagateActiveState($handler, $parentId, $state, array &$updated) | ||
| { | ||
| $criteria = new Criteria('items_pid', (int)$parentId); | ||
| /** @var \XoopsObject[] $children */ | ||
| $children = $handler->getAll($criteria); | ||
| foreach ($children as $child) { | ||
| $childId = $child->getVar('items_id'); | ||
| if ((int)$child->getVar('items_active') !== $state) { | ||
| $child->setVar('items_active', $state); | ||
| if ($handler->insert($child, true)) { | ||
| $updated[] = $childId; | ||
| } | ||
| } | ||
| propagateActiveState($handler, $childId, $state, $updated); | ||
| } | ||
| } |
Comment on lines
+15
to
+33
| <a class="dropdown-item<{if $item.children}> dropdown-toggle<{/if}>" | ||
| href="<{if $item.url neq ''}><{$item.url|escape}><{else}>#<{/if}>"<{if $item.children}> role="button" data-bs-toggle="dropdown" aria-expanded="false"<{/if}> target="<{$item.target}>"> | ||
| <{$item.prefix}> <{$item.title|escape}> <{$item.suffix}> | ||
| </a> | ||
| <{if $item.children}> | ||
| <{call name=renderMenu items=$item.children level=$level+1}> | ||
| <{/if}> | ||
| <{else}> | ||
| <a class="dropdown-item" href="<{$xoops_url}>/user.php"><{$smarty.const.THEME_ACCOUNT_LOGIN}></a> | ||
| <a class="dropdown-item" href="<{$xoops_url}>/register.php"><{$smarty.const.THEME_ACCOUNT_REGISTER}></a> | ||
| </li> | ||
| <{/foreach}> | ||
| </ul> | ||
| <{/function}> | ||
| <ul class="navbar-nav me-auto"> | ||
| <{foreach $xoMenuCategories as $cat}> | ||
| <li class="nav-item<{if $cat.items}> dropdown<{/if}>"> | ||
| <a class="nav-link<{if $cat.items}> dropdown-toggle<{/if}>" | ||
| href="<{$cat.category_url|escape|default:'#'}>" | ||
| <{if $cat.items}>role="button" data-bs-toggle="dropdown" aria-expanded="false"<{/if}> target="<{$cat.category_target}>"> | ||
| <{$cat.category_prefix}> <{$cat.category_title|escape}> <{$cat.category_suffix}> | ||
| </a> |
Comment on lines
+72
to
+76
| $sql = "INSERT INTO " . $xoopsDB->prefix('') . "_menusitems VALUES (5, 0, 3, 'MENUS_ACCOUNT_NOTIFICATIONS', '<span class=\"fa fa-info-circle fa-fw\" ></span>', '', 'notifications.php', 0, 4, 1, 1)"; | ||
| $xoopsDB->query($sql); | ||
| $sql = "INSERT INTO " . $xoopsDB->prefix('') . "_menusitems VALUES (6, 0, 3, 'MENUS_ACCOUNT_TOOLBAR', '<span class=\"fa-solid fa-screwdriver-wrench fa-fw\" ></span>', '<span id=\"xswatch-toolbar-ind\"></span>', 'javascript:xswatchToolbarToggle();', 0, 5, 1, 1)"; | ||
| $xoopsDB->query($sql); | ||
| $sql = "INSERT INTO " . $xoopsDB->prefix('') . "_menusitems VALUES (7, 0, 3, 'MENUS_ACCOUNT_LOGOUT', '<span class=\"fa fa-sign-out fa-fw\" ></span>', '', 'user.php?op=logout', 0, 5, 1, 1)"; |
Comment on lines
+503
to
+507
| 'title' => $child->getResolvedTitle(), | ||
| 'prefix' => $this->renderMenuAffix($child->getVar('items_prefix')), | ||
| 'suffix' => $this->renderMenuAffix($child->getVar('items_suffix')), | ||
| 'url' => $child->getVar('items_url'), | ||
| 'target' => ($child->getVar('items_target') == 1) ? '_blank' : '_self', |
| $category['position'] = $category_arr[$i]->getVar('category_position'); | ||
| $category['active'] = $category_arr[$i]->getVar('category_active'); | ||
| $category['protected'] = $category_arr[$i]->getVar('category_protected'); | ||
| $category_img = $category_arr[$i]->getVar('category_logo'); |
Collaborator
|
From my AI friends: Critical Issues
Major Issues
Language & Localization Issues
Minor Issues
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Addition of the menu management system from the system module. Only with the “cpadmin” theme for the backend and xswatch 5 for the frontend.
Summary by CodeRabbit