Adopt plugin table API and relocate schema to includes/database.php - #254
Merged
Merged
Conversation
added 3 commits
September 30, 2026 00:21
TheWitness
requested review from
bmfmancini,
browniebraun,
cigamit and
xmacan
and
a balanced review from Copilot
September 30, 2026 04:23
browniebraun
previously approved these changes
Sep 30, 2026
Contributor
There was a problem hiding this comment.
Copilot review overview
🟡 Changes recommended
Fresh-install compatibility, dashboard preservation, and schema coverage issues remain unresolved.
Review effort: Balanced
Findings: 1
Open (2)
What changed in this PR
Centralizes plugin schema management and adopts Cacti’s table APIs.
Changes:
- Adds shared table definitions and lifecycle helpers.
- Updates setup, upgrade, uninstall, and include behavior.
- Extends database test stubs and coverage configuration.
| File | Description |
|---|---|
includes/database.php |
Defines and manages plugin tables. |
setup.php |
Integrates schema lifecycle helpers. |
monitor.php |
Uses mandatory authentication loading. |
monitor_controller.php |
Uses mandatory syslog dependency loading. |
tests/bootstrap-unit.php |
Adds schema API stubs. |
tests/Unit/MonitorSetupTableAndPollerTest.php |
Loads relocated schema helpers. |
tests/bin/patch-coverage.php |
Adds coverage exclusions. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
api_plugin_db_table_create() records plugin_monitor_dashboards in plugin_db_changes, so api_plugin_db_changes_remove() (run by api_plugin_uninstall() after plugin_monitor_uninstall()) would drop it even though monitor_drop_tables() intentionally leaves it in place. Remove the 'create' ownership record after provisioning (install and upgrade paths) so user dashboards survive an uninstall. Adds a regression test.
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.
db_functions.php, monitor_render.php and monitor_controller.php move into includes/ (the latter two lose the monitor_ prefix, becoming render.php and controller.php); monitor.php and the tests load them from there. Each per-theme stylesheet css/<theme>/monitor.css becomes css/<theme>.css so users can drop their own css/<theme>.css into the css base, and plugin_monitor_page_head() looks there, falling back to css/monitor.css. The unreferenced sonar-project.properties is removed. Old paths are tombstoned so existing installs drop the stale copies on upgrade.
Follows the fleet convention for the plugin's shared library filename; monitor.php, the measured-source list and the unit test load it from the new name.
Moving the plugin's library files into includes/ changed the source-reference lines in the translation template; regenerate it so the 'Verify translation template' CI check passes.
Contributor
There was a problem hiding this comment.
Copilot review overview
🟡 Changes recommended
Existing v2.9 installations skip the version-gated migration, and unresolved SQL-safety and permission-view failures remain.
Review effort: Balanced
Findings: 1
Open (3)
Resolved since last review (2)
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.
Point the structure tree at the relocated files (libraries now under includes/, data files under docs/, stylesheets under css/) and align the trailing '# ...' comments to a single column so they no longer drift right.
- 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.
Align the compat metadata (and the contributor-guide references) with the team decision to standardize the minimum supported Cacti version at 1.2.29; the previously proposed 1.2.32 floor was not adopted.
Add a 3.0 CHANGELOG section for this cycle's schema/manifest work and set INFO version = 3.0 so existing 2.9 installs detect the version change and run the schema refresh and manifest-based file prune.
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.
Adopt the plugin table API, relocate schema to includes/database.php, consolidate themes/ → css/, and add the fleet file manifest
Adds the fleet-wide
manifest.json+ upgrade-time file-pruning mechanism and finishes the stylesheet-directory consolidation undercss/.includes/database.php.themes/<theme>/monitor.css→css/<theme>/monitor.css— the per-Cacti-theme override stylesheets now live alongside the basecss/monitor.css;plugin_monitor_page_head()repointed. The oldthemes/directory is removed.manifest.jsonwithtombstones(themes/),expected(top-level files/directories shipping today, directories with a trailing/), andwhitelist(empty).monitor_prune_files()(insetup.php, called from the version-change block ofmonitor_check_upgrade()) removes the tombstonedthemes/and 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, aMonitorPageHeadTestcovering the per-theme/base stylesheet selection, and amonitor_poller_bottomfallback test; sandboxedbase_pathfile-wide inMonitorLifecycleTest(pre-loading the realincludes/database.php) so its upgrade-path tests run the prune against a throwaway tree; added aget_selected_themestub.Validation
Full Pest suite green (74 passed) and patch-coverage passes at 100% (85/85 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.monitor_prune_files()/monitor_rmtree()(theplugin_monitor_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.