refactor: extract duplicated alert command execution paths#281
refactor: extract duplicated alert command execution paths#281somethingwithproof wants to merge 3 commits intoCacti:developfrom
Conversation
There was a problem hiding this comment.
Pull request overview
This PR addresses issue #278 by extracting two duplicated command execution code blocks within syslog_process_alert() into shared helper functions in functions.php, reducing maintenance risk and preventing future divergence between the two branches. A regression test validates the refactoring structure.
Changes:
- Introduces
syslog_execute_ticket_command()andsyslog_execute_alert_command()as shared helpers infunctions.php, replacing two near-identical inline blocks in each branch ofsyslog_process_alert(). - Adds a regression test (
issue278_command_execution_refactor_test.php) that uses string-matching onfunctions.phpto verify helper presence and call-site count. - Updates
CHANGELOG.mdwith issue #278 entry.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
functions.php |
Adds two new helper functions above syslog_process_alerts() and replaces the two duplicated inline command blocks at the call sites |
tests/regression/issue278_command_execution_refactor_test.php |
New regression test verifying helpers exist and are called from both branches via text-based string matching |
CHANGELOG.md |
Adds changelog entry for issue #278 |
| exec($command, $output, $returnCode); | ||
| } else { | ||
| exec('/bin/sh ' . $command, $output, $returnCode); |
There was a problem hiding this comment.
In syslog_execute_alert_command, the variables $output and $returnCode are used in the exec() calls (lines 1119 and 1121) without being initialized beforehand. PHP's exec() function appends to $output rather than replacing it, meaning that if this function is ever called more than once in a request, the $output array would accumulate lines from previous runs. Additionally, if exec() fails to set $returnCode for any reason, line 1125 would produce an "undefined variable" notice and log 0 or an empty string as the return code. The variables should be initialized before the exec() call: $output = []; and $returnCode = 0;.
| function syslog_execute_ticket_command($alert, $hostlist, $error_message) { | ||
| $command = read_config_option('syslog_ticket_command'); | ||
|
|
||
| if ($command != '') { | ||
| $command = trim($command); | ||
| } | ||
|
|
||
| if ($alert['open_ticket'] == 'on' && $command != '') { | ||
| if (is_executable($command)) { | ||
| $command = $command . | ||
| ' --alert-name=' . cacti_escapeshellarg(clean_up_name($alert['name'])) . | ||
| ' --severity=' . cacti_escapeshellarg($alert['severity']) . | ||
| ' --hostlist=' . cacti_escapeshellarg(implode(',', $hostlist)) . | ||
| ' --message=' . cacti_escapeshellarg($alert['message']); | ||
|
|
||
| $output = array(); | ||
| $return = 0; | ||
|
|
||
| exec($command, $output, $return); | ||
|
|
||
| if ($return != 0) { | ||
| cacti_log(sprintf($error_message, $alert['name'], $return, implode(', ', $output)), false, 'SYSLOG'); | ||
| } | ||
| } | ||
| } | ||
| } | ||
|
|
||
| function syslog_execute_alert_command($alert, $results, $hostname) { | ||
| $command = alert_replace_variables($alert, $results, $hostname); | ||
|
|
||
| $logMessage = "SYSLOG NOTICE: Executing '$command'"; | ||
|
|
||
| $cparts = explode(' ', $command); | ||
|
|
||
| if (is_executable($cparts[0])) { | ||
| exec($command, $output, $returnCode); | ||
| } else { | ||
| exec('/bin/sh ' . $command, $output, $returnCode); | ||
| } | ||
|
|
||
| // Append the return code to the log message without the dot | ||
| $logMessage .= " Command return code: $returnCode"; | ||
|
|
||
| // Log the combined message | ||
| cacti_log($logMessage, true, 'SYSTEM'); | ||
| } |
There was a problem hiding this comment.
Both new helper functions syslog_execute_ticket_command and syslog_execute_alert_command are missing PHPDoc block comments. All other comparable helper functions in this file (e.g., alert_setup_environment at line 2340, alert_replace_variables at line 2369, syslog_process_alert at line 1242) have docblocks describing their parameters and return values. Adding docblocks to these new functions would maintain consistency with the established codebase documentation convention.
| exit(1); | ||
| } | ||
|
|
||
| if (substr_count($functions, 'syslog_execute_ticket_command($alert, $hostlist') < 2) { |
There was a problem hiding this comment.
The check on line 20 searches for 'syslog_execute_ticket_command($alert, $hostlist' and requires at least 2 matches. However, the function definition itself at line 1084 in functions.php (function syslog_execute_ticket_command($alert, $hostlist, $error_message)) also contains this substring, making the total match count 3 (1 definition + 2 call sites). If one call site were removed, the count would be 2, which still satisfies the >= 2 condition — meaning this check would fail to detect the regression it is intended to guard against. The assertion should require >= 3 instead of >= 2 (i.e., the condition should be < 3), or the search string should be made specific enough to only match call sites (e.g., include more of the call expression that is not part of the function signature).
| if (substr_count($functions, 'syslog_execute_ticket_command($alert, $hostlist') < 2) { | |
| if (substr_count($functions, 'syslog_execute_ticket_command($alert, $hostlist, $error_message);') < 2) { |
There was a problem hiding this comment.
Fixed -- using the full call-site pattern (with semicolon) to avoid matching the function signature.
|
Fixed in 70b2147 -- added PHPDoc blocks to both execute helpers matching the existing convention; narrowed the test assertion from the broad prefix to the full call-site signature (including trailing |
Fixes Cacti#278 Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Signed-off-by: Thomas Vincent <thomasvincent@gmail.com>
…et helpers Signed-off-by: Thomas Vincent <thomasvincent@gmail.com>
Add PHPDoc to syslog_execute_ticket_command() and syslog_execute_alert_command() to match the documentation convention used by surrounding helpers. Narrow the test assertion from a broad substring that matched the function definition to the full call signature with trailing semicolon, so the check correctly detects missing call sites. Signed-off-by: Thomas Vincent <thomasvincent@gmail.com>
somethingwithproof
force-pushed
the
refactor/extract-alert-command-execution
branch
from
70b2147 to
f7966e5
Compare
Summary
Fixes #278
Validation