| Project: | Mime Mail |
| Version: | 7.x-1.x-dev |
| Component: | Code |
| Category: | bug report |
| Priority: | normal |
| Assigned: | Unassigned |
| Status: | closed (fixed) |
Issue Summary
I noticed that the mimemail_alter_mail() function actually removes the html tags from the body !?
is this the correct behaviour?
as the following is written in comment above the line in question:
// attempt to fixup non-html messages that are being sent through drupal_mail
// I'm open to suggestions for better ways of doing this
but actually this check_markup is stripping the html
$body = check_markup($body, FILTER_FORMAT_DEFAULT);
as, in my case, the html string came from the system.mail service (services module, so originally a string coming from flex/flash),
could it be that the function expected another type of string?
the FIX is in fact simply to comment out this line, which results in normal behaviour
line 281:
//$body = check_markup($body, FILTER_FORMAT_DEFAULT);
Comments
#1
I'd suggest trying this, as I think what you might be running into is an access control issue as to what types of filters can be used for your submission. Try this:
$body = check_markup($body, FILTER_FORMAT_DEFAULT, FALSE);
If that works in your environment let us know.
#2
I had a similar issue. I had been using mimemail without problem, but when I changed my default filter format from Full HTML to Filtered HTML, all html tags were stripped out of my emails.
The suggested fix:
$body = check_markup($body, FILTER_FORMAT_DEFAULT, FALSE);
worked for me.
#3
Sorry, scratch that. It's commenting out the line that solves the problem.
If the goal is to try to fixup non-html messages that are being sent through drupal_mail, I'm not sure how this line would really help anyway. If mail is run through the default "Full HTML" filter, it's not going to strip out any HTML... and if mail is run through the default "Filtered HTML" filter, then all email will be missing HTML.
Or am I missing something? Maybe my setup is different then most folks...
Best,
Chad
#4
I'll take another look at this when I can to make sure there isn't something odd going on.
#5
Right, so the problem is that most messages aren't expecting to go out as HTML at all. System messages and other emails are generated in plaintext format and then sent along through drupal_mail().
If you put a series of plaintext paragraphs into an HTML message, all of your spacing and text-based formatting are lost and the whole message runs together. This is because your plain text does not have line breaks, paragraphs, hyperlinks, etc.
If you have checked the "Use mime mail for all messages" option, mimemail_mail_alter() attempts to rectify this problem by check_markup()ing its way to victory. In a default configuration, this usually looks "OK", but I can see how it might cause a problem once in awhile ( hence the comment )
I figure we can try:
* Creating a separate input format just for mail messages
* Having "mail format" become an option on the mime mail configuration page ( so you could, for example choose filtered html, full html, something else, or nothing at all
* Attempting to determine whether the message is already in HTML format, and bypassing the processing if it is.
Still, as ever, open to any idea that doesn't involve commenting out functionality without addressing the side effects ;)
#6
I think a simpler way to fix up text messages being sent out as HTML is to simply format them in drupal_mail_wrapper(). Then, as a follow up, implementing #263142: support hook_mail_alter for better integration with other modules won't cause this filter to be invoked on modules calling mimemail() directly. This means we could very simply insert a call to check_markup() without trying to guess if its already HTML or text or not.
Another option would be to put the filter in mimemail(), with a new argument to control whether the filter is run. This would allow modules that are sending already filtered nodes through email to avoid running the filter again, while modules that use mimemail() with unfiltered input could benefit from a system-wide mimemail filter. I actually like this option best, I think, though it means an API change.
#7
Well, no response. I changed my mind, and decided I liked the first one better. The idea is to fix up text emails into HTML, and not to mess up ones that are already HTML, so drupal_mail_wrapper() seems appropriate.
This adds a select box to the admin page which enables the user to pick which format to apply to text messages, then applies the selected format in drupal_mail_wrapper(). It then removes the mimemail_mail_alter() hook.
It allows one to select "No filter" which would enable you to go all out HTML on your registration emails and things.
I think as a follow up issue we can probably explain the whole "Use mimemail for all messages" as something more along the lines of "Use mimemail for plain text messages" with a link to documentation further elaborating the differences between modules that use mimemail and modules that send text messages that mimemail can convert.
#8
Actually, we don't want to remove this function as it does serve a purpose. Instead, what we likely need to do is expose a filter selection choice on the admin page for mimemail, so that users choosing to pass all email through mimemail can choose what filter format they wish to use as well. Then respect that filer selection within the mimemail_alter_mail() function.
If someone wants to volunteer to write that, speak up! Otherwise I'll try to get to it in the next few days.
#9
I kind of get the impression you barely read my post or my patch. My patch does include a selector on the admin page, and it does functionally the same thing as mail_alter(), only with less side effects.
If we put the format in mimemail_mail_alter(), when implementing #263142: support hook_mail_alter for better integration with other modules, the filter will be run even on HTML emails. Meanwhile, putting it in drupal_mail_wrapper() ensures that the filter is run only on text emails going out as HTML.
Additionally, Simplenews 6.x already fires off mail hooks explicitly before sending, which is what currently causes a majority of the problems with simplenews and mimemail in 6.x. It will even fire this mail hooks when not using the mimemail backend, but calling drupal_mail_send() explicitly instead (i.e, even though mimemail_alter is set, some code will not be using mimemail, yet the filter will still run).
Another downside is that when using a hook_alter(), we might HTMLitize the email before other modules expecting text get their hands on it. drupal_mail_wrapper() is the only time we're completely positive that we've got text, and it needs to become HTML. By simply putting the filter there, it is a lot less likely to end up running when it shouldn't be.
I could very easily move the code back into mimemail_mail_alter(), but it would create all of the above bugs, of which the cleanest solution I can see is moving it back into drupal_mail_wrapper().
#10
I apologize, I did miss your filter implementation in the patch - it was late and I was focusing on the other bits.
I've gone through the workflow a bit more, and looked at the mail_alter patch referenced as well. I'm going to do some testing of both, but I'm almost convinced. Thanks again Matthewbot for your work, it is greatly appreciated.
#11
After further review and testing, both this and the mail_alter() patch from #263142 have been committed. Thanks again for all of your work Matthewbot.
#12
Hmm, somehow the code got changed a little bit and now it no longer works. Here's a patch to take HEAD to what I originally wrote.
#13
Ah yes, that was my bad - I missed the $format placement in the check_markup() call.
I've just committed an update to this issue which fixes that and also replaces the input filter code you submitted with a more efficient and consistent API form_filter() call.
Thanks for double checking this!
#14
Well, my original reason for not using filter_form() was that there is no way to document it, and it disallows one to select "No format", replicating the behavior of the comment out fix the above posters were using. I guess there's no reason why a simple make URLs into links format couldn't be used instead, so thats not really a big issue. But the documentation is. Looking at admin page, there's no really clear indication of exactly what format the user is selecting. Because its directly below "Use mimemail for all messages" I'd have a hunch it was somehow related to that, but I'd still have to dive into the code to understand exactly what it was doing.
Another issue is that before the user visits the Mimemail page and pushes save, FILTER_FORMAT_DEFAULT is used as the mimemail filter. This has the side effect of that when the default format is changed, the mimemail format will also change, which seems confusing. I can imagine a user seeing that the format they wish to use with mimemail is already selected because it is their site's default format, not pushing save, and then later changing the default format and wondering why all their emails lost their formatting. I guess its kind of nitpicking, as the user would eventually figure it out and push save, separating the mimemail format from the sites default format, but we could avoid surprising the user simply by copying the sites current default format as the mimemail format in a hook_install or in a wrapper function that performs the copy when the mimemail_filter variable doesn't exist. I wouldn't hard code the ID of a built in filter, like Full HTML, because there's no guarantee the user hasn't deleted it, and we'd probably crash trying to use an invalid filter.
#15
Automatically closed -- issue fixed for two weeks with no activity.