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
Appearance settings

[Webhook] Add bridges for Webhook #19268

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
Dec 12, 2023
Merged

[Webhook] Add bridges for Webhook #19268

merged 1 commit into from
Dec 12, 2023

Conversation

alexandre-daubois
Copy link
Member

@carsonbot carsonbot added this to the 6.4 milestone Dec 12, 2023
@OskarStark OskarStark mentioned this pull request Dec 12, 2023
Merged
@alexandre-daubois alexandre-daubois force-pushed the webhook-bridges branch 2 times, most recently from bddee6f to 0eaff15 Compare December 12, 2023 10:46
OskarStark
OskarStark reviewed Dec 12, 2023
View reviewed changes
webhook.rst Outdated
Mailjet ``mailer.webhook.request_parser.mailjet``
Brevo ``mailer.webhook.request_parser.brevo``
Vonage ``mailer.webhook.request_parser.vonage``
Sendgrid ``mailer.webhook.request_parser.sendgrid``
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you please rebase, and add versionadded directives for 6.3 too?

Also Bravo, Vonage and Mallet needs to get a yes in the mailer.rst file in webhook column

Copy link
Member Author

Choose a reason for hiding this comment

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

Vonage is not present in Mailer. But I added for the others

Copy link
Member

Choose a reason for hiding this comment

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

Vonage is part of the Notifier. Should we mention webhook support there as well?

Copy link
Member Author

Choose a reason for hiding this comment

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

I'd say yes! I reworded the sentence above the table to remove "mailer". What do you think?

Copy link
Member

@TimoBakx TimoBakx Dec 12, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

Sounds good, but then also remove/rename "Mailer" from the table header. ^_^

Copy link
Member Author

Choose a reason for hiding this comment

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

Removed 👍

OskarStark
OskarStark reviewed Dec 12, 2023
View reviewed changes
webhook.rst Outdated Show resolved Hide resolved
@TimoBakx
Copy link
Member

TimoBakx commented Dec 12, 2023 •
edited
Loading

Might the wiring be missing incorrect for these bridges? They seem to be missing here: https://github.com/symfony/symfony/blob/a3686fbfbbbfec317213f831eabeb5a450720545/src/Symfony/Bundle/FrameworkBundle/DependencyInjection/FrameworkExtension.php#L2589C15-L2589C15

@alexandre-daubois alexandre-daubois force-pushed the webhook-bridges branch 2 times, most recently from cac9486 to 5264f05 Compare December 12, 2023 10:53
@alexandre-daubois alexandre-daubois mentioned this pull request Dec 12, 2023
Merged
@alexandre-daubois
Copy link
Member Author

alexandre-daubois commented Dec 12, 2023 •
edited
Loading

Ok I think this should be put on hold and waiting code merge of symfony/symfony#53004

Then we'll see if this goes in 7.1 or if it is considered as a bugfix that should target 6.4.

@TimoBakx
Copy link
Member

TimoBakx commented Dec 12, 2023 •
edited
Loading

I have a slightly different alternative for Twilio and Vonage, as they use a different event from the Mailer implementations:
#19269

Vonage is added in 6.4, I can make a follow-up PR for that bit.

@alexandre-daubois
Copy link
Member Author

alexandre-daubois commented Dec 12, 2023 •
edited
Loading

Alright I think we can actually merge this as is. The PR I opened on the code repo only checks that we remove some ids from the container conditionally, but the ids already exist. 👍

Your follow is a great idea, Timo!

TimoBakx
TimoBakx reviewed Dec 12, 2023
View reviewed changes
webhook.rst Outdated Show resolved Hide resolved
@OskarStark
Copy link
Contributor

Thank you Alexandre.

@OskarStark OskarStark merged commit 9d26845 into symfony:6.4 Dec 12, 2023
@OskarStark
Copy link
Contributor

Below 50 issues 💪

fabpot added a commit to symfony/symfony that referenced this pull request Dec 13, 2023
@fabpot
bug #53004 [FrameworkBundle] Add missing webhook parsers (alexandre-d…
877b173 
…aubois)

This PR was merged into the 6.4 branch.

Discussion
----------

[FrameworkBundle] Add missing webhook parsers

| Q             | A
| ------------- | ---
| Branch?       | 6.4
| Bug fix?      | yes
| New feature?  | no
| Deprecations? | no
| Issues        | -
| License       | MIT

Detected when reviewing this documentation PR: symfony/symfony-docs#19268

Should be merged after #53007, which brings notifier services removal conditionally

Commits
-------

b52b9e1 [FrameworkBundle] Add missing webhook parsers
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@TimoBakx TimoBakx TimoBakx approved these changes

@OskarStark OskarStark OskarStark left review comments

Assignees
No one assigned
Labels
Status: Reviewed Webhook
Projects
None yet
Milestone
6.4
Development

Successfully merging this pull request may close these issues.

[RemoteEvent][Webhook] Add Mailjet support
4 participants
@alexandre-daubois @TimoBakx @OskarStark @carsonbot
Morty Proxy This is a proxified and sanitized view of the page, visit original site.