fix(rpn): correct the expression evaluator against rrdtool semantics

[somethingwithproof]

Fixes #782.

Each defect below was confirmed by running the function, not by reading it. The tests were written first and fail on develop.

Stack and set operators

EXC and REV were both no-ops. Popping a span already reverses it, and both then reversed it back:

// EXC, before
$v1 = pop();  // top
$v2 = pop();  // next
push($v2); push($v1);   // original order restored

[1,2] EXC returned [1,2]; rrdtool gives [2,1]. An expression using either operator to order operands before a non-commutative operator computed the wrong comparison, silently.

AVG raised an uncaught TypeError: Unsupported operand types when 'U' or 'NAN' reached $total += $v. That is a fatal in the poller process, so every threshold after it in the run went unevaluated. It now follows rrdtool: unknown samples are skipped, the average is over the known ones, and an all-unknown span yields UNKN.

Both operators now also stop on stack underflow rather than pushing false back and carrying on.

Expression result

thold_calculate_expression() ended with return $stack[0] — the bottom of the stack. rrdtool yields the top. For a well-formed expression these are the same element, so this only bit when the stack was unbalanced, which the EXC/REV bugs made more likely. 1,2,3,+ returned 1 instead of 5.

It now returns the top, and an expression that does not reduce to a single value is reported as the authoring error it is rather than quietly returning an operand.

Missing dispatch entry

$spectypes = ['CURRENT_DATA_SOURCE', 'CURRENT_GRAPH_MINIMUM_VALUE',
    'CURRENT_GRAPH_MINIMUM_VALUE', ...];

The minimum appeared twice and CURRENT_GRAPH_MAXIMUM_VALUE was absent, although thold_expression_specialtype_rpn() handles it. Expressions using it fell through to Unsupported Field, set $rpn_error, and returned 0 — so a low threshold on such an expression alerted permanently.

CDEF division and modulo by zero

thold_rpn() guarded division but not modulo, so % by zero was fatal on PHP 8. The division guard returned -1, a plausible-looking number that is then compared against the bounds; the function already uses '' as its sentinel for operands it cannot use, so both now do that.

Also

$data_sources = $rrd_reindexed[$thold['local_data_id']] was an unguarded array read that produced null for a data source with no readings this cycle, then a TypeError from array_key_exists(). It now defaults, and the no-op element-by-element copy that followed it is gone.

Tests

43 tests covering the stack, set and CDEF operators plus end-to-end expression evaluation. Every changed line is covered.

The harness (tests/, composer.json, phpunit.xml, the unit-test workflow) is the same one added in #773, byte-identical, so whichever lands first the other merges cleanly. If #773 goes in first this PR's diff shrinks to the thold_functions.php changes and its own test files.

PHP version

The new code uses PHP 8 idioms (typed signatures, ??, short array syntax) to match the 8.1–8.4 CI matrix and Cacti 1.2.31's "php": ">=8.0". Worth noting that the plugin contained no PHP 8-only syntax before this, and INFO still declares compat = 1.2.25 — if running under an older Cacti on PHP 7 is meant to be supported, say so and I will keep these functions 7.4-compatible.

[somethingwithproof]

The four Integration Test failures are an upstream break, not this PR. The workflow checks out Cacti/cacti unpinned, and Install Cacti via CLI dies in core:

PHP Fatal error: Uncaught Error: Call to undefined function __()
  in cacti/lib/functions.php:7973

That is format_cacti_version_text() calling __() before the translation layer is loaded, on Cacti develop at 96033267c. Nothing in this PR touches Cacti core.

#776 pins the checkout to release/1.2.31 and its integration run is green, which is the same failure mode it was opened for. The PHPUnit job here passes.

[Copilot]

Pull request overview

This PR corrects several RPN-expression evaluation behaviors in thold_functions.php to better match RRDtool semantics (stack/set operators, special token dispatch, expression result selection, and safer CDEF division/modulo handling). It also introduces a PHPUnit-based unit-test harness with Docker + GitHub Actions CI to enforce changed-line coverage for the evaluator.

Changes:

• Fix stack/set operator behavior (e.g., EXC, REV, AVG) and make expression evaluation return the top-of-stack while rejecting unbalanced expressions.
• Fix special-token dispatch for CURRENT_GRAPH_MAXIMUM_VALUE and harden expression evaluation when a data source has no readings in the current cycle.
• Add PHPUnit test suite + Docker runner + GitHub Actions workflow, including a “patch coverage” gate for changed PHP lines.

Reviewed changes

Copilot reviewed 14 out of 16 changed files in this pull request and generated 1 comment.

Show a summary per file

File	Description
thold_functions.php	Corrects RPN operator semantics, fixes special-token dispatch, hardens expression/CDEF evaluation edge cases.
tests/Unit/TholdRpnCdefTest.php	Adds unit coverage for CDEF arithmetic, including division/modulo by zero behavior.
tests/Unit/TholdExpressionStackOpsTest.php	Adds unit coverage for stack + set operators against RRDtool-like semantics.
tests/Unit/TholdCalculateExpressionTest.php	Adds end-to-end coverage for expression evaluation and special-token dispatch.
tests/TestCase.php	Defines shared PHPUnit base test case and plugin source loader helper.
tests/Support/CactiStub.php	Adds a recording stub for Cacti framework globals used by the plugin.
tests/fixtures/optional-core-functions.php	Adds optional-core function fallbacks to exercise function_exists() branches in isolation.
tests/fixtures/cacti-lib/variables.php	Provides a minimal file to satisfy runtime includes during tests.
tests/docker/Dockerfile	Provides a reproducible PHP 8.1 + pcov test runner image.
tests/docker/docker-compose.yml	Adds a local docker-compose entrypoint mirroring CI.
tests/bootstrap.php	Boots the test environment and defines Cacti-function shims.
tests/bin/patch-coverage.php	Implements changed-line coverage enforcement using git diff + Clover.
phpunit.xml	Configures PHPUnit and scopes coverage to thold_functions.php.
composer.json	Adds PHPUnit dependency and test scripts.
.gitignore	Ignores Composer/vendor and PHPUnit/coverage artifacts.
.github/workflows/php-unit-tests.yml	Adds CI job to run tests in Docker and enforce patch coverage on PRs.

---

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

[TheWitness]

No composer.jaon in plugins.

[somethingwithproof]

Review follow-up: TheWitness feedback is addressed—the plugin diff contains no composer.json, composer.lock, or vendor directory, and tests run through Cacti’s Composer-managed Pest toolchain. Copilot’s fractional set-count finding is fixed with a regression test, and its review thread is resolved. Current CI is rerunning on the updated branch.