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.

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

Only configure smtp_settings if provided on configuration #44162

Merged

Conversation

andrehjr
Copy link
Contributor

Summary

There's a bug when ActionMailer::Base.smtp_settings= is set before ActionMailer gets initialized and config.action_mailer.smtp_settings is not set. This ends up overriding ActionMailer::Base.smtp_settings.

Some users may configure ActionMailer::Base.smtp_directly instead of using it via config.action_mailer.smtp_settings.

In order to fix that this PR does not try to configure smtp_settings when it's not present on the config hash.

Fixes #44161 and Follow up from #44061

@andrehjr andrehjr mentioned this pull request Jan 12, 2022
@andrehjr andrehjr force-pushed the only-configure-smtp-settings-when-provided branch from a1e966e to dbb741a Compare January 13, 2022 20:39
Copy link
Member

@Edouard-chin Edouard-chin left a comment

Choose a reason for hiding this comment

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

Thank you for the Pull Request @andrehjr . The issue you are describing is a common mistake where users would prematurely reference a constant (such as ActionMailer::Base, ActionController::Base) which messes up the boot process and ultimately result in the problem you were seeing.

Application shouldn't be doing that and should instead use a Lazy Load hook. In your example it would look something like

ActiveSupport.on_load(:action_mailer) do
  self.smtp_settings = { ... }
end
# This code will be executed once Rails itself uses the ActionMailer::Base constant. 

@andrehjr
Copy link
Contributor Author

You're totally right @Edouard-chin. I also advise on using Lazy Load Hooks and avoiding calling those constants prematurely. Thanks for taking a look into this PR.

The issue is that on #44061 there is still another behavior missing. Something that used to work on previous Rails versions is not working on Rails 7+ properly. This PR attempts to keep that between versions.

That means fewer surprises when upgrading if someone by mistake was using ActionMailer::Base directly. But, as it's not a good practice, rails may also decide not to fix it.

@Edouard-chin
Copy link
Member

Ah yeah I understand the nice intention to keep everyone's application working the same way despite the new changes, but IMO this kind of problem will always arise at some and it's better for users to fix the root cause.

Just my 2cents, I'll let the core team decides.

@andrehjr andrehjr force-pushed the only-configure-smtp-settings-when-provided branch from dbb741a to 51f531e Compare March 16, 2022 01:07
@andrehjr andrehjr force-pushed the only-configure-smtp-settings-when-provided branch from 51f531e to 921263b Compare March 16, 2022 01:31
@rafaelfranca rafaelfranca merged commit 7293f22 into rails:main Mar 16, 2022
rafaelfranca added a commit that referenced this pull request Mar 16, 2022
…when-provided

Only configure smtp_settings if provided on configuration
@andrehjr andrehjr deleted the only-configure-smtp-settings-when-provided branch March 16, 2022 21:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

SMTP Connection Error
3 participants