-
Notifications
You must be signed in to change notification settings - Fork 3
Support JSON string body in fetch #15
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
WalkthroughThe Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20–25 minutes
Pre-merge checks and finishing touches✅ Passed checks (3 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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/Client.php (1)
278-285: String bodies correctly bypass encoding, but form-urlencoded array handling is incorrect.The condition correctly restricts encoding to array bodies. String bodies pass through unmodified to curl, allowing pre-serialized JSON and GraphQL queries to work as intended.
However, there is a bug with
application/x-www-form-urlencoded: when an array body is passed,flatten()returns an array, which curl will then encode asmultipart/form-databy default. This contradicts the user-specified content-type header. The form-urlencoded case should usehttp_build_query()to convert the flattened array to a URL-encoded string, matching the JSON case which correctly converts arrays to strings. Currently, only JSON properly converts array → string; form-urlencoded converts array → array (which curl re-encodes as multipart).
🧹 Nitpick comments (1)
src/Client.php (1)
258-258: Simplify the docblock type annotation.The type annotation
array<string>|array<string, mixed>|string|nullis overly specific. Sincearray<string>is a subset ofarray<string, mixed>, listing both is redundant.🔎 Apply this diff to simplify the type annotation:
- * @param array<string>|array<string, mixed>|string|null $body + * @param array<mixed>|string|null $body
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/Client.php(3 hunks)tests/ClientTest.php(3 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
tests/ClientTest.php (1)
src/Client.php (1)
Client(9-447)
🔇 Additional comments (4)
tests/ClientTest.php (3)
16-16: LGTM!The docblock update correctly reflects the new parameter type that accepts string bodies in addition to arrays.
56-56: LGTM!The updated assertion logic correctly handles both string and array bodies, comparing string bodies directly without re-encoding.
310-328: LGTM!The new test cases effectively cover sending pre-serialized JSON strings as request bodies, testing both single-line and multi-line formats. This aligns well with the updated
fetchmethod signature.src/Client.php (1)
268-268: LGTM!The method signature correctly accepts
array|string|nullfor the body parameter, enabling the new functionality while maintaining backward compatibility with the default empty array.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
This PR adds support for passing JSON string bodies directly to the fetch() method, eliminating the need to always use array format. Previously, the method only accepted arrays for the body parameter, which would then be encoded to JSON. Now developers can pass pre-encoded JSON strings directly.
Key changes:
- Updated type signatures to accept
array|string|nullfor body parameters - Modified body processing logic to skip array transformation when a string is provided
- Added test coverage for both single-line and multi-line JSON string bodies
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| src/Client.php | Updated fetch() method to accept string bodies and modified the body processing condition to only transform arrays, allowing strings to pass through unchanged |
| tests/ClientTest.php | Updated test method signature and assertion logic to handle string bodies, and added two new test cases for single-line and multi-line JSON string bodies |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Related SER-728
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.