8000 [Notifier] Add push channel to notifier by norkunas · Pull Request #39402 · symfony/symfony · GitHub
[go: up one dir, main page]
Skip to content

[Notifier] Add push channel to notifier #39402

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.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
merged 1 commit into from
Oct 19, 2021
Merged

[Notifier] Add push channel to notifier #39402

merged 1 commit into from
Oct 19, 2021

Conversation

norkunas
Copy link
Contributor
@norkunas norkunas commented Dec 9, 2020 •
edited by OskarStark
Loading
Q A
Branch? 5.4
Bug fix? no
New feature? yes
Deprecations? no
Tickets Fix #...
License MIT
Doc PR symfony/symfony-docs#...

I am proposing to add a new push channel for sending web/mobile push notifications to users.
This will open possibility to integrate providers like OneSignal, WonderPush, etc.
And as requested added the first implementation.

@derrabus derrabus added this to the 5.x milestone Dec 9, 2020
@nicolas-grekas
Copy link
Member

Why does this require a new interface? Can't a webpusher we implemented as a texter or a chatter like the other bridges?

@norkunas
Copy link
Contributor Author
norkunas commented Dec 14, 2020 •
edited
Loading

Why does this require a new interface? Can't a webpusher we implemented as a texter or a chatter like the other bridges?

No preference, just thought that it'd be better to separate web push transports from texter/chatter.

@nicolas-grekas
Copy link
Member
nicolas-grekas commented Dec 14, 2020 •
edited
Loading

No preference, just thought that it'd be better to separate web push transports from texter/chatter.

OK thanks. Then, unless there is a real reason that'd need to be discussed, this should be implemented as a transport really.

@norkunas norkunas closed this Dec 14, 2020
@norkunas norkunas reopened this Dec 14, 2020
@norkunas
Copy link
Contributor Author
norkunas commented Dec 14, 2020 •
edited
Loading

No preference, just thought that it'd be better to separate web push transports from texter/chatter.

OK thanks. Then, unless there is a real reason that'd need to be discussed, this should be implemented as a transport really.

Hmm, but then if we don't introduce webpusher as a transport, all the texter or chatter transports would be injected to the web push channel. Is that okay?

@norkunas norkunas closed this Dec 14, 2020
@norkunas norkunas reopened this Dec 14, 2020
@norkunas norkunas force-pushed the notifier-web-push branch 2 times, most recently from 0dbfd78 to d3c2a26 Compare December 14, 2020 12:43
@Nyholm
Copy link
Member
Nyholm commented Apr 8, 2021

What is the status on this PR.

Isn't this something @dunglas would be super interested to see?

@norkunas
Copy link
Contributor Author
norkunas commented Apr 8, 2021

I am just waiting for reviews :)

@Nyholm Nyholm requested a review from OskarStark April 8, 2021 07:50
@Nyholm
Copy link
Member
Nyholm commented Apr 8, 2021

=)

While waiting, could you do a rebase on this PR to the latest CI jobs will run?

@norkunas norkunas mentioned this pull request Aug 6, 2021
Merged
Nyholm
Nyholm reviewed Aug 7, 2021
View reviewed changes
Copy link
Member
@Nyholm Nyholm left a comment

Choose a reason for hiding this comment

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

Thank you. I did a quick review. Look at the other mailer or notifier transports. I’ve just updated their exception handling.

@norkunas norkunas force-pushed the notifier-web-push branch from 1939c02 to aa13585 Compare August 9, 2021 08:15
@norkunas norkunas force-pushed the notifier-web-push branch 3 times, most recently from 0bccf1d to ac2c895 Compare September 9, 2021 12:37
@michaelKaefer
Copy link
Contributor

Not sure but if a "Push Channel" is introduced then it would make sense to move symfony/firebase-notifier from the "Chat Channel" (docs: https://symfony.com/doc/current/notifier.html#chat-channel) to the new "Push Channel" because it is not a chat but a service for push notifications?

Maybe this affects other chatters too, this I don't know.

I guess this would break backward compatibility.

I could try to make a PR for symfony/firebase-notifier if it is wanted.

@OskarStark
Copy link
Contributor

Firebase Notifier can provide chat AND push functionality, while we can deprecate chat functionality in 5.4 and remove it in 6.0

From my POV we should merge this new channel

cc @fabpot

@norkunas
Copy link
Contributor Author
< 8000 p dir="auto">What's so special with this PR that nobody other wants to approve? 😁

@Nyholm Nyholm requested a review from OskarStark October 11, 2021 06:39
Nyholm
Nyholm approved these changes Oct 11, 2021
View reviewed changes
Copy link
Member
@Nyholm Nyholm left a comment

Choose a reason for hiding this comment

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

Sorry for the delay.

Im also happy with the changes.

@Nyholm Nyholm added the Ready label Oct 11, 2021
@fabpot
Copy link
Member
fabpot commented Oct 19, 2021

Thank you @norkunas.

@fabpot fabpot force-pushed the notifier-web-push branch from 4abd280 to e8638ab Compare October 19, 2021 07:24
@fabpot fabpot merged commit 847c011 into symfony:5.4 Oct 19, 2021
@fabpot fabpot mentioned this pull request Oct 19, 2021
Closed
@fabpot
Copy link
Member
fabpot commented Oct 19, 2021

@norkunas Can you work on updating the docs? See symfony/symfony-docs#15946
Thank you.

@norkunas
Copy link
Contributor Author

@norkunas Can you work on updating the docs? See symfony/symfony-docs#15946 Thank you.

Will do :)

@norkunas norkunas deleted the notifier-web-push branch October 19, 2021 07:27
@fabpot
Copy link
Member
fabpot commented Oct 28, 2021

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

@norkunas
Copy link
Contributor Author

submitted

This was referenced Nov 5, 2021
Merged
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@nicolas-grekas nicolas-grekas nicolas-grekas left review comments

@alanpoulain alanpoulain alanpoulain left review comments

@fabpot fabpot fabpot approved these changes

@colinodell colinodell colinodell approved these changes

@OskarStark OskarStark OskarStark approved these changes

@Nyholm Nyholm Nyholm approved these changes

@StaffNowa StaffNowa StaffNowa approved these changes

@bykclk bykclk bykclk approved these changes

@chalasr chalasr Awaiting requested review from chalasr

@stof stof Awaiting requested review from stof

@jderusse jderusse Awaiting requested review from jderusse

Assignees
No one assigned
Labels
Feature Notifier Ready Status: Reviewed
Projects
None yet
Milestone
5.4
Development

Successfully merging this pull request may close these issues.
0