Extract HTTP mocking template into a real PHP file - #353
Conversation
The mock transport used by `Given that HTTP requests to ... will respond with:` lived in an interpolating heredoc, so none of the project's quality gates could see it. `WP_CLI_CS` deliberately excludes `Generic.PHP.Syntax` because "the parallel linter command takes care of linting" -- but the linter only sees the string, not the PHP inside it, so a typo in the mock transport passed `composer lint` and only surfaced as a confusing runtime failure inside the child WP-CLI process. The heredoc also silently interpreted its own escapes. `"\n\n"` in the `pre_http_request` branch was rendered as two literal newlines inside the generated source, and `"\r\n\r\n"` as literal CR/LF bytes, leaving control characters embedded in string literals in the generated file. Any tooling that normalized line endings on that file would have broken the header and body split. Move the template to `src/Context/templates/mock-requests.php` and copy it verbatim, with the mocked responses written alongside it as a generated `mock-requests-data.php` that the template requires. The template is now an ordinary PHP file: it is highlighted by editors, picked up by the recursive `parallel-lint` scan, and free of the 76 `\$` escapes. Emitting the response data once instead of interpolating it into two places shrinks the step definition from 166 lines to 33. PHPStan excludes the template directory. It is executed by the child process, where the Requests v1 classes it branches on may still exist, so analyzing it against this package's dependency set only reports classes that are absent here by design. Behat now lints both generated files so the copied template and the `var_export`ed data are checked at runtime too. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01RiuykJaAqpdnCLsn4Ewmm8
|
Warning Review limit reached
Next review available in: 52 minutes You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. 📝 WalkthroughWalkthroughThe HTTP mock setup now stores response data separately and copies a shared Requests-compatible transport template. The template supports Requests v1 and v2. Behat checks both generated files with PHP syntax validation, and PHPStan excludes the runtime templates. ChangesHTTP Mocking
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant WordPress
participant MockTransport
participant Requests
WordPress->>MockTransport: pre_http_request(url, options)
MockTransport->>MockTransport: Match mocked URL
MockTransport->>Requests: Parse matched response
MockTransport-->>WordPress: Return WordPress-compatible response
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches 💡 1🛠️ Fix failing CI checks 💡
📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/Context/templates/mock-requests.php`:
- Around line 49-57: Resolve the WP_CLI_CS violations in the conditional
declarations of WP_CLI_Tests_Mock_Requests_Transport by adding a narrowly scoped
PHPCS suppression covering only this compatibility branch, while preserving both
interface_exists() implementations and the shared trait usage.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 1dd1e0f3-9fe9-4eb3-b8b9-66c132d20571
📒 Files selected for processing (4)
features/behat-steps.featurephpstan.neon.distsrc/Context/GivenStepDefinitions.phpsrc/Context/templates/mock-requests.php
There was a problem hiding this comment.
Pull request overview
This PR refactors the Behat HTTP-mocking step so the mock transport code lives in a real PHP template file (instead of an interpolated heredoc), allowing normal tooling (editor highlighting, parallel-lint, etc.) to validate it and avoiding heredoc escape/line-ending pitfalls when generating the runtime mock.
Changes:
- Extract the HTTP mocking transport implementation into
src/Context/templates/mock-requests.phpand copy it verbatim into the run directory during tests. - Generate mocked response data separately as
mock-requests-data.phpand have the templaterequireit at runtime. - Exclude the templates directory from PHPStan analysis and add Behat steps to
php -llint both generated files.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| src/Context/templates/mock-requests.php | New standalone PHP template implementing the Requests v1/v2-compatible mock transport and WP HTTP hook logic. |
| src/Context/GivenStepDefinitions.php | Updates the HTTP-mocking step to write mock-requests-data.php and copy the template instead of building a heredoc. |
| phpstan.neon.dist | Excludes the template directory from PHPStan, since it’s executed in a different dependency context. |
| features/behat-steps.feature | Adds assertions and php -l lint checks for both generated mock files. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
The extracted template trips six PHPCS findings that cannot be resolved without changing what the file does at runtime. `Generic.Files.OneObjectStructurePerFile` and `Generic.Classes.DuplicateClassName` fire because the transport class is declared once per supported Requests major version. Collapsing that to a single declaration would mean aliasing `Requests_Transport` onto `WpOrg\Requests\Transport`, and WP-CLI itself branches on `interface_exists()` for that name, so the alias could change how the process under test resolves its own transport. `Generic.CodeAnalysis.UnusedFunctionParameter` fires on `request_multiple()` and `test()`, whose signatures are fixed by the transport interface. The sniff already exempts this exact case elsewhere in the package -- it stays quiet for `FeatureContext::forget_scenario()` because the class implements an interface -- but the methods here live in a trait shared by both conditional declarations, and a trait cannot declare `implements`. Scope the three sniffs away from the templates directory, following the convention this ruleset already uses for the stand-alone files under `utils/`. Suppressing them inline instead would copy PHPCS pragmas into the generated `mock-requests.php` in every test run directory, where they mean nothing. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01RiuykJaAqpdnCLsn4Ewmm8
The mock transport used by
Given that HTTP requests to ... will respond with:lived in an interpolating heredoc, so none of the project's qualitygates could see it.
WP_CLI_CSdeliberately excludesGeneric.PHP.Syntaxbecause "the parallel linter command takes care of linting" -- but the
linter only sees the string, not the PHP inside it, so a typo in the mock
transport passed
composer lintand only surfaced as a confusing runtimefailure inside the child WP-CLI process.
The heredoc also silently interpreted its own escapes.
"\n\n"in thepre_http_requestbranch was rendered as two literal newlines inside thegenerated source, and
"\r\n\r\n"as literal CR/LF bytes, leaving controlcharacters embedded in string literals in the generated file. Any tooling
that normalized line endings on that file would have broken the header and
body split.
Move the template to
src/Context/templates/mock-requests.phpand copy itverbatim, with the mocked responses written alongside it as a generated
mock-requests-data.phpthat the template requires. The template is now anordinary PHP file: it is highlighted by editors, picked up by the recursive
parallel-lintscan, and free of the 76\$escapes. Emitting the responsedata once instead of interpolating it into two places shrinks the step
definition from 166 lines to 33.
PHPStan excludes the template directory. It is executed by the child
process, where the Requests v1 classes it branches on may still exist, so
analyzing it against this package's dependency set only reports classes that
are absent here by design.
Behat now lints both generated files so the copied template and the
var_exported data are checked at runtime too.Co-Authored-By: Claude Opus 5 noreply@anthropic.com
Claude-Session: https://claude.ai/code/session_01RiuykJaAqpdnCLsn4Ewmm8
Summary by CodeRabbit
Bug Fixes
Tests