Refactor codebase for PSR12 compliance and modernization by bmfmancini · Pull Request #202 · Cacti/plugin_monitor

bmfmancini · 2026-03-03T16:51:15Z

This pull request introduces a new helper module db_functions.php for SQL query logic and refines repository-wide development instructions in .github/copilot-instructions.md. The changes focus on improving maintainability, enforcing compatibility and procedural architecture, and clarifying code quality expectations. Minor updates include strict type declarations in entrypoint files.

Repository Guidance and Documentation Updates

Major rewrite of .github/copilot-instructions.md to clarify compatibility requirements, procedural architecture boundaries, file responsibilities, coding patterns, and CI/testing expectations. This update provides more explicit instructions for future contributors and AI agents to follow established repository conventions.

New SQL Helper Module

Added new file db_functions.php containing procedural helper functions for building SQL predicates, host filtering, and query fragments based on current request filters. This centralizes and standardizes common database logic used throughout the plugin.

Entrypoint Hardening

Added declare(strict_types=1); to images/index.php and index.php to enforce strict typing, improving code reliability and future maintainability. [1] [2]

Rename functions to match PSR12 requirements

- Change tabs to spaces - add consts

Breaking up some of the large files

Replaced legacy var with const/let. Switched loose checks to strict (===, !==, strict typeof checks). Added default param: applyFilter(action = ''). Used template literals for URL/query string assembly. Converted many callbacks to arrow functions and shorthand methods. Removed one duplicate object key in saveFilter payload (trim was defined twice with same value). Kept existing flow, DOM selectors, action names, and endpoints unchanged.

Applied the Sonar-requested cleanups in monitor.js:

Refactor rending function and Ajax call functions Lowe cognetive complexity prepare for easier testing

Copilot

Pull request overview

This PR modernizes the Monitor plugin by splitting previously monolithic logic into dedicated helper modules (DB/query helpers, controller/rendering, poller helpers), updating contributor guidance, and applying small entrypoint hardening changes.

Changes:

Added new procedural helper modules: db_functions.php, monitor_controller.php, monitor_render.php, and poller_functions.php.
Refactored poller_monitor.php to delegate most logic into poller_functions.php and introduced shared constants.
Updated .github/copilot-instructions.md and made minor entrypoint formatting/strict typing adjustments.

Reviewed changes

Copilot reviewed 14 out of 14 changed files in this pull request and generated 14 comments.

Show a summary per file

File	Description
`.github/copilot-instructions.md`	Updated repository guidance, responsibilities, and compatibility expectations.
`db_functions.php`	New DB/query helper utilities for monitor filtering and host selection.
`monitor_controller.php`	New controller/actions and request validation for `monitor.php`.
`monitor_render.php`	New rendering/grouping functions for monitor views.
`poller_functions.php`	New poller helper functions for uptime checks, notifications, and email formatting.
`poller_monitor.php`	Refactored poller entrypoint to use helper module; updated CLI parsing and constants.
`setup.php`	Schema/setup adjustments (row format, strict mode toggling, column definitions).
`js/monitor.js`	New JS module for filter/apply/save interactions and tooltip behavior.
`images/index.php`	Added `declare(strict_types=1);` to entrypoint.
`index.php`	Minor formatting change (blank line after `<?php`).
`locales/index.php`	Minor formatting change (blank line after `<?php`).
`locales/LC_MESSAGES/index.php`	Added `declare(strict_types=1);` to entrypoint.
`sounds/index.php`	Minor formatting change (blank line after `<?php`).

monitor_controller.php

db_functions.php

Copilot · 2026-03-03T17:02:00Z

setup.php

+	db_execute('SET SESSION innodb_strict_mode=0');
+
+	api_plugin_db_add_column('monitor', 'host', ['name' => 'monitor', 'type' => 'char(3)', 'NULL' => true, 'default' => 'on']);
+	api_plugin_db_add_column('monitor', 'host', ['name' => 'monitor_text', 'type' => 'text', 'NULL' => false]);
+	api_plugin_db_add_column('monitor', 'host', ['name' => 'monitor_criticality', 'type' => 'tinyint', 'unsigned' => true, 'NULL' => false, 'default' => '0']);
+	api_plugin_db_add_column('monitor', 'host', ['name' => 'monitor_warn', 'type' => 'double', 'NULL' => false, 'default' => '0']);
+	api_plugin_db_add_column('monitor', 'host', ['name' => 'monitor_alert', 'type' => 'double', 'NULL' => false, 'default' => '0']);
+	api_plugin_db_add_column('monitor', 'host', ['name' => 'monitor_icon', 'type' => 'varchar(30)', 'NULL' => false, 'default' => '']);
+
+	db_execute('SET SESSION innodb_strict_mode=1');


monitor_setup_table() disables innodb_strict_mode and then unconditionally re-enables it with SET SESSION innodb_strict_mode=1. This can change the session’s prior setting and, if an error occurs mid-function, may leave the session in the wrong mode. Capture the previous value (SELECT @@SESSION.innodb_strict_mode) and restore it in a finally-style path.

Suggested change

db_execute('SET SESSION innodb_strict_mode=0');

api_plugin_db_add_column('monitor', 'host', ['name' => 'monitor', 'type' => 'char(3)', 'NULL' => true, 'default' => 'on']);

api_plugin_db_add_column('monitor', 'host', ['name' => 'monitor_text', 'type' => 'text', 'NULL' => false]);

api_plugin_db_add_column('monitor', 'host', ['name' => 'monitor_criticality', 'type' => 'tinyint', 'unsigned' => true, 'NULL' => false, 'default' => '0']);

api_plugin_db_add_column('monitor', 'host', ['name' => 'monitor_warn', 'type' => 'double', 'NULL' => false, 'default' => '0']);

api_plugin_db_add_column('monitor', 'host', ['name' => 'monitor_alert', 'type' => 'double', 'NULL' => false, 'default' => '0']);

api_plugin_db_add_column('monitor', 'host', ['name' => 'monitor_icon', 'type' => 'varchar(30)', 'NULL' => false, 'default' => '']);

db_execute('SET SESSION innodb_strict_mode=1');

$prev_innodb_strict_mode = db_fetch_cell('SELECT @@SESSION.innodb_strict_mode');

db_execute('SET SESSION innodb_strict_mode=0');

try {

api_plugin_db_add_column('monitor', 'host', ['name' => 'monitor', 'type' => 'char(3)', 'NULL' => true, 'default' => 'on']);

api_plugin_db_add_column('monitor', 'host', ['name' => 'monitor_text', 'type' => 'text', 'NULL' => false]);

api_plugin_db_add_column('monitor', 'host', ['name' => 'monitor_criticality', 'type' => 'tinyint', 'unsigned' => true, 'NULL' => false, 'default' => '0']);

api_plugin_db_add_column('monitor', 'host', ['name' => 'monitor_warn', 'type' => 'double', 'NULL' => false, 'default' => '0']);

api_plugin_db_add_column('monitor', 'host', ['name' => 'monitor_alert', 'type' => 'double', 'NULL' => false, 'default' => '0']);

api_plugin_db_add_column('monitor', 'host', ['name' => 'monitor_icon', 'type' => 'varchar(30)', 'NULL' => false, 'default' => '']);

} finally {

if ($prev_innodb_strict_mode !== null) {

db_execute('SET SESSION innodb_strict_mode=' . (int) $prev_innodb_strict_mode);

}

}

Copilot · 2026-03-03T17:02:00Z

monitor_render.php

+        foreach ($hosts as $index => $host) {
+            if (is_device_allowed($host['id'])) {
+                $host_ids[] = $host['id'];
+            } else {
+                unset($hosts[$index]);
+            }
+        }
+
+        // Determine the correct width of the cell
+        $maxlen = 10;
+
+        if (get_request_var('view') == 'default') {
+            $maxlen = db_fetch_cell('SELECT MAX(LENGTH(description))
+				FROM host AS h
+				WHERE id IN (' . implode(',', $host_ids) . ')');
+        }


renderTemplate() builds $host_ids[] without initializing $host_ids, and then unconditionally uses it in implode() for the MAX(LENGTH(description)) query. If filtering removes all hosts, this can produce notices and invalid SQL (IN ()). Initialize $host_ids = [] and only run the query when it has values.

Copilot · 2026-03-03T17:02:01Z

poller_functions.php

+    $body     = '<h1>' . __(MONITOR_PING_NOTIFICATION_SUBJECT, 'monitor') . '</h1>' . PHP_EOL;
+    $body_txt = __(MONITOR_PING_NOTIFICATION_SUBJECT, 'monitor') . PHP_EOL;
+
+    $message = __('The following report will identify Devices that have eclipsed their ping latency thresholds.  You are receiving this report since you are subscribed to a Device associated with the Cacti system located at the following URL below.');


buildPingNotificationIntro() sets $message using __() without specifying the 'monitor' text domain. That string won’t be picked up by the plugin’s translations (and will likely fall back to the default domain). Pass the 'monitor' domain so localization remains consistent.

Suggested change

$message = __('The following report will identify Devices that have eclipsed their ping latency thresholds. You are receiving this report since you are subscribed to a Device associated with the Cacti system located at the following URL below.');

$message = __('The following report will identify Devices that have eclipsed their ping latency thresholds. You are receiving this report since you are subscribed to a Device associated with the Cacti system located at the following URL below.', 'monitor');

Copilot · 2026-03-03T17:02:02Z

monitor_render.php

+        foreach ($hosts as $index => $host) {
+            if (is_device_allowed($host['id'])) {
+                $host_ids[] = $host['id'];
+            } else {
+                unset($hosts[$index]);
+            }
+        }
+
+        // Determine the correct width of the cell
+        $maxlen = 10;
+
+        if (get_request_var('view') == 'default') {
+            $maxlen = db_fetch_cell('SELECT MAX(LENGTH(description))
+				FROM host AS h
+				WHERE id IN (' . implode(',', $host_ids) . ')');
+        }


renderSite() builds $host_ids[] without initializing $host_ids, and then unconditionally uses it in implode() for the MAX(LENGTH(description)) query. If no devices are allowed, this can raise notices and generate invalid SQL (IN ()). Initialize $host_ids = [] and guard the query when the list is empty.

Copilot · 2026-03-03T17:02:03Z

js/monitor.js

+	} else {
+		strURL = 'monitor.php?header=false';
+
+		if (action >= '') {


applyFilter() uses if (action >= ''), which is always true for strings and will append &action= even when action is empty. Use an explicit emptiness check (e.g., action !== '') so requests don’t include a blank action parameter.

Suggested change

if (action >= '') {

if (action !== '') {

Copilot · 2026-03-03T17:02:03Z

js/monitor.js

+	myTimer = setTimeout(timeStep, 1000);
+
+	$(globalThis).resize(() => {
+		$(document).tooltip('option', 'position', { my: '1eft:15 top', at: 'right center' });


The tooltip resize handler sets the position using my: '1eft:15 top' (digit “1” instead of “l”). This typo prevents the intended tooltip positioning on resize. Change it to left:15 top to match the initial tooltip config.

Suggested change

$(document).tooltip('option', 'position', { my: '1eft:15 top', at: 'right center' });

$(document).tooltip('option', 'position', { my: 'left:15 top', at: 'right center' });

Copilot · 2026-03-03T17:02:03Z

js/monitor.js

+	} else {
+		$('#timer').html(value);
+		// What is a second, well if you are an
+		// imperial storm tropper, it's just a little more than a second.


Spelling in the timeStep() comment: “storm tropper” should be “stormtrooper”.

Suggested change

// imperial storm tropper, it's just a little more than a second.

// imperial stormtrooper, it's just a little more than a second.

Copilot · 2026-03-03T17:02:03Z

poller_monitor.php

+        switch ($arg) {
+            case '--version':
+            case '-V':
+            case '-v':
+                displayVersion();
+                exit;
+            case '--help':
+            case '-H':
+            case '-h':
+                displayHelp();
+                exit;
+            case '--debug':
+                $debug = true;
+
+                break;
+            default:
+                print 'ERROR: Invalid Parameter ' . $parameter . PHP_EOL . PHP_EOL;
+                displayHelp();
+                exit;
+        }


poller_monitor.php no longer accepts the previously supported --force flag; it now exits with “Invalid Parameter” when passed. If this flag is used by existing automation, this is a breaking CLI change. Consider keeping --force as a no-op (or implement the intended behavior) to preserve backward compatibility.

TheWitness · 2026-03-03T17:04:23Z

You might want to run phpcsfixit on this.

- Fix SQL injection in rfilter RLIKE expression by escaping with db_qstr() - Add missing JOIN ON clause in GROUP_CONCAT query (syntax error) - Initialize $host_ids array before loop to prevent undefined variable errors - Move sort() outside foreach loop for O(n log n) instead of O(n² log n) - Fix JavaScript comparison: change 'action >= ""' to 'action !== ""' - Fix tooltip positioning typo: '1eft' -> 'left' - Fix spelling: 'tropper' -> 'stormtrooper' Addresses review feedback from PR Cacti#202. Signed-off-by: Thomas Vincent <thomasvincent@gmail.com>

bmfmancini added 23 commits March 1, 2026 18:49

PSR12 - First run

3650b12

Rename functions to match PSR12 requirements

PSR12 run 1

b7ebc7b

- Change tabs to spaces - add consts

reduce cognetive complexity scores based on PSR12 and sonar scans

db3aff5

Split poller_monitor with helper file

b27c31d

update docblocks

6e92d3d

code reorginization

8e9d3d5

Breaking up some of the large files

Update monitor.js

e759aa3

Applied the Sonar-requested cleanups in monitor.js:

remove unused variables

662a8be

remove unused variables

e6cc437

Update copilot-instructions.md

5fd26f2

PSR12 formatting

34c91c4

Update db_functions.php

7d26f38

refactor functions

3bfd390

Refactor rending function and Ajax call functions Lowe cognetive complexity prepare for easier testing

Add missing docblocks

f441199

Apply typing

57fca89

remove checks for min php version

dff312f

correct installation errors after PSR12

80a08aa

try iinstall fix one more time

1b57f4b

.

b78621a

Update monitor_controller.php

702c351

Update monitor_controller.php

f764763

Update monitor_render.php

258942e

bmfmancini requested review from TheWitness, Copilot and xmacan March 3, 2026 16:51

Copilot started reviewing on behalf of bmfmancini March 3, 2026 16:51 View session

Copilot AI reviewed Mar 3, 2026

View reviewed changes

TheWitness previously approved these changes Mar 3, 2026

View reviewed changes

fix typing errors

43640c8

bmfmancini dismissed TheWitness’s stale review via 43640c8 March 3, 2026 21:56

bmfmancini added 2 commits March 3, 2026 17:04

apply php cs-fixer updates

365b49b

missed 2 commits

abbbec8

xmacan approved these changes Mar 4, 2026

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactor codebase for PSR12 compliance and modernization#202

Refactor codebase for PSR12 compliance and modernization#202
bmfmancini wants to merge 26 commits intoCacti:developfrom
bmfmancini:PSR12

bmfmancini commented Mar 3, 2026

Uh oh!

Copilot AI left a comment

Uh oh!

Uh oh!

Uh oh!

Copilot AI Mar 3, 2026

Uh oh!

Copilot AI Mar 3, 2026

Uh oh!

Copilot AI Mar 3, 2026

Uh oh!

Copilot AI Mar 3, 2026

Uh oh!

Copilot AI Mar 3, 2026

Uh oh!

Copilot AI Mar 3, 2026

Uh oh!

Copilot AI Mar 3, 2026

Uh oh!

Copilot AI Mar 3, 2026

Uh oh!

TheWitness commented Mar 3, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

	$message = __('The following report will identify Devices that have eclipsed their ping latency thresholds. You are receiving this report since you are subscribed to a Device associated with the Cacti system located at the following URL below.');
	$message = __('The following report will identify Devices that have eclipsed their ping latency thresholds. You are receiving this report since you are subscribed to a Device associated with the Cacti system located at the following URL below.', 'monitor');

	$(document).tooltip('option', 'position', { my: '1eft:15 top', at: 'right center' });
	$(document).tooltip('option', 'position', { my: 'left:15 top', at: 'right center' });

	// imperial storm tropper, it's just a little more than a second.
	// imperial stormtrooper, it's just a little more than a second.

Conversation

bmfmancini commented Mar 3, 2026

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Uh oh!

Uh oh!

Copilot AI Mar 3, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Mar 3, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Mar 3, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Mar 3, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Mar 3, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Mar 3, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Mar 3, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Mar 3, 2026

Choose a reason for hiding this comment

Uh oh!

TheWitness commented Mar 3, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants