Skip to content

[Notifier] add SentMessageEvent and FailedMessageEvent #39601

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
chalasr merged 1 commit into from
Aug 5, 2021
Merged

[Notifier] add SentMessageEvent and FailedMessageEvent #39601

chalasr merged 1 commit into from
Aug 5, 2021

Conversation

@ismail1432
Copy link
Contributor

@ismail1432 ismail1432 commented Dec 21, 2020 •
edited by derrabus
Loading

Q A
Branch? 5.4
Bug fix? no
New feature? yes
License MIT
Doc PR

Dispatch a new event SentMessageEvent which is dispatched once the notification is sent. The Symfony\Component\Notifier\Transport\AbstractTransport return an instance of SentMessage that contains the original message + an id that can be returned by the API it can be helpful to pass this object to the event.

Dispatch a new event FailedMessageEvent which is dispatched if sending the notification fails it can be helpful for a retry strategy

@carsonbot carsonbot changed the title add SentMessageEvent [Notifier] add SentMessageEvent Dec 21, 2020
@ismail1432 ismail1432 mentioned this pull request Dec 21, 2020
Closed
@ismail1432 ismail1432 force-pushed the notifier-add-sent-message-event branch 2 times, most recently from 97876db to c28ea75 Compare December 21, 2020 23:13
OskarStark
OskarStark reviewed Dec 22, 2020
View reviewed changes
OskarStark
OskarStark reviewed Dec 22, 2020
View reviewed changes
@ismail1432 ismail1432 force-pushed the notifier-add-sent-message-event branch from b8bb755 to d6c7aa2 Compare December 23, 2020 08:02
OskarStark
OskarStark reviewed Dec 23, 2020
View reviewed changes
@nicolas-grekas nicolas-grekas added this to the 5.x milestone Dec 23, 2020
@ismail1432 ismail1432 force-pushed the notifier-add-sent-message-event branch from b37752e to 34eb92a Compare December 28, 2020 13:59
@ismail1432
Copy link
Contributor Author

ismail1432 commented Dec 28, 2020

the failed test seems not related

jderusse
jderusse reviewed Dec 28, 2020
View reviewed changes
@ismail1432 ismail1432 changed the title [Notifier] add SentMessageEvent [Notifier] add SentMessageEvent and FailedMessageEvent Feb 4, 2021
@ismail1432 ismail1432 force-pushed the notifier-add-sent-message-event branch from 091dff1 to 9c1ccde Compare February 4, 2021 10:30
@ismail1432 ismail1432 force-pushed the notifier-add-sent-message-event branch from 394e2e4 to 51941e0 Compare February 9, 2021 16:08
@ismail1432 ismail1432 force-pushed the notifier-add-sent-message-event branch 3 times, most recently from f463ebb to 6af363a Compare February 11, 2021 16:34
@OskarStark OskarStark requested review from jderusse and removed request for jderusse April 6, 2021 12:34
@OskarStark OskarStark requested a review from chalasr April 15, 2021 14:49
@OskarStark OskarStark requested a review from Nyholm April 15, 2021 14:49
@OskarStark
Copy link
Contributor

OskarStark commented Jun 17, 2021

can you give us a final review here @jderusse ? Thanks

@ismail1432
Copy link
Contributor Author

ismail1432 commented Jul 13, 2021

friendly pump 😃

@OskarStark OskarStark requested review from fabpot and jderusse and removed request for jderusse July 13, 2021 08:04
@OskarStark OskarStark requested a review from derrabus August 4, 2021 20:09
@OskarStark OskarStark changed the title [Notifier] add SentMessageEvent and FailedMessageEvent [Notifier] add SentMessageEvent and FailedMessageEvent Aug 4, 2021
jderusse
jderusse reviewed Aug 4, 2021
View reviewed changes
Copy link
Member

@jderusse jderusse left a comment

Choose a reason for hiding this comment

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

I see Travis tests, did you rebased the PR recently?

@ismail1432 ismail1432 force-pushed the notifier-add-sent-message-event branch from c2531e7 to df0f304 Compare August 5, 2021 00:37
derrabus
derrabus approved these changes Aug 5, 2021
View reviewed changes
Copy link
Member

@derrabus derrabus left a comment

Choose a reason for hiding this comment

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

LGTM, but we need to add an entry to src/Symfony/Component/Notifier/CHANGELOG.md.

derrabus
derrabus reviewed Aug 5, 2021
View reviewed changes
@ismail1432
Copy link
Contributor Author

ismail1432 commented Aug 5, 2021

LGTM, but we need to add an entry to src/Symfony/Component/Notifier/CHANGELOG.md.

Updated, let me know if it's not good.

@ismail1432 ismail1432 force-pushed the notifier-add-sent-message-event branch from 0a9af2b to 9cef413 Compare August 5, 2021 08:58
OskarStark
OskarStark reviewed Aug 5, 2021
View reviewed changes
@chalasr chalasr force-pushed the notifier-add-sent-message-event branch from 234621d to 025256d Compare August 5, 2021 22:50
@chalasr
Copy link
Member

chalasr commented Aug 5, 2021

Thank you Smaine.

@chalasr chalasr mentioned this pull request Aug 5, 2021
Closed
@chalasr chalasr merged commit 7d370f1 into symfony:5.4 Aug 5, 2021
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

@jderusse jderusse jderusse left review comments

@OskarStark OskarStark OskarStark approved these changes

@derrabus derrabus derrabus approved these changes

@chalasr chalasr Awaiting requested review from chalasr

@Nyholm Nyholm Awaiting requested review from Nyholm

@fabpot fabpot Awaiting requested review from fabpot

Assignees

No one assigned

Labels

Feature Notifier Status: Reviewed

Projects

None yet

Milestone

5.4

Development

Successfully merging this pull request may close these issues.

7 participants

@ismail1432 @OskarStark @chalasr @jderusse @derrabus @nicolas-grekas @carsonbot