ci: Run the plugin unit test suite by somethingwithproof · Pull Request #243 · Cacti/plugin_weathermap

somethingwithproof · 2026-08-30T18:40:11Z

The repository has 23 test files and 119 tests that no workflow ever ran, so a
failing prepared-statement check and two tests that asserted nothing had gone
unnoticed on develop.

adds a unit-test job across PHP 8.1-8.4 that installs the plugin's own
composer dependencies and runs lint plus the suite
moves the toolchain to Pest 2; Pest 1 is not PHP 8.4-clean, which the old
phpunit.xml worked around by muting E_DEPRECATED
narrows the prepared-statement check to raw db calls that actually build SQL
from a variable, so the fully static query in cli/cacti-mapper.php no
longer fails it
makes the two XSS handoff tests assert on a collected list, so the
expectation still runs when nothing matches

Verification: composer run-script test 119 passed on 8.1, 8.3 and 8.4;
composer run-script lint clean.

First of seven; merge before the others.

Closes #245

The suite has 23 files and 119 tests that no workflow ever invoked, so a failing prepared-statement check and two assertion-less tests sat unnoticed on develop. Adds a unit-test job across PHP 8.1-8.4, moves the toolchain to Pest 2 (Pest 1 is not PHP 8.4-clean, which the old phpunit.xml worked around by muting E_DEPRECATED), and fixes the three broken tests. Signed-off-by: Thomas Vincent <thomasvincent@gmail.com>

TheWitness

Remove composer.json and refactor using base Cacti install.

…ces (#244) string_or_null() is called in the cacti and wmdata sources but is defined nowhere in the plugin or in Cacti core, so every cacti: target read ended in a fatal Error; the surrounding files already use an inline null check, so these now match. wmdata carried a target regex with no closing delimiter, which left $datafile unset and made file_exists() a TypeError under that file's strict_types, opened $targetstring instead of $datafile, and never closed the handle. The rrd_options guard used '/["\'\\]/', which PCRE reads as an unterminated character class, so preg_match() returned false and the check never rejected anything; it is now one strpbrk() predicate shared by both call sites. Options were escaped with cacti_escapeshellarg() either way, so no injection was reachable through the dead guard. Signed-off-by: Thomas Vincent <thomasvincent@gmail.com>

The plugin no longer carries composer.json or a lock file. Cacti already ships pest, phplint, phpstan and php-cs-fixer in include/vendor, so the unit-test job now checks the plugin out under cacti/plugins/weathermap, installs Cacti's dependencies once, and runs the suite through include/vendor/bin/pest against the plugin's phpunit.xml. That is the same route the integration job already takes for linting. Signed-off-by: Thomas Vincent <thomasvincent@gmail.com>

The guard only runs when PHP_SAPI is not cli, so the case that proves an unprivileged user is refused needs a non-cli binary to drive the page. Signed-off-by: Thomas Vincent <thomasvincent@gmail.com>

Signed-off-by: Thomas Vincent <thomasvincent@gmail.com>

Cacti core now requires PHP >= 8.2, so the 8.1 integration job fails the composer platform check; Ubuntu Noble also lacks libapache2-mod-php for non-native versions without the ondrej PPA. Signed-off-by: Thomas Vincent <thomasvincent@gmail.com>

Ubuntu Noble lacks php8.2/8.4-cgi without the PPA. Signed-off-by: Thomas Vincent <thomasvincent@gmail.com>

somethingwithproof · 2026-09-03T20:28:18Z

Done in e8d3a84. The plugin no longer carries a Composer manifest; the unit test job installs Composer dependencies from the base Cacti checkout and runs lint and the tests through it.

Addressed in e8d3a84. The plugin no longer ships a Composer manifest and the unit test job uses the base Cacti install.

Copilot

🟡 Changes recommended

The CI PHP version matrices conflict with the PR description/verification scope (8.1 omitted), and WeatherMapDataSource_wmdata::ReadData() can emit misleading/duplicate warnings (and can still raise undefined-offset notices on malformed rows).

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Pull request overview

This PR adds CI coverage for the plugin’s previously-unrun unit test suite (via a new workflow job), updates the test harness/tests to make previously ineffective assertions actually assert, and tightens a couple of security-related guards/checks in datasources and test scanning.

Changes:

Add a unit-test GitHub Actions job to run plugin lint + Pest tests against a PHP version matrix.
Add/adjust tests and harness utilities to exercise datasource behavior and security guards (RRD options, XSS handoff).
Refine prepared-statement consistency checks and datasource implementations to align checks with actual risk.

File summaries

File	Description
`.github/workflows/plugin-ci-workflow.yml`	Adds a unit-test job and adjusts the PHP matrix used in CI runs.
`.gitignore`	Ignores vendor, lockfile, and tooling caches/reports.
`CHANGELOG.md`	Notes CI now runs the plugin unit test suite.
`composer.json`	Removes the plugin’s own Composer manifest (tooling comes from base Cacti install).
`phpunit.xml`	Updates PHPUnit config for stricter runs and defines source include/exclude.
`lib/datasources/WeatherMapDataSource_cacti.php`	Tweaks debug output formatting.
`lib/datasources/WeatherMapDataSource_rrd.php`	Fixes/strengthens `rrd_options` safety guard via a dedicated helper.
`lib/datasources/WeatherMapDataSource_wmdata.php`	Hardens parsing/file handling, closes file handles, and refines logging.
`tests/Helpers/DataSourceHarness.php`	Adds a minimal datasource test harness/stubs/constants.
`tests/Handoff/XssEscapingHandoffTest.php`	Fixes tests to assert after scanning, even when no matches exist.
`tests/Security/PreparedStatementConsistencyTest.php`	Narrows the raw-db-call check to variable-built SQL and adds an argument collector.
`tests/Security/RrdOptionsGuardTest.php`	Adds cases that pin intended quote/backslash rejection behavior.
`tests/Unit/DataSourceReadDataTest.php`	Adds datasource smoke tests for `ReadData()` behavior.

Review details

Suppressed comments (1)

.github/workflows/plugin-ci-workflow.yml:93

Integration-test matrix also drops PHP 8.1, which conflicts with the PR description/issue text that calls out 8.1–8.4 coverage (and the verification note mentioning 8.1).

    strategy:
      fail-fast: false
      matrix:
        php: ['8.2', '8.3', '8.4']
        os: [ubuntu-latest]

Files reviewed: 12/13 changed files
Comments generated: 3
Review effort level: Lite

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

+    strategy:
+      fail-fast: false
+      matrix:
+        php: ['8.2', '8.3', '8.4']
+


+					$fields = explode("\t", rtrim($buffer, "\n"));

-					if ($fields[0] == $dataname) {
+					if (isset($fields[2]) && $fields[0] == $dataname) {


 		} else {
 			wm_warn("WMData ReadData: $datafile doesn't exist [WMWMDATA01]");
 		}


TheWitness previously requested changes Aug 30, 2026

View reviewed changes

somethingwithproof added 4 commits August 30, 2026 13:22

ci: Install the CGI SAPI for the check.php guard test

9364fff

The guard only runs when PHP_SAPI is not cli, so the case that proves an unprivileged user is refused needs a non-cli binary to drive the page. Signed-off-by: Thomas Vincent <thomasvincent@gmail.com>

docs: Note the unit test job in the changelog

f92f47f

Signed-off-by: Thomas Vincent <thomasvincent@gmail.com>

somethingwithproof mentioned this pull request Aug 30, 2026

Run the plugin unit test suite #245

Open

somethingwithproof added 2 commits August 30, 2026 18:08

ci: add ondrej PPA before installing php-cgi in the unit test job

2bffc69

Ubuntu Noble lacks php8.2/8.4-cgi without the PPA. Signed-off-by: Thomas Vincent <thomasvincent@gmail.com>

somethingwithproof requested a review from TheWitness September 3, 2026 20:28

somethingwithproof requested a lite review from Copilot September 5, 2026 01:33

Copilot started reviewing on behalf of somethingwithproof September 5, 2026 01:33 View session

Copilot AI reviewed Sep 5, 2026

View reviewed changes

somethingwithproof requested a review from netniV September 6, 2026 12:07

somethingwithproof self-assigned this Sep 6, 2026

Navigation Menu

Uh oh!

ci: Run the plugin unit test suite - #243

ci: Run the plugin unit test suite#243
somethingwithproof wants to merge 7 commits into
developfrom
build/test-harness

somethingwithproof commented Aug 30, 2026 •

edited

Loading

Uh oh!

TheWitness left a comment

Uh oh!

somethingwithproof commented Sep 3, 2026

Uh oh!

Copilot AI left a comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Uh oh!

Conversation

somethingwithproof commented Aug 30, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

TheWitness left a comment

Choose a reason for hiding this comment

Uh oh!

somethingwithproof commented Sep 3, 2026

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

🟡 Changes recommended

Suppressed comments (1)

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

somethingwithproof commented Aug 30, 2026 •

edited

Loading