Skip to content

Navigation Menu

Sign in

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

[Mailer] Add retry_period option for email transport #54939

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 2 commits into from
Jan 4, 2025
Merged

[Mailer] Add retry_period option for email transport #54939

merged 2 commits into from
Jan 4, 2025

Conversation

sdespont
Copy link
Contributor

@sdespont sdespont commented May 16, 2024 •
edited by nicolas-grekas
Loading

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

RoundRobinTransport constructor has retryPeriod set to 60 seconds. This cannot be configured right now.

Let's say all the transports fail (e.g. email address with domain that does not exist).
Problems:

  1. When sending more than one email synchronously we can't send emails following the one that failed. We have to sleep(x) where x >= 60sec. (of course we have to handle the TransportException thrown by invalid email but this is another topic)
  2. When using Messenger and async emails we don't have to worry about handling TransportException because invalid message will be sent back to the queue however the worker cannot consume another messages for the next 60 seconds. Also logs will be flooded with exceptions because in this 60s window a lot of messages could be tried.

This PR permits to specify a retry period using a new DNS option retry_period like MAILER_DSN="roundrobin(postmark+api://ID@default sendgrid+smtp://KEY@default)?retry_period=15"

@carsonbot

This comment was marked as off-topic.

Sign in to view
@carsonbot carsonbot added this to the 7.1 milestone May 16, 2024
@nicolas-grekas nicolas-grekas modified the milestones: 7.1, 7.2 May 16, 2024
@carsonbot carsonbot changed the title Add retry_period option for email transport [Mailer] Add retry_period option for email transport May 17, 2024
@sdespont
Copy link
Contributor Author

sdespont commented Jun 20, 2024 •
edited
Loading

Any chance to see this PR merged in 7.2 ?

@OskarStark OskarStark changed the title [Mailer] Add retry_period option for email transport [Mailer] Add retry_period option for email transport Aug 2, 2024
@sdespont
Copy link
Contributor Author

Is there anything more you'd like to do before reviewing this PR? Let me know what you think.

@OskarStark
Copy link
Contributor

OskarStark commented Aug 14, 2024 •
edited
Loading

It looks like your commiter email is not associated with your GitHub account. You may want to change that

@sdespont
Copy link
Contributor Author

Thanks, but how can you see that? And is it a problem to review the PR?

@OskarStark
Copy link
Contributor

image

There is this GitHub Pictogramm

@sdespont
Copy link
Contributor Author

OK, I don't know why. Is this blocking the review? I don't really care whether I get credit for this PR or not.

@sdespont
Copy link
Contributor Author

sdespont commented Oct 9, 2024 •
edited
Loading

@MrYamous Thanks, that's a good idea. Hope this PR will be accepted one day

@fabpot fabpot modified the milestones: 7.2, 7.3 Nov 20, 2024
@fabpot fabpot force-pushed the add-retry-period-option branch from 5749659 to 9716a89 Compare January 4, 2025 17:29
fabpot
fabpot approved these changes Jan 4, 2025
View reviewed changes
Copy link
Member

@fabpot fabpot left a comment

Choose a reason for hiding this comment

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

I've simplified the implementation in a second commit

@fabpot
Copy link
Member

fabpot commented Jan 4, 2025

Thank you @sdespont.

@fabpot fabpot merged commit 41bee39 into symfony:7.3 Jan 4, 2025
19 checks passed
@fabpot fabpot mentioned this pull request Jan 4, 2025
Closed
@alamirault alamirault mentioned this pull request Jan 6, 2025
Merged
nicolas-grekas added a commit that referenced this pull request Jan 7, 2025
@nicolas-grekas
minor #59380 [Mailer] Add missing retry_period DSN option (alamirault)
17d6c3d 
This PR was merged into the 7.3 branch.

Discussion
----------

[Mailer] Add missing `retry_period` DSN option

| Q             | A
| ------------- | ---
| Branch?       | 7.3
| Bug fix?      | no
| New feature?  | no <!-- please update src/**/CHANGELOG.md files -->
| Deprecations? | no <!-- please update UPGRADE-*.md and src/**/CHANGELOG.md files -->
| Issues        | Fix #... <!-- prefix each issue number with "Fix #", no need to create an issue if none exists, explain below instead -->
| License       | MIT

I found changelog is missing while documenting symfony/symfony-docs#20511

Code PR: #54939

Commits
-------

2512b64 [Mailer] Add missing retry_period DSN option
@fabpot fabpot mentioned this pull request May 2, 2025
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@MrYamous MrYamous MrYamous left review comments

@fabpot fabpot fabpot approved these changes

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

Successfully merging this pull request may close these issues.

[Mailer] Make RoundRobinTransport retryPeriod configurable
6 participants
@sdespont @carsonbot @OskarStark @fabpot @MrYamous @nicolas-grekas
Morty Proxy This is a proxified and sanitized view of the page, visit original site.