refactor: safe PHP 7.4 modernization#770
refactor: safe PHP 7.4 modernization#770somethingwithproof wants to merge 4 commits intoCacti:developfrom
Conversation
There was a problem hiding this comment.
Pull request overview
This PR aims to modernize the thold plugin by enabling strict_types and refactoring various PHP constructs across the codebase, as well as adjusting ancillary files.
Changes:
- Added
declare(strict_types=1);to many PHP entrypoints and helper files. - Refactored several array/XML helper call sites and action hook functions (currently introduces invalid PHP syntax in multiple places).
- Added
.omc/sessions/*.jsonsession metadata files to the repo.
Reviewed changes
Copilot reviewed 57 out of 57 changed files in this pull request and generated 19 comments.
Show a summary per file
| File | Description |
|---|---|
| thold.php | Adds strict_types. |
| thold_webapi.php | Adds strict_types; introduces invalid function declaration syntax in an action hook. |
| thold_templates.php | Adds strict_types; introduces invalid array-check syntax. |
| thold_process.php | Adds strict_types. |
| thold_notify.php | Adds strict_types. |
| thold_graph.php | Adds strict_types. |
| thold_functions.php | Adds strict_types; multiple invalid replacements for in_array/is_array and XML parsing. |
| setup.php | Adds strict_types; multiple invalid function declarations and invalid is_array replacements. |
| includes/polling.php | Adds strict_types; introduces invalid array-check syntax. |
| includes/tab.php | Adds strict_types. |
| includes/settings.php | Adds strict_types. |
| includes/index.php | Adds strict_types. |
| includes/database.php | Adds strict_types. |
| includes/arrays.php | Adds strict_types. |
| index.php | Adds strict_types. |
| poller_thold.php | Adds strict_types. |
| notify_queue.php | Adds strict_types. |
| notify_lists.php | Adds strict_types. |
| cli_thresholds.php | Adds strict_types. |
| cli_import.php | Adds strict_types; introduces invalid XML parsing / array-check syntax. |
| extras/upgrade.php | Adds strict_types. |
| extras/index.php | Adds strict_types. |
| extras/apply_realms.php | Adds strict_types. |
| images/index.php | Adds strict_types. |
| locales/index.php | Adds strict_types. |
| locales/po/index.php | Adds strict_types. |
| locales/LC_MESSAGES/index.php | Adds strict_types. |
| themes/index.php | Adds strict_types. |
| themes/classic/index.php | Adds strict_types. |
| themes/dark/index.php | Adds strict_types. |
| themes/modern/index.php | Adds strict_types. |
| themes/paper-plane/index.php | Adds strict_types. |
| themes/paw/index.php | Adds strict_types. |
| themes/sunrise/index.php | Adds strict_types. |
| service/index.php | Adds strict_types. |
| service/systemd/index.php | Adds strict_types; also highlights infra-removal mismatch with PR description. |
| .omc/sessions/f945f45a-c0d4-4a7e-b89f-0b41c163869a.json | Adds local session metadata (should not be versioned). |
| .omc/sessions/f7263a26-c009-490e-992e-53a418b29eec.json | Adds local session metadata (should not be versioned). |
| .omc/sessions/e783cff2-839b-4686-bbc3-c8758d9c7b6c.json | Adds local session metadata (should not be versioned). |
| .omc/sessions/d6e019bb-7a92-4df7-a0f5-34964c37bd3d.json | Adds local session metadata (should not be versioned). |
| .omc/sessions/d541a73b-f1aa-4e3c-a6c6-6f00f7f2a58a.json | Adds local session metadata (should not be versioned). |
| .omc/sessions/cf3ccfa8-c9cf-42df-876d-cbe0cd78ad2f.json | Adds local session metadata (should not be versioned). |
| .omc/sessions/cbf69f79-e236-4d9c-9289-4333f5f3c545.json | Adds local session metadata (should not be versioned). |
| .omc/sessions/bf809b2e-0eed-4a7e-b895-01510e5d6b9d.json | Adds local session metadata (should not be versioned). |
| .omc/sessions/a5b592a5-38aa-4b11-bf0a-5d0d087615b5.json | Adds local session metadata (should not be versioned). |
| .omc/sessions/a408d1a5-f516-4379-9a1e-35398aa29599.json | Adds local session metadata (should not be versioned). |
| .omc/sessions/96a4b59a-d5a8-43cd-af25-0597cbf12107.json | Adds local session metadata (should not be versioned). |
| .omc/sessions/9641f786-5142-4ae1-9e18-b11f5e6fb816.json | Adds local session metadata (should not be versioned). |
| .omc/sessions/827ec296-29cc-4cd8-8331-f73fdd756f37.json | Adds local session metadata (should not be versioned). |
| .omc/sessions/7f1e4ef4-707d-49b3-b57d-16f7220a8d12.json | Adds local session metadata (should not be versioned). |
| .omc/sessions/71d55ede-992a-425f-8f99-53fbce80a343.json | Adds local session metadata (should not be versioned). |
| .omc/sessions/692e33b9-e73c-4d0f-8f73-2aa864795a7b.json | Adds local session metadata (should not be versioned). |
| .omc/sessions/4ce27694-4715-413d-b907-b9f5a1265d19.json | Adds local session metadata (should not be versioned). |
| .omc/sessions/3bb2f627-a069-454e-92ae-36433f072136.json | Adds local session metadata (should not be versioned). |
| .omc/sessions/329f355f-e88f-4898-8ed5-0982ae2e7e4d.json | Adds local session metadata (should not be versioned). |
| .omc/sessions/1d8db818-d03e-49db-b914-885d353ec217.json | Adds local session metadata (should not be versioned). |
| .omc/sessions/13078be2-490d-4463-b1cb-d4f18ed4f17b.json | Adds local session metadata (should not be versioned). |
Comments suppressed due to low confidence (5)
thold_webapi.php:184
- This function declaration is invalid PHP syntax (square brackets in the parameter list). It will cause a parse error and break the plugin; restore a normal parameter list (e.g., keep the original thold_add_graphs_action_array($action) signature).
setup.php:739 - This function declaration is invalid PHP syntax (
function ..._[$device_action_array]). It should be a normal function name and parameter list; otherwise setup.php will not parse.
setup.php:1026 - This function declaration is invalid PHP syntax (
function ..._[$action]). It will cause a parse error; keep the originalthold_data_source_action_array($action)style signature.
setup.php:1190 - This function declaration is invalid PHP syntax (
function ..._[$action]) and will prevent the plugin from loading. Restore a normal parameter list (e.g.,thold_graphs_action_array($action)).
setup.php:1688 - This function declaration is invalid PHP syntax (
function ..._[$regex_array]). It will cause a parse error; restore a normal function signature (e.g.,thold_clog_regex_array($regex_array)).
| $defs = explode("\\\n", $g['graph_defs'], -1); | ||
|
|
||
| if (is_array($defs)) { | ||
| if (is_[$defs]) { | ||
| foreach ($defs as $def) { |
There was a problem hiding this comment.
is_[$defs] is not a valid PHP call and will trigger a parse error. This should remain a normal array check (e.g., is_array($defs)) before iterating.
| $i = 0; | ||
| $unsets = []; | ||
|
|
||
| if (is_array($replacements)) { | ||
| if (is_[$replacements]) { | ||
| foreach ($txt_items as $item) { |
There was a problem hiding this comment.
is_[$replacements] is invalid PHP syntax and will cause a parse error. Use a standard array check (e.g., is_array($replacements)) or remove the condition if it’s redundant.
thold_templates.php
Outdated
| if ((isset($_SESSION['import_debug_info'])) && (is_[$_SESSION['import_debug_info']])) { | ||
| html_start_box(__('Import Results', 'thold'), '80%', false, '3', 'center', ''); | ||
|
|
There was a problem hiding this comment.
is_[$_SESSION['import_debug_info']] is invalid PHP syntax and will cause a parse error. This should remain a standard is_array(...) check before iterating.
| foreach ($hosts as $host) { | ||
| // hosts in recovery status record only if they was in failed status | ||
| if (($host['status'] != HOST_RECOVERING) || ($host['status'] == HOST_RECOVERING && (is_array($failed) && array_search($host['id'], array_column($failed, 'host_id'), true) !== false))) { | ||
| if (($host['status'] != HOST_RECOVERING) || ($host['status'] == HOST_RECOVERING && (is_[$failed] && array_search($host['id'], array_column($failed, 'host_id'), true) !== false))) { | ||
| $failed_ids .= ($failed_ids != '' ? '), (' : '(') . $host['id']; | ||
| } |
There was a problem hiding this comment.
is_[$failed] is invalid PHP syntax and will cause a parse error. This should remain a standard is_array($failed) (or equivalent) check before using array_column / array_search.
cli_import.php
Outdated
| $data = file_get_contents($filename); | ||
|
|
||
| $xml = xml2array($data); | ||
| $xml = xml2[$data]; | ||
|
|
||
| if (!is_array($xml) || !cacti_sizeof($xml)) { | ||
| if (!is_[$xml] || !cacti_sizeof($xml)) { |
There was a problem hiding this comment.
xml2[$data] and is_[$xml] are invalid PHP syntax and will cause a parse error. This should call the XML parser function (e.g., xml2array($data)) and then validate with is_array($xml) / cacti_sizeof($xml).
| $version = db_fetch_cell("SELECT version | ||
| FROM plugin_config | ||
| WHERE directory='thold'"); | ||
|
|
||
| if (!is_array($headers)) { | ||
| if (!is_[$headers]) { | ||
| $headers = []; | ||
| } |
There was a problem hiding this comment.
!is_[$headers] is invalid PHP syntax and will cause a parse error during email generation. Use !is_array($headers) (or remove the guard if $headers is always an array).
| if (cacti_sizeof($array)) { | ||
| foreach ($array as $key=>$value) { | ||
| if (is_array($value)) { | ||
| if (is_[$value]) { | ||
| $xml .= "\t<$key>" . ia2xml($value) . "</$key>\n"; | ||
| } else { |
There was a problem hiding this comment.
is_[$value] is invalid PHP syntax and will cause a parse error in this XML serialization helper. This should be is_array($value).
| if ($xml_data != '') { | ||
| // obtain debug information if it's set | ||
| $xml_array = xml2array($xml_data); | ||
| $xml_array = xml2[$xml_data]; | ||
|
|
There was a problem hiding this comment.
xml2[$xml_data] is invalid PHP syntax and will cause a parse error. This should call the XML parsing function (e.g., xml2array($xml_data)) before iterating the resulting array.
| <?php | ||
|
|
||
| declare(strict_types=1); | ||
| /* |
There was a problem hiding this comment.
The PR description mentions that standalone infrastructure files were removed, but the systemd artifacts are still present in the tree (e.g., service/systemd/thold_daemon.service). Either update the PR description or remove/move the infrastructure files as intended.
| { | ||
| "session_id": "cf3ccfa8-c9cf-42df-876d-cbe0cd78ad2f", | ||
| "ended_at": "2026-04-09T00:39:19.607Z", | ||
| "reason": "other", | ||
| "agents_spawned": 1, | ||
| "agents_completed": 0, | ||
| "modes_used": [] | ||
| } No newline at end of file |
There was a problem hiding this comment.
This appears to be tool/session metadata (.omc/sessions/...) and not part of the plugin runtime. It should be removed from the PR and added to .gitignore to avoid committing local session artifacts.
| { | |
| "session_id": "cf3ccfa8-c9cf-42df-876d-cbe0cd78ad2f", | |
| "ended_at": "2026-04-09T00:39:19.607Z", | |
| "reason": "other", | |
| "agents_spawned": 1, | |
| "agents_completed": 0, | |
| "modes_used": [] | |
| } |
Revert bulk array()->[] rewrite damage affecting: - is_array, in_array, xml2array - call_user_func_array, filter_var_array - Function declarations with _array suffix Signed-off-by: Thomas Vincent <thomasvincent@gmail.com>
Signed-off-by: Thomas Vincent <thomasvincent@gmail.com>
99df064 to
907391e
Compare
This PR adds strict typing, short array syntax, and null coalescing operators across the plugin. Standalone infrastructure files were removed per architectural mandate.