Movatterモバイル変換

[0]ホーム
URL:

Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

 Sign up
Appearance settings

[Notifier] Add Expo bridge#42414

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

Merged
fabpot merged 1 commit intosymfony:5.4fromzairigimad:expo-notifier
Oct 28, 2021
Merged

[Notifier] Add Expo bridge#42414

fabpot merged 1 commit intosymfony:5.4fromzairigimad:expo-notifier
Oct 28, 2021

Conversation

@zairigimad
Copy link
Contributor

@zairigimadzairigimad commentedAug 6, 2021•
edited
Loading

QA
Branch?5.4
Bug fix?no
New feature?yes
Deprecations?no
Ticketsno ticket
LicenseMIT

Expo makes implementing push notifications almost too easy. All the hassle with native device information and communicating with APNs (Apple Push Notification service) or FCM (Firebase Cloud Messaging) is taken care of behind the scenes, so that you can treat iOS and Android notifications the same, saving you time on the front-end, and back-end!

this PR will add the support ofExpo Notification as a bridge to Symfony

screenshot from a real application build with expo

2DA98E58-4F0A-4A89-B6CB-937878E00E4A

✏️ this is a work in progress I would love to hear your feedbacks to improve it.

ismail1432 and oussama-aitmi reacted with thumbs up emojimouadziani reacted with rocket emojiphcorp and oussama-aitmi reacted with eyes emoji
@carsonbot
Copy link

Hey!

To help keep things organized, we don't allow "Draft" pull requests. Could you please click the "ready for review" button or close this PR and open a new one when you are done?

Note that a pull request does not have to be "perfect" or "ready for merge" when you first open it. We just want it to be ready for a first review.

Cheers!

Carsonbot

@norkunas
Copy link
Contributor

Maybe push channel#39402 should be merged before? :)

OskarStark reacted with thumbs up emoji

@OskarStarkOskarStark added this to the5.4 milestoneAug 7, 2021
$jsonContents = 0 === strpos($contentType, 'application/json') ? $response->toArray(false) : null;

if (200 !== $statusCode) {
$errorMessage = $jsonContents ? $jsonContents['error']['message'] : $response->getContent(false);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

what if$jsonContents['error']['message'] does not exist?

Copy link
ContributorAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

if i understand well the official documentationhttps://docs.expo.dev/accounts/programmatic-access/#personal-account-personal-access-tokens
normally when the status code is not OK we are in error mode for expo and we should have the error payload as explained in the article.

Copy link
ContributorAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

Done, you are right I spent some time testing the possibles cases, I found out that when it's 401 the response is different, I updated this part of handling the exception.

Copy link
Member

@nicolas-grekasnicolas-grekas left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

random notes :)

zairigimad reacted with laugh emoji

throw new TransportException('Unable to post the Expo message: '.$errorMessage, $response);
}
if (isset($jsonContents['error']['message'])) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

Is it possible to get a 200 with errors in the response? If not, the 1st check is enough.

Copy link
ContributorAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

you are right I did revisit the API documentation again, I will delete this check the first one is enough. good catch

Copy link
ContributorAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

fixed

@fabpot
Copy link
Member

Now that the push channel feature has been merged (see#39402), I think this PR can be finished.@zairigimad Do you have time to update the code?

@zairigimad
Copy link
ContributorAuthor

Now that the push channel feature has been merged (see#39402), I think this PR can be finished.@zairigimad Do you have time to update the code?

Yes, I will take some time today to update the code.

OskarStark and WildChildForLife reacted with thumbs up emoji

@zairigimad
Copy link
ContributorAuthor

@fabpot I updated the code with the new PushChannel.

@@ -0,0 +1,22 @@
Expo Notifier
=================
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

Suggested change
=================
=============

@fabpot
Copy link
Member

Thank you@zairigimad.

@fabpotfabpot merged commite399e9d intosymfony:5.4Oct 28, 2021
@fabpot
Copy link
Member

@zairigimad Can you create a PR on symfony/recipes like done for other notifiers?

@zairigimad
Copy link
ContributorAuthor

@zairigimad Can you create a PR on symfony/recipes like done for other notifiers?

Yes i will check how it’s done and do a PR asap

WildChildForLife reacted with thumbs up emoji

This was referencedNov 5, 2021
Sign up for freeto join this conversation on GitHub. Already have an account?Sign in to comment

Reviewers

@jderussejderussejderusse left review comments

@fabpotfabpotfabpot approved these changes

@nicolas-grekasnicolas-grekasnicolas-grekas approved these changes

@OskarStarkOskarStarkAwaiting requested review from OskarStarkOskarStark is a code owner

+1 more reviewer

@ismail1432ismail1432ismail1432 left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Projects

None yet

Milestone

5.4

Development

Successfully merging this pull request may close these issues.

8 participants

@zairigimad@carsonbot@norkunas@fabpot@nicolas-grekas@jderusse@OskarStark@ismail1432
[8]ページ先頭
©2009-2025 Movatter.jp