Repository navigation
Relocate schema + functions to includes/ and add db_update_table upgrade - #73
Merged
Merged
Conversation
Moves the plugin_maint_schedules/plugin_maint_hosts schema into a new includes/database.php (thold model) and functions.php into includes/functions.php. setup.php delegates via require_once; the is_device_in_maintenance hook now registers includes/functions.php. Implements plugin_maint_check_upgrade() to version-gate, refresh the schema via db_update_table() (string/backtick composite primaries normalized to arrays so db_update_table is safe), and update the full plugin_config row. Converts include/include_once to require/require_once. Tests updated for the moved paths.
Contributor
There was a problem hiding this comment.
Copilot review overview
🟡 Changes recommended
The relocation breaks existing plugin consumers, and the schema definitions are incompatible with the supported Cacti 1.2.0 baseline.
Review effort: Balanced
Findings: 1
Open (3)
What changed in this PR
Relocates plugin helpers into includes/ and adds version-gated schema upgrades.
Changes:
- Centralizes table definitions and upgrade logic.
- Moves runtime maintenance functions and updates references.
- Updates tests, documentation, and changelog.
| File | Description |
|---|---|
.github/copilot-instructions.md |
Documents the new layout and conventions. |
CHANGELOG.md |
Records the refactor. |
includes/database.php |
Adds schema definitions and upgrade logic. |
includes/functions.php |
Relocates maintenance-check helpers. |
maint.php |
Loads the relocated helper library. |
setup.php |
Updates hooks and lifecycle upgrades. |
tests/bootstrap-unit.php |
Adds schema-upgrade stubs. |
tests/Unit/MaintDeviceActionsAndDatabaseTest.php |
Loads the schema library. |
tests/Unit/CheckScheduleTest.php |
Updates the helper path. |
tests/Unit/CheckHostTest.php |
Updates the helper path. |
tests/Security/RedirectSafetyTest.php |
Updates scanned paths. |
tests/Security/PreparedStatementConsistencyTest.php |
Updates scanned paths. |
tests/Security/OutputEscapingTest.php |
Updates scanned paths. |
tests/Security/AuthGuardTest.php |
Updates scanned paths. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
TheWitness
requested review from
bmfmancini,
browniebraun,
cigamit and
xmacan
September 30, 2026 02:54
Member
Author
|
This branch now also sets the plugin's |
…clude
Cacti core's lib/snmpagent.php (snmpagent_poller_bottom) hard-codes
include_once('plugins/maint/functions.php') to load
plugin_maint_check_cacti_host() during the poller run. Relocating the
functions to includes/functions.php broke that fixed path, which logged a
PHP WARNING (failing the 'Verify Cacti log' CI step) and silently disabled
the poller-side maintenance check. Add a thin root functions.php that
forwards to includes/functions.php so the core path keeps working.
The relocation moved functions.php to includes/functions.php but left the coverage <source> pointing at the old root path, so the patch-coverage gate saw includes/functions.php as changed-but-unmeasured and failed. Point <source> at includes/functions.php so its changed lines are measured, and add the thin root functions.php shim to the unmeasured allowlist (it only forwards to includes/functions.php for core's hard-coded snmpagent include).
…uccess, drop orphaned docblock
A shorter changed path can be a suffix of a longer one - the root back-compat functions.php shim vs includes/functions.php - so first-match attributed the includes/functions.php clover entry to the root shim and reported the real file as unmeasured (failing the gate despite 100% coverage). Select the most specific (longest) matching candidate instead.
browniebraun
previously approved these changes
Sep 30, 2026
…des/functions.php plugin_maint_install() registers the hook with the relocated includes/functions.php path, but install never re-runs on upgrade and core's api_plugin_upgrade_register() only touches plugin_config, never plugin_hooks. Existing installs therefore keep the stale functions.php hook file. Re-register the hook in plugin_maint_check_upgrade()'s version-change branch so upgraded sites pick up the new path (currently only surviving via the back-compat root shim). Assert the repoint in MaintCheckUpgradeTest.
cigamit
previously approved these changes
Sep 30, 2026
browniebraun
previously approved these changes
Sep 30, 2026
TheWitness
dismissed stale reviews from browniebraun and cigamit
via
September 30, 2026 14:54
f6245b9
…ions.php Core PR Cacti/cacti#8123 updates lib/snmpagent.php to load the maint library from includes/functions.php (with a legacy root fallback), so the back-compat shim added in 31fd2f3 is no longer needed once that lands.
added 5 commits
September 30, 2026 19:55
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.
Relocate schema + functions to includes/, add db_update_table upgrade, and add the fleet file manifest
Adds the fleet-wide
manifest.json+ upgrade-time file-pruning mechanism (rebased onto the success-gatedmaint_upgrade_tables()+ hook re-registration change).manifest.jsonwithtombstones(empty),expected(top-level files/directories shipping today, directories with a trailing/), andwhitelist(empty).maint_prune_files()(insetup.php, called from the version-change block ofplugin_maint_check_upgrade()only after a successfulmaint_upgrade_tables()) 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 inMaintCheckUpgradeTestand pinnedMaintLifecycleTestto a non-plugin page so the upgrade-path tests run the prune against a throwaway tree; addedcacti_logrecording for the prune log assertions.Validation
Full Pest suite green (60 passed) and patch-coverage passes at 100% (137/137 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.maint_prune_files()/maint_rmtree()(theplugin_maint_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.