Include ActionMailer::DeliverLater::Mixin in ActionMailer::Base by default
Opened this issue · 45 comments
When you install this plugin, it should just be available. No need to add include ActionMailer::DeliverLater::Mixin by hand.
@cristianbica, do you want to take a stab at this?
I will to this tomorrow.
👍
On Jul 16, 2014, at 4:06 PM, Abdelkader Boudih notifications@github.com wrote:
I will to this tomorrow.
—
Reply to this email directly or view it on GitHub.
Do you think it better if i do it with railtie ?
Yeah, that sounds like a good plan.
On Jul 16, 2014, at 4:08 PM, Abdelkader Boudih notifications@github.com wrote:
Do you think it better if i do it with railtie ?
—
Reply to this email directly or view it on GitHub.
When I install this gem from github, my mailer still doesn't have deliver_later method.
AuthMailer.confirmation_instructions(user).deliver_later
> NoMethodError:
undefined method `deliver_later' for #<Mail::Message:0x007ff3625c01b8>
Is there anything more required to make this work?
I'm using rails 4.1.4
I will look into it.
So i located the bug not i'm not sure yet how to fix it.
When we include the mixing manually we have
DelayedMailer, ActionMailer::DeliverLater::Mixin, ActionMailer::Base, ActionView::Layouts, ....
with the initializer we get (i changed include to prepend)
DelayedMailer, #<Module:0x007f086be68d48>, #<Module:0x007f086c2a7aa8>, ActionMailer::DeliverLater::Mixin, ActionMailer::Base,....
I need to find a way to include the mixin before the unnamed module. The unamed module don't call super
@dhh @rafaelfranca @cristianbica any though ?
One thin we need to do is to fix this anonymous module to call super.
I'm looking at the rails source since there is no extra gem that could have added these module.
We will have to patch rails 4.1 and 4.2, right ?
We need to fix it for 4.2 as it will be included in 4.2. I've created a sample app at https://github.com/cristianbica/actionmailer_deliver_later_sample_app. The issue seems to be that the default method_missing is called from ActionMailer::Base and not the one from the mixin
irb(main):002:0> TestMailer.ancestors
=> [TestMailer, #<Module:0x007f83a8231a08>, #<Module:0x007f83a823afb8>, #<Module:0x007f83a46b6cd0>, #<Module:0x007f83a46b6d70>, ActionMailer::Base, ActionMailer::DeliverLater::Mixin, ......]
irb(main):003:0> ActionMailer::Base.ancestors
=> [ActionMailer::Base, ActionMailer::DeliverLater::Mixin, ...]
irb(main):004:0> TestMailer.method(:method_missing).source_location
=> ["/Users/cristi/.rbenv/versions/2.0.0-p451/lib/ruby/gems/2.0.0/bundler/gems/rails-b45b99894a60/actionmailer/lib/action_mailer/base.rb", 550]
I'm sending a little fix right now @cristianbica
@cristianbica https://github.com/seuros/actionmailer-deliver_later/tree/prepend << this show that ActionMailer::Base is not the problem , the anonymous modules don't call super.
Actually the problem is with ActionMailer::Base's method_missing:
[8] pry(main)> TestMailer.method(:method_missing)
=> #<Method: TestMailer(ActionMailer::Base).method_missing>
The real problem isn't rails as it is a ruby mixins issue. Ruby's include isn't suitable for this as you cannot override methods and you can't call super (see: http://stackoverflow.com/questions/4470108/when-monkey-patching-a-method-can-you-call-the-overridden-method-from-the-new-i). Ruby's Module#prepend fixed this issue and that's why it's working using prepend so I suggest go ahead and use prepend for this.
That still won't fix the issue.
Actually it will fix it:
[1] pry(main)> TestMailer.test_email.class
=> ActionMailer::DeliverLater::MailMessageWrapper
[2] pry(main)> TestMailer.test_email.deliver_later
Enqueued ActionMailer::DeliverLater::Job (Job ID: 422c4472-2d94-4151-8074-7202008fe4dd) to Inline(active_jobs_mailers) with arguments: "TestMailer", "test_email", "deliver"
But ActiveSupport::Concern doesn't supports Module#prepend so I had to do:
diff --git a/lib/action_mailer/deliver_later/railtie.rb b/lib/action_mailer/deliver_later/railtie.rb
index 2a4711d..a3d3254 100644
--- a/lib/action_mailer/deliver_later/railtie.rb
+++ b/lib/action_mailer/deliver_later/railtie.rb
@@ -6,9 +6,12 @@ module Actionmailer
class Railtie < Rails::Railtie # :nodoc:
initializer 'actionmailer-deliver_later' do
ActiveSupport.on_load(:action_mailer) do
- include ActionMailer::DeliverLater::Mixin
+ prepend ActionMailer::DeliverLater::Mixin
+ class << self
+ prepend ActionMailer::DeliverLater::Mixin::ClassMethods
+ end
end
end
end
end
-end
\ No newline at end of file
+endWhile this works the Module#prepend is a ruby 2.0+ method and rails 4.2 will still support ruby 1.9.3. So we'll have to find another solution
To make this work as a gem we need to monkey patch ActionMailer::Base but as the plan is to merge activejob and this into rails we can hook directly into the ActionMailer::Base implementation when merging. The current working solution is to include the mixin in you ActionMailer::Base subclasses and I think we should leave it as it is until merged in rails
/cc @dhh @rafaelfranca
Sounds good to me. Do you want to prepare the PR for merging into Rails itself? Would like to get it in this week.
On Aug 11, 2014, at 22:51, Cristian Bica notifications@github.com wrote:
To make this work as a gem we need to monkey patch ActionMailer::Base but as the plan is to merge activejob and this into rails we can hook directly into the ActionMailer::Base implementation when merging. The current working solution is to include the mixin in you ActionMailer::Base subclasses and I think we should leave it as it is until merged in rails
/cc @dhh @rafaelfranca
—
Reply to this email directly or view it on GitHub.
Shouldn't we merge activejob first ? and yes, i can prepare a PR.
Then we can turn this gem to support rails 4.1 only.
Yep. I'll give it a try later today. I think we should skip the integration tests as it adds a lot of dependencies (gems and system - redis, postgres, rabbitmq), has some sleep in it and it's kind of slow--
Cristian Bica
On Tue, Aug 12, 2014 at 8:52 AM, David Heinemeier Hansson
notifications@github.com wrote:
Sounds good to me. Do you want to prepare the PR for merging into Rails itself? Would like to get it in this week.
On Aug 11, 2014, at 22:51, Cristian Bica notifications@github.com wrote:
To make this work as a gem we need to monkey patch ActionMailer::Base but as the plan is to merge activejob and this into rails we can hook directly into the ActionMailer::Base implementation when merging. The current working solution is to include the mixin in you ActionMailer::Base subclasses and I think we should leave it as it is until merged in rails
/cc @dhh @rafaelfranca
—
Reply to this email directly or view it on GitHub.
Reply to this email directly or view it on GitHub:
#6 (comment)
That kills the git history, i think the git history should be included.
I don't know how to do that and I don't think we need to add tens of commits to rails--
Cristian Bica
On Tue, Aug 12, 2014 at 12:02 PM, Abdelkader Boudih
notifications@github.com wrote:
That kills the git history, i think the git history should be included.
Reply to this email directly or view it on GitHub:
#6 (comment)
I already did it when i said i can prepare the PR, rails repository will see just one commit but the git history is preserved. I will push my changes when i return to my other computer.
Ok. There are some changes in Rakefile, railtie.rb, test/cases/logging_test.rb--
Cristian Bica
On Tue, Aug 12, 2014 at 12:33 PM, Abdelkader Boudih
notifications@github.com wrote:
I already did it when i said i can prepare the PR, rails repository will see just one commit but the git history is preserved. I will push my changes when i return to my other computer.
Reply to this email directly or view it on GitHub:
#6 (comment)
Thanks, i just want to know if we should remove the work you did in the integration testing. I want to keep it, but i dunno what your opinion and @dhh, @rafaelfranca are.
Sorry i should have skipped reading your message early. I see that you decided to remove the integration testing.
Remove them and if rails team decides they should be added we'll add them--
Cristian Bica
On Tue, Aug 12, 2014 at 12:44 PM, Abdelkader Boudih
notifications@github.com wrote:
Sorry i should have skipped reading your message early. I see that you decided to remove the integration testing.
Reply to this email directly or view it on GitHub:
#6 (comment)
How did you merge AJ into rails while preserving git history? Format patch and search and replace on paths?--
Cristian Bica
On Tue, Aug 12, 2014 at 12:44 PM, Abdelkader Boudih
notifications@github.com wrote:
Sorry i should have skipped reading your message early. I see that you decided to remove the integration testing.
Reply to this email directly or view it on GitHub:
#6 (comment)
@cristianbica with git-subtree
Nice. Didn't knew about it. Let me know when you push to your rails fork and I can send you the changes to Rakefile and others--
Cristian Bica
On Tue, Aug 12, 2014 at 12:52 PM, Abdelkader Boudih
notifications@github.com wrote:
@cristianbica with git-subtree
Reply to this email directly or view it on GitHub:
#6 (comment)
Also ActionMailer deliver_later should probably be partially rewritten to move the classes under ActionMailer. You can take a look at rails repo jobs branch --
Cristian Bica
On Tue, Aug 12, 2014 at 12:56 PM, Cristian Bica cristian.bica@gmail.com
wrote:
Nice. Didn't knew about it. Let me know when you push to your rails fork and I can send you the changes to Rakefile and others--
Cristian Bica
On Tue, Aug 12, 2014 at 12:52 PM, Abdelkader Boudih
notifications@github.com wrote:@cristianbica with git-subtree
Reply to this email directly or view it on GitHub:
#6 (comment)
About the merging ... from what I see in the past the practice is to ignore the git history. See the merge of strong_parameters at rails/rails#7251. Anyway if it's required to ditch the history you'll rebase and that's it :)
https://github.com/seuros/rails/tree/activejob.
I'm fine with both methods, i however like the subtree approche to keep sync with the original repo.
@dhh / @rafaelfranca : We are merging activejob under rails at https://github.com/seuros/rails/tree/activejob and having some discussions at seuros/rails#1. Can you guys help us a little on clarifying some stuff?
thanks
I didn't know of git-subtree. It wasn't intentional to kill the history with the previous merges. I like preserving the history, if we can, and it looks like we can.
Thanks for working together on this, guys!
@cristianbica, why wouldn't you want to keep the integration tests?
Can you guys open a PR for the merge? Remember, we also need to get activemodel-global_id merged.
A lot of development dependencies: all the adapters gems, redis, rabbitmq, postgres, beanstalkd. Also the tests are pretty slow as I have a 2 second sleep to wait for the adapter to process the jobs. --
Cristian Bica
On Tue, Aug 12, 2014 at 6:31 PM, David Heinemeier Hansson
notifications@github.com wrote:
I didn't know of git-subtree. It wasn't intentional to kill the history with the previous merges. I like preserving the history, if we can, and it looks like we can.
Thanks for working together on this, guys!@cristianbica, why wouldn't you want to keep the integration tests?
Reply to this email directly or view it on GitHub:
#6 (comment)
I don’t mind any of that stuff. It’s better to be comprehensively covered, than to be fast. We can always be fast if it doesn’t have to work :).
On Aug 12, 2014, at 8:34, Cristian Bica notifications@github.com wrote:
A lot of development dependencies: all the adapters gems, redis, rabbitmq, postgres, beanstalkd. Also the tests are pretty slow as I have a 2 second sleep to wait for the adapter to process the jobs. --
Cristian BicaOn Tue, Aug 12, 2014 at 6:31 PM, David Heinemeier Hansson
notifications@github.com wrote:I didn't know of git-subtree. It wasn't intentional to kill the history with the previous merges. I like preserving the history, if we can, and it looks like we can.
Thanks for working together on this, guys!@cristianbica, why wouldn't you want to keep the integration tests?
Reply to this email directly or view it on GitHub:
#6 (comment)
—
Reply to this email directly or view it on GitHub.
:) ok. We'll probably need to update the contributing guides, rails dev box, right?--
Cristian Bica
On Tue, Aug 12, 2014 at 6:37 PM, David Heinemeier Hansson
notifications@github.com wrote:
I don’t mind any of that stuff. It’s better to be comprehensively covered, than to be fast. We can always be fast if it doesn’t have to work :).
On Aug 12, 2014, at 8:34, Cristian Bica notifications@github.com wrote:A lot of development dependencies: all the adapters gems, redis, rabbitmq, postgres, beanstalkd. Also the tests are pretty slow as I have a 2 second sleep to wait for the adapter to process the jobs. --
Cristian BicaOn Tue, Aug 12, 2014 at 6:31 PM, David Heinemeier Hansson
notifications@github.com wrote:I didn't know of git-subtree. It wasn't intentional to kill the history with the previous merges. I like preserving the history, if we can, and it looks like we can.
Thanks for working together on this, guys!@cristianbica, why wouldn't you want to keep the integration tests?
Reply to this email directly or view it on GitHub:
#6 (comment)
—
Reply to this email directly or view it on GitHub.
Reply to this email directly or view it on GitHub:
#6 (comment)
@dhh, should we wait until @jeremy merge activemodel-global_id or can we do it ?
Once we have initial version ready @cristianbica or me will open the PR, is that ok for you ?
I think we can use GlobalID as a gem dependency until it's merged--
Cristian Bica
On Tue, Aug 12, 2014 at 6:46 PM, Abdelkader Boudih
notifications@github.com wrote:
@dhh, should we wait until @jeremy merge activemodel-global_id or can we do it ?
Once we have initial version ready @cristianbica or me will open the PR, is that ok for you ?
Reply to this email directly or view it on GitHub:
#6 (comment)
I’d love for you guys to run with this entire project: activejob, actionmailer-deliver_later, and activemodel-global_id. Let’s get everything into the same PR, since it’s all one-big feature, and then we can solicit comments there.
The proposed timeline at the moment is that we’ll release 4.2.0.beta1 with this in place on Sunday, August 17.
Great work, once again, guys.
On Aug 12, 2014, at 8:46, Abdelkader Boudih notifications@github.com wrote:
@dhh, should we wait until @jeremy merge activemodel-global_id or can we do it ?
Once we have initial version ready @cristianbica or me will open the PR, is that ok for you ?—
Reply to this email directly or view it on GitHub.