Standardize schema management: api_plugin_db_table_create() + db_update_table() - #86
Merged
Merged
Conversation
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.
There was a problem hiding this comment.
Copilot review overview
🟡 Changes recommended
Existing-table upgrades can throw type errors, preserve an incorrect column default, and currently break a lifecycle test.
Review effort: Balanced
Findings: 2
Open (3)
What changed in this PR
Standardizes gexport table installation and upgrades using Cacti’s schema APIs and shared definitions.
Changes:
- Adds canonical definitions for both plugin tables.
- Replaces raw schema migrations with create/update APIs.
- Updates tests, guidance, and changelog documentation.
| File | Description |
|---|---|
setup.php |
Implements shared schema definitions and upgrade refreshes. |
tests/bootstrap-unit.php |
Records schema API calls in test stubs. |
tests/Unit/GexportSetupTableTest.php |
Verifies tracked table creation. |
tests/Integration/GexportInstallHooksTest.php |
Verifies installation provisions both tables. |
.github/copilot-instructions.md |
Documents the standardized schema pattern. |
CHANGELOG.md |
Records the schema-management refactor. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
All table definitions and the create/upgrade/drop helpers now live in includes/database.php (with the Cacti copyright header); setup.php's install/uninstall/upgrade paths include it and delegate. Behavior is unchanged - api_plugin_db_table_create() on install, db_update_table()-else-create refresh on upgrade.
Switches every file inclusion from include/include_once to require/require_once for fail-fast consistency, and relocates the functions.php and gexport_security.php library files into includes/ (updating all references across setup.php, gexport.php, poller_export.php, and the tests). export_graph_monitor_tasks() now resolves poller_export.php via __DIR__/../ since functions.php moved down a directory.
The gettext location comments (--add-location=file) and entry ordering in locales/po/cacti.pot are derived from the sorted list of scanned PHP files. Moving functions.php and gexport_security.php into includes/ reordered those references, so regenerate the template to match. No translatable strings were added or removed (msgid set is unchanged); only source-location comments and ordering differ.
…e gaps The Enforce-coverage gate failed because pcov auto-scopes to the Composer root (cacti/) and skips cacti/plugins/, so no plugin source was instrumented. The coverage <source> set also still referenced the pre-move functions.php / gexport_security.php paths and omitted the new includes/database.php. - Switch the setup-php coverage driver from pcov to xdebug (matches the ci/coverage-xdebug rollout) so this plugin's own sources are measured. - Point the coverage <source> set at includes/functions.php and includes/gexport_security.php and add includes/database.php. - Update the stale GexportLifecycleTest assertion: the upgrade path now writes plugin_config via db_execute_prepared (not db_execute), and add a test that exercises gexport_upgrade_tables()'s legacy-column rename pre-step and the db_update_table() refresh branch (the previously-uncovered changed lines). Route the db_column_exists() test stub through the fixture layer so the rename branch can be driven. - Allowlist the entry points gexport.php (web UI) and poller_export.php (CLI), which cannot load in the isolated unit process, in tests/bin/patch-coverage.php.
…-db-update-table # Conflicts: # CHANGELOG.md
TheWitness
requested review from
bmfmancini,
browniebraun,
cigamit and
xmacan
September 30, 2026 02:54
Member
Author
|
This branch now also sets the plugin's |
…pdate_table on 1.2.x does not reconcile column defaults)
browniebraun
previously approved these changes
Sep 30, 2026
added 5 commits
September 30, 2026 19:48
phpunit.xml ships in the repo for CI but is a dev-only test artifact, so it is removed from manifest.json 'expected', pruned from installs on upgrade (like tests/), and excluded from the manifest drift check. The retired markdown-lint configs (.mdlrc, .md_style.rb) are deleted, and .md* is now ignored like .git*: protected from pruning and excluded from drift if it reappears.
The upgrade-time prune now rejects any tombstone containing '.'/'..' segments (which could escape the plugin directory or resolve to its root) and treats ancestors of whitelist entries as protected, so a tombstone on a parent directory can no longer delete a whitelisted file beneath it.
The trailing '# ...' comments in the Project Structure block drifted further right down the tree; align them all to a single column.
- Use the full license header (from the plugin's own setup.php) in tests/Unit/PruneFilesTest.php instead of the abbreviated copyright banner. - Document that the manifest drift check and the upgrade-time prune also handle phpunit.xml and .md* files, matching the implemented behavior.
The repository naming contract reserves the plugin_<name>_ prefix for plugin lifecycle / hook-registration functions; all other functions use the plain <name>_ prefix. Rename the internal upgrade helpers accordingly: plugin_<name>_prune_files() -> <name>_prune_files() plugin_<name>_rmtree() -> <name>_rmtree() The call site, unit tests, and the copilot-instructions.md references are updated to match. No behavioral change.
cigamit
approved these changes
Oct 1, 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.
Standardize schema management (api_plugin_db_table_create + db_update_table) and add the fleet file manifest
Adds the fleet-wide
manifest.json+ upgrade-time file-pruning mechanism.manifest.jsonwithtombstones(empty),expected(top-level files/directories shipping today, directories with a trailing/), andwhitelist(empty).gexport_prune_files()(insetup.php, called from the version-change block ofgexport_check_upgrade()) removes the dev-onlytests/tree on upgrade, refuses any path that resolves outside the plugin directory (a tamperedmanifest.json), warns on any file/directory it cannot remove, leaveswhitelist/.git*alone, and logs — without removing — any top-level entry the manifest does not account for.tests/bin/validate-manifest.php(wired intoplugin-ci-workflow.yml) fails on drift betweenexpectedand the real top-level tree.PruneFilesTest; sandboxedbase_pathfile-wide inGexportLifecycleTest(pre-loading the realincludes/database.php) so its upgrade-path tests run the prune against a throwaway tree.Validation
Full Pest suite green (63 passed) and patch-coverage passes at 100% (166/166 changed measured lines); manifest drift-check passes and the translation template is up to date.
Revision: hardening & fleet cleanup
Since the initial description, this PR also:
phpunit.xmlon upgrade (alongside the dev-onlytests/tree) and leaves.md*lint configs in place — the drift check now ignorestests/,phpunit.xml,.git*,.md*, and whitelisted paths../..traversal segment or resolves outside the plugin directory (including via a symlinked directory), and protects a directory when a whitelisted entry lives beneath it — each with added unit coverage.gexport_prune_files()/gexport_rmtree()(theplugin_gexport_prefix is reserved for lifecycle/hook-registration functions).tests/Unit/PruneFilesTest.phpthe full standard GPL v2 header and aligns the Project Structure block in.github/copilot-instructions.md.