From 90fc94e4b92ee39bc7c59c7075fb11553fdbeb3e Mon Sep 17 00:00:00 2001 From: Copilot Date: Tue, 29 Sep 2026 17:50:53 -0400 Subject: [PATCH 01/15] Manage schema via api_plugin_db_table_create() + db_update_table() Replace the raw CREATE TABLE install and the version-gated ALTER TABLE upgrade migrations for graph_exports/graph_exports_tasks with the plugin schema API: a single *_table_data() definition per table, api_plugin_db_table_create() on install, and db_update_table() (else create) on upgrade. The one historical column rename is kept as a guarded pre-step since db_update_table() can not rename. Tests and copilot-instructions updated to the new pattern. --- .github/copilot-instructions.md | 26 +- CHANGELOG.md | 1 + setup.php | 243 +++++++++--------- tests/Integration/GexportInstallHooksTest.php | 10 +- tests/Unit/GexportSetupTableTest.php | 25 +- tests/bootstrap-unit.php | 8 + 6 files changed, 166 insertions(+), 147 deletions(-) diff --git a/.github/copilot-instructions.md b/.github/copilot-instructions.md index 23d9f84..fbb7460 100644 --- a/.github/copilot-instructions.md +++ b/.github/copilot-instructions.md @@ -164,7 +164,7 @@ Document all changes in `CHANGELOG.md`; use descriptive commit messages referenc - Translatable strings are managed with GNU gettext via `locales/build_gettext.sh`. `locales/po/cacti.pot` is the source template; Weblate owns syncing the per-language `.po`/`.mo` files from it. - **Never commit the per-language `.po` or compiled `.mo` files** (`locales/po/*.po`, `locales/LC_MESSAGES/*.mo`) in a plugin PR. Weblate is the sole owner of those catalogs, and regenerating them here produces spurious diffs and merge conflicts. `locales/po/cacti.pot` is the ONLY translation artifact a PR may add or modify. -- When a pull request adds or changes a string wrapped in `__()`/`__n()`/`__esc()`/`__x()`/`__xn()`/`__gettext()`, run `locales/build_gettext.sh` before pushing and stage `locales/po/cacti.pot` only. `build_gettext.sh` also rewrites the `.po`/`.mo` files as a side effect; revert those before committing (`git checkout -- locales/po/*.po locales/LC_MESSAGES`), or run only the `xgettext` step that targets `cacti.pot`. +- When a pull request adds or changes a string wrapped in `__()`/`__n()`/`__esc()`/`__x()`/`__xn()`/`__gettext()`, run `locales/build_gettext.sh` before pushing and stage `locales/po/cacti.pot` only. `build_gettext.sh` also rewrites the `.po`/`.mo` files as a side effect; revert those before committing (`git checkout -- locales/po/*.po locales/LC_MESSAGES`), or run only the `xgettext` step that targets `cacti.pot`. ## References @@ -189,10 +189,26 @@ existing code or adding new code, not just in dedicated cleanup passes: - **i18n text domain.** Every `__()`/`__esc()` call must include this plugin's text domain as the final argument, except when deliberately comparing against a literal, untranslated Cacti-core label. -- **Plugin table-creation API.** Use `api_plugin_db_table_create()`/`api_plugin_db_add_column()` - (from Cacti core's `lib/plugins.php`) instead of raw `CREATE TABLE`/`ALTER TABLE ... ADD COLUMN`. - Both are idempotent (safe no-ops when already applied), so the same call can run unconditionally - from both the install AND upgrade paths. +- **Plugin schema management.** Own every plugin-created table through Cacti core's schema API in + `lib/plugins.php`; never use raw `CREATE TABLE`/`ALTER TABLE` for a plugin-owned table. + - Define each table once in a `*_table_data()` helper that returns the Cacti table-definition + array (`columns`/`primary`/`keys`/`type`/`comment`). Both the install and upgrade paths consume + that single definition so they can never drift. + - **Install:** create every table with + `api_plugin_db_table_create('', '',
_table_data())`. + - **Upgrade:** refresh each table from the same definition — `db_update_table('
',
_table_data())` + when the table already exists (it diffs the live schema and issues the exact combined `ALTER`), + otherwise `api_plugin_db_table_create()` to create it. Do **not** hand-write + `db_column_exists()`/`db_index_exists()` guards around `ALTER TABLE`. The one exception is a true + column **rename**, which `db_update_table()` cannot express: keep a guarded + `ALTER TABLE ... CHANGE COLUMN` as a pre-step immediately before the refresh. + - Avoid `api_plugin_db_add_column()`, `api_plugin_db_add_index()`, and `api_plugin_db_drop_*()` for + this plugin's own tables — the create + `db_update_table()` pair already covers new columns, + indexes, and type changes. Those helpers are only appropriate when modifying a **non-plugin** + Cacti core table (for example adding a column to `host`). +- **Plugin upgrade bookkeeping.** When the stored version differs from the INFO version, update the + whole `plugin_config` record, not just `version`: + `db_execute_prepared('UPDATE plugin_config SET version = ?, name = ?, author = ?, webpage = ? WHERE directory = ?', [$info['version'], $info['longname'], $info['author'], $info['homepage'], $info['name']])`. - **PHPDoc shape.** Every function gets a PHPDoc block: a one-line description, a blank comment line, `@param` lines, a blank comment line, then `@return`. Infer parameter/return types from actual usage; don't change the function's real type-hints in the same pass (let static analysis diff --git a/CHANGELOG.md b/CHANGELOG.md index e351c68..f84135c 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -2,6 +2,7 @@ --- 2.1 --- +- refactor: Manage the graph_exports/graph_exports_tasks schema through Cacti's plugin table API - create with api_plugin_db_table_create() and refresh existing tables with db_update_table() from a single shared definition, replacing the raw CREATE TABLE/version-gated ALTER TABLE migrations (the one column rename is kept as a pre-step since db_update_table() can not rename) - dev: Enforce patch coverage of changed lines in CI and remove the inert COMPOSER_ROOT_VERSION env from the Pest step - security: Add a version-safe CSP nonce (`plugin_gexport_csp_nonce()`) to every inline `