Fix response header parsing and content-type detection#87
Conversation
Fixed critical bugs in AbstractRequester response handling: 1. Fixed header value extraction - PSR-7 headers are arrays, now properly extracts first element 2. Fixed content-type parsing to handle parameters (e.g., "application/json; charset=utf-8") 3. Restored JSON-by-default parsing for backwards compatibility 4. Added support for both text/xml and application/xml content types This fixes all 17 test failures that were occurring due to response bodies being passed as strings instead of parsed arrays. Tests: All 112 tests now passing (was 95/112)
There was a problem hiding this comment.
Review by Korbit AI
Korbit automatically attempts to detect when you fix issues in new commits.
| Category | Issue | Status |
|---|---|---|
| JSON Default Behavior Context Missing ▹ view | ||
| Response Parser Logic Should Be Extracted ▹ view | ||
| Inadequate JSON parsing error detection ▹ view | ||
| JSON null responses incorrectly treated as parsing failures ▹ view | ||
| Content-Type Split Rationale Missing ▹ view |
Files scanned
| File Path | Reviewed |
|---|---|
| src/AbstractRequester.php | ✅ |
Explore our documentation to understand the languages and file types we support and the files we ignore.
Check out our docs on how you can make Korbit work best for you and your team.
| // Default to JSON parsing (for backwards compatibility and when content-type is not set) | ||
| $responseBodyParsed = json_decode($responseBodyStr, true); |
There was a problem hiding this comment.
JSON Default Behavior Context Missing 
Tell me more
What is the issue?
The comment mentions backwards compatibility but doesn't explain the historical context or why this default is important.
Why this matters
Without understanding the historical context, developers might change this default behavior, breaking existing integrations.
Suggested change ∙ Feature Preview
// Legacy APIs often return JSON without setting content-type. We default to JSON parsing
// to maintain compatibility with these APIs and our pre-v1.0 behavior where we assumed JSON
$responseBodyParsed = json_decode($responseBodyStr, true);
Provide feedback to improve future suggestions
💬 Looking for more details? Reply to this comment to chat with Korbit.
| if ($mainContentType === 'text/xml' || $mainContentType === 'application/xml') { | ||
| $responseBodyParsed = simplexml_load_string($responseBodyStr); | ||
| } else { | ||
| $responseBodyParsed = $responseBodyStr; | ||
| // Default to JSON parsing (for backwards compatibility and when content-type is not set) | ||
| $responseBodyParsed = json_decode($responseBodyStr, true); | ||
| // If JSON parsing fails and content-type is explicitly not JSON, use raw string | ||
| if ($responseBodyParsed === null && $mainContentType !== '' && $mainContentType !== 'application/json') { | ||
| $responseBodyParsed = $responseBodyStr; | ||
| } | ||
| } |
There was a problem hiding this comment.
Response Parser Logic Should Be Extracted 
Tell me more
What is the issue?
Response body parsing logic is embedded within the send() method, violating Single Responsibility Principle and making the code harder to maintain and extend for new content types.
Why this matters
This tightly coupled design makes it difficult to add support for new content types and complicates testing. It also increases the cognitive complexity of the send() method.
Suggested change ∙ Feature Preview
Create a dedicated ResponseParser class or service:
class ResponseParser {
public function parse(string $body, string $contentType): mixed {
$mainContentType = $this->extractMainContentType($contentType);
return match($mainContentType) {
'text/xml', 'application/xml' => $this->parseXml($body),
'application/json', '' => $this->parseJson($body),
default => $this->parseDefault($body)
};
}
}Provide feedback to improve future suggestions
💬 Looking for more details? Reply to this comment to chat with Korbit.
| if ($responseBodyParsed === null && $mainContentType !== '' && $mainContentType !== 'application/json') { | ||
| $responseBodyParsed = $responseBodyStr; | ||
| } |
There was a problem hiding this comment.
JSON null responses incorrectly treated as parsing failures 
Tell me more
What is the issue?
The JSON parsing fallback logic incorrectly treats valid JSON null responses as parsing failures, causing them to be returned as raw strings instead of null values.
Why this matters
When an API legitimately returns JSON null (which is valid JSON), json_decode() returns PHP null, but the condition treats this as a parsing failure. This breaks APIs that intentionally return null values and violates the principle of least surprise for JSON API consumers.
Suggested change ∙ Feature Preview
Use json_last_error() to properly detect JSON parsing failures instead of checking for null return value:
// Default to JSON parsing (for backwards compatibility and when content-type is not set)
$responseBodyParsed = json_decode($responseBodyStr, true);
// If JSON parsing fails and content-type is explicitly not JSON, use raw string
if (json_last_error() !== JSON_ERROR_NONE && $mainContentType !== '' && $mainContentType !== 'application/json') {
$responseBodyParsed = $responseBodyStr;
}Provide feedback to improve future suggestions
💬 Looking for more details? Reply to this comment to chat with Korbit.
| // Extract the main content type (before semicolon) to handle "application/json; charset=utf-8" | ||
| $mainContentType = explode(';', $contentType)[0]; | ||
| $mainContentType = trim($mainContentType); |
There was a problem hiding this comment.
Content-Type Split Rationale Missing 
Tell me more
What is the issue?
The comment only describes what the code does, not why this handling is necessary.
Why this matters
Without understanding why content type needs to be split, future developers might remove this code thinking it's unnecessary, leading to content type parsing issues.
Suggested change ∙ Feature Preview
// Content-Type headers may include parameters (e.g., charset) that interfere with type matching.
// Extract only the MIME type portion for accurate content type comparison
$mainContentType = explode(';', $contentType)[0];
$mainContentType = trim($mainContentType);
Provide feedback to improve future suggestions
💬 Looking for more details? Reply to this comment to chat with Korbit.
| $responseBodyParsed = json_decode($responseBodyStr, true); | ||
| // If JSON parsing fails and content-type is explicitly not JSON, use raw string | ||
| if ($responseBodyParsed === null && $mainContentType !== '' && $mainContentType !== 'application/json') { | ||
| $responseBodyParsed = $responseBodyStr; | ||
| } |
There was a problem hiding this comment.
Inadequate JSON parsing error detection 
Tell me more
What is the issue?
JSON parsing errors are only partially handled by checking for null return value, but json_decode can return null for valid JSON (null value) and doesn't distinguish between parsing errors and valid null responses.
Why this matters
This approach cannot differentiate between a valid JSON null response and a JSON parsing error, potentially masking actual parsing failures and leading to incorrect fallback behavior.
Suggested change ∙ Feature Preview
Use json_last_error() to properly detect JSON parsing errors:
$responseBodyParsed = json_decode($responseBodyStr, true);
// Check for JSON parsing errors, not just null return value
if (json_last_error() !== JSON_ERROR_NONE && $mainContentType !== '' && $mainContentType !== 'application/json') {
$responseBodyParsed = $responseBodyStr;
}Provide feedback to improve future suggestions
💬 Looking for more details? Reply to this comment to chat with Korbit.
Fixed critical bugs in AbstractRequester response handling:
This fixes all 17 test failures that were occurring due to response bodies being passed as strings instead of parsed arrays.
Tests: All 112 tests now passing (was 95/112)