Skip to content

Conversation

@hmacr
Copy link
Contributor

@hmacr hmacr commented Dec 18, 2025

Related SER-728

Summary by CodeRabbit

  • New Features
    • The HTTP client now accepts raw string bodies in addition to array format, providing greater flexibility when sending requests with string-based payloads.

✏️ Tip: You can customize this high-level summary in your review settings.

@coderabbitai
Copy link

coderabbitai bot commented Dec 18, 2025

Walkthrough

The fetch method in the Client class is updated to accept array|string|null for the $body parameter instead of only nullable arrays. The body handling logic is refactored to conditionally JSON-encode only array bodies; string bodies are passed through without encoding. Two new test cases are added using data providers to cover scenarios where raw JSON strings are sent as request bodies, with assertions adjusted to compare string bodies directly without JSON-encoding.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20–25 minutes

  • The type signature change from ?array to array|string|null alters the method's input contract and requires careful review of the body handling logic.
  • The conditional encoding logic (encoding only when body is an array) introduces a branching path that needs verification to ensure both array and string bodies are processed correctly.
  • New test cases cover string body scenarios, but edge cases (empty strings, invalid JSON strings, null handling) should be validated.
  • Potential impact on callers and downstream consumers of this method should be assessed to ensure compatibility.

Pre-merge checks and finishing touches

✅ Passed checks (3 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title 'Support JSON string body in fetch' clearly and concisely summarizes the main change: enabling the fetch method to accept JSON as a string body in addition to arrays.
Docstring Coverage ✅ Passed Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.
✨ Finishing touches
  • 📝 Generate docstrings
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment
  • Commit unit tests in branch ser-728

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

Copy link

@coderabbitai coderabbitai bot left a 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 as multipart/form-data by default. This contradicts the user-specified content-type header. The form-urlencoded case should use http_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|null is overly specific. Since array<string> is a subset of array<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

📥 Commits

Reviewing files that changed from the base of the PR and between c8cd9a5 and 33d461a.

📒 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 fetch method signature.

src/Client.php (1)

268-268: LGTM!

The method signature correctly accepts array|string|null for the body parameter, enabling the new functionality while maintaining backward compatibility with the default empty array.

Copy link

Copilot AI left a 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|null for 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.

@Meldiron Meldiron merged commit a96a010 into main Dec 18, 2025
10 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants