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

Don't force ActionMailer to lazy load before configuration. #162

Closed
wants to merge 1 commit into from

Conversation

Benjamin-Dobell
Copy link

Closes #86

@Benjamin-Dobell
Copy link
Author

This has failed CI on 2.2.10 and 2.3.7, but passed on the other 3 ruby versions (older and newer). This appears to be an issue with the CI/project setup and is unrelated to this PR:

Could not find gem 'bundler (~> 1.16.2)' in any of the relevant sources:

@cgunther
Copy link

As a result of upgrading from Rails 5.2 to 6.0, I'm hitting a similar problem as #86 and I believe this fix is in the right direction, but not perfect yet.

Rails 6.0 deprecates the old jobs used to send emails in the background in favor of a new job (ActionMailer::MailDeliveryJob). The new_framework_defaults_6_0.rb initializer file has a line for you to uncomment to opt into the new job once you're stable on 6.0, however uncommenting it doesn't have the intended effect of using the new job.

The issue is that the initializer file sets the new job via Rails.application.config.action_mailer, and when ActionMailer::Base is loaded, the config you've set is copied to ActionMailer::Base. This gem currently eagerly loads ActionMailer::Base before configuration (and thus before initializers) as a result of adding the delivery method:

config.before_configuration do
ActionMailer::Base.add_delivery_method :mailgun, Railgun::Mailer
end

That means the config is already copied from Rails.application.config.action_mailer to ActionMailer::Base before the initializer file is run to set the delivery job, thus yielding no effect.

Using ActiveSupport.on_load(:action_mailer) as this PR does is better so we don't force eagerly loading ActionMailer::Base, however I believe the initializer in this PR ends up running after the initializer defined by ActionMailer:
https://github.com/rails/rails/blob/464a86f90ba89ac97aac48e4191fffc8309c445f/actionmailer/lib/action_mailer/railtie.rb#L17

Which copies your config to ActionMailer::Base:
https://github.com/rails/rails/blob/464a86f90ba89ac97aac48e4191fffc8309c445f/actionmailer/lib/action_mailer/railtie.rb#L53

Which errors when it gets to Rails.application.config.action_mailer.mailgun_settings since there is no ActionMailer::Base.mailgun_settings= method (yet). The initializer defined in this PR would be run later, adding the delivery method, thus defining the ActionMailer::Base.mailgun_settings= method.

To fix this, I believe this gem's initializer needs to run before the one provided by ActionMailer so that we can add the delivery method before we try copying the mailgun_settings. I believe this can be achieved by changing the initializer line as follows:

initializer "railgun.configure_rails_initialization", before: 'action_mailer.set_configs' do

Separately, I'm not sure where this line is coming from, perhaps something for your own application code?

ActiveSupport.run_load_hooks(:mailgun_mailer, Railgun::Mailer)

@obahareth
Copy link

@cgunther @Benjamin-Dobell Any idea when this might be merged guys? Thanks!

obahareth added a commit to theamazingteam/mailgun-ruby that referenced this pull request Dec 27, 2020
This commit implements the latest fix mentioned in mailgun#162 and is just a temporary fork until that PR is merged back to the main repo.
@cgunther
Copy link

I'm just a fellow user of this library, so I have no control over merging. I stumbled onto the original fix and provided my feedback on how it worked for me.

@javierav
Copy link

Three hours of my life wasted until I found that the fault was in this gem! 🤯 And the problem was detected in February 2019 and still, almost two years later, it has not been fixed! 🤬 Bye bye Mailgun...

@javierav
Copy link

For future reference, the error that I was getting is:

DEPRECATION WARNING: Initialization autoloaded the constants ActionText::ContentHelper and ActionText::TagHelper.

Related with rails/rails#36546

@donrestarone
Copy link

Any updates on if this change will be merged? Its Feb 2021 and this is still an issue.

@subdigital
Copy link
Contributor

The lack of response on this thread is concerning. Any updates from mailgun on when we can expect this to be resolved?

@Benjamin-Dobell
Copy link
Author

Benjamin-Dobell commented Apr 5, 2021

How Not to Do Open Source 101, with your lecturer Mailgun: #227

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Including 'mailgun-ruby' causes action_mailer configs in an initializer to not be used
6 participants