Repository navigation
Enhanced CSV handling with escape character support and sanitized input - #1397
Conversation
There was a problem hiding this comment.
Note
Copilot was unable to run its full agentic suite in this review.
Copilot review overview
5 open issues
CSV export behavior changed by explicitly passing an escape character. There’s now a test for CSV… · New$_GET['s']/$_GET['filter']can be an array (e.g.,?s[]=pie), andsanitize_text_field()will… · New This test overwrites$_GETbut doesn’t restore it, which can leak state into later tests in the… · Newwp_tempnam()can returnfalse; in that casefile_put_contents()will emit warnings and the… · New This hard-codes the delimiter and enclosure even though the codebase also has… · New
What changed in this PR
Adds explicit CSV escape-character handling and updates request sanitization to avoid deprecated PHP APIs, backed by new PHPUnit coverage.
Changes:
- Define and use a new
VISUALIZER_CSV_ESCAPEconstant across CSV import/export (fgetcsv/fputcsv). - Replace deprecated
FILTER_SANITIZE_STRINGusage with WP sanitization helpers for chart filtering. - Add tests for CSV escape behavior and chart-type filtering via both supported request params.
| File | Description |
|---|---|
| tests/test-csv-escape.php | Adds coverage ensuring CSV import uses backslash escape without emitting deprecations |
| tests/test-ajax.php | Adds data-driven test for chart-type filtering via s and filter request params |
| tests/php/static-analysis-stubs/symbols.php | Adds stub definition for VISUALIZER_CSV_ESCAPE for static analysis |
| index.php | Defines VISUALIZER_CSV_ESCAPE at plugin launch |
| classes/Visualizer/Source/Csv.php | Passes explicit escape arg to fgetcsv() |
| classes/Visualizer/Module/Chart.php | Uses WP sanitization for request params; passes escape arg to fputcsv() |
| classes/Visualizer/Module/Abilities.php | Passes explicit escape arg to fputcsv() |
| classes/Visualizer/Module.php | Passes explicit escape arg to fputcsv()/fgetcsv() when generating CSV |
Review effort: Lite
There was a problem hiding this comment.
🔵 Needs a closer look
Dev review: No defects were found. Confirm how index.php:84 handles a site that overrides VISUALIZER_CSV_ESCAPE in wp-config.php.
Validation details
- Files reviewed: 8/8 changed files. Four optional-setting rows stay unchecked (see Untested areas).
- All nine listed
fgetcsv()/fputcsv()calls and bothFILTER_SANITIZE_STRINGreads are fixed. - On PHP 8.4,
tests/test-csv-escape.phpfails without the change:use-php.sh 8.4 2>&1 | tail -3; php -v | head -1; which php; cd /workspace; set -o pipefail; timeout 600 sandbox-phpunit-…printedthe $escape parameter must be provided(run R8). It passes with the change. - On PHP 8.3, the new
test-ajax.phpcases fail without the change: "Failed asserting that null is true" (not in the output of run R12). They pass with the change. - An isolated run showed that an empty escape keeps export rows unchanged:
cp /repo/classes/Visualizer/Source.php /workspace/checks/L1/Source.php; cp /repo/classes/Visualizer/Source/Csv.php /work…printedidentical=true(run R4).
Untested areas
- Optional setting off: Setting off. Open
index.php:84. The constant has no off state, so confirm that'\\'is the intended default. - Optional setting off: Non-default value. Open
index.php:84. A multi-character value makesfgetcsv()throwValueError. Confirm that this needs no guard. - Optional setting off: Empty data. Not checked.
- Optional setting off: Stale data. Not checked.
- PHPUnit did not run on PHP 7.4, the CI version.
- The export tests were not rerun on PHP 8.4.
🤖 Automated review · run code-review-agent_6ad8ae4e0a0cc7.77119773.
🤖 Review agent — review posted ✅ on 0419e8a7 · commented · 0 findings · 18 min
Run code-review-agent_6ad8ae4e0a0cc7.77119773 · trail
|
🎉 This PR is included in version 4.0.10 🎉 The release is available on GitHub release Your semantic-release bot 📦🚀 |


Summary
Improves CSV handling across the codebase by introducing and consistently applying a dedicated CSV escape-character constant. It also updates input sanitization for chart filtering and adds comprehensive tests to ensure correct CSV parsing and escaping behavior.
Check before Pull Request is ready:
Closes #1395