fix(install): add menusitems FK during fresh install for schema parity (#9) (#27)

mambax7 · web-flow · commit 222f4695b1e3 · 2026-04-20T03:17:07.000-04:00
* fix(install): add menusitems FK during fresh install for schema parity (#9) Fresh installs previously omitted the menusitems -> menuscategory FK because SqlUtility::prefixQuery() does not rewrite REFERENCES targets inside CREATE TABLE bodies. PR #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. * Remove the unused function parameter "$sql".
diff --git a/htdocs/install/include/makedata.php b/htdocs/install/include/makedata.php
@@ -143,6 +143,56 @@ function system_menu_install_seed_defaults($dbm, array $groups, int $moduleId):
     return true;
 }
 
+/**
+ * Add the menusitems -> menuscategory foreign key during fresh install.
+ *
+ * The install SQL in sql/mysql.structure.sql cannot carry this FK inline
+ * because SqlUtility::prefixQuery() only rewrites the leading table name
+ * and does not rewrite REFERENCES targets inside CREATE TABLE bodies.
+ * Running a prefixed ALTER TABLE ADD CONSTRAINT after the menu tables are
+ * created and seeded keeps fresh-install schema in parity with the runtime
+ * system-module upgrade path in modules/system/include/update.php
+ * (see system_menu_create_tables()).
+ *
+ * Idempotent: returns true early if the FK already exists. Non-fatal on
+ * failure — the caller continues and the next system module upgrade
+ * retries the add via the same constraint name.
+ *
+ * @param Db_manager $dbm
+ *
+ * @return bool true on success or already-present, false if the ALTER failed
+ */
+function system_menu_install_add_category_fk($dbm): bool
+{
+    $db        = $dbm->db;
+    $itemTable = $db->prefix('menusitems');
+    $catTable  = $db->prefix('menuscategory');
+    $fkName    = $db->prefix('fk_items_category');
+
+    $result = $db->query(
+        'SELECT 1 FROM INFORMATION_SCHEMA.TABLE_CONSTRAINTS'
+        . ' WHERE CONSTRAINT_NAME = ' . $db->quote($fkName)
+        . ' AND TABLE_NAME = ' . $db->quote($itemTable)
+        . ' AND TABLE_SCHEMA = DATABASE()'
+    );
+    if ($db->isResultSet($result) && ($result instanceof \mysqli_result) && $db->getRowsNum($result) > 0) {
+        return true;
+    }
+
+    $sql = 'ALTER TABLE `' . $itemTable . '` ADD CONSTRAINT `' . $fkName . '`'
+         . ' FOREIGN KEY (`items_cid`) REFERENCES `' . $catTable . '` (`category_id`)'
+         . ' ON DELETE CASCADE';
+    if (false === $db->exec($sql)) {
+        trigger_error(
+            'Failed to add menusitems foreign key during install; next system module upgrade will retry.',
+            E_USER_WARNING
+        );
+        return false;
+    }
+
+    return true;
+}
+
 /**
  * @param $dbm
  * @param $adminname
@@ -200,6 +250,9 @@ function make_data($dbm, $adminname, $hashedAdminPass, $adminmail, $language, $g
     if (!system_menu_install_seed_defaults($dbm, $groups, 1)) {
         return false;
     }
+    // Best-effort: add the menusitems FK after seeding completes. Non-fatal on
+    // failure — install continues and the next system module upgrade retries.
+    system_menu_install_add_category_fk($dbm);
 
     foreach ($modversion['templates'] as $tplfile) {
         $templateType = isset($tplfile['type']) && 'admin' === $tplfile['type'] ? 'admin' : 'module';
diff --git a/tests/unit/htdocs/modules/system/SystemMenuInstallationTest.php b/tests/unit/htdocs/modules/system/SystemMenuInstallationTest.php
@@ -173,6 +173,194 @@ public function installerUsesSharedSeedDefinitionsAndSeedsMenusAfterSystemModule
         );
     }
 
+    #[Test]
+    public function installerInvokesForeignKeyHelperAfterSuccessfulMenuSeeding(): void
+    {
+        $source = $this->readSourceFile('install/include/makedata.php');
+
+        $this->assertMatchesRegularExpression(
+            '/if\\s*\\(\\s*!system_menu_install_seed_defaults\\(\\$dbm, \\$groups, 1\\)\\s*\\).*?'
+            . 'system_menu_install_add_category_fk\\(\\$dbm\\);/s',
+            $source,
+            'make_data() must call system_menu_install_add_category_fk() after the seed-success guard'
+        );
+    }
+
+    #[Test]
+    public function installerForeignKeyHelperBuildsPrefixedAlterTableStatement(): void
+    {
+        $source = $this->readSourceFile('install/include/makedata.php');
+
+        $this->assertStringContainsString(
+            'function system_menu_install_add_category_fk($dbm): bool',
+            $source,
+            'Helper must be defined in install/include/makedata.php'
+        );
+        $this->assertStringContainsString(
+            "\$db->prefix('menusitems')",
+            $source
+        );
+        $this->assertStringContainsString(
+            "\$db->prefix('menuscategory')",
+            $source
+        );
+        $this->assertStringContainsString(
+            "\$db->prefix('fk_items_category')",
+            $source
+        );
+        $this->assertMatchesRegularExpression(
+            '/ALTER TABLE `[^`]*` ADD CONSTRAINT `[^`]*`/',
+            $source
+        );
+        $this->assertStringContainsString(
+            'FOREIGN KEY (`items_cid`) REFERENCES',
+            $source
+        );
+        $this->assertStringContainsString(
+            'ON DELETE CASCADE',
+            $source
+        );
+    }
+
+    #[Test]
+    public function installerForeignKeyHelperIsIdempotentAndNonFatal(): void
+    {
+        $source = $this->readSourceFile('install/include/makedata.php');
+
+        $this->assertStringContainsString(
+            'INFORMATION_SCHEMA.TABLE_CONSTRAINTS',
+            $source,
+            'Helper must check INFORMATION_SCHEMA to stay idempotent'
+        );
+        $this->assertMatchesRegularExpression(
+            '/getRowsNum\\(\\$result\\)\\s*>\\s*0\\s*\\)\\s*\\{\\s*return true;/',
+            $source,
+            'Helper must short-circuit when the FK already exists'
+        );
+        $this->assertMatchesRegularExpression(
+            '/trigger_error\\(\\s*[\'"][^\'"]*foreign key[^\'"]*[\'"]\\s*,\\s*E_USER_WARNING\\s*\\)/i',
+            $source,
+            'Helper must surface exec failure via trigger_error(E_USER_WARNING)'
+        );
+        $this->assertMatchesRegularExpression(
+            '/false === \\$db->exec\\(\\$sql\\)\\)\\s*\\{[^}]*return false;/s',
+            $source,
+            'Helper must return false on exec failure without aborting install'
+        );
+    }
+
+    #[Test]
+    public function foreignKeyHelperEmitsPrefixedAlterTableWhenFkMissing(): void
+    {
+        $dbm = new class {
+            public object $db;
+            public function __construct()
+            {
+                // 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;
+                    }
+                    public function isResultSet(): bool
+                    {
+                        return false;
+                    }
+                    public function getRowsNum(): int
+                    {
+                        return 0;
+                    }
+                    public function exec(string $sql)
+                    {
+                        $this->execCalls[] = $sql;
+                        return 1;
+                    }
+                };
+            }
+        };
+
+        $result = system_menu_install_add_category_fk($dbm);
+
+        $this->assertTrue($result, 'Helper must return true when the ALTER succeeds');
+        $this->assertCount(1, $dbm->db->execCalls, 'Helper must issue exactly one ALTER');
+        $alter = $dbm->db->execCalls[0];
+        $this->assertStringContainsString('ALTER TABLE `xo_menusitems`', $alter);
+        $this->assertStringContainsString('ADD CONSTRAINT `xo_fk_items_category`', $alter);
+        $this->assertStringContainsString('FOREIGN KEY (`items_cid`)', $alter);
+        $this->assertStringContainsString('REFERENCES `xo_menuscategory` (`category_id`)', $alter);
+        $this->assertStringContainsString('ON DELETE CASCADE', $alter);
+    }
+
+    #[Test]
+    public function foreignKeyHelperWarnsAndReturnsFalseWhenAlterFails(): void
+    {
+        $dbm = new class {
+            public object $db;
+            public function __construct()
+            {
+                $this->db = new class {
+                    public int $execCallCount = 0;
+                    public function prefix(string $name): string
+                    {
+                        return 'xo_' . $name;
+                    }
+                    public function quote(string $value): string
+                    {
+                        return "'" . addslashes($value) . "'";
+                    }
+                    public function query(): bool
+                    {
+                        return false;
+                    }
+                    public function isResultSet(): bool
+                    {
+                        return false;
+                    }
+                    public function getRowsNum(): int
+                    {
+                        return 0;
+                    }
+                    public function exec(): bool
+                    {
+                        $this->execCallCount++;
+                        return false;
+                    }
+                };
+            }
+        };
+
+        $warnings = [];
+        set_error_handler(static function (int $errno, string $errstr) use (&$warnings): bool {
+            $warnings[] = [$errno, $errstr];
+            return true;
+        });
+
+        try {
+            $result = system_menu_install_add_category_fk($dbm);
+        } finally {
+            restore_error_handler();
+        }
+
+        $this->assertFalse($result, 'Helper must return false when the ALTER fails');
+        $this->assertSame(1, $dbm->db->execCallCount, 'Helper must attempt the ALTER exactly once');
+        $this->assertCount(1, $warnings, 'Helper must emit exactly one E_USER_WARNING on failure');
+        $this->assertSame(E_USER_WARNING, $warnings[0][0]);
+        $this->assertStringContainsString('foreign key', $warnings[0][1]);
+    }
+
     #[Test]
     public function systemModuleRegistersInstallAndUpdateHooksToSameScript(): void
     {
