Skip to content

[Mailer][Sendgrid] Support region #58264

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 7, 2024
Merged

[Mailer][Sendgrid] Support region #58264

merged 1 commit into from
Oct 7, 2024

Conversation

MrYamous
Copy link
Contributor

@MrYamous MrYamous commented Sep 14, 2024 •
edited by fabpot
Loading

Q A
Branch? 7.2
Bug fix? no
New feature? yes
Deprecations? no
Issues Fix #58230
License MIT

Still TODO

  • Fix failing test

@carsonbot carsonbot added this to the 7.2 milestone Sep 14, 2024
@MrYamous
Copy link
Contributor Author

MrYamous commented Sep 14, 2024 •
edited
Loading

I think I am missing something, I am in trouble with the failing test

@OskarStark
Copy link
Contributor

Do we need this? We can set it via the host, right?

@MrYamous
Copy link
Contributor Author

MrYamous commented Sep 14, 2024 •
edited
Loading

Do we need this? We can set it via the host, right?

Probably yes (setting via the host). But this feel more convenient to use, and considering we are already providing this kind of interaction with host for some bridge (like Mailjet or Mailgun) I did not think it would be a problem

@ev-gor
Copy link

ev-gor commented Sep 14, 2024

@MrYamous for EU region we should use 'api.eu.sendgrid.com', but according to your logic 'eu' !== ($this->region ?: 'eu') for 'eu' region we get 'api.sendgrid.com' that should be returned for global region, not for EU. That's why the test failed.
The same is true for smtp server.
By the way, what about case sensitivity: 'EU' vs 'eu'.

@OskarStark
Copy link
Contributor

I think I am 👎 on this one, we can already achieve that by setting the correct host.

@stof
Copy link
Member

stof commented Sep 30, 2024

@OskarStark for providers that have the concept of multiple regions, we support that natively in other bridges.

nicolas-grekas
nicolas-grekas requested changes Oct 3, 2024
View reviewed changes
Copy link
Member

@nicolas-grekas nicolas-grekas left a comment

Choose a reason for hiding this comment

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

Works for me once comments have been addressed.

@MrYamous
Copy link
Contributor Author

MrYamous commented Oct 4, 2024

I will take care of that over the weekend :)

@fabpot fabpot force-pushed the mailer/sendgrid-support-region branch from 0508e51 to 2b6c459 Compare October 7, 2024 09:30
@fabpot
Copy link
Member

fabpot commented Oct 7, 2024

Thank you @MrYamous.

@fabpot fabpot merged commit e2fa11b into symfony:7.2 Oct 7, 2024
8 of 10 checks passed
@fabpot fabpot mentioned this pull request Oct 7, 2024
Closed
@OskarStark OskarStark changed the title [Mailer] Support region in sendgrid bridge [Mailer][Sendgrid] Support region Oct 11, 2024
@fabpot fabpot mentioned this pull request Oct 27, 2024
Merged
pkruithof
pkruithof reviewed Dec 6, 2024
View reviewed changes
src/Symfony/Component/Mailer/Bridge/Sendgrid/Transport/SendgridSmtpTransport.php
{
parent::__construct('smtp.sendgrid.net', 465, true, $dispatcher, $logger);
parent::__construct('null' !== $region ? \sprintf('smtp.%s.sendgrid.net', $region) : 'smtp.sendgrid.net', 465, true, $dispatcher, $logger);
Copy link
Contributor

Choose a reason for hiding this comment

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

Sorry to comment on a merged PR, but I'm not sure this works as intended: we noticed mails were not being sent after upgrading to 7.2. Our region was null at this point, but the check is performed using 'null' (as a string). So this ended up as the following error:

Connection could not be established with host "ssl://smtp..sendgrid.net:465": stream_socket_client(): php_network_getaddresses: getaddrinfo for smtp..sendgrid.net failed: Name does not resolve

After appending ?region=global to the existing DSN it did work again.

Copy link
Member

Choose a reason for hiding this comment

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

Can you confirm that #59099 fixes this?

Copy link
Contributor

Choose a reason for hiding this comment

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

Ah, missed that in my search. Yes that fixes it, thanks!

@xabbuh xabbuh mentioned this pull request Jan 28, 2025
Closed
@GuiForce GuiForce mentioned this pull request Mar 12, 2025
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@pkruithof pkruithof pkruithof left review comments

@stof stof stof left review comments

@xabbuh xabbuh xabbuh left review comments

@mirandaguillaume mirandaguillaume mirandaguillaume left review comments

@ev-gor ev-gor ev-gor left review comments

@nicolas-grekas nicolas-grekas nicolas-grekas requested changes

@fabpot fabpot fabpot approved these changes

Assignees
No one assigned
Labels
Feature Mailer Status: Reviewed
Projects
None yet
Milestone
7.2
Development

Successfully merging this pull request may close these issues.

[Mailer] Sendgrid: Support EU based sendgrid accounts
10 participants
@MrYamous @OskarStark @ev-gor @stof @fabpot @nicolas-grekas @pkruithof @xabbuh @mirandaguillaume @carsonbot