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

Do not render views when mail() isn't called. (NullMail refactoring) #8048

Merged
merged 1 commit into from Oct 28, 2012

Conversation

Projects
None yet
3 participants
@senny
Copy link
Member

senny commented Oct 28, 2012

I introduced a NullMail class. When mail() is never called an instance of NullMail is returned in place of a regular Mail.

This refactoring fixes #7761

@senny

This comment has been minimized.

Copy link
Member

senny commented Oct 28, 2012

I created a very small class called NullMail. What is the best practice in the rails codebase for tiny classes. Do you embed those in side the class that uses them or do you put them in a new file?

Also my BlankSlate implementation is very basic. Do you use a different pattern in the rails source?

@KieranP @jeremy @rafaelfranca What do you think?

@frodsan

View changes

actionmailer/lib/action_mailer/null_mail.rb Outdated
@@ -0,0 +1,7 @@
class NullMail

This comment has been minimized.

@frodsan

frodsan Oct 28, 2012

Contributor

Could you please add a # :nodoc: directive here?

@senny

This comment has been minimized.

Copy link
Member

senny commented Oct 28, 2012

@frodsan done.

@rafaelfranca

View changes

actionmailer/lib/action_mailer/base.rb Outdated
@@ -504,13 +505,17 @@ def method_missing(method_name, *args)
# method, for instance).
def initialize(method_name=nil, *args)
super()
@_message = Mail.new

This comment has been minimized.

@rafaelfranca

rafaelfranca Oct 28, 2012

Member

Could we leave this initialization here? Without it we can receive a "non-initialized instance variable" warning

This comment has been minimized.

@senny

senny Oct 28, 2012

Member

yes I'll move it there. I put it in process because I think it communicates the intent better, than overwriting the variable later. If it produces warnings though I'll put it back.

@rafaelfranca

This comment has been minimized.

Copy link
Member

rafaelfranca commented Oct 28, 2012

I would put NullMail inside of ActionMailer::Base

@senny

This comment has been minimized.

Copy link
Member

senny commented Oct 28, 2012

@rafaelfranca moved NullMail. Can you check again?

@rafaelfranca

This comment has been minimized.

Copy link
Member

rafaelfranca commented Oct 28, 2012

Seems good to me. Thank you

rafaelfranca added a commit that referenced this pull request Oct 28, 2012

Merge pull request #8048 from senny/7761_dont_render_view_without_mai…
…l_call

Do not render views when mail() isn't called. (NullMail refactoring)

@rafaelfranca rafaelfranca merged commit a273b6b into rails:master Oct 28, 2012

rafaelfranca added a commit that referenced this pull request Oct 28, 2012

Merge pull request #8048 from senny/7761_dont_render_view_without_mai…
…l_call

Do not render views when mail() isn't called. (NullMail refactoring)
Conflicts:
	actionmailer/CHANGELOG.md
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment