Repository navigation
Rename include/ to includes/, use require, and re-register hooks on upgrade - #53
Merged
Merged
Conversation
TheWitness
requested review from
bmfmancini,
browniebraun,
cigamit and
xmacan
and
a balanced review from Copilot
September 30, 2026 03:23
Contributor
There was a problem hiding this comment.
Copilot review overview
🟡 Changes recommended
Existing installations may retain broken hook paths, and the coverage configuration does not reliably measure the relocated code.
Review effort: Balanced
Findings: 1
Open (1)
What changed in this PR
This PR moves Evidence’s plugin files to includes/ and makes PHP loading fail fast, aligning the plugin layout with the stated fleet convention.
Changes:
- Relocates plugin files and updates their references, including hook registrations.
- Changes
includecalls torequireand represents two schema primary keys as arrays. - Updates tests and translation source references for the new paths.
| File | Description |
|---|---|
| tests/Unit/EvidenceLifecycleTest.php | Updates a path in a test comment. |
| tests/Unit/EvidenceDatabaseTest.php | Updates a path in a test comment. |
| tests/Security/PhpCompatibilityTest.php | Updates the vendor-directory exclusion. |
| tests/bin/patch-coverage.php | Allowlists three entry points for coverage. |
| setup.php | Updates hook paths and database loading. |
| poller_evidence.php | Updates required files and loading behavior. |
| locales/po/cacti.pot | Updates translation source references. |
| includes/settings.php | Relocates settings definitions. |
| includes/functions.php | Relocates functions and updates required paths. |
| includes/database.php | Relocates schema code and changes two primary-key definitions. |
| includes/arrays.php | Relocates configuration arrays. |
| evidence.php | Updates required files and loading behavior. |
| evidence_tab.php | Updates required files and loading behavior. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Existing installs persist api_plugin_register_hook() file paths as include/functions.php and include/settings.php, and plugin_evidence_check_config() does not refresh those registrations on upgrade. Renaming the directory to includes/ would leave those hooks pointing at missing files. Keep the directory as include/ and retain only the require/require_once fail-fast conversion, the array-form primaries in include/database.php, and the patch-coverage allowlist. Reverts the cacti.pot regeneration and test-comment path churn that the rename introduced.
TheWitness
changed the title
added 2 commits
September 30, 2026 08:50
The include_once->require_once conversions in plugin_evidence_poller_bottom(), plugin_evidence_host_edit_bottom(), evidence_show_host_info() and evidence_show_actual_data() are the only changes in the measured include/functions.php, and those live hook/render paths are not unit-loadable. A measured file cannot be allowlisted in patch-coverage, so restore the base include_once forms to drop the file from the changed set and clear the 100% patch-coverage gate.
Per author approval, complete the fleet directory convention. - git mv include/ -> includes/ and repoint every plugin reference (entry points, setup.php hook file args, poller, internal library includes). - Re-register the affected hooks on upgrade: plugin_evidence_upgrade_database() now repoints plugin_hooks.file from include/* to includes/* for the plugin, so existing installations load their UI/settings/poller hooks from the new path instead of the removed directory. - Regenerate the translation template for the moved files.
TheWitness
changed the title
added 6 commits
September 30, 2026 13:36
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.
Rename
include/→includes/, re-register hooks on upgrade, and add the fleet file manifestAdopts the fleet
includes/convention (with the upgrade migration that makes the rename safe) and adds the fleet-widemanifest.json+ upgrade-time file-pruning mechanism.Rename + hook re-registration
git mv include/ → includes/(arrays.php,database.php,functions.php,settings.php) and repoint every caller.plugin_evidence_upgrade_database()repointsplugin_hooks.filefrominclude/*toincludes/*for this plugin on a version change, so existing installs load their hooks from the new path after upgrade.File manifest + upgrade pruning
manifest.json(root) withtombstones(include/),expected(top-level files/directories shipping today, directories with a trailing/), andwhitelist(empty — evidence'sdata/holds shipped reference SQL, not user data).evidence_prune_files()(insetup.php, called from the version-change block ofplugin_evidence_upgrade_database()) deletes the tombstonedinclude/tree 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, 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..gitprotection, and the missing/malformed-manifest no-ops; the upgrade test sandboxesbase_pathso the prune runs against a temp tree.Validation
Full Pest suite green (31 passed) and the patch-coverage gate passes for all 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.evidence_prune_files()/evidence_rmtree()(theplugin_evidence_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.