Skip to content

Allow other fields that specify recipients, not just "to"#520

Merged
jc00ke merged 1 commit intosidekiq:masterfrom
bbhoss:fix_actionmailer_extension
Nov 14, 2012
Merged

Allow other fields that specify recipients, not just "to"#520
jc00ke merged 1 commit intosidekiq:masterfrom
bbhoss:fix_actionmailer_extension

Conversation

@bbhoss
Copy link
Contributor

@bbhoss bbhoss commented Nov 14, 2012

We had some code that BCCs everyone instead of putting them in the "to:" field. The ActionMailer extension won't actually call the deliver method though unless "to" is specified. This fixes it so that any of the recipient-specifying methods should be delivered.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can an email be sent if the to isn't set? Looks like msg.deliver would fire w/ msg.cc or msg.bcc but w/o msg.to

@bbhoss
Copy link
Contributor Author

bbhoss commented Nov 14, 2012

Yes, I've tested sending emails through this code without anything but BCC set and it works fine. Also, I get emails all of the time without a to:, just BCC. I also tested sending bcc only emails using my normal email client and they go through fine. Finally, I'm not quite sure why this check is even here. If it returns an error because ActionMailer complains, IMO it shouldn't just fail silently.

@bbhoss
Copy link
Contributor Author

bbhoss commented Nov 14, 2012

SMTP-wise, it's all still RCPT TO:, but "cc" and "to" are actually message headers, which are optional. Check out 7.2 in RFC2821 for more details.

jc00ke added a commit that referenced this pull request Nov 14, 2012
Allow other fields that specify recipients, not just "to"
@jc00ke jc00ke merged commit ac4a920 into sidekiq:master Nov 14, 2012
@jc00ke
Copy link
Contributor

jc00ke commented Nov 14, 2012

Looks good to me. Thanks for explaining the to/cc/bcc thing. I didn't know it worked w/o to.

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.

2 participants