Respect a string in body when that's what we get

Hmmm.

The patch is empty.

Yes, the patch is empty. can you reupload the patch.

Thanks!

This is a patch to fix this issue. First off it uses a string instead of an array for the $msg variable, that way we can just implode the message body if it's an array and append it. Secondly, I've added a validate function to validate the emails that the user sets in the form. Also removed whitespace.

Sorry, forgot the --no-prefix. This one works :)

Rerolled again with missing t() on the form set error

you've got some '\n' where i think you mean to use "\n"

Rerolled with Shiny's fix to '\n'

Works nicely.

Is this going to be rolled into dev?

I do apologize - I'm not the "real maintainer" but have been going through the queue.

@Zombienaute if you wouldn't mind rerolling this against current I'll try to get it in. And please use a standard -p1 git patch (created with git diff or git format-patch).

Looks like the validation already got committed to --dev so here's a patch with the remaining changes against the latest 6.x-1.x branch rolled with git format-patch origin/6.x-1.x --stdout

Thanks. It does seem odd that we have to tiptoe around another misbehaving module.

I encountered this error while using the smtp + team_notifications + reroute_email -modules on a dev site.

Had to apply patch manually, but it works as expected. Neat! Now waiting for a commit on this 2+ years old issue.

You can patch it with `patch -p1 < 488032-fix-array-bug-5.patch` from the reroute_email module directory.

I ran into this with htmlmail just this week, so have a clear reason to look at it.

@Zombienaute, "git apply" is the more mainstream technique, but patch -p1 does work fine.

One key thing to consider here though, is that modules like htmlmail need to act *last*. In my case, I added something to the site module to move reroute_email ahead of htmlmail, which, of course, would be another way to solve this problem.

And I know of no way that a module like htmlmail can do its job properly without combining everything together. It *should* be dealing with the body as an array still, but even then, if somebody dropped something in, it ruins the html mail message.

Do you think we should add a button to the reroute_email configuration to offer to set it to a weight of -1 or something? Or I suppose we could just do that in hook_update_N() and hook_install. But I think it's key to this problem (although I still think we should do what this patch does).

I prefer this more explicit code explaining what's going on, if it's OK. I'm not completely sure that #13 ended up doing the right thing in all cases.

BTW, the issue I had with another module (not this one) colliding with the body array handling in htmlmail was due to *that* module also trying to have the weight 10. htmlmail for some time has increased its weight to avoid this problem.

Committed: 0fd29e1 (for D6). Thanks for crusading on this, @Zombienaute

Committed to D7 in c6061b0. I really would like tests for this though. I guess we should probably backport all the tests to D6. But I would like a test for this one at least in D7.

Hi rfay,

Thank you so much for your follow-up on this issue.

This is just a quick comment, as I was going through the issue queue, it would seem this has already been rolled in a while ago, or am I missing something?

There hasn't been any activity on this issue for more than a year now, do you think we could consider this isse as fixed?
If you wouldn't have any other objections, anything outstanding or to be discussed, could you maybe mark the issue directly as fixed?

Otherwise, could you please kindly indicate what would remain to be done in order to move this issue forward, towards a potential resolution?
In which case, you could perhaps assign the issue to me and change the status back to needs work.

Feel free to let me know if there would be anything we could do on this issue, whether testing, coding, documenting or replying to support requests, I would certainly be glad to help.

I would greatly appreciate if you could have a quick look at this issue and perhaps give us an update on the status.
Thanks very much in advance for your comments, feedback and replies.
Cheers!

Hi guys,

It took me some time to try go through the discussions in this ticket and do some more testing.

I have recently tested the Reroute Email module along with Messaging and it doesn't seem like this patch would still be needed anymore.

Indeed, it seems that the Messaging module now handles the body as an array and not a string, which seems to also be more along the lines of the Drupal standards (or at least what is done in Core).

Therefore, I would like to know if anybody would have any objections, comments, suggestions , concerns or issues, if we were to roll-back and revert the patch that was rolled in at #20.

I would greatly appreciate your comments, feedback, questions, issues, objections, suggestions or concerns on any aspects of the suggested change (rolling back the patch and removing this case from code) or this ticket in general, I would be glad to provide more information or explain in more details.

Thanks very much to all in advance for your help, reviews, testing, feedback and comments on this issue.
Cheers!

It's not just messaging that can do this. I've seen it in other places too...

Alright. Then could you please provide some examples for which this could be an issue?

Basically, I'm trying to find a proper way this could be tested.
How would you recommend this should be tested (with test cases)?

I have already thought of altering the body to set it as a string or from an implementation of hook_mail, in a hidden submodule in a /tests sub-folder, but I seems I didn't manage to figure out how the sending method could be overriden to use a body in a string format.

I would greatly appreciate some help about the test to be written, as requested in #20.

Any further advice, ideas, suggestions, recommendations, or questions would be highly appreciated.
Thanks to all in advance.

Just to sum up quickly the situation here.

The standard Drupal mail system defines the body of the email message as an array.
See hook_mail:

body: An array of lines containing the message to be sent. Drupal will format the correct line endings for you. drupal_mail() sets this to an empty array when the hook is invoked.

It appears that some modules override this default/standard behavior with their own logic to allow the body of the email to be a string.
The patch applied in this issue check if the body

came in as a string (despite the fact it should be an array) we'll respect that and leave it as a string.

This issue can't be marked as resolved for now, for an absence of tests for this issue.

After spending some time carrying further investigations, currently, the messaging module doesn't use this behavior anymore and I was unable to reproduce or find any other module that would allow overriding the default mail handling behavior with a string (in D7).

I would surely be glad to provide more feedback, work or reviews on this issue if anybody could point me to the right direction, but currently, I don't really see how this could be tested and in which cases the code of this patch/ticket would be used for Drupal 7.

Therefore, I allowed myself to set this to postponed (maintainer needs more info) for now and would surely be delighted to get back to this issue when we get more feedback on how this could be tested and whether this code would still be useful for D7 or not.

Any further advice, ideas, suggestions, recommendations, or questions would be highly appreciated.
Thanks to all in advance.

I think you might find that htmlmail mailhandler + htmlmail still does this. It does on 6.x anyway.

There are/were a lot of modules out there exhibiting this behavior in various contexts, and since I've seen fatals from more than one of them, I'm reluctant *not* to work around this problem. Since this is a *dev* module, it's easier to take risks here. There's no real cost in working around this one way or another.

Hi @rfay,

Thanks a lot for stepping out on this issue and giving me a bit of feedback:

There are/were a lot of modules out there exhibiting this behavior in various contexts, and since I've seen fatals from more than one of them, I'm reluctant *not* to work around this problem. Since this is a *dev* module, it's easier to take risks here. There's no real cost in working around this one way or another.

Fine, I would be completely fine with this and there would be no problem at all to leave this code as it is currently.

However, if I understood well, this ticket is *still not marked as fixed due to the absence of simple tests for this, for D7*.

Since I don't know how this could be tested for Drupal 7, I am currently unable to write any code or get this issue to move any further. I even tried to write a hidden submodule for tests with alter or mail hooks .... I wasn't able to reproduce this issue on D7.

Would it be possible to consider this issue as fixed and assume that this case can't be tested with Drupal 7?

Once again, thanks very much for everyone in advance for your great work, feedback and help on this issue.
Cheers!

I'm in favor of just letting this go. The fix was committed long ago. We don't have a test yet.

I could easily make a mock module that would do drupal_mail() with a body that was a string, and we could then test it. But it doesn't seem worth the effort.

Hi @rfay,

Thanks again very much for your prompt feedback and great help on this issue.

I tried to quickly change module's code from the the dev branch which already implements hook_mail to

<?php
/**
 * Implements hook_mail().
 */
function reroute_email_mail($key, &$message, $params) {
  if ($key != 'test_email_form') {
    return;
  }
  $message['headers']['Cc'] = $params['cc'];
  $message['headers']['Bcc'] = $params['bcc'];
  $message['subject'] = $params['subject'];
  // $message['body'][] = $params['body']; // Before.
  $message['body'] = "test123456"; // After.
}
?>

and triggered an email from: admin/config/development/reroute_email/test (the new admin test form).

which got me the following warning (with commerce_devel enabled for the stack trace):

Warning: implode() [function.implode]: Invalid arguments passed in DefaultMailSystem->format() (line 23 of myd7site\modules\system\system.mail.inc). Backtrace:
implode('

', 'This email was rerouted.
Web site: XXXXX
Mail key: reroute_email_test_email_form
Originally to: example@example.com
-----------------------
test123456') system.mail.inc:23
DefaultMailSystem->format(Array) mail.inc:169
drupal_mail('reroute_email', 'test_email_form', 'example@example.com', Object, Array) reroute_email.admin.inc:99
reroute_email_test_email_form_submit(Array, Array) form.inc:1464
form_execute_handlers('submit', Array, Array) form.inc:860
drupal_process_form('reroute_email_test_email_form', Array, Array) form.inc:374
drupal_build_form('reroute_email_test_email_form', Array) form.inc:131
drupal_get_form('reroute_email_test_email_form') 
call_user_func_array('drupal_get_form', Array) menu.inc:517
menu_execute_active_handler() index.php:21

In which we can clearly see:

DefaultMailSystem->format(Array) mail.inc:169
drupal_mail('reroute_email', 'test_email_form', 'example@example.com', Object, Array) reroute_email.admin.inc:99

that Drupal Core system requires an Array to be passed, otherwise, there would be a warning and the tests would fail.
This is where I was stuck for a while and tried to get back several times to on this ticket to try to get some help or advice.
 
Which got me moving slightly forward on this issue and I think I finally managed finding an appropriate method to test this particular case.
 
Basically, the tests requires a hidden test module, called reroute_email_test with hook_mail and hook_mail_alter implementations:

1. reroute_email_test_mail is triggered through a call to drupal_mail from the test case which passes a string for the message body.
2. reroute_email_mail_alter: the case in which we're interested, to test the module.
3. reroute_email_test_mail_alter is called to catch message's body and make it an array to be compliant with drupal_mail and prevent a Warning: implode(): Invalid arguments passed in DefaultMailSystem->format().
   (Ensure the test alter function always runs after module's)

 
Several methods are used to test:
First, we check whether the message from reroute_email_test_mail, or the one appended by Reroute Email are found in the message body.
Then, reroute_email_test_mail_alter logs an entry in watchdog if a string is detected for message's body (right after reroute_email_mail_alter), and the test attempts to find this record in the Recent log messages.

Additionally, I took the opportunity to add a few more tests in this case, that would most likely be committed separately for #1571508: More robust Cc / Bcc suppression, to check Cc and Bcc headers keys robustness (related with other commits, see 03fab23). For example:
Capturing message header keys such as cC or bCc:

<?php
  // Provide Cc and Bcc headers with an unexpected case.
  $message['headers']['cC'] = "test_cc_key@example.com";
  $message['headers']['bCc'] = "test_bcc_key@example.com";
?>

 
Please find attached to this comment a patch against reroute_email-7.x-1.x at 0cc2d8d which should provide the necessary test cases for this issue and #1571508: More robust Cc / Bcc suppression.
File attached as: reroute_email-respect-body-as-string-test-488032-30.patch.
 
It would be great if you could please let me know if this code could potentially be committed and get this issue's status updated.
 
I would greatly appreciate to have your comments, issues, questions, objections, recommendations, testing, reporting or concerns on the attached patch or any other aspects of this ticket in general, I would be glad to provide more information or explain in further details.
 
Any further advice, ideas, suggestions, recommendations, or questions would be highly appreciated.
Thanks in advance to all for your feedbacks, comments, testing, reviews and reporting.
Cheers!

Brilliantly done.

I took a test run and looked through the code, and IMO this is exactly what was required.

Thanks!
-Randy

Hi @rfay,

Thank you very much, once again, for your assitance, guidance, reviews and great feedback.

I am certainly delighted to hear that we could proceed without too much back and forth and without waiting any longer, I went ahead and committed the patch from #30, split up into two, against the 7.x-1.x branch at 0439561 and 8a5ed26, the second commit being for adding tests related to #1571508: More robust Cc / Bcc suppression.
Followed by a minor commit at 7011f7c to fix coding standards errors introduced at 0439561.

At last, I am glad to be able to bring an appropriate closure to this issue and allowed myself to mark this issue as fixed for now, but feel free to re-open it, or post a new ticket, at any time if you have any further objections with this issue or any of the related commits (0439561, 8a5ed26 and 7011f7c - we would surely be happy to hear your feedback).

Please let me know if you would have any further comments, feedback, questions, issues, objections, suggestions or concerns on any of these commits or this ticket in general, I would be glad to provide more information or explain in more details.

Special thanks to @rfay for the guidance, help, testing, reviews, patience and great work on this issue.
Thanks again to everyone for your patches, testing, reviews, feedback and comments.
Cheers!