Repository navigation
Guard maint maintenance-window checks against a missing maint functions file - #859
Merged
Merged
Conversation
…ns file #858 corrected the maint functions include path but left three plugin_maint_check_cacti_host() calls guarded only by api_plugin_is_enabled('maint'). When maint is enabled but its functions file cannot be included (the include block now logs and continues rather than fataling), those calls hit an undefined function and fatal the poll. Add a function_exists('plugin_maint_check_cacti_host') guard to all three call sites (thold_check_threshold() and both the recovering- and down-device loops in thold_update_host_status()) so a missing maint include degrades gracefully, matching the guard other plugins use. Cover both maintenance-gated notification branches with a new MaintCheckGuardTest; add an html_split_string() test stub so the recovering-device path can be driven in the isolated unit process. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
TheWitness
force-pushed
the
fix_maint_include_correct
branch
from
October 15, 2026 14:48
3bd450a to
4b2dd6d
Compare
Contributor
There was a problem hiding this comment.
🟡 Changes recommended
Both new tests lack required notification configuration, and the missing-function regression remains untested.
2 open findings
What changed in this PR
Hardens thold’s maintenance integration after #858 so unavailable maint functions do not crash polling.
Changes:
- Guards all three maintenance-check calls with
function_exists(). - Adds device-status tests and a supporting HTML helper stub.
| File | Description |
|---|---|
| tests/Unit/MaintCheckGuardTest.php | Adds recovery and down-device maintenance tests. |
| tests/bootstrap-unit.php | Adds an html_split_string() passthrough stub. |
| includes/polling.php | Guards maintenance checks for device notifications. |
| includes/functions.php | Guards the threshold maintenance check. |
🧠 Review effort: Balanced
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Drop the broad catch(Throwable) in the host-status test helper. A device inside a maintenance window is logged and the loop continues without the downstream notification work, so the call returns on its own; removing the catch means an unexpected error (such as a removed guard fataling on an undefined maint function) now fails the test instead of being swallowed. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
xmacan
approved these changes
Oct 15, 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
Follow-up hardening to #858. That PR corrected the maint
functions.phpinclude path, but threeplugin_maint_check_cacti_host()calls remained guarded only byapi_plugin_is_enabled('maint'). Because the include block now logs and continues when the maint functions file is missing (rather than fataling on the include), those later calls hit an undefined function and fatal the poll.Changes
Add a
function_exists('plugin_maint_check_cacti_host')guard to all three call sites:includes/functions.php—thold_check_threshold()includes/polling.php— the recovering-device loop inthold_update_host_status()includes/polling.php— the down-device loop inthold_update_host_status()This matches the defensive pattern used in the other maint-aware plugins (servcheck, webseer): when maint is enabled but its functions file cannot be included, the maintenance check is simply skipped instead of crashing the poller.
Tests
tests/Unit/MaintCheckGuardTest.phpdrives both maintenance-gated notification branches (recovering device and down device) and asserts the "Only logging, maint device" log messages, covering the two changed polling lines.thold_check_threshold()is already exercised by the existing characterization/maint tests.html_split_string()passthrough stub totests/bootstrap-unit.phpso the recovering-device path can be driven in the isolated unit process. (Changes undertests/are excluded from the patch-coverage gate.)Notes
The anti-pattern to avoid here is
file_exists(require_once(...))— the merged #858 already uses a correctfile_exists($path)probe, which this PR leaves intact; it only adds the missingfunction_exists()guard at the call sites.