Skip to content

chore: PHPStan level 8 typing pass + bugfixes - #26

Merged
bmfmancini merged 2 commits into
mainfrom
chore/phpstan-level-8-typing
Sep 25, 2026
Merged

bmfmancini merged 2 commits into
mainfrom
chore/phpstan-level-8-typing

Conversation

@TheWitness

Copy link
Copy Markdown
Member

Summary

PHPStan level 8 + PHPDoc/native typing pass for plugin_quicktree, following the same process as the prior repos in this rollout (plugin_cjobs/backup/schema/exlist/lsflog). Native return types added where a function's own body guarantees a single type; PHPDoc-only types kept at Cacti hook-boundary functions (quicktree_graph_buttons, quicktree_draw_navigation_text) whose arguments are supplied by an external dispatcher.

(a) Error count

PHPStan level 8: 18 -> 0 errors.

(b) Genuine bugs fixed

  • setup.php: quicktree_show_tab() built its tab link URL as '...quicktree.php?location=tab' . quicktree_page_location('tab') - the literal tab before the function call was a leftover, producing an invalid ?location=tabtab or ?location=tabconsole query string. Fixed to '...quicktree.php?location=' . quicktree_page_location('tab').
  • quicktree.php: the "no graph specified" branch of the add action called __('...', $title) where $title is undefined in that branch (only ever set when a graph was specified) - the format string has no %s placeholder, so this was a copy/paste leftover; replaced with the correct 'quicktree' text-domain argument matching every other __() call in the file.
  • quicktree.php: removed 3 unreachable break; statements sitting after exit; in the clear/save/remove action cases (dead code, no behavior change).
  • quicktree.php: simplified an always-true if (empty($save['id'])) { $save['user_id'] = ...; } - $save['id'] is unconditionally set to '' two lines above, so the condition can never be false; PHPStan correctly flagged this as dead logic. Behavior preserved (the assignment always ran either way), just removed the vestigial conditional.
  • setup.php: parse_ini_file() result narrowed before array access (can return false).
  • setup.php / ui_helpers.php / quicktree.php: html_start_box() $div/$cell_padding args passed as strings instead of native bool/int, matching Cacti core's typed signature (recurring pattern across this whole rollout).

(c) Stubs added/verified

Scratch PHPStan bootstrap stubs added for this repo's Cacti core dependencies (api_plugin_db_table_create, api_user_realm_auth, get_current_page, get_filter_request_var, isset_request_var, draw_actions_dropdown, general_header, raise_message, sql_save, api_tree_item_save, set_default_action, api_plugin_upgrade_register, plus the already-established reusable base set), all sourced from Cacti/cacti's develop branch.

(d) Test result

Full Pest suite: 58 passed (192 assertions), run against the real 1.2.x host via bind-mount.

Two pre-existing tests in this repo initially failed after two changes I made for PHPStan/style compliance, and I reverted both changes rather than touch the tests (per "tests win" policy) since both are behaviorally-neutral:

  • One test asserts api_plugin_register_hook(...)'s 5th arg is recorded as the exact literal 1; I'd changed the call site from 1 to true to satisfy PHPStan's native bool param stub. Reverted the call site back to 1 (PHP coerces this to true at the real bool $enable parameter boundary regardless) and loosened that one parameter's type in the scratch stub instead - no shipped code or real behavior is affected either way.
  • One test's own regex-based "no SQL string interpolation" check specifically looks for , array( to separate a prepared query's SQL literal from its bound params; php-cs-fixer's array() -> [] conversion on one single-line db_execute_prepared(...) call broke that heuristic (false positive, not real interpolation). Reverted that one call's params back to array($user) syntax; left cs-fixer's conversion everywhere else in the file. php-cs-fixer --dry-run therefore still shows this one line as "could be fixed" - a deliberate, documented exception, not an oversight (CI does not run php-cs-fixer as a gate for this repo).

(e) Unresolved / flagged for maintainer judgement

None beyond the two documented, behaviorally-neutral exceptions above.

Files changed

quicktree.php, quicktree_security.php, setup.php, ui_helpers.php, locales/po/cacti.pot (translation template refresh only; per-language .po files reverted to avoid Weblate churn).

Adds native param/return types where safe, PHPDoc-only types at Cacti
hook boundaries, and fixes several genuine bugs surfaced during the
typing pass:

- setup.php: quicktree_show_tab() built its tab link URL with a leftover
  literal "tab" concatenated directly before quicktree_page_location()'s
  result, producing an invalid ?location=tabtab / ?location=tabconsole
  query string. Fixed to concatenate only the function's return value.
- quicktree.php: the "no graph specified" branch of the add action passed
  an undefined $title into __() where the format string has no %s
  placeholder - clearly meant to be the 'quicktree' text-domain arg used
  everywhere else in the file; fixed.
- quicktree.php: removed 3 unreachable break; statements after exit;.
- quicktree.php: simplified an always-true empty($save['id']) check
  (the key is unconditionally set to '' immediately before) - PHPStan
  correctly flagged this as dead logic; behavior unchanged.
- parse_ini_file() result narrowed before array access (can return false).
- html_start_box() $div/$cell_padding args fixed to native bool/int in
  3 call sites across setup.php/ui_helpers.php/quicktree.php.

Result: PHPStan level 8 clean (18 -> 0 errors), full Pest suite (58/58)
passing against the 1.2.x baseline. Two behaviorally-neutral exceptions
made to keep pre-existing tests green (see PR body for details): reverted
one api_plugin_register_hook() call's 5th arg from true back to the
literal 1 a test asserts on (loosened only the scratch PHPStan stub's
param type instead), and reverted one db_execute_prepared() call's params
back to array() syntax since php-cs-fixer's []-shorthand conversion broke
a test's own array()-based SQL-interpolation-detection regex.

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot review overview

🟡 Changes recommended

Unresolved moderate issues remain in translation handling, request normalization, and setup.php parse-failure handling.

Get a fresh assessment by requesting another Copilot review.

Review effort: Lite
Findings: 2 Medium severity · 2 Low severity

Open (4)
What changed in this PR

This PR adds PHPStan level 8 typing and cleanup to the QuickTree Cacti plugin while fixing URL, translation, and control-flow issues.

Changes:

  • Adds native/PHPDoc typing and modernizes syntax.
  • Fixes tab URL generation, translation handling, and dead code.
  • Refreshes gettext references and validates against the test suite.
File Summary Review findings
ui_helpers.php Adds typed UI helpers and corrects Cacti argument types. None
setup.php Adds typing and fixes tab URL generation. Moderate parse failure handling issue; nit changelog entry and regression test requested.
quicktree.php Cleans up actions, database/UI code, and typing. Moderate misspelled qucktree text domain (3 votes).
quicktree_security.php Adds location normalization typing. Moderate non-string request values can cause a TypeError (2 votes).
locales/​po/​cacti.pot Refreshes translation source references. None

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

Comment thread quicktree.php Outdated
Comment thread quicktree_security.php Outdated
Comment thread setup.php
Comment thread setup.php
- quicktree.php: fixed a misspelled 'qucktree' text domain in a
  translated string (was silently falling back to untranslated text).
- quicktree_security.php: removed the native `string` param type from
  quicktree_normalize_location() - its only caller passes
  get_nfilter_request_var('location'), which returns `mixed` and can be
  a non-string (e.g. an array via `location[]=console`), which would
  raise a TypeError instead of falling back to 'tab'. Return type stays
  `string` since the body always returns one of two string literals.
- CHANGELOG.md: added a develop entry documenting this pass's bugfixes.
- tests/Unit/QuicktreeShowTabTest.php: new regression test suite for
  quicktree_show_tab(), asserting its rendered link is exactly
  '?location=tab' (never '?location=tabtab' or '?location=tabconsole'),
  covering the Tab-only, Tab+Console, Console-only, and unauthorized
  cases. Added a minimal api_user_realm_auth() stub to
  tests/bootstrap-unit.php to support it.

Re-verified: PHPStan level 8 clean, php -l clean, full Pest suite
(62/62, 4 new) passing against the 1.2.x baseline.
@bmfmancini
bmfmancini merged commit b80aff4 into main Sep 25, 2026
5 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants