chore: PHPStan level 8 typing pass + bugfixes - #34
Merged
Merged
Conversation
Adds native param/return types where safe, PHPDoc-only types at Cacti
hook boundaries, and fixes several genuine bugs surfaced during the
typing pass:
- poller_apcupsd.php: both add_ups_device() calls referenced
$host_template_id, never defined anywhere in the script - every
newly-discovered UPS device was created with a null host template.
Fixed by resolving the real template id via the same
host_template.hash query already used (existence-only) by
apcupsd_host_template_imported().
- setup.php: plugin_apcupsd_install() registered the replicate_out
hook twice - removed the duplicate.
- setup.php: apcupsd_config_arrays()'s "UPSes" menu entry used the
wrong text domain ('webseer' instead of 'apcupsd').
- upses.php: the Location filter dropdown's "selected" check compared
against site_id instead of location (copy/paste from the adjacent
Site dropdown) - the Location dropdown never reflected the current
selection.
- upses.php: form_actions()'s $save_html/$ups_actions index was only
guarded for the 3 known drp_action values, but drp_action is only
regex-validated - same bug shape Copilot flagged in plugin_tags
earlier in this rollout; fixed proactively with the same
reject-invalid-values-early pattern.
- upses.php: removed a duplicate 'none_value' key in the host_id edit
field (dead, immediately overwritten).
- poller_apcupsd.php: collect_ups_data() appended to $sql_params[]
without initializing it first (works via auto-vivification but warns
on every poll) - added the missing initialization.
- db_fetch_row_prepared()/db_fetch_row() results narrowed before array
access in form_save(), ups_edit(), and duplicate_ups().
- parse_ini_file() narrowing, $config narrowing at include boundaries,
one always-true isset($_POST) simplification, and 4 html_start_box()
call sites fixed to native bool/int args.
Result: PHPStan level 8 clean (38 -> 0 errors), php-cs-fixer clean
(Cacti develop config, array() -> [] shorthand applied), php -l clean,
full Pest suite (68/68) passing against the 1.2.x baseline.
TheWitness
requested review from
bmfmancini and
xmacan
and
a lite review from Copilot
September 25, 2026 12:22
There was a problem hiding this comment.
Copilot review overview
🟡 Changes recommended
The duplicate-hook migration and malformed configuration-section handling remain unresolved.
Get a fresh assessment by requesting another Copilot review.
Review effort: Lite
Findings: 1
Open (3)
What changed in this PR
Adds PHPStan level 8 typing and fixes polling, setup, filtering, validation, and translation issues in the APCUPSD plugin.
Changes:
- Adds native typing and modernizes array syntax.
- Fixes host-template resolution, duplicate hooks, filters, and bulk-action validation.
- Refreshes translation references and poller initialization.
| File | Summary |
|---|---|
upses.php |
Form handling, filtering, validation, and UI fixes |
ui_helpers.php |
Layout helper typing |
setup.php |
Lifecycle, hook, menu, and configuration updates |
poller_apcupsd.php |
Poller typing and host-template resolution |
locales/po/cacti.pot |
Translation template refresh |
database.php |
Array syntax modernization |
apcupsd_functions.php |
Helper return typing |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
- setup.php: apcupsd_check_upgrade() now cleans up any stale duplicate 'replicate_out' plugin_hooks row (from installations that ran the old, double-registering install routine) instead of only preventing new duplicates on fresh installs. - setup.php: plugin_apcupsd_version() now also validates that the parsed INFO file actually has an 'info' section before returning it, instead of assuming it exists once the top-level parse succeeded - the previous narrowing could otherwise return null from a function declared to return array, a hard TypeError. - setup.php: converted a raw sizeof() to cacti_sizeof() to match this plugin's own established convention. Re-verified: PHPStan level 8 clean, php-cs-fixer clean, php -l clean, full Pest suite (68/68) passing against the 1.2.x baseline.
bmfmancini
approved these changes
Sep 25, 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
PHPStan level 8 + PHPDoc/native typing pass for plugin_apcupsd, following the same process as the prior repos in this rollout. Native return types added where a function's own body guarantees a single type; PHPDoc-only types kept at Cacti hook-boundary functions (
apcupsd_replicate_out,apcupsd_draw_navigation_text) whose arguments are supplied by an external dispatcher.(a) Error count
PHPStan level 8: 38 -> 0 errors.
(b) Genuine bugs fixed
poller_apcupsd.php: both calls toadd_ups_device($ups, $host_template_id, ...)referenced$host_template_id, which is never defined anywhere in the script - every newly-discovered UPS device was created with a null/undefined host template id instead of the imported APC UPS Host Template. Fixed by resolving the real template id via the samehost_template.hash = APCUPSD_HOST_TEMPLATE_HASHquery already used (existence-only) byapcupsd_host_template_imported().setup.php:plugin_apcupsd_install()registered thereplicate_outhook twice (an apparent copy/paste leftover, one labeled "hook for table replication"), which would runapcupsd_replicate_out()twice per replication event; removed the duplicate registration.setup.php:apcupsd_config_arrays()added the "UPSes" menu entry with the wrong text domain ('webseer'instead of'apcupsd'), silently breaking translation for that one menu label. Fixed.upses.php: the Location filter dropdown's "selected" check compared againstget_request_var('site_id')instead ofget_request_var('location')- a copy/paste bug from the adjacent Site dropdown block - so the Location dropdown never correctly reflected the currently-selected location. Fixed.upses.php:form_actions()'s$save_htmlwas only assigned inside the 3 knowndrp_actionbranches (1-3), and$ups_actions[get_nfilter_request_var('drp_action')]was indexed with the same unrestricted value, butdrp_actionis only regex-validated (any alphanumeric string is accepted) - matches the exact same bug shape Copilot flagged in the plugin_tags PR earlier in this rollout. Fixed proactively with the same "reject any value outside the known set up front" pattern.upses.php's edit form field array ($fields_ups_edit['host_id']) had a duplicate'none_value'key (__('None')immediately overwritten by__('Autocreate on First Poll', 'apcupsd')two lines later) - removed the dead first occurrence.poller_apcupsd.php:collect_ups_data()appended to$sql_params[]without initializing it first - works via PHP's array auto-vivification but raises an "Undefined variable" warning on every apcaccess-based poll; added the missing$sql_params = array();.db_fetch_row_prepared()/db_fetch_row()results (which can befalse) narrowed to arrays before offset access inform_save(),ups_edit(), andduplicate_ups().setup.php:parse_ini_file()result narrowed before array access (can returnfalse).setup.php/upses.php:$config/superglobal narrowing at include boundaries and one always-trueisset($_POST)simplification, matching the recurring pattern from every prior repo in this rollout.upses.php:html_start_box()$div/$cell_paddingargs fixed to nativebool/intin 4 call sites.(c) Stubs added/verified
Scratch PHPStan bootstrap stubs added for this repo's many Cacti core dependencies (
api_device_save,automation_update_device,cacti_snmp_get,push_out_host,array_rekey,api_reapply_suggested_graph_title,update_graph_title_cache,api_reapply_suggested_data_source_data,update_data_source_title_cache,get_colored_device_status,form_text_box,api_plugin_is_enabled,api_plugin_enable_hooks,db_column_exists,replicate_out_table,auth_augment_roles,form_selectable_ecell,cacti_escapeshellcmd,cacti_escapeshellarg, plus the already-established reusable base set), all sourced from Cacti/cacti'sdevelopbranch.$fields_snmp_item(a real Cacti core global frominclude/global_form.php, loaded transitively viaupses.php's ownauth.phpbootstrap at runtime) stubbed as an empty array for static-analysis purposes only.(d) Test result
Full Pest suite: 68 passed (176 assertions), run against the real
1.2.xhost via bind-mount, unchanged before and after this pass.php-cs-fixer(Cactidevelopconfig,array()->[]shorthand applied) andphp -lboth clean on all touched files.(e) Unresolved / flagged for maintainer judgement
None.
Files changed
apcupsd_functions.php,database.php(cs-fixer style only),poller_apcupsd.php,setup.php,ui_helpers.php,upses.php,locales/po/cacti.pot(translation template refresh only; per-language.po/.mofiles reverted to avoid Weblate churn).