Add cc/bcc support for Send mail

subscribing

subscribe

my current solution is to create another action to send email to admin email

That would be nice. Also a more generic solution that would allow any field (like Reply-to:) would be awesome.

Here is a patch to add CC and BCC as fields similar to TO in the Send Mail action. What do you think?

Add check for array exists.

It work for me, and I think this is a feature which needed for lots of people. Please commit it if possible.

Tested this patch. Working as expected.

Can this patch be modified to work on version Rules 6.x-1.5

I was able to get BCC in Rules version 6 by adding Mimemail module. Then when creating a rule action - choose to send with Mimemail - this offers BCC

rules-cc_and_bcc-856358-8.patch This Works.

+1 for RTBC

+1 RTBC

A valueable enhancement

+1

rules-cc_and_bcc-856358-8.patch : cc works but not bcc, for me. Bcc mail not sent ( = not received, not in spam) I don't know why.

Drupal: 7.53
Rules: 7.x-2.9
Mailsystem: 7.x-2.34
Mimemail: 7.x-1.0-beta4

Any ideas?

Forget it : it works. Operator error. Thanks.

#58
Definitely a valuable enhancement - Could it be added to release?

Verified that #8 applies cleanly to 7.x-2.x

One problem:

-function rules_action_mail($to, $subject, $message, $from = NULL, $langcode, $settings, RulesState $state, RulesPlugin $element) {
+function rules_action_mail($to, $cc, $bcc, $subject, $message, $from = NULL, $langcode, $settings, RulesState $state, RulesPlugin $element) {

I wondered how adding a couple of new parameters near the start of the list here might affect existing sites using this action.

Setting up a vanilla site with an rules action to send a mail, testing that works... then applying the patch, and triggering the rule again, I got:

66 28/Apr 13:53 php error Recoverable fatal error: Argument 9 passed to rules_action_mail() must be an instance of RulesState, string given in rules_action_mail() (line 78 of /var/www/html/sites/all/modules/contrib ...

This problem seems to have gone away after a cache clear; the existing rule (which doesn't have either of the new params configured) can be triggered without producing the error.

So I think perhaps this needs a hook_update_n to clear the (relevant) caches / metadata in order to prevent existing mail actions from causing errors when they don't provide the correct params?

Looks like the patch should work fine for new installs, but putting back to Needs Work because of this error for existing rules (actions) when the function signature changes.

Definitely interested in this! @mcdruid: dumb question, what would be the relevant cache to clear?

I would have thought it would be cache_rules that should be cleared in a hook_update_N, but haven't tested doing so.

Hello - Any potential traction to help address the last issue noted by mcdruid? Patch would be of help to us.

Thanks - HG

Marked #911462: Email a list of users using bcc as a duplicate.

Also, doesn't MimeMail support BCC and CC in its Rules action? As well as HTML e-mail, etc. I question the need for core Rules to become a full-featured mailer, if there are other contributed modules which specialize in this task.

@TR, I understand the point of view, but what about sites like ours where we have no interest in using any extra modules like Mime Mail, etc., and instead just have a simple implementation using SMTP delivery?

I agree Rules should not be a "full-featured mailer" as you said, but I don't feel as though cc/bcc support is that extensive.

See attached, an updated patch that includes code to rebuild the Rules cache.

Sorry, wrong ID got added to the patch name, but it's good enough for now.

OK, I guess cc and bcc support is reasonable. Give me some time to review the patch and test the upgrade path so I can make sure this won't break existing sites, then I'll commit this.

Thanks, I appreciate the consideration.

Not sure if this would be any value, but could also include a message at the top of the "Send mail" rule page, or in the README, that directs developers to other modules (Mime Mail, HTML Mail, Message Notify, etc.) if they need more advanced mail delivery capabilities?

Maybe that's overkill, but just thought I'd offer the suggestion.

I just try to patch against 2.x-dev. However, I got these error:

patching file modules/system.eval.inc
Hunk #1 succeeded at 76 (offset 1 line).
Hunk #2 succeeded at 155 (offset 1 line).
patching file modules/system.rules.inc
patching file rules.install
Hunk #1 succeeded at 547 (offset 1 line).

Meanwhile, the current 2.x-dev is not stable yet and have a lot of conflict with Rules Conditional. Then I try to manually patch on version 2.11. The patch looks good so far. Both CC and BCC received the email when the rules were triggered. FYI, my site using SMTP modules to send the email.

Well, there have been new commits to -dev since August. I'd recommend re-rolling the patch.

Here we go. Reroll the patch based on current 2.x-dev. Haven't test yet on 2.11 version.

@parasolx: I don't understand what you are trying to communicate.

However, I got these error:

Those are not errors. The patch is still good. Nice of you to re-roll it, but not necessary.

Meanwhile, the current 2.x-dev is not stable yet

What does that mean? Rules 7.x-2.x-dev works just fine, -dev is 7.x-2.11 plus bug fixes, so there are fewer problems in -dev than in 7.x-2.11 and there have been no reports of NEW problems introduced by -dev.

and have a lot of conflict with Rules Conditional.

Again, what does that mean? Conditional Rules has not been maintained in a long time, and I don't see that you've posted any issues either here or in that module about problems. By saying "2.x-dev is not stable yet and have a lot of conflict..." you are implying that there are problems introduced in Rules 7.x-2.x-dev, but that's not true as far as I can tell. If you know about new problems with Rules 7.x-2.x-dev, you need to open an issue.

Haven't test yet on 2.11 version

You should be testing on -dev. In Drupal, fixed point releases like 7.x-2.11 cannot be changed. Changes are made in -dev, so patches apply to -dev and testing is done in -dev and when a patch gets committed it gets committed to -dev and will show up in the NEXT fixed point release.

SMTP authentication module doesn't like to receive empty header values for Cc and Bcc, so with #27/#34 it throws a single error for each of the two values that are empty. I added some logic to conditionally add to the headers only when Cc and/or Bcc values exist. Also the previous patch was fuzzy against the most recent 2.x-dev so hey, free re-roll.

Note to anyone using this patch and sending mail through SMTP Authentication module: The development feature E-mail address to reroute all emails to: in SMTP Authentication module is expecting to unset lowercase 'cc' and 'bcc' headers, so if you are sending emails with Cc or Bcc when using this patch, they will still be delivered to those addresses. Unknown what the impact would potentially be by setting 'Cc' and 'Bcc' headers to 'cc' and 'bcc' (probably none) but worth getting some opinions before making them lowercase.

 
Since patch #36..

Changed:

- 'Cc' and 'Bcc' headers have been changed to lowercase for compatibility with SMTP Authentication module.

SMTP headers like Cc: and Bcc: are actually case insensitive according to the spec, and Bcc: is allowed to be empty. See RFC 5322. The case insensitivity of the ABNF definition of the header fields is stated in Section 1.2.2, and the Cc: and Bcc: fields themselves are defined in Section 3.6.3, where the definition and also the text description of Bcc: state that it is allowed to be empty.

Just sayin' - maybe the "SMTP Authentication" module should be fixed rather than change every other module ...

@TR, thank you for the quick response. Very thorough!

I'm hiding patch #38, as RFC 5322 has damn near everything capitalized and all of the headers are capitalized in the rules send mail output. Case-insensitive or not in IMF, no reason to make lowercase. I submitted both patches so that whoever would be committing it has options without having to make any edits. I agree 100% we shouldn't be making concessions to a bug in a single module that may or may not be used downstream.

I see what you're saying about 3.6.3 regarding Bcc being allowed to be empty:

.. since a "Bcc:" field may contain no addresses, a "Bcc:" field can be sent without any addresses indicating to the recipients that blind copies were sent to someone. Which method to use with "Bcc:" fields is implementation dependent..

If I'm reading this right, it sounds like an empty Bcc header can be added to make an email appear as if it has been Bcc'd when it hasn't. If that's the case, then by always adding an empty Bcc header, we're saying that in this implementation we'd like the resulting email to always appear as if it has been Bcc'd. (unknown if this comes into play with modern email clients)

That being said, as far as the conditional in #36 I don't see the harm in adding a small amount of logic to produce a cleaner output. Plus we were never adding Cc or Bcc to the headers before this patch, so we have an opportunity to decide on these things before committing. You have something like 3300+ commits in history, so I defer to you!

If I'm reading this right, it sounds like an empty Bcc header can be added to make an email appear as if it has been Bcc'd when it hasn't.

I wouldn't describe it that way.

The actual standard contained in the RFC is the ABNF description of the protocol. This is like a regular expression in PHP. The words that surround the ABNF are there to provide context about how this applies to real-life situations, and to provide intent in case there is some ambiguity in interpretation. But the ABNF is the bottom line - a program that strictly adheres to the ABNF should work and be compliant with the RFC.

From section 3.6.3, the ABNF for the Bcc: header looks like this:
bcc = "Bcc:" [address-list / CFWS] CRLF
This says that the bcc header starts with the case-insensitive (as per section 1.2.2) string "Bcc:" followed by an optional list of addresses (or comment, or folding white space) followed by a CRLF (\r\n) as a line terminator. The text after the ABNF is simply explaining why you might have an empty Bcc: sent - specifically that the mail sending program may choose to send individual emails and delete the addresses from the Bcc: line before sending so that the recipient mail server doesn't receive those supposedly blind addresses. The text says this is one way you can do it, or you can remove the entire Bcc: line if you want, etc.

Note the Cc: ABNF looks like this:
cc = "Cc:" address-list CRLF
So in this case, if the Cc: header is included, it must start with the case-insensitive string "Cc": followed by a MANDATORY list of addresses, followed by a CRLF line terminator.

That being said, as far as the conditional in #36 I don't see the harm in adding a small amount of logic to produce a cleaner output.

I agree. I have no problem with the patch removing the Bcc: if it is empty, and the Cc: MUST be removed if there are no Cc: addresses, so that part of the patch is absolutely needed. But the SMTP Authentication module MUST be able to deal with an empty Bcc: header, and MUST be able to deal with case-insensitive header field names. If it doesn't it is not compliant with the SMTP specification, which means that it won't be able to properly handle the enormous variations that it encounters from various mail programs and modules. (BTW, "MUST" is standards-speak for required.)

I think the only thing I would push back on is changing Bcc: and Cc: to lower case. While lower case is valid, so it upper case and so is mixed case. If that causes an error in the SMTP Authentication module, it really ought to be fixed there regardless of what we do in Rules!

if you're getting an error like this:
Call to undefined method RulesState::root() or
Argument 7 passed to mine_rules_action_mail() must be an instance of RulesState, array given

You can mitigate the error by allowing the function to handle multiple versions of the call in the upgrade process.

/**
 * Action Implementation: Send mail.
 */
function mine_rules_action_mail() {
$args = func_get_args();
if (count($args) == 9 ) {
  list($to, $subject, $message, $from, $langcode, $settings, $state, $element) = $args;
}
if (count($args) == 10 ) {
  list($to, $cc, $subject, $message, $from, $langcode, $settings, $state, $element) = $args;
}
if (count($args) == 11 ) {
  list($to, $cc, $bcc, $subject, $message, $from, $langcode, $settings, $state, $element) = $args;
}
....
}

@mikeytown2:
Yeah, that's basically the problem brought up by @mcdruid in #20. Changing the arguments of a function is an API change, and while it won't affect NEW sites, there aren't many people making new D7 sites these days.

My concern is how it affects EXISTING sites ... do we need to force a registry rebuild or cache clear, or will the patch just work without those? If the update causes a problem on even 1% of the D7 Rules sites, that's still thousands of people who won't be happy ...

I'm not sure what mine_rules_action_mail() is - rules_action_mail() is not a hook so you can't override it. I'm guessing you replaced the action implementation with your own function. I don't expect that would be common, and certainly it's not supported

Evidently, there's not much interest in this feature.

D7 Rules is basically in maintenance-only mode right now, as D7 is nearing its end-of-life. So I'm moving this to the 8.x-3.x branch for consideration.

This patch needs to be re-rolled for D8 and reviewed, and we need some test cases to demonstrate this new feature works properly.

Evidently, there's not much interest in this feature.

How would you know? We never participated in the discussions of this issue, but are using the feature happily on our D7 sites.

Yes, exactly - how would I know, if no-one participates or reviews patches or writes tests or otherwise contributes to the issues? With 200,000 users, if I hear crickets I think I can assume that there's not much interest. And although this issue has far more participation than most, the fact remains that it's 10 years old and we still don't have a patch with tests that won't break existing D7 sites like yours by changing the API.

The patch exists and is available for anyone who wants to use it, but as I said D7 is in maintenance mode right now and it's far more important to me to keep existing sites from breaking than it is to introduce new features that aren't quite ready. That's why I left this issue open and moved it to D8 - like all the rest of Drupal new features should go into the most recent version first then be backported if necessary.

https://git.drupalcode.org/project/rules/-/merge_requests/2

MR for 8.x is here.

If you are using smtp, you should also apply the patch of this issue https://www.drupal.org/project/smtp/issues/3214505 on branch 8.x-1.0(smtp)(8.x-1.x-dev has another bug https://www.drupal.org/project/smtp/issues/3214887)

Several problems:

Yes, technically "to" is not required, BUT if you don't specify "to" then you MUST specify Cc or Bcc. If you are going to change the action so that "to" is not required, then you have to add validation to ensure that there is at least one recipient. Because I personally don't have a good use case for omitting "to", I would prefer this remain as required.

The tests fail because you didn't modify the tests to work with your new input parameters. And that brings up another point - changing the arguments to the action is an API change that breaks compatibility with previous versions. In Drupal, we generally forbid this sort of change between minor point updates. But if this change is to be made, then we absoulutely need a hook_update_N() to modify any existing Ruyles to use the new version of the action. That will be a lot of work ...

Also, new features need new tests. Please add test cases for the Cc and Bcc options.

BTW, I would prefer to use patches in this issue queue instead of merge requests. While merge requests are convenient for full-time
developers, they make it difficult to impossible for anyone else to participate in the issue queue.

Patch #36 for 7.x no longer applies cleanly due to the change made to the rules_action_mail function when 7.x-2.13 was released. Attached is a re-rolled patch and interdiff from #36.

@TR Reply #51
Making Cc and Bcc parameters optional fixes broken tests and compatibility issues. Now just need to modify the tests for the interface.
(Sorry, because I used composer to manage patch, but I could not successfully apply one commit of the above MR, so I tried to submit the correction many times.)