Relocate schema into includes/database.php and standardize includes - #37
Merged
Merged
Conversation
- Move plugin_wmi_setup_tables() verbatim into includes/database.php, loaded via require_once from plugin_wmi_install(). - Rename functions.php -> includes/functions.php and update its callers. - Convert include/include_once to require/require_once across production entry points. - Update tests, phpunit source, patch-coverage allowlist, and the .pot to track the relocated files.
TheWitness
requested review from
bmfmancini,
browniebraun,
cigamit and
xmacan
and
a balanced review from Copilot
September 30, 2026 11:00
Contributor
There was a problem hiding this comment.
Copilot review overview
🟡 Changes recommended
A registered hook still loads the removed functions path, and the relocated schema is omitted from two coverage checks.
Review effort: Balanced
Findings: 2
Open (2)
What changed in this PR
This PR reorganizes the WMI plugin’s schema and shared functions to match the fleet-wide includes/ layout.
Changes:
- Move schema provisioning and shared functions into
includes/, updating the affected loaders. - Standardize production includes on
requireandrequire_once. - Update test paths, coverage configuration, and translation source references.
| File | Description |
|---|---|
wmi_tools.php |
Requires dependencies. |
wmi_script.php |
Requires dependencies. |
wmi_queries.php |
Loads relocated functions. |
wmi_accounts.php |
Loads relocated functions. |
tests/Unit/WmiLifecycleTest.php |
Loads relocated schema. |
tests/Security/PreparedStatementConsistencyTest.php |
Updates the functions path. |
tests/bin/patch-coverage.php |
Allowlists the schema file. |
setup.php |
Loads relocated schema during installation. |
script/wmi-script.php |
Requires dependencies. |
poller_wmi.php |
Loads relocated functions. |
phpunit.xml |
Updates the coverage source path. |
locales/po/cacti.pot |
Updates translation source references. |
includes/functions.php |
Relocates shared functions and requires dependencies. |
includes/database.php |
Houses relocated schema provisioning. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
browniebraun
previously approved these changes
Sep 30, 2026
The include->require standardization touched only top-level chdir+require lines in the web pages (auth.php) and CLI/script-server entry points (cli_check.php/global.php), which cannot be loaded into the isolated unit process and therefore cannot be covered. Move those six files out of phpunit.xml <source> and into patch-coverage.php's \, matching the established plugin_thold pattern. setup.php/includes/functions.php/linux_wmi.php remain measured.
Adds the fleet manifest.json (tombstones/expected/whitelist), a plugin_wmi_prune_files() run from plugin_wmi_upgrade() that removes the tombstoned functions.php and the dev-only tests/ tree, refuses any path resolving outside the plugin directory, and warns on files it cannot remove. Also repoints the api_device_new hook's include to the relocated includes/functions.php, adds manifest drift validation to CI, and unit tests for the prune helper and the device_new hook.
…tabase.php in prepared-statement 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.
The Linux_WMI client library moves into includes/; includes/functions.php loads it at the top (so run_store_wmi_query() keeps working), and the console pages, poller, script and tests reference the new path. The old path is tombstoned so existing installs drop the stale top-level copy on upgrade.
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.
The CLI/script-server bootstrap resolved to the non-existent plugins/global/cli_check.php. Point it at the parent Cacti include directory (../../include/cli_check.php) which ships cli_check.php and loads the global bootstrap. lib/snmp.php remains require_once.
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.
Summary
Aligns
plugin_wmiwith the fleet-wide schema/includes convention and adds the fleetmanifest.json+ upgrade-time file-pruning mechanism.plugin_wmi_setup_tables()verbatim intoincludes/database.php, now loaded viarequire_oncefromplugin_wmi_install(). The function is unchanged (corehost.wmi_accountcolumn add, seven table creates, and the twodata_inputseed rows), so behavior is identical.functions.php→includes/functions.phpand updated every caller (setup.php,poller_wmi.php,wmi_queries.php,wmi_accounts.php), including the previously-missedapi_device_newhook (wmi_api_device_new()).include/include_once→require/require_onceacross production entry points.manifest.jsonwith three arrays:tombstones(paths older versions shipped that have since moved/been removed — herefunctions.php),expected(top-level files/directories shipping today, directories with a trailing/), andwhitelist(user-data paths never to touch — empty for wmi).wmi_prune_files()(insetup.php, called fromplugin_wmi_upgrade()) deletes the tombstoned paths and the dev-onlytests/tree, refuses any path that resolves outside the plugin directory (a tamperedmanifest.json), warns on any file/directory it cannot remove, 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 tree.WmiLifecycleTest,PreparedStatementConsistencyTest, and the phpunit<source>at the relocated files; allowlisted the install-onlyincludes/database.phpand the web/CLI entry points in the patch-coverage gate; addedPruneFilesTest(tombstone/file/dir removal, path-escape refusal, permission-warning, whitelist/.gitprotection, missing/malformed no-ops), sandboxedbase_pathin the upgrade test, added awmi_api_device_newunit test, and regeneratedlocales/po/cacti.pot.Scope note
This is the lighter relocation variant: the schema is moved as-is (raw
CREATE TABLE IF NOT EXISTS) rather than transcribed toapi_plugin_db_table_create()+db_update_table(), given wmi's seven tables (including the composite-PKhost_wmi_cache, aMEMORY-engine table, and intermixeddata_inputseeding).Validation
Full Pest suite green (44 passed) and the patch-coverage gate passes at 100% of changed measured lines against
develop; 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.wmi_prune_files()/wmi_rmtree()(theplugin_wmi_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.