Coerce non-string API error bodies to string in ApiResult::getError() - #56
Open
spencerkittleson wants to merge 1 commit into
Open
Coerce non-string API error bodies to string in ApiResult::getError()#56spencerkittleson wants to merge 1 commit into
spencerkittleson wants to merge 1 commit into
Conversation
getError() is declared ": ?string" but could return non-string scalars (bool/int/float) deserialized straight out of the API response body, which throws a TypeError under strict_types instead of returning a value. This TypeError is not an Exception, so it isn't caught by the try/catch blocks that construct ApiResultException, turning a handled API error into an uncaught fatal for the current request. Coerce every returned value with StringEx::stringify() so getError() always honors its declared ?string contract regardless of what shape the API serialized. Deki issue: MindTouch/Deki#38741
spencerkittleson
requested review from
luis-andrade-incontact and
tkaeg
and
a lite review from Copilot
August 29, 2026 16:21
There was a problem hiding this comment.
Pull request overview
This pull request fixes a strict typing contract violation in ApiResult::getError() by ensuring any non-string scalar values returned from deserialized API error bodies are coerced to strings, preventing uncaught TypeError fatals under declare(strict_types=1).
Changes:
- Update
ApiResult::getError()to coerce returned error/body values viaStringEx::stringify()to always satisfy the declared?stringreturn type. - Extend
getError_Testdata provider and test signature to cover boolean, integer, and float body/error message cases that previously would have thrownTypeError.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| src/ApiResult.php | Coerces non-string API error/body values to strings before returning from getError(). |
| tests/ApiResult/getError_Test.php | Adds regression coverage for non-string scalar error/body values under strict typing. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Issue: https://github.com/MindTouch/Deki/issues/38741
Fixes MindTouch/Deki#38741
ELI5
getError()promises callers a string (or nothing), but it was handing back whatever the API actually sent — sometimes a boolean or a number instead of text. Under PHP's strict mode, breaking that promise doesn't just give the wrong answer, it crashes immediately with a fatal error that the normal error-handling code around it can't catch. This fix makesgetError()always convert what it finds into an actual string before handing it back, so it can no longer break its own promise.Summary
ApiResult::getError()is declared: ?stringbut its implementation returned deserialized API response body values (body/error/message,body/exception/message, or the rawbodyfallback) as-is without checking their type. Underdeclare(strict_types=1), PHP enforces the return type at thereturnstatement, so a non-string scalar (bool/int/float) at any of these paths throws aTypeErrorinstead of returning a value.This
TypeErroris a\Error, not an\Exception, so it is not caught by thetry/catch (ApiResultException $e)/catch (Exception $e)blocks callers use around API calls — turning a normal "API returned an error" case into an uncaught fatal error for the current request. Worse, this happens while constructingApiResultExceptioninApiPlug::invokeComplete(), soApiResultException's own defensiveis_string($error) ? $error : 'unknown api error'check never runs — theTypeErrorfires from insidegetError()before control ever reaches the constructor body.Seen in production as ~5k occurrences since May 2026 in deki-web (Rollbar item deki-web/6531).
Fix
Coerce every returned value with
StringEx::stringify()(already used elsewhere in this class) sogetError()always honors its declared?stringcontract regardless of what type the API serialized. The array branch (is_array($error)→getXml('body')) and the curl-error branch (already?string-safe) are unchanged.Tests
Extended
getError_Test::body_expected_dataProvider()with cases covering a boolean message, an integer message, and a float body — all previously would have thrown aTypeError, now correctly coerce to their string representation.Follow-up
Once merged,
web/composer.lockin MindTouch/Deki needs to be bumped to pick up this fix (package is pinned todev-main-php82).