fix(system-menus): create and seed menu tables during fresh install#8
Merged
mambax7 merged 4 commits intoXOOPS:masterfrom Apr 11, 2026
Merged
Conversation
Fresh XOOPS installs did not create or populate `menuscategory` and `menusitems` because menu setup only ran from `modules/system/include/update.php`. As a result, menus were missing until the System module was manually updated. Move menu setup into the real install path by: - adding the menu tables to `htdocs/install/sql/mysql.structure.sql` - seeding default menu rows and permissions from `htdocs/install/include/makedata.php` - extracting shared seed definitions to `modules/system/include/menu_seed.php` Keep the existing System module update flow as the idempotent upgrade path. Align `system_menu_create_tables()` column types with install SQL (INT UNSIGNED for auto-increment IDs and items_cid FK, TINYINT(1) UNSIGNED for boolean flags) to avoid schema drift between fresh installs and upgraded sites. Add `onInstall` for the System module so reinstall flows reuse the same update logic. This hook does not fire on fresh install — the installer inserts the System module row directly via `makedata.php` — but it protects the module-admin reinstall path. Add test coverage in `tests/unit/htdocs/modules/system/SystemMenuInstallationTest.php` covering: shared seed definitions and group-key mapping, installer schema for both menu tables including the FK cascade clause, update-script schema parity, installer wiring, behavioral verification of the `$modversion` hooks, and presence of the lifecycle functions in `update.php`. Update `SystemMenusRegressionTest.php` so its toolbar fragment-URL assertion reads from the new shared seed source.
WalkthroughAdds a menu-seeding subsystem: new seed definitions and mapping, installer/update integration to create Changes
Sequence Diagram(s)sequenceDiagram
participant Installer as "Fresh Installer"
participant MakeData as "install/include/makedata.php"
participant SeedDef as "modules/system/include/menu_seed.php"
participant GroupMap as "group mapping logic"
participant Database as "Database (menuscategory/menusitems/group_permission)"
Installer->>MakeData: make_data()
MakeData->>MakeData: insert `system` module row (moduleId=1)
MakeData->>SeedDef: system_menu_get_seed_definitions()
SeedDef-->>MakeData: { categories[], items[] }
MakeData->>GroupMap: system_menu_map_group_keys(cat.group_keys, groupMap)
loop categories
MakeData->>Database: INSERT INTO `menuscategory` (category data)
Database-->>MakeData: category_id
MakeData->>Database: INSERT INTO `group_permission` (menus_category_view) for mapped groups
end
MakeData->>GroupMap: system_menu_map_group_keys(item.group_keys, groupMap)
loop items
MakeData->>Database: INSERT INTO `menusitems` (items data, items_cid)
Database-->>MakeData: items_id
MakeData->>Database: INSERT INTO `group_permission` (menus_items_view) for mapped groups
end
Database-->>Installer: success or E_USER_WARNING on failure
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related issues
Possibly related PRs
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 PHPStan (2.1.46)Invalid entry in excludePaths: If the excluded path can sometimes exist, append (?) parameters: 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 |
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## master #8 +/- ##
============================================
- Coverage 19.23% 18.05% -1.18%
- Complexity 7584 7759 +175
============================================
Files 621 665 +44
Lines 40085 42700 +2615
============================================
Hits 7709 7709
- Misses 32376 34991 +2615 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Contributor
There was a problem hiding this comment.
Pull request overview
This PR fixes missing System menus on fresh XOOPS installs by moving menu table creation and default seeding into the installer path, while keeping the System module update script as the idempotent upgrade path.
Changes:
- Add
menuscategory/menusitemstable definitions to the installer SQL and seed default menu rows + permissions during install. - Extract protected menu seed definitions into a shared
menu_seed.phpand reuse it from both installer and update flows. - Add System module
onInstallhook and expand unit tests to validate schema/seed wiring and regression coverage.
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/unit/htdocs/modules/system/SystemMenusRegressionTest.php | Updates regression assertion to read toolbar seed from the new shared seed file. |
| tests/unit/htdocs/modules/system/SystemMenuInstallationTest.php | Adds coverage for shared seed definitions, schema expectations, installer wiring, and lifecycle hooks. |
| htdocs/modules/system/xoops_version.php | Registers onInstall to reuse update logic for reinstall flows. |
| htdocs/modules/system/include/update.php | Reuses shared seeds, adds install hook, and aligns schema types with installer definitions. |
| htdocs/modules/system/include/menu_seed.php | Introduces shared protected menu seed data + group-key mapping helper. |
| htdocs/install/sql/mysql.structure.sql | Adds installer DDL for menu tables (including FK clause). |
| htdocs/install/include/makedata.php | Seeds protected menus + permissions during fresh install using shared seeds. |
Comment on lines
+270
to
+273
| KEY idx_items_pid (items_pid), | ||
| FOREIGN KEY (items_cid) | ||
| REFERENCES menuscategory (category_id) | ||
| ON DELETE CASCADE |
Comment on lines
+66
to
+70
| $categoryId = $dbm->insert( | ||
| 'menuscategory', | ||
| " (`category_title`, `category_prefix`, `category_suffix`, `category_url`, `category_target`, `category_position`, `category_protected`, `category_active`) VALUES (" | ||
| . "'" . addslashes($definition['title']) . "', " | ||
| . "'" . addslashes($definition['prefix']) . "', " |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
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/include/update.php (1)
305-336:⚠️ Potential issue | 🟠 MajorThese type changes do not migrate existing menu tables.
Changing the
CREATE TABLE IF NOT EXISTSdefinitions fixes fresh installs only. On upgraded sites, those statements are a no-op, andsystem_menu_normalize_schema()neverMODIFYscategory_id,category_target,category_active,items_id,items_cid,items_target, oritems_active. Existing installs therefore keep the old signed columns, which means the fresh-install/update parity this PR is targeting is still not achieved.Please add explicit
ALTER TABLE ... MODIFYsteps for the existing tables in the normalization path.🤖 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 305 - 336, The CREATE TABLE changes only affect fresh installs; to migrate existing databases update system_menu_normalize_schema() to run explicit ALTER TABLE ... MODIFY statements for the existing tables (use $db->prefix('menuscategory') and $db->prefix('menusitems')) to convert category_id and items_id to INT UNSIGNED AUTO_INCREMENT, category_target/items_target and category_active/items_active to TINYINT(1) UNSIGNED, and items_cid to INT UNSIGNED (preserve NOT NULL/defaults), and execute them via system_menu_exec_or_throw($db, $sql); also ensure foreign key names (fk_items_category) and parent FK handling in system_menu_drop_parent_foreign_keys are compatible after type changes.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@htdocs/install/include/makedata.php`:
- Around line 65-112: Check the return value of each Db_manager::insert call
(the uses of $dbm->insert that set $categoryId and $itemId) and abort seeding if
it is falsey: when $categoryId or $itemId is false/0, call trigger_error with a
clear message and E_USER_WARNING and stop further processing (return/throw/exit
from the makedata flow) so you don't insert permissions or items for an invalid
id; also avoid using the invalid id when building the subsequent
group_permission inserts and ensure system_menu_map_group_keys is only iterated
if the insert succeeded.
In `@tests/unit/htdocs/modules/system/SystemMenuInstallationTest.php`:
- Around line 1-13: Add the repository-standard XOOPS header block at the top of
the PHP file (above declare(strict_types=1)) and add a class docblock
immediately above the SystemMenuInstallationTest class declaration that includes
the required metadata tags: `@category`, `@package`, `@author`, `@copyright`, `@license`,
and `@link`; ensure the header and docblock follow the project's formatting/style
conventions.
- Around line 7-12: The test methods rely on the #[Test] attribute which PHPUnit
9.6 does not support, so rename each test method to the test{MethodName}
convention so discovery works across PHPUnit 9.6/10/11; specifically rename
sharedSeedDefinitionsExposeExpectedProtectedMenus() →
testSharedSeedDefinitionsExposeExpectedProtectedMenus(),
sharedGroupKeyMapperResolvesKnownGroupsOnly() →
testSharedGroupKeyMapperResolvesKnownGroupsOnly(),
installerSchemaDeclaresMenuTablesUsingUnsignedIds() →
testInstallerSchemaDeclaresMenuTablesUsingUnsignedIds(),
updateScriptCreateTablesMatchInstallerSignedness() →
testUpdateScriptCreateTablesMatchInstallerSignedness(),
installerUsesSharedSeedDefinitionsAndSeedsMenusAfterSystemModuleInsert() →
testInstallerUsesSharedSeedDefinitionsAndSeedsMenusAfterSystemModuleInsert(),
systemModuleRegistersInstallAndUpdateHooksToSameScript() →
testSystemModuleRegistersInstallAndUpdateHooksToSameScript(), and
updateScriptExposesMenuLifecycleFunctions() →
testUpdateScriptExposesMenuLifecycleFunctions(); leave the #[CoversNothing] and
#[Test] attributes in place.
---
Outside diff comments:
In `@htdocs/modules/system/include/update.php`:
- Around line 305-336: The CREATE TABLE changes only affect fresh installs; to
migrate existing databases update system_menu_normalize_schema() to run explicit
ALTER TABLE ... MODIFY statements for the existing tables (use
$db->prefix('menuscategory') and $db->prefix('menusitems')) to convert
category_id and items_id to INT UNSIGNED AUTO_INCREMENT,
category_target/items_target and category_active/items_active to TINYINT(1)
UNSIGNED, and items_cid to INT UNSIGNED (preserve NOT NULL/defaults), and
execute them via system_menu_exec_or_throw($db, $sql); also ensure foreign key
names (fk_items_category) and parent FK handling in
system_menu_drop_parent_foreign_keys are compatible after type changes.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 7dbedd31-c3eb-47a9-b75c-a0f07834f1c2
📒 Files selected for processing (7)
htdocs/install/include/makedata.phphtdocs/install/sql/mysql.structure.sqlhtdocs/modules/system/include/menu_seed.phphtdocs/modules/system/include/update.phphtdocs/modules/system/xoops_version.phptests/unit/htdocs/modules/system/SystemMenuInstallationTest.phptests/unit/htdocs/modules/system/SystemMenusRegressionTest.php
Comment on lines
+7
to
+12
| use PHPUnit\Framework\Attributes\CoversNothing; | ||
| use PHPUnit\Framework\Attributes\Test; | ||
| use PHPUnit\Framework\TestCase; | ||
|
|
||
| #[CoversNothing] | ||
| final class SystemMenuInstallationTest extends TestCase |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
Does PHPUnit 9.6 support PHP 8 attributes like #[Test] and #[CoversNothing], or are they only supported in PHPUnit 10+? Please answer with links to the official PHPUnit 9.6 and 10.x documentation.
💡 Result:
No, PHPUnit 9.6 does not support PHP 8 attributes like #[Test] and #[CoversNothing]. These are only supported in PHPUnit 10 and later. Official documentation links: - PHPUnit 9.6 Manual (covers annotations only, no attributes section): https://docs.phpunit.de/en/9.6/ (Appendix 2 lists annotations like @coversNothing, @test; no mention of attributes or #[...]) - PHPUnit 10 Announcement (introduces PHP 8 attributes support): https://phpunit.de/announcements/phpunit-10.html - PHPUnit 10.5 Attributes docs: https://docs.phpunit.de/en/10.5/attributes.html (explicitly states "Prior to PHPUnit 10, annotations ... were the only means"; lists #[Test], #[CoversNothing] etc. in PHPUnit\Framework\Attributes)
Citations:
- 1: https://phpunit.de/announcements/phpunit-10.html
- 2: https://github.com/sebastianbergmann/phpunit/blob/9.6/ChangeLog-9.6.md
- 3: https://docs.phpunit.de/en/13.0/attributes.html
- 4: https://docs.phpunit.de/en/9.6/installation.html
- 5: https://docs.phpunit.de/en/9.6/
- 6: https://phpunit.de/documentation.html
- 7: https://docs.phpunit.de/en/11.0/attributes.html
- 8: https://docs.phpunit.de/en/10.5/attributes.html
🏁 Script executed:
cat -n tests/unit/htdocs/modules/system/SystemMenuInstallationTest.php | sed -n '1,50p'Repository: XOOPS/XoopsCore27
Length of output: 2175
🏁 Script executed:
cat -n tests/unit/htdocs/modules/system/SystemMenuInstallationTest.php | sed -n '35,145p'Repository: XOOPS/XoopsCore27
Length of output: 5300
Rename all test methods to use test{MethodName} convention to ensure discovery across PHPUnit 9.6, 10.5, and 11.x.
All seven test methods in this file (lines 35–145) currently depend on the #[Test] attribute for discovery. PHPUnit 9.6 does not support PHP 8 attributes—they were introduced in PHPUnit 10. Since the test method names do not follow the test{MethodName} pattern, the entire test suite will fail discovery when run against PHPUnit 9.6, breaking the requirement that "Tests must work across PHPUnit 9.6, 10.5, and 11.x."
Rename each method:
sharedSeedDefinitionsExposeExpectedProtectedMenus()→testSharedSeedDefinitionsExposeExpectedProtectedMenus()sharedGroupKeyMapperResolvesKnownGroupsOnly()→testSharedGroupKeyMapperResolvesKnownGroupsOnly()installerSchemaDeclaresMenuTablesUsingUnsignedIds()→testInstallerSchemaDeclaresMenuTablesUsingUnsignedIds()updateScriptCreateTablesMatchInstallerSignedness()→testUpdateScriptCreateTablesMatchInstallerSignedness()installerUsesSharedSeedDefinitionsAndSeedsMenusAfterSystemModuleInsert()→testInstallerUsesSharedSeedDefinitionsAndSeedsMenusAfterSystemModuleInsert()systemModuleRegistersInstallAndUpdateHooksToSameScript()→testSystemModuleRegistersInstallAndUpdateHooksToSameScript()updateScriptExposesMenuLifecycleFunctions()→testUpdateScriptExposesMenuLifecycleFunctions()
Keep the #[CoversNothing] and #[Test] attributes; they will be ignored gracefully by PHPUnit 9.6 while remaining effective in 10+.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tests/unit/htdocs/modules/system/SystemMenuInstallationTest.php` around lines
7 - 12, The test methods rely on the #[Test] attribute which PHPUnit 9.6 does
not support, so rename each test method to the test{MethodName} convention so
discovery works across PHPUnit 9.6/10/11; specifically rename
sharedSeedDefinitionsExposeExpectedProtectedMenus() →
testSharedSeedDefinitionsExposeExpectedProtectedMenus(),
sharedGroupKeyMapperResolvesKnownGroupsOnly() →
testSharedGroupKeyMapperResolvesKnownGroupsOnly(),
installerSchemaDeclaresMenuTablesUsingUnsignedIds() →
testInstallerSchemaDeclaresMenuTablesUsingUnsignedIds(),
updateScriptCreateTablesMatchInstallerSignedness() →
testUpdateScriptCreateTablesMatchInstallerSignedness(),
installerUsesSharedSeedDefinitionsAndSeedsMenusAfterSystemModuleInsert() →
testInstallerUsesSharedSeedDefinitionsAndSeedsMenusAfterSystemModuleInsert(),
systemModuleRegistersInstallAndUpdateHooksToSameScript() →
testSystemModuleRegistersInstallAndUpdateHooksToSameScript(), and
updateScriptExposesMenuLifecycleFunctions() →
testUpdateScriptExposesMenuLifecycleFunctions(); leave the #[CoversNothing] and
#[Test] attributes in place.
…ade schema Seed protected menu categories, items, and permissions during fresh install from shared definitions, instead of relying on a later System module update. Keep the update path idempotent for upgrades and normalize existing menu tables to the unsigned schema used by fresh installs. Abort install-time menu seeding if category or item inserts fail to avoid invalid child rows or permissions. Add menu installation tests covering shared seed definitions, installer seeding behavior, FK ordering, and upgrade schema normalization.
Contributor
There was a problem hiding this comment.
Pull request overview
This PR fixes missing System menus on fresh XOOPS installs by moving menu table creation and default menu/permission seeding into the installer path, while keeping the System module update script as the idempotent upgrade path (and aligning schemas between install vs upgrade).
Changes:
- Add
menuscategoryandmenusitemstable definitions to the installer SQL, and seed default menu rows + group permissions during install. - Extract shared menu seed definitions into
modules/system/include/menu_seed.phpand reuse them for both installer seeding and update-script seeding. - Add PHPUnit coverage to verify seed definitions, installer wiring, and schema expectations.
Reviewed changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/unit/htdocs/modules/system/SystemMenusRegressionTest.php | Updates toolbar seed assertion to read from the new shared seed source. |
| tests/unit/htdocs/modules/system/SystemMenuInstallationTest.php | New unit tests for installer seeding, shared seed mapping, and schema parity checks. |
| tests/phpunit.xml.dist | Adds htdocs/install to PHPUnit source include paths. |
| htdocs/modules/system/xoops_version.php | Adds onInstall hook to reuse update logic on module reinstall flows. |
| htdocs/modules/system/include/update.php | Requires shared seed file, adds install hook, aligns schema types, and switches seeding to shared seed definitions. |
| htdocs/modules/system/include/menu_seed.php | New shared seed definitions + group key mapping helper. |
| htdocs/install/sql/mysql.structure.sql | Adds installer schema for menu tables (including a FK clause). |
| htdocs/install/include/makedata.php | Seeds default menus and permissions during fresh install using shared seed definitions. |
Comment on lines
+270
to
+273
| KEY idx_items_pid (items_pid), | ||
| FOREIGN KEY (items_cid) | ||
| REFERENCES menuscategory (category_id) | ||
| ON DELETE CASCADE |
Comment on lines
+1
to
+18
| <?php | ||
| /* | ||
| * You may not change or alter any portion of this comment or credits | ||
| * of supporting developers from this source code or any supporting source code | ||
| * which is considered copyrighted (c) material of the original comment or credit authors. | ||
| * | ||
| * This program is distributed in the hope that it will be useful, | ||
| * but WITHOUT ANY WARRANTY; without even the implied warranty of | ||
| * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. | ||
| */ | ||
|
|
||
| /** | ||
| * Shared protected menu seed data for install and upgrade flows. | ||
| * | ||
| * Titles are stored as language constant identifiers and resolved at render time. | ||
| * | ||
| * @return array{categories: array<string, array<string, mixed>>, items: array<int, array<string, mixed>>} | ||
| */ |
Comment on lines
+274
to
+280
| $itemInserts = array_values(array_filter( | ||
| $dbm->inserts, | ||
| static fn(array $insert): bool => $insert['table'] === 'menusitems' | ||
| )); | ||
| foreach ($itemInserts as $insert) { | ||
| $this->assertMatchesRegularExpression( | ||
| "/VALUES \\(0, 2, 'MENUS_ACCOUNT_[A-Z_]+'/", |
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
tests/unit/htdocs/modules/system/SystemMenuInstallationTest.php (2)
1-12: 🛠️ Refactor suggestion | 🟠 MajorAdd the standard XOOPS header and class metadata block.
This new test file still starts without the repository header, and
SystemMenuInstallationTeststill has no class docblock with the required metadata tags.As per coding guidelines, "Every source file begins with the XOOPS copyright header block" and "Class docblocks include
@category,@package,@author,@copyright,@license, and@linktags".🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/unit/htdocs/modules/system/SystemMenuInstallationTest.php` around lines 1 - 12, Add the standard XOOPS copyright header block at the top of the file and add a class docblock above the SystemMenuInstallationTest class that includes the required metadata tags: `@category`, `@package`, `@author`, `@copyright`, `@license`, and `@link`; ensure the docblock describes the test class and uses the same namespace modulessystem and class name SystemMenuInstallationTest.
7-8:⚠️ Potential issue | 🟠 MajorRename these tests to the
test...convention.Right now discovery depends on
#[Test]. PHPUnit 9.6 does not use attribute-based discovery, so these methods will be skipped there unless their names also start withtest.Does PHPUnit 9.6 discover tests via PHP 8 attributes like #[Test], or only via method names / `@test` annotations? Please cite the official PHPUnit 9.6 and 10.5+ documentation.As per coding guidelines, "Tests must work across PHPUnit 9.6, 10.5, and 11.x" and "Name test methods
test{MethodName}ortest{MethodName}{Scenario}".Also applies to: 36-285
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/unit/htdocs/modules/system/SystemMenuInstallationTest.php` around lines 7 - 8, Rename all test methods in the SystemMenuInstallationTest class to follow the test... naming convention (e.g., change methods like installWorks to testInstallWorks) so PHPUnit 9.6 discovers them; keep or remove the #[Test] attribute as desired but ensure every test method name begins with "test" (apply same renaming to methods in the 36-285 range) and update any references/usages of those method names in the class (e.g., in data providers or annotations) to match the new names.
🤖 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/install/include/makedata.php`:
- Around line 55-127: Change system_menu_install_seed_defaults to return a
boolean instead of void and propagate insert failures (including
group_permission inserts) back to the caller: update function signature
system_menu_install_seed_defaults($dbm, array $groups, int $moduleId): bool,
return false whenever a category/item insert fails (the current trigger_error
branches) and also check the return value of each $dbm->insert that writes to
'group_permission' and return false on failure; return true at the end on
success. Then update callers (e.g. make_data) to check the boolean result and
abort/propagate the install failure if system_menu_install_seed_defaults returns
false.
---
Duplicate comments:
In `@tests/unit/htdocs/modules/system/SystemMenuInstallationTest.php`:
- Around line 1-12: Add the standard XOOPS copyright header block at the top of
the file and add a class docblock above the SystemMenuInstallationTest class
that includes the required metadata tags: `@category`, `@package`, `@author`,
`@copyright`, `@license`, and `@link`; ensure the docblock describes the test class
and uses the same namespace modulessystem and class name
SystemMenuInstallationTest.
- Around line 7-8: Rename all test methods in the SystemMenuInstallationTest
class to follow the test... naming convention (e.g., change methods like
installWorks to testInstallWorks) so PHPUnit 9.6 discovers them; keep or remove
the #[Test] attribute as desired but ensure every test method name begins with
"test" (apply same renaming to methods in the 36-285 range) and update any
references/usages of those method names in the class (e.g., in data providers or
annotations) to match the new names.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: c035c427-8e69-4491-86cf-d014bf83beca
📒 Files selected for processing (4)
htdocs/install/include/makedata.phphtdocs/modules/system/include/update.phptests/phpunit.xml.disttests/unit/htdocs/modules/system/SystemMenuInstallationTest.php
…rors Remove the menu foreign key from installer SQL because SqlUtility::prefixQuery() does not rewrite REFERENCES targets inside CREATE TABLE bodies, which breaks fresh installs with a table prefix. Keep FK creation in the runtime system menu update path, make system_menu_install_seed_defaults() return bool, and abort installation when menu categories, items, or permissions cannot be seeded successfully. Update the menu installation tests to match the new installer/runtime split and derive seeded category IDs dynamically.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
tests/unit/htdocs/modules/system/SystemMenuInstallationTest.php (1)
55-321:⚠️ Potential issue | 🟠 MajorRename these tests to the
test...convention.Every method here depends on
#[Test]for discovery. PHPUnit 9.6 does not use those attributes, so this class will not be discovered there unless the methods are renamed totest.... Keep the attributes if you still want the 10+/11 metadata.Does PHPUnit 9.6 support the #[Test] attribute for test discovery, or must test methods still use the test* naming convention? Please answer with official PHPUnit 9.6 and 10.5 documentation links.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/unit/htdocs/modules/system/SystemMenuInstallationTest.php` around lines 55 - 321, Rename each test method so PHPUnit 9.6 will discover them by prefixing "test" to the existing method names while leaving the #[Test] attributes in place if you want to retain PHPUnit 10+ metadata; specifically rename sharedSeedDefinitionsExposeExpectedProtectedMenus -> testSharedSeedDefinitionsExposeExpectedProtectedMenus, sharedGroupKeyMapperResolvesKnownGroupsOnly -> testSharedGroupKeyMapperResolvesKnownGroupsOnly, installerSchemaDeclaresMenuTablesUsingUnsignedIds -> testInstallerSchemaDeclaresMenuTablesUsingUnsignedIds, updateScriptCreateTablesMatchInstallerSignedness -> testUpdateScriptCreateTablesMatchInstallerSignedness, updateScriptNormalizesExistingMenuTableTypesForUpgrades -> testUpdateScriptNormalizesExistingMenuTableTypesForUpgrades, installerUsesSharedSeedDefinitionsAndSeedsMenusAfterSystemModuleInsert -> testInstallerUsesSharedSeedDefinitionsAndSeedsMenusAfterSystemModuleInsert, systemModuleRegistersInstallAndUpdateHooksToSameScript -> testSystemModuleRegistersInstallAndUpdateHooksToSameScript, updateScriptExposesMenuLifecycleFunctions -> testUpdateScriptExposesMenuLifecycleFunctions, installerSeedingStopsWhenCategoryInsertFails -> testInstallerSeedingStopsWhenCategoryInsertFails, and installerSeedsExpectedRowsAndRespectsCategoryBeforeItemOrdering -> testInstallerSeedsExpectedRowsAndRespectsCategoryBeforeItemOrdering so PHPUnit 9.6 will detect them.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@tests/unit/htdocs/modules/system/SystemMenuInstallationTest.php`:
- Around line 100-110: The assertion that the whole installer schema contains no
'FOREIGN KEY' should be narrowed to just the menusitems table DDL: after calling
readSourceFile('install/sql/mysql.structure.sql') (in
SystemMenuInstallationTest), extract the CREATE TABLE menusitems (...) block
(e.g. with a preg_match on '/CREATE TABLE menusitems \\(.*?\\)\\s*;/s' or
similar) and then call assertStringNotContainsString('FOREIGN KEY',
$menusitemsBlock) instead of asserting against the entire $source; keep existing
tests (e.g. the assertMatchesRegularExpression checks) and only replace the
global FOREIGN KEY assertion with this scoped check.
---
Duplicate comments:
In `@tests/unit/htdocs/modules/system/SystemMenuInstallationTest.php`:
- Around line 55-321: Rename each test method so PHPUnit 9.6 will discover them
by prefixing "test" to the existing method names while leaving the #[Test]
attributes in place if you want to retain PHPUnit 10+ metadata; specifically
rename sharedSeedDefinitionsExposeExpectedProtectedMenus ->
testSharedSeedDefinitionsExposeExpectedProtectedMenus,
sharedGroupKeyMapperResolvesKnownGroupsOnly ->
testSharedGroupKeyMapperResolvesKnownGroupsOnly,
installerSchemaDeclaresMenuTablesUsingUnsignedIds ->
testInstallerSchemaDeclaresMenuTablesUsingUnsignedIds,
updateScriptCreateTablesMatchInstallerSignedness ->
testUpdateScriptCreateTablesMatchInstallerSignedness,
updateScriptNormalizesExistingMenuTableTypesForUpgrades ->
testUpdateScriptNormalizesExistingMenuTableTypesForUpgrades,
installerUsesSharedSeedDefinitionsAndSeedsMenusAfterSystemModuleInsert ->
testInstallerUsesSharedSeedDefinitionsAndSeedsMenusAfterSystemModuleInsert,
systemModuleRegistersInstallAndUpdateHooksToSameScript ->
testSystemModuleRegistersInstallAndUpdateHooksToSameScript,
updateScriptExposesMenuLifecycleFunctions ->
testUpdateScriptExposesMenuLifecycleFunctions,
installerSeedingStopsWhenCategoryInsertFails ->
testInstallerSeedingStopsWhenCategoryInsertFails, and
installerSeedsExpectedRowsAndRespectsCategoryBeforeItemOrdering ->
testInstallerSeedsExpectedRowsAndRespectsCategoryBeforeItemOrdering so PHPUnit
9.6 will detect them.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: a0ab58f6-d6db-44dd-89c2-1f94327a8f38
📒 Files selected for processing (3)
htdocs/install/include/makedata.phphtdocs/install/sql/mysql.structure.sqltests/unit/htdocs/modules/system/SystemMenuInstallationTest.php
tests/unit/htdocs/modules/system/SystemMenuInstallationTest.php
Outdated
Show resolved
Hide resolved
Contributor
There was a problem hiding this comment.
Pull request overview
This PR fixes missing System menus on fresh XOOPS installs by moving menu table creation and default menu/permission seeding into the installer path, while keeping the System module update script as the idempotent upgrade path and extracting shared seed definitions for reuse.
Changes:
- Add
menuscategoryandmenusitemstables to the installer SQL, and seed default protected menus + permissions frominstall/include/makedata.php. - Extract shared menu seed definitions / group-key mapping into
modules/system/include/menu_seed.phpand reuse them from the System module update script. - Add/adjust PHPUnit coverage to validate seed definitions, installer wiring, and update-script schema expectations.
Reviewed changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/unit/htdocs/modules/system/SystemMenusRegressionTest.php | Updates regression assertion to check the new shared seed source. |
| tests/unit/htdocs/modules/system/SystemMenuInstallationTest.php | New unit tests validating installer/update wiring and schema/seed expectations. |
| tests/phpunit.xml.dist | Adds htdocs/install to the PHPUnit source include set. |
| htdocs/modules/system/xoops_version.php | Registers onInstall hook to reuse update logic for reinstall flows. |
| htdocs/modules/system/include/update.php | Uses shared seed definitions; aligns column signedness; adds schema normalization/FK restoration logic. |
| htdocs/modules/system/include/menu_seed.php | New shared seed definitions + group-key mapping helper. |
| htdocs/install/sql/mysql.structure.sql | Adds installer-time creation of menuscategory / menusitems. |
| htdocs/install/include/makedata.php | Seeds default menus + permissions immediately after inserting the System module row. |
Comment on lines
+107
to
+111
| '/CREATE TABLE menusitems \\(.*?items_id int unsigned NOT NULL auto_increment.*?items_cid int unsigned NOT NULL default \\\'0\\\'/s', | ||
| $source | ||
| ); | ||
| $this->assertStringNotContainsString('FOREIGN KEY', $source); | ||
| } |
Comment on lines
+256
to
+271
| CREATE TABLE menusitems ( | ||
| items_id int unsigned NOT NULL auto_increment, | ||
| items_pid int NOT NULL default '0', | ||
| items_cid int unsigned NOT NULL default '0', | ||
| items_title varchar(100) NOT NULL default '', | ||
| items_prefix text NOT NULL, | ||
| items_suffix text NOT NULL, | ||
| items_url varchar(255) NOT NULL default '', | ||
| items_target tinyint(1) unsigned NOT NULL default '0', | ||
| items_position int NOT NULL default '0', | ||
| items_protected int NOT NULL default '0', | ||
| items_active tinyint(1) unsigned NOT NULL default '1', | ||
| PRIMARY KEY (items_id), | ||
| KEY idx_items_cid (items_cid), | ||
| KEY idx_items_pid (items_pid) | ||
| ) ENGINE=InnoDB DEFAULT CHARSET=utf8mb4; |
Comment on lines
204
to
+209
| $time = time(); | ||
| // RMV-NOTIFY (updated for extra column in table) | ||
| $dbm->insert('modules', " VALUES (1, '" . _MI_SYSTEM_NAME . "', '" . $modversion['version'] . "', " . $time . ", 0, 1, 'system', 0, 1, 0, 0, 0, 0)"); | ||
| if (!system_menu_install_seed_defaults($dbm, $groups, 1)) { | ||
| return false; | ||
| } |
Contributor
Author
|
Addressing the latest review round:
On the test side, the current branch narrows the |
Narrow the installer-schema test so the negative FOREIGN KEY assertion applies only to the menusitems CREATE TABLE block, not the entire mysql.structure.sql file. This keeps the test focused on the intentional menu installer behavior without blocking unrelated future FK additions elsewhere in the install schema.
Contributor
There was a problem hiding this comment.
Pull request overview
Moves System menu table creation + default seeding into the fresh install path so new XOOPS installs immediately have menuscategory / menusitems populated (without requiring a manual System module update), while keeping the existing System module update script as the idempotent upgrade/recovery path.
Changes:
- Adds
menuscategoryandmenusitemsDDL to the installer MySQL structure schema. - Seeds default protected menu categories/items + group permissions during install via
makedata.php, using shared seed definitions. - Refactors seed definitions into
modules/system/include/menu_seed.php, aligns update-script schema signedness, and adds unit/regression coverage for install + update behavior.
Reviewed changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
tests/unit/htdocs/modules/system/SystemMenusRegressionTest.php |
Updates regression test to assert toolbar seed URL safety from the new shared seed source. |
tests/unit/htdocs/modules/system/SystemMenuInstallationTest.php |
Adds unit coverage for seed definitions, installer schema, installer seeding wiring, and update-script schema/FK parity. |
tests/phpunit.xml.dist |
Includes htdocs/install in PHPUnit source scope (coverage/source discovery). |
htdocs/modules/system/xoops_version.php |
Registers onInstall hook to reuse include/update.php logic for module-admin reinstall flows. |
htdocs/modules/system/include/update.php |
Loads shared seed definitions, adds xoops_module_install_system(), aligns column signedness, normalizes upgrade schemas, and ensures FK restoration. |
htdocs/modules/system/include/menu_seed.php |
New shared seed definition + group-key-to-id mapping helpers used by installer + update paths. |
htdocs/install/sql/mysql.structure.sql |
Adds installer-time creation of menu tables (intentionally omitting FK for prefixing limitations). |
htdocs/install/include/makedata.php |
Seeds protected default menus + permissions during fresh install after inserting the System module row. |
There was a problem hiding this comment.
♻️ Duplicate comments (1)
tests/unit/htdocs/modules/system/SystemMenuInstallationTest.php (1)
55-323:⚠️ Potential issue | 🟠 MajorRename test methods to
test*to keep PHPUnit 9.6 discovery workingLines 55-323 currently rely on
#[Test]attributes while method names do not followtest{MethodName}. On PHPUnit 9.6, these methods are not reliably discovered, which breaks the cross-version test requirement.🔧 Proposed rename patch
- public function sharedSeedDefinitionsExposeExpectedProtectedMenus(): void + public function testSharedSeedDefinitionsExposeExpectedProtectedMenus(): void @@ - public function sharedGroupKeyMapperResolvesKnownGroupsOnly(): void + public function testSharedGroupKeyMapperResolvesKnownGroupsOnly(): void @@ - public function installerSchemaDeclaresMenuTablesUsingUnsignedIds(): void + public function testInstallerSchemaDeclaresMenuTablesUsingUnsignedIds(): void @@ - public function updateScriptCreateTablesMatchInstallerSignedness(): void + public function testUpdateScriptCreateTablesMatchInstallerSignedness(): void @@ - public function updateScriptNormalizesExistingMenuTableTypesForUpgrades(): void + public function testUpdateScriptNormalizesExistingMenuTableTypesForUpgrades(): void @@ - public function installerUsesSharedSeedDefinitionsAndSeedsMenusAfterSystemModuleInsert(): void + public function testInstallerUsesSharedSeedDefinitionsAndSeedsMenusAfterSystemModuleInsert(): void @@ - public function systemModuleRegistersInstallAndUpdateHooksToSameScript(): void + public function testSystemModuleRegistersInstallAndUpdateHooksToSameScript(): void @@ - public function updateScriptExposesMenuLifecycleFunctions(): void + public function testUpdateScriptExposesMenuLifecycleFunctions(): void @@ - public function installerSeedingStopsWhenCategoryInsertFails(): void + public function testInstallerSeedingStopsWhenCategoryInsertFails(): void @@ - public function installerSeedsExpectedRowsAndRespectsCategoryBeforeItemOrdering(): void + public function testInstallerSeedsExpectedRowsAndRespectsCategoryBeforeItemOrdering(): void#!/bin/bash set -euo pipefail file="tests/unit/htdocs/modules/system/SystemMenuInstallationTest.php" echo "Methods not matching test* convention:" rg -nP 'public function (?!test)[A-Za-z_][A-Za-z0-9_]*\s*\(' "$file" echo echo "#[Test] attribute usage:" rg -n '^\s*#\[Test\]' "$file"Expected result after fix: first command returns no matches.
As per coding guidelines: "Tests must work across PHPUnit 9.6, 10.5, and 11.x" and "Name test methodstest{MethodName}ortest{MethodName}{Scenario}".🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/unit/htdocs/modules/system/SystemMenuInstallationTest.php` around lines 55 - 323, The test methods with #[Test] attributes are not named with the PHPUnit 9.6 "test*" convention; rename each public function to prefix it with "test" (for example, sharedSeedDefinitionsExposeExpectedProtectedMenus -> testSharedSeedDefinitionsExposeExpectedProtectedMenus, sharedGroupKeyMapperResolvesKnownGroupsOnly -> testSharedGroupKeyMapperResolvesKnownGroupsOnly, installerSchemaDeclaresMenuTablesUsingUnsignedIds -> testInstallerSchemaDeclaresMenuTablesUsingUnsignedIds, updateScriptCreateTablesMatchInstallerSignedness -> testUpdateScriptCreateTablesMatchInstallerSignedness, updateScriptNormalizesExistingMenuTableTypesForUpgrades -> testUpdateScriptNormalizesExistingMenuTableTypesForUpgrades, installerUsesSharedSeedDefinitionsAndSeedsMenusAfterSystemModuleInsert -> testInstallerUsesSharedSeedDefinitionsAndSeedsMenusAfterSystemModuleInsert, systemModuleRegistersInstallAndUpdateHooksToSameScript -> testSystemModuleRegistersInstallAndUpdateHooksToSameScript, updateScriptExposesMenuLifecycleFunctions -> testUpdateScriptExposesMenuLifecycleFunctions, installerSeedingStopsWhenCategoryInsertFails -> testInstallerSeedingStopsWhenCategoryInsertFails, installerSeedsExpectedRowsAndRespectsCategoryBeforeItemOrdering -> testInstallerSeedsExpectedRowsAndRespectsCategoryBeforeItemOrdering) so PHPUnit 9.6 discovers them; keep the #[Test] attributes if desired but ensure all public test methods follow the test* naming convention.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@tests/unit/htdocs/modules/system/SystemMenuInstallationTest.php`:
- Around line 55-323: The test methods with #[Test] attributes are not named
with the PHPUnit 9.6 "test*" convention; rename each public function to prefix
it with "test" (for example, sharedSeedDefinitionsExposeExpectedProtectedMenus
-> testSharedSeedDefinitionsExposeExpectedProtectedMenus,
sharedGroupKeyMapperResolvesKnownGroupsOnly ->
testSharedGroupKeyMapperResolvesKnownGroupsOnly,
installerSchemaDeclaresMenuTablesUsingUnsignedIds ->
testInstallerSchemaDeclaresMenuTablesUsingUnsignedIds,
updateScriptCreateTablesMatchInstallerSignedness ->
testUpdateScriptCreateTablesMatchInstallerSignedness,
updateScriptNormalizesExistingMenuTableTypesForUpgrades ->
testUpdateScriptNormalizesExistingMenuTableTypesForUpgrades,
installerUsesSharedSeedDefinitionsAndSeedsMenusAfterSystemModuleInsert ->
testInstallerUsesSharedSeedDefinitionsAndSeedsMenusAfterSystemModuleInsert,
systemModuleRegistersInstallAndUpdateHooksToSameScript ->
testSystemModuleRegistersInstallAndUpdateHooksToSameScript,
updateScriptExposesMenuLifecycleFunctions ->
testUpdateScriptExposesMenuLifecycleFunctions,
installerSeedingStopsWhenCategoryInsertFails ->
testInstallerSeedingStopsWhenCategoryInsertFails,
installerSeedsExpectedRowsAndRespectsCategoryBeforeItemOrdering ->
testInstallerSeedsExpectedRowsAndRespectsCategoryBeforeItemOrdering) so PHPUnit
9.6 discovers them; keep the #[Test] attributes if desired but ensure all public
test methods follow the test* naming convention.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: d432685d-fbad-430f-abd4-edc991715495
📒 Files selected for processing (1)
tests/unit/htdocs/modules/system/SystemMenuInstallationTest.php
This was referenced Apr 14, 2026
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.
Fresh XOOPS installs did not create or populate
menuscategoryandmenusitemsbecause menu setup only ran frommodules/system/include/update.php. As a result, menus were missing until the System module was manually updated.Move menu setup into the real install path by:
htdocs/install/sql/mysql.structure.sqlhtdocs/install/include/makedata.phpmodules/system/include/menu_seed.phpKeep the existing System module update flow as the idempotent upgrade path.
Align
system_menu_create_tables()column types with install SQL (INT UNSIGNED for auto-increment IDs anditems_cid, TINYINT(1) UNSIGNED for boolean flags) to avoid signedness drift between fresh installs and upgraded sites.Add
onInstallfor the System module so reinstall flows reuse the same update logic. This hook does not fire on fresh install because the installer inserts the System module row directly viamakedata.php, but it protects the module-admin reinstall path.Fresh installs intentionally omit the
menusitems.items_cid -> menuscategory.category_idforeign key from installer SQL becauseSqlUtility::prefixQuery()does not rewriteREFERENCES ...targets insideCREATE TABLEbodies. Install-time integrity is maintained by seeding categories before items, and the runtime System module update path adds/restores the foreign key with properly prefixed table names. Install-time FK parity is tracked separately in #9.Add test coverage in
tests/unit/htdocs/modules/system/SystemMenuInstallationTest.phpcovering shared seed definitions and group-key mapping, installer schema for both menu tables, update-script schema parity and runtime FK restoration, installer wiring, and behavioral verification of the$modversionhooks and lifecycle functions inupdate.php. UpdateSystemMenusRegressionTest.phpso its toolbar fragment-URL assertion reads from the new shared seed source.Summary by CodeRabbit
New Features
Database
Tests