Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork9.7k
[Notifier] [Expo] Throw exception on error-response from expo api#47626
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
[Notifier] [Expo] Throw exception on error-response from expo api#47626
Uh oh!
There was an error while loading.Please reload this page.
Conversation
carsonbot commentedSep 20, 2022
Hey! I see that this is your first PR. That is great! Welcome! Symfony has acontribution 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 returnedfabpot commentedSep 20, 2022
Does it mean that the current way to deal with errors is wrong? Can we fix that instead? |
sdrewergutland commentedSep 21, 2022
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 symfony/src/Symfony/Component/Notifier/Bridge/AllMySms/AllMySmsTransport.php Lines 90 to 92 in3633475
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. |
fabpot commentedSep 21, 2022
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 toc31dfd2Comparesdrewergutland commentedSep 22, 2022
Alright, thanks for the feedback & PR is updated. |
messageId only if an id is returnedUh oh!
There was an error while loading.Please reload this page.
| $isError ='error' ===$success['data']['status']; | ||
| if ($isError) { | ||
| thrownewTransportException(sprintf('Unable to post the Expo message: "%s" (%s)',$success['data']['message'],$success['data']['details']['error']),$response); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
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
Uh oh!
There was an error while loading.Please reload this page.
OskarStark left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Looks good to me, after my comment
74dde0e to90638acComparefabpot commentedSep 30, 2022
Thank you@sdrewergutland. |
Uh oh!
There was an error while loading.Please reload this page.
Throw an actual
TransportExceptionin the case that an error is returned in the response from expo api.