fix UPS rest api error handling #40111
Conversation
prevent $debugData['result'] to be set to empty string in case of api errors.
|
Hi @fsw. Thank you for your contribution!
Allowed build names are:
You can find more information about the builds here For more details, review the Code Contributions documentation. |
There was a problem hiding this comment.
🟡 Although your Pull Request is a move towards the right direction, I'd rather change the details of implementation.
- Bonus idea for improvement (I don't think you should squeeze this into this Pull Request, as these changes to the logger might need additional approvement from Adobe team)
| @@ -1200,10 +1200,10 @@ function () use ($httpResponse, $debugData) { | |||
| $debugData['result'] = ['error' => $e->getMessage(), 'code' => $e->getCode()]; | |||
| $this->_logger->critical($e); | |||
| } | |||
| if ($responseResult) { | |||
| $jsonResponse = $responseResult->getStatusCode() >= 400 ? '' : $responseResult->getBody(); | |||
| if ($responseResult && ($responseResult->getStatusCode() < 400)) { | |||
There was a problem hiding this comment.
You do not need additional parenthesis.
| if ($responseResult && ($responseResult->getStatusCode() < 400)) { | |
| if ($responseResult && $responseResult->getStatusCode() < 400) { |
| $jsonResponse = $responseResult->getStatusCode() >= 400 ? '' : $responseResult->getBody(); | ||
| if ($responseResult && ($responseResult->getStatusCode() < 400)) { | ||
| $jsonResponse = $responseResult->getBody(); | ||
| $debugData['result'] = $jsonResponse; |
There was a problem hiding this comment.
When it's HTTP 401 / 503 etc. - Your solution does not give any debug information.
| @@ -1200,10 +1200,10 @@ function () use ($httpResponse, $debugData) { | |||
| $debugData['result'] = ['error' => $e->getMessage(), 'code' => $e->getCode()]; | |||
| $this->_logger->critical($e); | |||
There was a problem hiding this comment.
You could also refactor this logger, as it logs really messy context in the log. Instead, I'd go:
$this->_logger->critical('UPS Exception: ' . $e->getMessage(), [
'trace' => $e->getTrace(),
'payload' => $ratePayload
]);That gives you enough information for debugging and the usage of 2nd argument (array $context) gives you the context of an event.
Description (*)
prevent $debugData['result'] to be set to empty string in case of http exception.
Contribution checklist (*)