Clean-room system menu administration#1666
Conversation
This reverts commit b66d298.
… form descriptions
…ds, robust inbox count
…PDATE, permission cleanup
… hook to frontend JS
…expanded in theme templates
…nd disabled-item guards
…ystem_header.tpl, full parent chain check
| $this->assertLessThan( | ||
| $stripPos, | ||
| $inboxPos, | ||
| 'xoInboxCount expansion must happen BEFORE strip_tags sanitization' | ||
| ); |
There was a problem hiding this comment.
The assertion order here is reversed: to enforce “xoInboxCount expansion must happen BEFORE strip_tags”, the test should assert $inboxPos < $stripPos (or equivalent). As written, it expects strip_tags to occur first and will fail with the current theme.php implementation (both tokens even appear on the same comment line). Consider also scoping the search to the renderMenuAffix() method body to avoid brittle first-occurrence matches elsewhere in theme.php.
- Category title is now a normal navigable link (no data-toggle/data-bs-toggle) - Separate dropdown-toggle-split caret opens the submenu - Fixes: categories with both a URL and children are now clickable - Applied to xswatch4 (Bootstrap 4) and xbootstrap5 (Bootstrap 5) - xbootstrap theme unchanged (uses hardcoded demo links, not dynamic menus)
| redirect_header(MENUS_ADMIN_URL, 3, 'Failed to delete item: ' . $item->getVar('items_id')); | ||
| } | ||
| xoops_confirm([ | ||
| 'surdel' => true, | ||
| 'item_id' => $item_id, | ||
| 'category_id' => $objCid, | ||
| 'op' => 'delitem' | ||
| ], Request::getString('REQUEST_URI', '', 'SERVER'), sprintf(_AM_SYSTEM_MENUS_SUREDELITEM, htmlspecialchars((string)$obj->getVar('items_title'), ENT_QUOTES, 'UTF-8')) . $items); | ||
| } | ||
|
|
||
| // Delete category permissions and category | ||
| $permHelper->deletePermissionForItem('menus_category_view', $catId); | ||
| if (!menus_cat_handler()->delete($cat)) { | ||
| redirect_header(MENUS_ADMIN_URL, 3, 'Failed to delete category'); | ||
| } |
There was a problem hiding this comment.
These redirects use hard-coded English strings ('Failed to delete item/category...'), which won't be translated and are inconsistent with the rest of the menus admin UI strings. Prefer language constants (and optionally interpolate the item/category id) so admin-facing errors are localized.
| $db = \XoopsDatabaseFactory::getDatabaseConnection(); | ||
| foreach (['menuscategory', 'menusitems'] as $tableName) { | ||
| $result = $db->query('SHOW TABLES LIKE ' . $db->quote($db->prefix($tableName))); | ||
| if (!$db->isResultSet($result) || !($result instanceof \mysqli_result) || 0 === $db->getRowsNum($result)) { | ||
| return false; | ||
| } |
There was a problem hiding this comment.
hasMenuTables() uses SHOW TABLES LIKE with an unescaped pattern. Since _ and % are wildcards in LIKE, a prefix such as xoops_menuscategory can match unintended tables (false positives). Escape _/% (or query INFORMATION_SCHEMA) before checking existence.
| private function normalizeMenuUrl(string $url): string | ||
| { | ||
| $url = trim($url); | ||
| if ($url === '') { | ||
| return '#'; | ||
| } | ||
|
|
||
| if (preg_match('/^\s*javascript\s*:/i', $url)) { | ||
| return '#'; | ||
| } | ||
|
|
||
| if (preg_match('~^(https?://|mailto:|tel:|ftp://|/|#)~i', $url)) { | ||
| return $url; | ||
| } | ||
|
|
||
| return XOOPS_URL . '/' . ltrim($url, '/'); |
There was a problem hiding this comment.
normalizeMenuUrl() blocks only a literal javascript: prefix. A stored value like javascript:... (or other entity/whitespace obfuscations) can bypass this check and still execute once the template escapes/HTML-decodes the attribute. Decode entities first and enforce an allowlist of schemes (or reuse the admin-side URL sanitizer).
| $cacheTime = 'xo_inbox_count_time_' . $uid; | ||
|
|
||
| $currentScript = basename($_SERVER['SCRIPT_NAME'] ?? ''); | ||
| if ($currentScript === 'viewpmsg.php' || $currentScript === 'readpmsg.php') { |
There was a problem hiding this comment.
getInboxUnreadCount() only invalidates the cached count on viewpmsg.php and readpmsg.php. The existing xoInboxCount plugin also clears cache on pmlite.php, so counts can remain stale when messages are read in that flow.
| if ($currentScript === 'viewpmsg.php' || $currentScript === 'readpmsg.php') { | |
| if ($currentScript === 'viewpmsg.php' | |
| || $currentScript === 'readpmsg.php' | |
| || $currentScript === 'pmlite.php' | |
| ) { |
… hover highlight xswatch4: - Single link with dropdown-toggle, opens on hover via CSS (matches xbootstrap5 behavior) - Added dropdown-submenu CSS to my_xoops.css (right-pointing arrows for nested items) - Removed dropdown-toggle from submenu items (CSS ::after handles the caret) xbootstrap5: - Split link + toggle with div wrapper (link navigable, caret opens dropdown) - Removed dropdown-toggle from submenu items (multilevelmenu.css handles arrows) - Added blue hover highlight on dropdown items (matches xswatch4)
| // Absolute URLs must use an allowed scheme | ||
| if (preg_match('#^([a-zA-Z][a-zA-Z0-9+.-]*):#', $url, $m)) { | ||
| $scheme = strtolower($m[1]); | ||
| if (!in_array($scheme, ['http', 'https', 'ftp', 'mailto'], true)) { |
There was a problem hiding this comment.
Allowed URL schemes here differ from the front-end normalizer in class/theme.php (which treats tel: as allowed). As-is, admins won’t be able to save tel: links even though the renderer supports them. Consider aligning the allowed-scheme list between admin sanitization and theme normalization (either allow tel here or remove it from the renderer).
| if (!in_array($scheme, ['http', 'https', 'ftp', 'mailto'], true)) { | |
| if (!in_array($scheme, ['http', 'https', 'ftp', 'mailto', 'tel'], true)) { |
…igates
- xswatch4: added JS that intercepts first tap on dropdown links to show
the submenu, then allows the second tap to follow the href
- Fixes touch/mobile regression where hover-only CSS made child items unreachable
- Desktop hover behavior preserved unchanged
| <a class="dropdown-item" | ||
| href="<{if $subItem.url|default:'' neq ''}><{$subItem.url|escape}><{else}>#<{/if}>" | ||
| target="<{$subItem.target}>" |
There was a problem hiding this comment.
Nested submenu links with children are rendered without the dropdown-toggle class. The shared multilevelmenu.js click handler targets .dropdown-submenu > .dropdown-toggle, so nested submenus won’t be toggleable on touch/keyboard for this theme unless the class (and typically aria-expanded) is present.
| <a class="dropdown-item" | |
| href="<{if $subItem.url|default:'' neq ''}><{$subItem.url|escape}><{else}>#<{/if}>" | |
| target="<{$subItem.target}>" | |
| <a class="dropdown-item dropdown-toggle" | |
| href="<{if $subItem.url|default:'' neq ''}><{$subItem.url|escape}><{else}>#<{/if}>" | |
| target="<{$subItem.target}>" | |
| aria-haspopup="true" | |
| aria-expanded="false" |
| string $permName, | ||
| int $itemId, | ||
| array $groupIds | ||
| ): void { |
There was a problem hiding this comment.
PSR-12 brace placement: the opening brace for system_menu_seed_permissions() should be on the next line after the function signature (currently ): void {).
| ): void { | |
| ): void | |
| { |
- Touch script now intercepts taps on both top-level dropdown-toggle and
nested .dropdown-submenu > a elements
- First tap opens the submenu at any level, second tap follows the href
- Uses :scope > .dropdown-menu to target only the direct child menu
| var opened = null; | ||
| document.addEventListener('click', function(e) { | ||
| var link = e.target.closest('.xo-hover-dropdown > a.dropdown-toggle, .dropdown-submenu > a'); | ||
| if (!link) { opened = null; return; } |
There was a problem hiding this comment.
The touch dropdown handler clears opened when clicking outside a toggle (if (!link) { opened = null; return; }) but does not remove the .show classes from the currently opened menu. This can leave dropdowns stuck open on touch devices. Consider closing the previously opened menu when clicks occur outside the dropdown/toggles (or delegate to Bootstrap’s dropdown events instead of manual class toggling).
| if (!link) { opened = null; return; } | |
| if (!link) { | |
| if (opened) { | |
| opened.classList.remove('show'); | |
| var prev = opened.querySelector('.dropdown-menu'); | |
| if (prev) { | |
| prev.classList.remove('show'); | |
| } | |
| } | |
| opened = null; | |
| return; | |
| } |
Fix touch dropdown: track open state per depth, never close ancestors - Replace single global `opened` with `openedByDepth[depth]` map - Tapping a nested submenu closes only peer menus at same depth + deeper levels - Ancestor menus stay open so child submenus remain visible and reachable
- Outside-click handler now removes .show from all tracked open nodes before clearing the map - Fixes dropdowns getting stuck open on touch devices after tapping outside
|
| string $permName, | ||
| int $itemId, | ||
| array $groupIds | ||
| ): void { |
There was a problem hiding this comment.
system_menu_seed_permissions() uses ): void { with the opening brace on the same line as the signature, while the surrounding file consistently places function opening braces on the next line (PSR-12). Update the function formatting to match the rest of the file for consistency.
| ): void { | |
| ): void | |
| { |
| li.classList.add('show'); | ||
| var menu = li.querySelector(':scope > .dropdown-menu'); | ||
| if (menu) menu.classList.add('show'); | ||
| openedByDepth[depth] = li; |
There was a problem hiding this comment.
The xswatch4 menu script toggles Bootstrap's show class on nested submenus, but the shared multilevelmenu.css added by Theme::loadMenus() hides .dropdown-submenu > .dropdown-menu by default and only shows it via .dropdown-submenu.is-open (or hover). To avoid submenu toggles failing depending on CSS load order, align this script with the shared convention (toggle is-open / aria-expanded) or update the shared CSS to also respect .show for nested submenus.
Summary
Clean-room implementation of the system menu admin UI, replacing the
disputed PR #1665 / Core27 PR #20 code with a fresh implementation
that preserves the same user-visible functionality without reusing
the original source.
Test plan
Summary by CodeRabbit
New Features
Improvements
Security / Bug Fixes
Tests