Issue/566 native curl adapter - #568
Conversation
|
Warning Review limit reached
Next review available in: 40 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. Review details⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthrough
ChangesNative single-request CurlAdapter
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant HttpClient
participant CurlAdapter
participant NativePHPcURL
HttpClient->>CurlAdapter: configure request
CurlAdapter->>NativePHPcURL: execute native cURL request
NativePHPcURL-->>CurlAdapter: return response and error state
CurlAdapter-->>HttpClient: expose parsed response state
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 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! |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: be7732ced4
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/HttpClient/Adapters/CurlAdapter.php (2)
134-151: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winStore request headers case-insensitively.
Different casing currently creates duplicate headers and can make
hasJsonContentType()use a superseded value. Normalize names before assignment.Proposed fix
public function setHeader(string $key, $value): CurlAdapterInterface { - $this->headers[$key] = $value; + $this->headers[strtolower(trim($key))] = $value; $this->applyHeaders(); return $this; } @@ - $this->headers[trim((string) $key)] = trim((string) $value); + $this->headers[strtolower(trim((string) $key))] = trim((string) $value);🤖 Prompt for 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. In `@src/HttpClient/Adapters/CurlAdapter.php` around lines 134 - 151, Normalize header names case-insensitively before storing them in both setHeader and setHeaders, using a consistent lowercase representation so differently cased names overwrite the same entry. Preserve the existing value trimming behavior in setHeaders and ensure applyHeaders continues using the normalized headers.
101-151: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winForward request setters to the injected client.
src/HttpClient/Adapters/CurlAdapter.phpstill writes URL/header/option state only to the adapter’s native handle. In bridge mode,start()executes$this->client->exec(), so those settings never reach the client that actually runs. Add a regression intests/Unit/HttpClient/Adapters/CurlAdapterTest.phpforsetUrl()and representative header/option forwarding.🤖 Prompt for 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. In `@src/HttpClient/Adapters/CurlAdapter.php` around lines 101 - 151, The setter methods setUrl(), setOpt(), setOpts(), setHeader(), and setHeaders() must forward applied URL, option, and header values to the injected client used by start(), while preserving native-handle behavior. Update CurlAdapter.php accordingly, and add regression coverage in tests/Unit/HttpClient/Adapters/CurlAdapterTest.php at lines 135-136 for setUrl() plus representative header and option forwarding.
🤖 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/HttpClient/Adapters/CurlAdapter.php`:
- Around line 369-386: Update parseResponse to handle gzip Content-Encoding
before content-type deserialization: detect the encoding case-insensitively,
decompress into $response when successful, and retain the raw response on
failure. Then pass $response—not $rawResponse—to json_decode() and
simplexml_load_string(), preserving existing fallback behavior.
- Around line 214-218: Update the error-message assignment in CurlAdapter’s
response handling so $this->errorMessage is populated only when $this->error is
true; retain the cURL error details for cURL failures and the status-line
fallback for HTTP errors, while leaving it null on successful requests.
- Around line 319-323: Update parseCookieHeader to match and store empty cookie
values in Set-Cookie headers, including deletion headers such as sid=;
Max-Age=0. Adjust the value capture so the cookie entry is updated with an empty
string instead of retaining a stale earlier value, while preserving existing
decoding and trimming behavior.
- Around line 379-381: Update the XML parsing branch in CurlAdapter to enable
libxml internal errors before calling simplexml_load_string(), then restore the
previous libxml error setting afterward even when parsing fails. Preserve the
existing SimpleXMLElement return and raw response fallback behavior.
---
Outside diff comments:
In `@src/HttpClient/Adapters/CurlAdapter.php`:
- Around line 134-151: Normalize header names case-insensitively before storing
them in both setHeader and setHeaders, using a consistent lowercase
representation so differently cased names overwrite the same entry. Preserve the
existing value trimming behavior in setHeaders and ensure applyHeaders continues
using the normalized headers.
- Around line 101-151: The setter methods setUrl(), setOpt(), setOpts(),
setHeader(), and setHeaders() must forward applied URL, option, and header
values to the injected client used by start(), while preserving native-handle
behavior. Update CurlAdapter.php accordingly, and add regression coverage in
tests/Unit/HttpClient/Adapters/CurlAdapterTest.php at lines 135-136 for setUrl()
plus representative header and option forwarding.
🪄 Autofix (Beta)
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: 8902f2d0-6795-46db-8ec0-cac9c69329c0
📒 Files selected for processing (4)
CHANGELOG.mdsrc/HttpClient/Adapters/CurlAdapter.phptests/Unit/HttpClient/Adapters/CurlAdapterTest.phptests/Unit/HttpClient/HttpClientTest.php
💤 Files with no reviewable changes (1)
- tests/Unit/HttpClient/HttpClientTest.php
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 `@tests/Unit/HttpClient/Adapters/CurlAdapterTest.php`:
- Around line 206-217: Update tests/Unit/HttpClient/Adapters/CurlAdapterTest.php
lines 206-217 to execute CurlAdapter::start() without an injected client and
assert native success, null error state, and the expected response behavior
instead of setting private fields directly. Update
tests/Unit/HttpClient/HttpClientTest.php lines 111-138 to add a facade test
using createRequest($url) without a vendor client and verify the response
contract.
🪄 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: f0b0c145-40bd-45cc-b799-5e94344eeb0f
📒 Files selected for processing (5)
src/HttpClient/Adapters/CurlAdapter.phpsrc/HttpClient/ResponseHeaders.phptests/Unit/HttpClient/Adapters/CurlAdapterTest.phptests/Unit/HttpClient/HttpClientTest.phptests/Unit/HttpClient/ResponseHeadersTest.php
🚧 Files skipped from review as they are similar to previous changes (1)
- src/HttpClient/Adapters/CurlAdapter.php
3 participants
Closes #566
Summary by CodeRabbit
Changed
Tests