Standardize schema management: api_plugin_db_table_create() + db_update_table()

.github/copilot-instructions.md

@@ -25,15 +25,18 @@ When generating code for this repository:
 ## Project Structure
 
 ```
-gexport/               # Repository root (install to plugins/gexport/ in Cacti)
-├── locales/             # Internationalization files
-├── functions.php          # Export engine: run_export(), exporter(), rsync/scp helpers
+gexport/                     # Repository root (install to plugins/gexport/ in Cacti)
+├── includes/                # Library/helper files, require_once'd from the entry points
+│   ├── database.php         # Schema management: *_table_data() + create/upgrade/drop helpers
+│   ├── functions.php        # Export engine: run_export(), exporter(), rsync/scp helpers
+│   └── gexport_security.php # Bulk-action normalization and output-escaping helpers
+├── locales/                 # Internationalization files
 ├── gexport.php              # Main export definition administration UI
-├── poller_export.php         # Background export runner (CLI, spawned from poller_bottom)
-├── website.template            # HTML template used for exported site view
-├── INFO                          # Plugin metadata (name, version, compat)
+├── poller_export.php        # Background export runner (CLI, spawned from poller_bottom)
+├── website.template         # HTML template used for exported site view
+├── INFO                     # Plugin metadata (name, version, compat)
 ├── README.md
-└── setup.php                      # Plugin install/uninstall/upgrade hooks
+└── setup.php                # Plugin install/uninstall/upgrade hooks
 ```
 
 ## Naming Conventions
@@ -131,7 +134,7 @@ api_plugin_register_realm('gexport', 'gexport.php', __('Export Cacti Graphs Sett
 `gexport_poller_bottom()` only fires on `poller_id == 1` and only spawns `poller_export.php` in the background if at least one `graph_exports` row is `enabled='on'`; keep new export-triggering logic behind the same guard to avoid running exports on every poller cycle needlessly.
 
 ### Export Engine Conventions
-Logging inside the export engine uses a family of small helpers — `export_fatal()`, `export_warn()`, `export_note()`, `export_log()`, `export_debug()` — keep using the matching severity helper rather than calling `cacti_log()` directly from within `functions.php`.
+Logging inside the export engine uses a family of small helpers ΓÇö `export_fatal()`, `export_warn()`, `export_note()`, `export_log()`, `export_debug()` ΓÇö keep using the matching severity helper rather than calling `cacti_log()` directly from within `includes/functions.php`.
 
 ## Best Practices
 
@@ -164,7 +167,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,11 +192,38 @@ 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.
+- **File inclusion uses `require`/`require_once`.** Always use `require`/`require_once` (never
+  `include`/`include_once`) so a missing dependency fails fast and loudly. Keep library/helper files
+  (e.g. `functions.php`, `gexport_security.php`, `includes/database.php`) under `includes/` and
+  reference them from that path; entry points (`gexport.php`, `poller_export.php`, `setup.php`) stay
+  in the plugin root.
+- **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. Keep all of
+  these schema functions in the plugin's `includes/database.php`, included from `setup.php`'s
+  install/upgrade paths (the thold model).
+  - 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('<plugin>', '<table>', <table>_table_data())`.
+  - **Upgrade:** refresh each table from the same definition — `db_update_table('<table>', <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
   flag mismatches separately). Skip vendored third-party library files.
+
+## File manifest & upgrade pruning
+
+The plugin ships a root `manifest.json` with three arrays: `tombstones` (files/directories older versions shipped that have since moved or been removed), `expected` (the top-level files and directories that ship today, directories written with a trailing `/`), and `whitelist` (paths holding user data that must never be touched). Keep `expected` current: CI runs `tests/bin/validate-manifest.php`, which fails on any drift between `expected` and the real top-level tree (it ignores `tests/`, `phpunit.xml`, `.git*`, `.md*`, and whitelisted paths). Custom customer CSS/theme files belong in `expected`, and stylesheets live in `css/` (not `themes/`). On upgrade, `gexport_prune_files()` deletes the tombstoned paths, the dev-only `tests/` tree, and the `phpunit.xml` test config, leaves `whitelist`, `.git*`, and `.md*` alone, and logs (without removing) any top-level entry the manifest does not account for. As a safety measure it refuses any tombstone that resolves outside the plugin directory (a tampered manifest.json) and logs a warning for any file or directory it cannot remove. When you move or delete a shipped file, add its old path to `tombstones` and update `expected` in the same change.

.github/workflows/plugin-ci-workflow.yml

@@ -89,6 +89,9 @@ jobs:
     - name: Check PHP version
       run: php -v
 
+    - name: Validate plugin manifest (expected-file drift)
+      run: php cacti/plugins/gexport/tests/bin/validate-manifest.php
+
     - name: Run apt-get update
       run: sudo apt-get update
 

CHANGELOG.md

@@ -2,6 +2,8 @@
 
 --- 2.1 ---
 
+- refactor: Move the functions.php and gexport_security.php library files into includes/ and switch every file inclusion from include/include_once to require/require_once for fail-fast consistency (references updated across setup.php, gexport.php, poller_export.php, and the test suite)
+- refactor: Move all schema management into includes/database.php (the thold model) and 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: Measure CI coverage with xdebug instead of pcov so the plugin's own sources are instrumented (pcov auto-scopes to the Composer root and skipped cacti/plugins/, leaving the patch-coverage gate with nothing to measure)
 - 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 `<script>` tag so pages stay compatible with Cacti's Content-Security-Policy nonce enforcement, while falling back cleanly on older Cacti releases that lack the `CactiSecureHeaders` class

INFO

@@ -26,5 +26,5 @@ longname = Graph Export Package for Cacti
 author = The Cacti Group
 email = 
 homepage = http://www.cacti.net
-compat = 1.2.15
+compat = 1.2.29
 capabilities = online_view:1, online_mgmt:1, offline_view:0, offline_mgmt:0, remote_collect:0

gexport.php

@@ -23,9 +23,9 @@
 */
 
 chdir('../../');
-include('./include/auth.php');
-include_once('./plugins/gexport/functions.php');
-include_once('./plugins/gexport/gexport_security.php');
+require('./include/auth.php');
+require_once('./plugins/gexport/includes/functions.php');
+require_once('./plugins/gexport/includes/gexport_security.php');
 
 $export_actions = [
 	'1' => __('Delete', 'gexport'),
@@ -415,7 +415,7 @@ function export_disable($export_id) {
 function export_runnow($export_id) {
 	global $config;
 
-	include_once('./lib/poller.php');
+	require_once('./lib/poller.php');
 
 	$status = db_fetch_row_prepared('SELECT status, enabled FROM graph_exports WHERE id = ?', [$export_id]);
 	$status = is_array($status) ? $status : [];