Skip to content
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

Add failover transport to MailSetting #1315

Open
wants to merge 7 commits into
base: develop
Choose a base branch
from
Open

Add failover transport to MailSetting #1315

wants to merge 7 commits into from
+28 −0

Conversation

mjauvin
Copy link
Member

@mjauvin mjauvin commented Feb 23, 2025 •
edited
Loading

image

@mjauvin mjauvin added the enhancement PRs that implement a new feature or substantial change label Feb 23, 2025
@mjauvin mjauvin added this to the 1.2.8 milestone Feb 23, 2025
@mjauvin mjauvin requested a review from LukeTowers February 23, 2025 18:26
@mjauvin mjauvin self-assigned this Feb 23, 2025
matteotrubini
matteotrubini suggested changes Feb 24, 2025
View reviewed changes
Copy link
Contributor

@matteotrubini matteotrubini left a comment

Choose a reason for hiding this comment

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

add this validation rule:
'failover_mailers' => 'required_if:send_mode,'.self::MODE_FAILOVER

mjauvin and others added 5 commits February 24, 2025 11:20
@mjauvin @matteotrubini
add trigger
6aa45f6 
Co-authored-by: Matteo Trubini <7964032+matteotrubini@users.noreply.github.com>
@mjauvin @matteotrubini
remove default value as it won't be used.
f8bdf09 
Co-authored-by: Matteo Trubini <7964032+matteotrubini@users.noreply.github.com>
@mjauvin @matteotrubini
remove failover from the options
256aa85 
Co-authored-by: Matteo Trubini <7964032+matteotrubini@users.noreply.github.com>
@mjauvin @matteotrubini
initialize failover_mailers form config
917c76c 
Co-authored-by: Matteo Trubini <7964032+matteotrubini@users.noreply.github.com>
@mjauvin
add validation rule for failover_mailers
c4d4035
matteotrubini
matteotrubini approved these changes Feb 24, 2025
View reviewed changes
Copy link
Contributor

@matteotrubini matteotrubini left a comment

Choose a reason for hiding this comment

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

LGTM

@LukeTowers
Copy link
Member

@mjauvin are people able to configure the transports through the backend as well as use the failover transport?

@mjauvin
Copy link
Member Author

mjauvin commented Feb 25, 2025

@mjauvin are people able to configure the transports through the backend as well as use the failover transport?

No, they would need to configure the other mailers in the config

@mjauvin
Copy link
Member Author

mjauvin commented Feb 25, 2025

As I wrote in discord:

<@164272636988227584> I'm thinking we should move away from the SettingsModel behavior for the MailSetting model and have an active record instead (for the default mailer). That would allow setting multiple records for the different transports to support the failover (and roundrobin in Laravel 11)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@matteotrubini matteotrubini matteotrubini approved these changes

@LukeTowers LukeTowers Awaiting requested review from LukeTowers

Assignees

@mjauvin mjauvin

Labels
enhancement PRs that implement a new feature or substantial change
Projects
None yet
Milestone
1.2.8
Development

Successfully merging this pull request may close these issues.
3 participants
@mjauvin @LukeTowers @matteotrubini