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

Add ability to define callbacks in ActionMailer #5372

Merged
merged 1 commit into from Mar 11, 2012

Conversation

Projects
None yet
2 participants
@jsl
Copy link
Contributor

jsl commented Mar 10, 2012

Prior to this commit, there isn't a good way of adding things like default inline attachments to an email. This Stack Overflow thread shows people using hooks like the 'default' method in ActionMailer::Base to call a Proc for message configuration:

http://stackoverflow.com/questions/5113121/rails-use-same-attachment-for-all-emails-using-layout

This has the unintended side effect of setting a message header, so it's not a good solution.

This pull request adds support for message modifications by including AbstractController:Callbacks in ActionMailer::Base. It includes tests and documentation for the functionality provided by including this module.

@josevalim

This comment has been minimized.

Copy link
Contributor

josevalim commented Mar 10, 2012

Thanks for the pull request. I think before_send callbacks are a good idea, but it should probably be implemented using ActiveSupport::Callbacks, so it reflects the same API we have in models and controllers.

@jsl

This comment has been minimized.

Copy link
Contributor

jsl commented Mar 11, 2012

Thanks for the feedback, @josevalim. I agree that it should be implemented with ActiveSupport::Callbacks. I've updated the PR to use ActiveSupport::Callbacks. Is this in line with what you were thinking?

I did some minor refactoring of ActionMailer::Base#mail in this commit as well since it was getting way too long, especially after adding the run_callbacks block.

@josevalim

This comment has been minimized.

Copy link
Contributor

josevalim commented Mar 11, 2012

Thanks! One question: if I invoke MyMailer.name_of_email, it will still invoke the callback right? If so, before_send maybe not be the most appropriate name. Also, the fact the callback is being triggered after we do the whole setup may be dangerous, for example, maybe headers won't be copied over anymore or parsed properly.

That said, what if we follow the same API as the controller and provide both before_filter and after_filter? I guess those make clear it happens before and after the action, maybe we could even re-use AbstractController::Callbacks implementation. What do you think?

Add ability to define callbacks in ActionMailer using AbstractControl…
…ler::Callbacks.

Prior to this commit, there isn't a good way of adding things like
default inline attachments to an email.  This Stack Overflow thread
shows people using hooks like the 'default' method in ActionMailer::Base
to call a Proc for message configuration:

http://stackoverflow.com/questions/5113121/rails-use-same-attachment-for-all-emails-using-layout

This has the unintended side effect of setting a message header, so it's not a good solution.

This pull request adds support for message modifications by including AbstractController:Callbacks
in ActionMailer::Base. It includes tests and documentation for the functionality
provided by including this module.
@jsl

This comment has been minimized.

Copy link
Contributor

jsl commented Mar 11, 2012

Thanks again for the feedback @josevalim! I agree that before_filter and after_filter would be appropriate here, and including AbstractController::Callbacks seems to be working perfectly. I've added tests for before_filter and after_filter in ActionMailer, and they pass after including AbstractController::Callbacks in ActionMailer::Base.

I also added documentation that describes how to use these filters, and indicates that the user should prefer before_filter to after_filter to assure proper parsing of headers.

I realize that the three tests I added may be redundant, since I assume that filters are tested well in AbstractController::Callbacks, however I thought they served a purpose in documenting how these callbacks can be used in ActionMailer. Let me know, though, if you think they should be removed.

josevalim added a commit that referenced this pull request Mar 11, 2012

Merge pull request #5372 from jsl/add_before_send_to_actionmailer
Add ability to define callbacks in ActionMailer

@josevalim josevalim merged commit d6ec3f0 into rails:master Mar 11, 2012

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