Fix false positive for files with only require_once and class definitions - #1369
GeorgeWebDevCy wants to merge 8 commits into
Conversation
|
The following accounts have interacted with this PR and/or linked issues. I will continue to update these lists as activity occurs. You can also manually ask me to refresh this list by adding the If you're merging code through a pull request on GitHub, copy and paste the following into the bottom of the merge commit message. To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook. |
|
Thanks @GeorgeWebDevCy for your contribution. You need to review the tests that are failing before we can review the PR. |
|
Thanks for the heads up. I reviewed the failing tests and pushed a follow-up fix in 3813175. The CI matrix is now passing. |
| uses: codecov/codecov-action@fb8b3582c8e4def4969c97caa2f19720cb33a72f | ||
| with: | ||
| file: build/logs/*.xml | ||
| files: build/logs/*.xml |
There was a problem hiding this comment.
This updates the input name for the pinned Codecov action. At commit fb8b3582c8e4def4969c97caa2f19720cb33a72f, the action defines files (a comma-separated list of explicit coverage files) and passes it through as INPUT_FILES; it does not define a file input. Both occurrences use the same build/logs/*.xml pattern, so this makes the workflow use the action's supported input without changing which reports we intend to upload.
|
Thanks to Codex’s review, I found a potential false negative in the new include handling. Any Could we avoid skipping arbitrary includes, or verify that the included file is also structural-only? It would also be helpful to add a regression test for a class file that requires a file containing executable code. |
There was a problem hiding this comment.
Copilot review overview
🟡 Changes recommended
The new AST allowance for include/require can incorrectly treat dynamic include targets as safe, and the new test fixture references a missing dependency file.
Get a fresh assessment by requesting another Copilot review.
Review effort: Lite
Findings: 3
Open (3)
What changed in this PR
This PR updates the Direct File Access check to avoid flagging PHP files that only load dependencies (via require_once) and declare classes/interfaces/traits/enums—addressing the reported false positive in #1361.
Changes:
- Allow
include/requireAST expressions in otherwise “structural-only” files. - Add a PHPUnit fixture and assertion covering
require_once+ class-only files. - Update Codecov upload config in the PHP test workflow to use the
filesinput.
| File | Description |
|---|---|
| includes/Checker/Checks/Plugin_Repo/Direct_File_Access_Check.php | Adjusts AST/regex heuristics for determining whether a file is safe for direct access. |
| tests/phpunit/tests/Checker/Checks/Direct_File_Access_Check_Tests.php | Extends the existing PHPUnit coverage to assert the new “require_once + class-only” case is not flagged. |
| tests/phpunit/testdata/plugins/test-plugin-direct-file-access-without-errors/includes/require-once-class-only.php | Adds a new test fixture file that matches the reported false-positive scenario. |
| .github/workflows/php-test.yml | Aligns Codecov upload configuration with other workflows by using files:. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| if ( $node instanceof Stmt\Expression ) { | ||
| if ( $node->expr instanceof Expr\Include_ && $has_structural_declaration ) { | ||
| continue; | ||
| } | ||
|
|
| if ( $node instanceof Stmt\Namespace_ && ! empty( $node->stmts ) ) { | ||
| return $this->has_structural_declaration( $node->stmts ); | ||
| } |
| * File with require_once and class - safe for direct access. | ||
| */ | ||
|
|
||
| require_once __DIR__ . '/class-parent.php'; |

Fixes #1361