feat: enhance logging error handling with Push exception#46
Open
feat: enhance logging error handling with Push exception#46
Conversation
- Updated `composer.json` to require `utopia-php/console`. - Modified `Logger` class to catch `Push` exceptions during log pushes and log detailed error messages. - Updated `Adapter` implementations (AppSignal, LogOwl, Raygun, Sentry) to throw `Push` exceptions on failure instead of returning error codes.
WalkthroughThe pull request introduces exception-based error handling to the logger adapter system. A new Estimated code review effort🎯 3 (Moderate) | ⏱️ ~30 minutes 🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (5)
src/Logger/Adapter/LogOwl.php (1)
168-174: Same error-message issues asSentry.php—$curlErroris always0in the HTTP error branch, and$responseisfalse(→ empty string) in the curl error branch. Apply the same fix described inSentry.php.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/Logger/Adapter/LogOwl.php` around lines 168 - 174, The error messages in LogOwl.php use $curlError and $response incorrectly (they can be 0/false) causing misleading logs; update the push error handling in the same place that throws Push to capture and use the actual curl errno and curl_error() values and ensure $response is stringified (e.g., convert false to an explanatory string or cast to string) so the thrown messages include the real curl errno, curl error string, HTTP status and the response body; apply this change in the same conditional branches that currently reference $curlError and $response so the thrown Push exceptions contain accurate diagnostic details.composer.json (1)
48-48:vimeo/psalmis pinned to a very old version and is unused inscripts.
"vimeo/psalm": "4.0.1"dates to 2020 (current Psalm is 5.x). More importantly, thescripts.checkentry invokesphpstan, notpsalm, so this package is never actually called in the CI pipeline. Either update the version and wire it into a script, or remove it to avoid the vestigial dependency.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@composer.json` at line 48, composer.json currently declares "vimeo/psalm": "4.0.1" but that old pin is unused by the scripts; either update the dependency to a current Psalm v5 constraint (e.g. "^5.0") and add a scripts entry that runs Psalm (e.g. add or modify "scripts": { "check:psalm": "vendor/bin/psalm" } or call it from the existing "scripts.check"), or remove "vimeo/psalm" entirely from composer.json and keep CI using phpstan only; adjust the "scripts.check" entry accordingly so the declared dependency matches what CI actually runs.src/Logger/Adapter/Sentry.php (1)
178-184: Error message clarity: misleading$curlErrorin HTTP branch and empty$responsein curl branch.Two issues with the exception messages:
- Line 183 — HTTP error branch: At this point
$curlErroris alwaysCURLE_OK(0) because the preceding guard already filtered out non-zero curl errors. Interpolating{$curlError}produces messages like"status code 429: 0 (body)"where the0is confusing.- Line 179 — curl error branch:
curl_execreturnsfalseon failure, so{$response}interpolates to an empty string, making the message read"...curl error (6): ". Usecurl_strerror($curlError)for a human-readable description (it takes only the code, no handle required).The same pattern appears identically in
AppSignal.php,LogOwl.php, andRaygun.php.✏️ Proposed fix
- if ($curlError !== CURLE_OK || $httpCode === 0) { - throw new Push("Sentry push failed with curl error ({$curlError}): {$response}", 500); - } - - if ($httpCode >= 400) { - throw new Push("Sentry push failed with status code {$httpCode}: {$curlError} ({$response})", $httpCode); - } + if ($curlError !== CURLE_OK || $httpCode === 0) { + throw new Push("Sentry push failed with curl error ({$curlError}): ".\curl_strerror($curlError), 500); + } + + if ($httpCode >= 400) { + throw new Push("Sentry push failed with status code {$httpCode}: {$response}", $httpCode); + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/Logger/Adapter/Sentry.php` around lines 178 - 184, Exception messages mix up curl error code and HTTP response and can be empty; update the throw texts in Sentry.php's send/push logic (the branch using $curlError, $httpCode, $response that currently throws Push) to: for curl failures (when $curlError !== CURLE_OK || $httpCode === 0) include curl_strerror($curlError) as the human-readable error and the raw $response only if non-empty; for HTTP errors (when $httpCode >= 400) remove $curlError from the message and include the HTTP status code plus the response body (or a trimmed indicator if empty) so the message reads e.g. "Sentry push failed with status code {code}: {response}". Apply the same change to the identical throw sites in AppSignal.php, LogOwl.php, and Raygun.php, and keep the thrown exception type Push and original status code values.src/Logger/Adapter/AppSignal.php (1)
151-157: Same error-message issues asSentry.php—$curlErroris always0in the HTTP error branch, and$responseisfalse(→ empty string) in the curl error branch. Apply the same fix described inSentry.php.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/Logger/Adapter/AppSignal.php` around lines 151 - 157, The error messages use the wrong variables: $curlError (numeric) is meaningless in the HTTP-error branch and $response may be false in the curl-error branch; update AppSignal's push/error handling to mirror the Sentry.php fix by building a proper curl error string and a safe response string before throwing Push. Concretely, in the AppSignal sending code (the block that currently checks $curlError and $httpCode and throws new Push(...)), compute a human-readable curl error (e.g. from curl_error or a curl_strerror helper) and coerce $response to a string only when truthy, then throw Push with the curl-error message when curl failed (include the curl error string and any response if present) and throw Push with the HTTP status and the safe response when $httpCode >= 400 (do not rely on the numeric $curlError there).src/Logger/Adapter/Raygun.php (1)
136-142: Same error-message issues asSentry.php—$curlErroris always0in the HTTP error branch, and$responseisfalse(→ empty string) in the curl error branch. Apply the same fix described inSentry.php.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/Logger/Adapter/Raygun.php` around lines 136 - 142, The Raygun adapter builds error messages using stale/empty variables; update the error handling in Raygun.php (around the Push throws) to read the real curl status and text from the curl handle instead of relying on $curlError/$response as-is: call curl_errno($ch) and curl_error($ch) to get the numeric and textual curl error and include those in the curl-failure branch, and for the HTTP-status branch include the actual $response body (cast or json-encoded if needed) and the http code; keep throwing the same Push exceptions but substitute $curlError/$response with the freshly retrieved curl_errno/curl_error and a safe stringified $response so messages correctly reflect the underlying error.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/Logger/Logger.php`:
- Around line 74-82: The json_encode results for $tags and $extra can be false,
causing strlen/substr to fail; update the Logger logic around
getTags()/getExtra() so you capture the json_encode return into a variable (e.g.
$tagsJson and $extraJson), check is_string($tagsJson)/is_string($extraJson) (or
coerce with (string) only after handling false) before concatenating or calling
strlen/substr, and default to an empty string or a clear fallback when
json_encode returns false so Console::error always receives a string and PHPStan
type errors are resolved.
---
Nitpick comments:
In `@composer.json`:
- Line 48: composer.json currently declares "vimeo/psalm": "4.0.1" but that old
pin is unused by the scripts; either update the dependency to a current Psalm v5
constraint (e.g. "^5.0") and add a scripts entry that runs Psalm (e.g. add or
modify "scripts": { "check:psalm": "vendor/bin/psalm" } or call it from the
existing "scripts.check"), or remove "vimeo/psalm" entirely from composer.json
and keep CI using phpstan only; adjust the "scripts.check" entry accordingly so
the declared dependency matches what CI actually runs.
In `@src/Logger/Adapter/AppSignal.php`:
- Around line 151-157: The error messages use the wrong variables: $curlError
(numeric) is meaningless in the HTTP-error branch and $response may be false in
the curl-error branch; update AppSignal's push/error handling to mirror the
Sentry.php fix by building a proper curl error string and a safe response string
before throwing Push. Concretely, in the AppSignal sending code (the block that
currently checks $curlError and $httpCode and throws new Push(...)), compute a
human-readable curl error (e.g. from curl_error or a curl_strerror helper) and
coerce $response to a string only when truthy, then throw Push with the
curl-error message when curl failed (include the curl error string and any
response if present) and throw Push with the HTTP status and the safe response
when $httpCode >= 400 (do not rely on the numeric $curlError there).
In `@src/Logger/Adapter/LogOwl.php`:
- Around line 168-174: The error messages in LogOwl.php use $curlError and
$response incorrectly (they can be 0/false) causing misleading logs; update the
push error handling in the same place that throws Push to capture and use the
actual curl errno and curl_error() values and ensure $response is stringified
(e.g., convert false to an explanatory string or cast to string) so the thrown
messages include the real curl errno, curl error string, HTTP status and the
response body; apply this change in the same conditional branches that currently
reference $curlError and $response so the thrown Push exceptions contain
accurate diagnostic details.
In `@src/Logger/Adapter/Raygun.php`:
- Around line 136-142: The Raygun adapter builds error messages using
stale/empty variables; update the error handling in Raygun.php (around the Push
throws) to read the real curl status and text from the curl handle instead of
relying on $curlError/$response as-is: call curl_errno($ch) and curl_error($ch)
to get the numeric and textual curl error and include those in the curl-failure
branch, and for the HTTP-status branch include the actual $response body (cast
or json-encoded if needed) and the http code; keep throwing the same Push
exceptions but substitute $curlError/$response with the freshly retrieved
curl_errno/curl_error and a safe stringified $response so messages correctly
reflect the underlying error.
In `@src/Logger/Adapter/Sentry.php`:
- Around line 178-184: Exception messages mix up curl error code and HTTP
response and can be empty; update the throw texts in Sentry.php's send/push
logic (the branch using $curlError, $httpCode, $response that currently throws
Push) to: for curl failures (when $curlError !== CURLE_OK || $httpCode === 0)
include curl_strerror($curlError) as the human-readable error and the raw
$response only if non-empty; for HTTP errors (when $httpCode >= 400) remove
$curlError from the message and include the HTTP status code plus the response
body (or a trimmed indicator if empty) so the message reads e.g. "Sentry push
failed with status code {code}: {response}". Apply the same change to the
identical throw sites in AppSignal.php, LogOwl.php, and Raygun.php, and keep the
thrown exception type Push and original status code values.
Comment on lines
+74
to
+82
| $tags = $log->getTags(); | ||
| if ($tags !== []) { | ||
| Console::error('tags: '.\json_encode($tags)); | ||
| } | ||
| $extra = $log->getExtra(); | ||
| if ($extra !== []) { | ||
| $extraJson = \json_encode($extra); | ||
| Console::error('extra: '.(\strlen($extraJson) > 2000 ? \substr($extraJson, 0, 2000).'...' : $extraJson)); | ||
| } |
There was a problem hiding this comment.
Pipeline failure: json_encode() returns string|false but strlen/substr require string.
PHPStan reports 2 errors on line 81 (strlen and substr both reject string|false), blocking the pipeline. json_encode($extra) returns false when encoding fails; passing that value directly to strlen/substr violates their type contract.
Line 76 has the same root cause — \json_encode($tags) concatenated directly without a false guard — but PHPStan's concatenation analysis let it pass at the current level. Both should be fixed together.
🐛 Proposed fix
- $tags = $log->getTags();
- if ($tags !== []) {
- Console::error('tags: '.\json_encode($tags));
- }
- $extra = $log->getExtra();
- if ($extra !== []) {
- $extraJson = \json_encode($extra);
- Console::error('extra: '.(\strlen($extraJson) > 2000 ? \substr($extraJson, 0, 2000).'...' : $extraJson));
- }
+ $tags = $log->getTags();
+ if ($tags !== []) {
+ Console::error('tags: '.(\json_encode($tags) ?: '[]'));
+ }
+ $extra = $log->getExtra();
+ if ($extra !== []) {
+ $extraJson = \json_encode($extra) ?: '{}';
+ Console::error('extra: '.(\strlen($extraJson) > 2000 ? \substr($extraJson, 0, 2000).'...' : $extraJson));
+ }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| $tags = $log->getTags(); | |
| if ($tags !== []) { | |
| Console::error('tags: '.\json_encode($tags)); | |
| } | |
| $extra = $log->getExtra(); | |
| if ($extra !== []) { | |
| $extraJson = \json_encode($extra); | |
| Console::error('extra: '.(\strlen($extraJson) > 2000 ? \substr($extraJson, 0, 2000).'...' : $extraJson)); | |
| } | |
| $tags = $log->getTags(); | |
| if ($tags !== []) { | |
| Console::error('tags: '.(\json_encode($tags) ?: '[]')); | |
| } | |
| $extra = $log->getExtra(); | |
| if ($extra !== []) { | |
| $extraJson = \json_encode($extra) ?: '{}'; | |
| Console::error('extra: '.(\strlen($extraJson) > 2000 ? \substr($extraJson, 0, 2000).'...' : $extraJson)); | |
| } |
🧰 Tools
🪛 GitHub Actions: CodeQL
[error] 81-81: PHPStan: Found 2 errors. Parameter #1 $string of function strlen expects string, string|false given; Parameter #1 $string of function substr expects string, string|false given.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/Logger/Logger.php` around lines 74 - 82, The json_encode results for
$tags and $extra can be false, causing strlen/substr to fail; update the Logger
logic around getTags()/getExtra() so you capture the json_encode return into a
variable (e.g. $tagsJson and $extraJson), check
is_string($tagsJson)/is_string($extraJson) (or coerce with (string) only after
handling false) before concatenating or calling strlen/substr, and default to an
empty string or a clear fallback when json_encode returns false so
Console::error always receives a string and PHPStan type errors are resolved.
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
1 participant
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.
composer.jsonto requireutopia-php/console.Loggerclass to catchPushexceptions during log pushes and log detailed error messages.Adapterimplementations (AppSignal, LogOwl, Raygun, Sentry) to throwPushexceptions on failure instead of returning error codes.Summary by CodeRabbit
New Features
Bug Fixes
Chores