refactor: safe PHP 7.4 modernization + Pest unit-test framework - #98
Closed
TheWitness wants to merge 11 commits into
Closed
refactor: safe PHP 7.4 modernization + Pest unit-test framework#98TheWitness wants to merge 11 commits into
TheWitness wants to merge 11 commits into
Conversation
* add mqtt test * formatting * update * add DOH * update
Revert bulk array()->[] rewrite damage affecting: - is_array, in_array, xml2array - call_user_func_array, filter_var_array - Function declarations with _array suffix Signed-off-by: Thomas Vincent <thomasvincent@gmail.com>
…ction declarations Signed-off-by: Thomas Vincent <thomasvincent@gmail.com>
Contributor
There was a problem hiding this comment.
🟡 Changes recommended
The SQL quoting and notification mapping tests need correction, and CI coverage lacks a configured driver.
Get a fresh assessment by requesting another Copilot review.
Pull request overview
Modernizes the plugin for PHP 7.4, hardens proxy/CA SQL filters, and adds Pest testing with CI.
Changes:
- Updates PHP syntax and query arguments for PHP 7.4 compatibility.
- Escapes and groups administrative search filters.
- Adds Pest scaffolding, regression checks, and GitHub Actions CI.
File summaries
| File | Summary | Review note |
|---|---|---|
tests/Unit/test_filter_sql_quoting.php |
Adds SQL quoting checks. | Critical (3 votes): Assertion does not verify payload escaping. |
tests/Unit/ArraysDefinitionTest.php |
Tests array definitions and notification behavior. | Moderate (2 votes): Tests reimplement the mapping loop and do not exercise the populated branch. |
tests/TestCase.php |
Adds the PHPUnit base fixture. | — |
tests/Pest.php |
Adds Pest configuration. | — |
tests/Integration/test_servcheck_filter_sql_wiring.php |
Checks SQL hardening wiring. | — |
tests/e2e/test_servcheck_no_raw_filter_sql.php |
Detects raw filter SQL. | — |
tests/bootstrap-unit.php |
Provides test bootstrap paths and stubs. | — |
tests/.cacti-version |
Defines the expected Cacti version. | — |
setup.php |
Modernizes setup syntax. | — |
servcheck_test.php |
Modernizes request and query arrays. | — |
servcheck_proxies.php |
Hardens proxy filtering and modernizes arrays. | — |
servcheck_process.php |
Modernizes process query arguments. | — |
servcheck_ca.php |
Hardens CA filtering and modernizes arrays. | — |
poller_servcheck.php |
Modernizes poller query arguments. | — |
phpunit.xml |
Configures the Pest test suite. | — |
includes/tests.php |
Modernizes cURL and test arrays. | — |
includes/mxlookup.php |
Modernizes lookup arrays. | — |
includes/functions.php |
Modernizes graph and query arrays. | — |
includes/arrays.php |
Modernizes definitions and notification arrays. | — |
.gitignore |
Ignores .omc artifacts. |
— |
.github/workflows/plugin-ci-workflow.yml |
Adds CI installation and test execution. | Critical (3 votes): Coverage commands lack an installed Xdebug or PCOV driver. |
Review details
Suppressed comments (3)
.github/workflows/plugin-ci-workflow.yml:197
- This workflow only invokes Pest for
tests/Unit; the Integration and e2e scripts are explicitly excluded from that testsuite and are never run anywhere else. Consequently, the SQL-hardening checks in those new scripts cannot fail CI; add a direct PHP CLI step for the standalone checks or convert them to Pest tests.
- name: Run Pest Unit Tests
run: |
cd ${{ github.workspace }}/cacti
if [ -f plugins/servcheck/phpunit.xml ]; then
include/vendor/bin/pest --configuration=plugins/servcheck/phpunit.xml \
--coverage-clover=plugins/servcheck/coverage/clover.xml \
plugins/servcheck/tests/Unit
.github/workflows/plugin-ci-workflow.yml:114
MYSQL_AUTH_USRis expanded by the shell into--defaults-file=~/.my.cnf; tilde expansion does not run recursively after parameter expansion, so the MySQL client will look for a literal~/.my.cnfand then fail authentication. Use an absolute path (or pass--defaults-file="$HOME/.my.cnf"directly in each command).
MYSQL_AUTH_USR: '--defaults-file=~/.my.cnf'
tests/Unit/test_filter_sql_quoting.php:15
tests/Unitis the configured Pest testsuite and the workflow passes that directory to Pest, so this standalone script is loaded during test discovery;exit(0)terminates the whole runner before Pest can execute or reportArraysDefinitionTest. This can produce a false-green run. Convert this file to a Pest spec or move it to a standalone directory and invoke it with PHP.
exit(0);
- Files reviewed: 20/21 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.
bmfmancini
previously approved these changes
Sep 12, 2026
…cise real notify_accounts code path
bmfmancini
previously approved these changes
Sep 12, 2026
TheWitness
dismissed
bmfmancini’s stale review
September 12, 2026 23:51
The base branch was changed.
bmfmancini
approved these changes
Sep 12, 2026
Member
Author
|
Closing — the develop/main divergence made this too messy to merge cleanly. Will revisit separately. |
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
declare(strict_types=1)repo-wide for PHP 7.4 compatibility..github/workflows/plugin-ci-workflow.yml) and Pest unit-test scaffolding (phpunit.xml,tests/bootstrap-unit.php,tests/TestCase.php,tests/Pest.php,tests/.cacti-version), adapted for this branch's existingtests/Unitlayout:tests/Unit(whereArraysDefinitionTest.phplives) instead of atests/Securitydirectory that doesn't exist here.db_table_existsstub to the bootstrap, required byincludes/arrays.php.ArraysDefinitionTest.php's broken require of a non-existenttests/Helpers/GlobalStubs.php; the bootstrap now supplies those stubs automatically via Pest.tests/Integrationandtests/e2eremain standalone PHP CLI scripts and are intentionally excluded from the Pest testsuite.