Skip to content

chore: PHPStan level 8 clean-up and native type declarations - #145

Merged
TheWitness merged 3 commits into
developfrom
chore/phpstan-level-8-typing
Sep 29, 2026
Merged

TheWitness merged 3 commits into
developfrom
chore/phpstan-level-8-typing

Conversation

@TheWitness

Copy link
Copy Markdown
Member

Summary

Brings the entire plugin_reportit codebase to PHPStan level 8 with 0 errors (no baseline, no @phpstan-ignore), and adds native parameter/return type declarations across all files.

  • PHPStan level 8: 0 errors (baseline was 891)
  • php -l: clean on every touched file
  • Full Pest suite: 28 passed (255 assertions), unchanged before/after typing + formatting
  • php-cs-fixer applied using Cacti's shared style config (no risky in_array/array_search strict-param changes)
  • locales/po/cacti.pot regenerated (only source-location comments shifted; per-language .po/.mo left to Weblate)

Genuine bugs found & fixed (please validate)

These are behavior-affecting fixes uncovered by the type analysis:

  1. Bulk "Add Data Source" was a no-op — api_reportit_add_data_source() was declared as ($id) but its body iterated a $selected_items variable that was never passed in. Added the missing $selected_items parameter and updated the call site in reportit.php.
  2. Unbound prepared query — a report-template data-source lookup called db_fetch_assoc('… WHERE data_template_id = ?', [$id]), but db_fetch_assoc()'s 2nd argument is $log (bool), not params — the ? was never bound. Switched to db_fetch_assoc_prepared().
  3. Broken formula-rename dependency propagation — a $dependences / $dependencies variable typo meant renaming a measurand abbreviation never updated dependent formulas. Unified to $dependencies.
  4. Undefined show_export_wizard() — view.php?action=export called a function that is defined nowhere (a latent fatal). The call is now guarded with function_exists().

Intentional, documented deviations

  • view.php archive/alias inert code: $archive and $report_ds_alias in show_report() are initialized to [] and never populated (their source lookups are commented out / missing upstream). I annotated their intended types rather than populating them from $data, to avoid a speculative UI behavior change. The data-source alias substitution in the table view therefore remains inert exactly as before — flagging in case you want it wired up separately.
  • system/upgrade.php: removed a foreach over a hard-coded empty $result_tables array (dead migration loop; the source fetch was already commented out).
  • funct_export.php: the $no_formatting flag is assigned 0 in both branches of its if/else, so the "raw value" ternary branches were dead — collapsed to the formatted (get_unit()) branch.
  • templates.php: dropped a stale URL 2nd argument to draw_actions_dropdown() (current signature is (array $actions, int $delete_action = 0)).

Testing

  • phpstan analyse (level 8): 0 errors
  • php -l on all 17 touched source files: clean
  • Pest: 28 passed / 255 assertions

Bring the entire plugin to PHPStan level 8 (0 errors, no baseline, no
suppressions) and add native parameter/return type declarations across
all files. Genuine bugs uncovered and fixed along the way:

- api_reportit_add_data_source() was missing its $selected_items
  parameter, so the bulk "Add Data Source" action never added anything.
- A report-template data-source lookup passed a prepared-statement
  placeholder to db_fetch_assoc() (never bound); switched to
  db_fetch_assoc_prepared().
- Measurand formula-rename dependency propagation was broken by a
  $dependences/$dependencies variable typo.
- The export action referenced an undefined show_export_wizard(); the
  call is now guarded.

Verified: PHPStan level 8 = 0 errors, php -l clean on every touched
file, full Pest suite 28 passed (255 assertions). Regenerated
locales/po/cacti.pot (per-language .po/.mo left to Weblate).

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Copilot encountered an error and was unable to review this pull request. You can try again by re-requesting a review.

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

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 critical and moderate correctness, reliability, and security issues remain.

Review effort: Lite
Findings: 1 High severity · 2 Medium severity

Open (3)

Comment thread view.php Outdated
Comment thread lib/funct_runtime.php Outdated
Comment thread reportit.php Outdated
…l-8-typing

# Conflicts:
#	locales/po/cacti.pot
bmfmancini
bmfmancini previously approved these changes Sep 29, 2026
…esult return, restore preset non-empty check
@TheWitness

Copy link
Copy Markdown
Member Author

Thanks for the review — addressed all three inline comments in 3db9d2d:

  1. view.php export dispatch (security): constrained the dynamic export_to_* dispatch to the $export_formats allowlist (CSV/XML/SML); unknown drp_action values no longer construct or call a function.
  2. lib/funct_runtime.php: reportit_prepare_store_report_results() now captures and returns the reports_log_and_notify() result, restoring runtime()'s ability to detect email/export failures via run_error().
  3. reportit.php: restored the non-empty preset guard (cacti_sizeof($rrdlist_data)), so reports without a preset no longer read undefined timezone/time/day keys.

php -l clean on all three files; all three review threads resolved.

@TheWitness
TheWitness merged commit 53d1d6a into develop Sep 29, 2026
2 checks passed
@TheWitness
TheWitness deleted the chore/phpstan-level-8-typing branch September 29, 2026 02:27
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