Let an app load ActionMailer/ActionView #56
Conversation
Requiring ActionMailer while the gem is loading might break initialization logic of a Rails app using Rails 6.0 or later. ActionMailer 6.0 introduces the new `delivery_job` setting (https://github.com/rails/rails/blob/6-0-stable/actionmailer/lib/action_mailer/railtie.rb#L49-L51) whose value is being constantized and applied as soon as ActionMailer is loaded. Normally, the delivery job would be configured during the app initialization, so if ActionMailer is loaded **before** the initializer is being executed, the configuration won’t be applied and a default `delivery_job` class would be used. The problem here is that Maildown is required before Rails initializers run, and it forces loading of ActionMailer by requiring `ActionMailer::Base` for monkey-patching. This causes ActionMailer to never apply `delivery_job` config. The solution seems simple enough: delay monkey-patching until ActionMailer is loaded. Because it's easy and seems safer, we'll also delay monkey-patching of ActionView until it's loaded as well.
Fix the CI build.
|
Interesting. Thanks for the report and the fix at the same time! My concern here is if this gets loaded before ActionMailer then the methods that we (re)define will be over-written by ActionMailer based on load. I'm surprised this works when you load ActionMailer after this. Do you have an example app that demonstrates the existing problem with the currently released version and is fixed with this patch? https://www.codetriage.com/example_app That would allow me to exercise the patch to manually test and investigate the fix since you weren't able to write an automated test. |
|
@schneems don't have an example app at the moment, but I'll prepare one later today.
But we subscribe to the |
|
@schneems here's the repro app with a single test that fails under 3.2: https://github.com/sidonath/maildown-load-order-repro |
|
I can confirm i'm able to see the failing test without your patch and then see the fix with it. I still need to look some more, but wanted to report back initial findings. Thanks for the repro case! |
|
Thanks for the PR and everything, merging this in! |
|
Released 3.3.0 |
|
Woo hoo, thank you 🚀 |
Hi there and thanks for the work on Maildown!
We noticed an issue with Maildown as we were upgrading to Rails 6.0.
Requiring ActionMailer while the gem is loading might break initialization logic of a Rails app using Rails 6.0 or later.
ActionMailer 6.0 introduces the new
delivery_jobsetting whose value is being constantized and applied as soon as ActionMailer is loaded. Normally, the delivery job would be configured during the app initialization, so if ActionMailer is loaded before the initializer is being executed, the configuration won’t be applied and a defaultdelivery_jobclass would be used.The problem here is that Maildown is required before Rails initializers run, and it forces loading of ActionMailer by requiring
ActionMailer::Basefor monkey-patching. This causes ActionMailer to never applydelivery_jobconfig.The solution seems simple enough: delay monkey-patching until ActionMailer is loaded.
Because it's easy and seems safer, we'll also delay monkey-patching of ActionView until it's loaded as well.
This fix seems almost too easy, but all tests are passing and the app works as expected, hopefully I didn't miss something. Well, one thing is that there's now an explicit dependency on ActiveSupport and we don't require it, but I can't tell under which scenario that might be a problem.
I also couldn't figure out how to write a test case for this scenario that wouldn't break on pre-6 Rails.