| Project: | Reroute Email |
| Version: | 7.x-1.x-dev |
| Component: | Code |
| Category: | bug report |
| Priority: | normal |
| Assigned: | Unassigned |
| Status: | needs work |
| Issue tags: | Needs tests |
Issue Summary
(This isn't necessarily reroute_email's bug to fix, but it's easy to fix in reroute_email.)
At least one module, Messaging, invokes hook_mail_alter itself, bypassing drupal_mail_send.
After reroute_email does its alter, notifications calls drupal_mail directly and this error results:
"warning: mail() expects parameter 3 to be string, array given in /var/www/example/drupal/includes/mail.inc on line 193."
As Stefan of Agaric wrote in IRC while debugging this: It is not wrong to have an array as mail body because if the function drupal_mail is used for sending it it will be imploded. However, messaging_mail bypasses drupal_mail and uses drupal_mail_send directly which does not do that. (It calls the hook_mail_alter but does not convert the body if necessary, whereas drupal_mail converts the array and calls all the hooks. drupal_mail_send is like a private function not intended as api part, probably there's a reason but it bypasses some important logic.)
Comments
#1
This attached patch makes sure that reroute_email returns a message body in the same type (array or string) as was handed to it.
#2
Hmmm.
The patch is empty.
#3
Yes, the patch is empty. can you reupload the patch.
Thanks!
#4
Ugh, i don't know how i missed it at the time, but i've got no trace of that patch on my computer or in the SCF repository anymore. Sorry.
fnikola, did you run into this problem?
#5
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.
#6
Sorry, forgot the --no-prefix. This one works :)
#7
Rerolled again with missing t() on the form set error
#8
you've got some '\n' where i think you mean to use "\n"
#9
Rerolled with Shiny's fix to '\n'
#10
Works nicely.
#11
Is this going to be rolled into dev?
#12
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).
#13
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
#14
Thanks. It does seem odd that we have to tiptoe around another misbehaving module.
#15
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.
#16
You can patch it with `patch -p1 < 488032-fix-array-bug-5.patch` from the reroute_email module directory.
#17
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).
#18
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.
#19
Committed: 0fd29e1 (for D6). Thanks for crusading on this, @Zombienaute
#20
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.
#21