[Notifier] [Expo] Throw exception on error-response from expo api #47626
Conversation
|
Hey! I see that this is your first PR. That is great! Welcome! Symfony has a contribution guide which I suggest you to read. In short:
Review the GitHub status checks of your pull request and try to solve the reported issues. If some tests are failing, try to see if they are failing because of this change. When two Symfony core team members approve this change, it will be merged and you will become an official Symfony contributor! I am going to sit back now and wait for the reviews. Cheers! Carsonbot |
messageId only if an id is returned
|
Does it mean that the current way to deal with errors is wrong? Can we fix that instead? |
|
Yes, I suppose that would be another option; that would make it more consistent with how other transports handle errors that are not Connection/Authorization based. So a solution in the manner of But that would cause a BC break I assume since now receiving an Exception when a device is not registered is very different behaviour. But if that is acceptable I would close this PR and open one with the correct description and implementation of the solution then. |
|
But IIUC, right now, the message is not sent and you don't get any error. So, that's a bug that should be fixed. |
62966bb to
c31dfd2
Compare
|
Alright, thanks for the feedback & PR is updated. |
messageId only if an id is returned| @@ -93,6 +93,11 @@ protected function doSend(MessageInterface $message): SentMessage | |||
|
|
|||
| $success = $response->toArray(false); | |||
|
|
|||
| $isError = 'error' === $success['data']['status']; | |||
| if ($isError) { | |||
| throw new TransportException(sprintf('Unable to post the Expo message: "%s" (%s)', $success['data']['message'], $success['data']['details']['error']), $response); | |||
There was a problem hiding this comment.
Would you have an example of a resulting message?
Maybe a link to the doc about this structure also?
There was a problem hiding this comment.
Added a link to the API docs in the transport;
And as for an response for an error case:
{
"data": {
"status": "error",
"message": "\"ExponentPushToken[xxxxxxxxxxxxxxxxxxxxxx]\" is not a registered push notification recipient",
"details": {
"error": "DeviceNotRegistered",
"expoPushToken": "ExponentPushToken[xxxxxxxxxxxxxxxxxxxxxx]"
}
}
}This can be reproduced with
curl -H "Content-Type: application/json" -X POST "https://exp.host/--/api/v2/push/send" -d '{
"to": "ExponentPushToken[xxxxxxxxxxxxxxxxxxxxxx]",
"title":"hello",
"body": "world"
}'Any other possible error like invalid auth tokens etc. would be handled by a non 200 http status. AFAIK this is the only case that is still an error but would come back as HTTP/200
74dde0e to
90638ac
Compare
|
Thank you @sdrewergutland. |
Throw an actual
TransportExceptionin the case that an error is returned in the response from expo api.