fix(install): add menusitems FK during fresh install for schema parity (#9)#27
Conversation
XOOPS#9) Fresh installs previously omitted the menusitems -> menuscategory FK because SqlUtility::prefixQuery() does not rewrite REFERENCES targets inside CREATE TABLE bodies. PR XOOPS#8 kept install-time integrity by seeding categories before items and relied on the runtime system module upgrade path to add the FK later. That left a parity gap: freshly installed sites that never ran the system module upgrade had no FK. Add system_menu_install_add_category_fk() in install/include/makedata.php that runs a prefixed ALTER TABLE ADD CONSTRAINT after menu seeding succeeds. The helper is idempotent (INFORMATION_SCHEMA lookup) and non-fatal (trigger_error on failure; the next system module upgrade retries via the same FK name). Implementation stays local to the installer — SqlUtility::prefixQuery() and install SQL are unchanged any regressions.
|
Warning Rate limit exceeded
Your organization is not enrolled in usage-based pricing. Contact your admin to enable usage-based pricing to continue reviews beyond the rate limit, or try again in 17 minutes and 34 seconds. ⌛ 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: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (2)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
mambax7
changed the title
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #27 +/- ##
============================================
- Coverage 19.23% 18.00% -1.24%
- Complexity 7584 7803 +219
============================================
Files 621 665 +44
Lines 40085 42827 +2742
============================================
Hits 7709 7709
- Misses 32376 35118 +2742 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Pull request overview
Closes the fresh-install schema parity gap by adding the missing menusitems.items_cid -> menuscategory.category_id foreign key during installation (without changing installer SQL rewriting behavior).
Changes:
- Add an installer-only helper (
system_menu_install_add_category_fk()) that performs a prefixedALTER TABLE ... ADD CONSTRAINTafter menu seeding, with an INFORMATION_SCHEMA idempotency check and non-fatal failure behavior. - Invoke the helper from
make_data()immediately after successful menu seeding. - Extend unit tests to assert installer wiring, helper behavior (idempotency/non-fatal), and prefixed SQL composition.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
htdocs/install/include/makedata.php |
Adds and calls an install-time FK helper to keep fresh installs aligned with the runtime System module upgrade path. |
tests/unit/htdocs/modules/system/SystemMenuInstallationTest.php |
Adds tests validating the installer call site and the FK helper’s SQL/idempotency/non-fatal behavior. |
|
| // Stubs declare no parameters; PHP accepts extra positional args silently. | ||
| // This keeps the helper's $db->query($sql) etc. call sites working while | ||
| // not declaring (therefore not leaving unused) the args the stub ignores. | ||
| $this->db = new class { | ||
| public array $execCalls = []; | ||
| public function prefix(string $name): string | ||
| { | ||
| return 'xo_' . $name; | ||
| } | ||
| public function quote(string $value): string | ||
| { | ||
| return "'" . addslashes($value) . "'"; | ||
| } | ||
| public function query(): bool | ||
| { | ||
| // Returning false makes the existence-short-circuit fall through | ||
| // to the ALTER branch (the path being tested). | ||
| return false; |
There was a problem hiding this comment.
The anonymous DB stub defines methods like query()/isResultSet()/getRowsNum() with zero parameters, but the helper calls them with arguments (e.g., $db->query($sql), $db->isResultSet($result), $db->getRowsNum($result)). In PHP 8+, passing extra args to user-defined methods throws ArgumentCountError, so this test will fail. Update the stub method signatures to accept the expected parameters (or use variadic ...$args) to match the production DB API.
| public function exec(): bool | ||
| { | ||
| $this->execCallCount++; | ||
| return false; | ||
| } |
There was a problem hiding this comment.
In this DB stub, exec() is declared with no parameters, but system_menu_install_add_category_fk() calls $db->exec($sql). This will raise an ArgumentCountError under PHP 8+. Adjust exec() (and the other stubbed DB methods here) to accept the same parameters as the real DB object, or use a variadic signature.
(#9)
any regressions.