View Issue Details

IDProjectCategoryView StatusLast Update
05421User patchesTokenspublic2012-06-21 14:22
Reporterpeterhol Assigned To 
PrioritynormalSeveritytweak 
Status confirmedResolutionopen 
Product Version 
Target VersionFixed in Version 
Summary05421: Make it possible to include binary images in html emails
Description

Regarding issue 05156 (Insert picture not possible in email templates), where somewhere in the middle you said:
"In fact this is not a bug. It is not currently supported to add images to email templates simply because we have not implemented a way to join the image binary to the email."

Well, I found an easy way to join the image binary to the email...

TagsNo tags attached.
Complete LimeSurvey version number (& build)10315

Activities

peterhol

peterhol

2011-08-23 22:42

reporter  

diff.txt (1,135 bytes)
Index: common_functions.php
===================================================================
--- common_functions.php	(revision 10315)
+++ common_functions.php	(working copy)
@@ -4865,7 +4865,7 @@
 {
 
     global $emailmethod, $emailsmtphost, $emailsmtpuser, $emailsmtppassword, $defaultlang, $emailsmtpdebug;
-    global $rootdir, $maildebug, $maildebugbody, $emailsmtpssl, $clang, $demoModeOnly, $emailcharset;
+    global $rootdir, $maildebug, $maildebugbody, $emailsmtpssl, $clang, $demoModeOnly, $emailcharset, $rooturl;
 
     if (!is_array($customheaders) && $customheaders == '')
     {
@@ -4973,10 +4973,9 @@
 	$mail->AddCustomHeader("X-Surveymailer: $sitename Emailer (LimeSurvey.sourceforge.net)");
 	if (get_magic_quotes_gpc() != "0")	{$body = stripcslashes($body);}
     if ($ishtml) {
-        $mail->IsHTML(true);
-    	$mail->Body = $body;
-        $mail->AltBody = strip_tags(br2nl(html_entity_decode($body,ENT_QUOTES,'UTF-8')));
-    } else
+		$body = str_replace('src="'.$rooturl, 'src="'.$rootdir,$body);
+		$mail->MsgHTML($body);
+	} else
        {
         $mail->IsHTML(false);
         $mail->Body = $body;
diff.txt (1,135 bytes)
peterhol

peterhol

2011-08-24 15:12

reporter  

diff-new.txt (2,001 bytes)
Index: common_functions.php
===================================================================
--- common_functions.php	(revision 10315)
+++ common_functions.php	(working copy)
@@ -4865,7 +4865,7 @@
 {
 
     global $emailmethod, $emailsmtphost, $emailsmtpuser, $emailsmtppassword, $defaultlang, $emailsmtpdebug;
-    global $rootdir, $maildebug, $maildebugbody, $emailsmtpssl, $clang, $demoModeOnly, $emailcharset;
+    global $rootdir, $maildebug, $maildebugbody, $emailsmtpssl, $clang, $demoModeOnly, $emailcharset, $rooturl;
 
     if (!is_array($customheaders) && $customheaders == '')
     {
@@ -4973,10 +4973,26 @@
 	$mail->AddCustomHeader("X-Surveymailer: $sitename Emailer (LimeSurvey.sourceforge.net)");
 	if (get_magic_quotes_gpc() != "0")	{$body = stripcslashes($body);}
     if ($ishtml) {
-        $mail->IsHTML(true);
-    	$mail->Body = $body;
-        $mail->AltBody = strip_tags(br2nl(html_entity_decode($body,ENT_QUOTES,'UTF-8')));
-    } else
+		$body = str_replace('src="'.$rooturl, 'src="'.$rootdir, $body);
+		$mail->MsgHTML($body);
+        $mail->AltBody = 
+			preg_replace('/\t/', '', 											//remove tabs (generated by ckeditor)
+	        	preg_replace("/((^[\r\n]*|[\r\n]+)[\s\t]*[\r\n]+){3,}/", "\n",	//replace many empty lines (generated by ckeditor) with just one 
+    	    		preg_replace('@<head[^>]*?>.*?</head>@siU', '',				//remove <head> (might contain tags that are not filtered out by strip_tags)
+						preg_replace(' {2,}', ' ',								//remove multiple spaces 
+							strip_tags( 										//remove other tags
+								br2nl( 											//<br> to \n
+									html_entity_decode($body,ENT_QUOTES,'ISO-8859-15') //correctly encode html entities for email messages
+								)
+							)
+						)
+					)
+				)
+			);
+//        $mail->IsHTML(true);
+//        $mail->Body = $body;
+//        $mail->AltBody = strip_tags(br2nl(html_entity_decode($body,ENT_QUOTES,'UTF-8')));
+	} else
        {
         $mail->IsHTML(false);
         $mail->Body = $body;
diff-new.txt (2,001 bytes)
mot

mot

2011-08-24 15:14

reporter   ~16130

While quickly looking at the patch: Please ensure this is not breaking 05331

peterhol

peterhol

2011-08-24 15:14

reporter   ~16131

Last edited: 2011-08-25 18:01

View 2 revisions

I made some tweaks to the plaintext part of the message as well. ckeditor adds too many tabs and newlines for my taste... this also changes decoding of htmlentities from utf-8 to working ISO standard for plaintext emails...

ahh.. saw your message now... in the diff-new.txt, that's taken care of!

mot

mot

2011-08-24 15:54

reporter   ~16136

Last edited: 2011-08-25 18:01

View 7 revisions

Hmm, haven't you noticed that it has already been taken care of for whitespaces and lines in 05331?

Are you probably not using the latest stable version from trunk you test against?

If the solution made in 05331 does not fit for you, please create a separate ticket referring to it and explain what you're missing.

And I'm against using PHPMailer::MsgHTML() as it's conceptually broken using regex instead of a HTML parser. This can not work and will only introduce problems.

peterhol

peterhol

2011-08-24 16:44

reporter   ~16138

Last edited: 2011-08-25 18:00

View 2 revisions

regarding 16136:
tables, I still get a lot of empty lines and excess tabs.

  1. Sorry, I see now that it has been discussed there.
  2. I svn up-ed, and my patch still works. Uploading new patch.
  3. OK, I'll create a separate ticket for that. Since I use email templates with among other things nested: 05423

regarding 16137:
I don't know what problems you are referring to. It works for me though… What part of MsgHTML() are you talking about? the substituting of absolute (or relative) paths for CIDs?

peterhol

peterhol

2011-08-24 16:47

reporter  

diff-v3.txt (1,064 bytes)
Index: common_functions.php
===================================================================
--- common_functions.php	(revision 10836)
+++ common_functions.php	(working copy)
@@ -4211,7 +4211,7 @@
 {
 
     global $emailmethod, $emailsmtphost, $emailsmtpuser, $emailsmtppassword, $defaultlang, $emailsmtpdebug;
-    global $rootdir, $maildebug, $maildebugbody, $emailsmtpssl, $clang, $demoModeOnly, $emailcharset;
+    global $rootdir, $maildebug, $maildebugbody, $emailsmtpssl, $clang, $demoModeOnly, $emailcharset, $rooturl;
 
     if (!is_array($customheaders) && $customheaders == '')
     {
@@ -4328,8 +4328,8 @@
 	$mail->AddCustomHeader("X-Surveymailer: $sitename Emailer (LimeSurvey.sourceforge.net)");
 	if (get_magic_quotes_gpc() != "0")	{$body = stripcslashes($body);}
     if ($ishtml) {
-        $mail->IsHTML(true);
-    	$mail->Body = $body;
+		$body = str_replace('src="'.$rooturl, 'src="'.$rootdir, $body);
+		$mail->MsgHTML($body);
         $mail->AltBody = trim(strip_tags(html_entity_decode($body,ENT_QUOTES,'UTF-8')));
     } else
        {
diff-v3.txt (1,064 bytes)
mot

mot

2011-08-25 17:59

reporter   ~16147

Regarding the problems using MsgHTML: You can not parse HTML with regex. That function tries to. That's conceptually broken and basically that's just it.

I don't understand also why you can not just enter image URLs that are working from the beginning, but that might be subjective. However I know it does work.

peterhol

peterhol

2011-08-25 21:42

reporter   ~16150

Ok but anyway somehow you have to find the src urls for all the images, so that you can pass them to AddEmbeddedImage and exchange them for CID references, right? If not using regex, how will you accomplish that? So that's the first chunk of regex usage. Would it be acceptable to use that part of the function?

Next: why you cannot enter image urls that are working from the beginning: I want to be able to use the ckeditor/kcfinder to upload images easily... AddEmbeddedImage only accepts absolute or relative (to the script) file paths, not urls. So for it to work both in "wysiwyg" mode, to maintain the possibility of linking to other images not attaching them to the email, but still attaching the images whenever possible (which lowers spam scores and enhances readability in most email readers) I thought this was a nifty solution...

The last chunk of regex in MsgHTML is of course the strip_tags with included removal of head, style and other tags. I take it this is the most conceptually broken part of MsgHTML? I'd be perfectly happy not using that section of the MsgHTML function if the whitespace is removed anyhow from the beginning in ckeditor. But: if there are style definitions, title definition, javascript or other stuff in the head of the HTML body of the message (which there really shouldn't be, but seems to be quite common since many email clients supports style definitons in head), these won't be removed by strip_tags, and you'll end up (as I have a few times when I wasn't careful) with chunks of incomprehensible text in beginning of the plain text part of the message... But if you have another, correct way of doing this without using regex, that would of course be great!

mot

mot

2011-08-25 23:20

reporter   ~16153

I see the point with the images being embedded instead of linked and spam scores/displaying the email.

The problematic part of MsgHTML at first glance is the regex. Should use a HTML parser, taking care of the src's and add the embeds based on images found.

The function should also ensure not to double process images if used more than once.

The strip_tags usage combined with preg_replce is indeed very problematic, good you pointed that out.

I could imagine either a helper function that takes the text and the phpmailer object or and extension to the phpmailer class itself to integrate the changes.

For the mockup, a function probably suits better.

Additionally I currently can't remmeber from the top of my head which HTML variant CKEditor is producing. This might need to be reflected for the HTML parser.

peterhol

peterhol

2011-08-25 23:36

reporter   ~16154

Hmm, as you have demonstrated, there are many considerations here which I don't know enough about, so I'm afraid I don't have much more to say as of now. I don't know how to solve this without using regex, and I'm not sure how to correctly extend the phpmailer class.. But one thing: I do believe that AddEmbeddedImagge automatically discards any duplicate images in the markup, so at least that won't be a problem :)

peterhol

peterhol

2011-08-26 12:47

reporter   ~16157

Last edited: 2011-08-26 12:53

View 3 revisions

update: patch proposal in http://bugs.limesurvey.org/view.php?id=5423#c16156

One might want to make image attachment optional, though... maybe through email settings?

peterhol

peterhol

2011-08-26 15:23

reporter  

diff-common_functions.php.txt (1,064 bytes)
Index: common_functions.php
===================================================================
--- common_functions.php	(revision 10836)
+++ common_functions.php	(working copy)
@@ -4211,7 +4211,7 @@
 {
 
     global $emailmethod, $emailsmtphost, $emailsmtpuser, $emailsmtppassword, $defaultlang, $emailsmtpdebug;
-    global $rootdir, $maildebug, $maildebugbody, $emailsmtpssl, $clang, $demoModeOnly, $emailcharset;
+    global $rootdir, $maildebug, $maildebugbody, $emailsmtpssl, $clang, $demoModeOnly, $emailcharset, $rooturl;
 
     if (!is_array($customheaders) && $customheaders == '')
     {
@@ -4328,8 +4328,8 @@
 	$mail->AddCustomHeader("X-Surveymailer: $sitename Emailer (LimeSurvey.sourceforge.net)");
 	if (get_magic_quotes_gpc() != "0")	{$body = stripcslashes($body);}
     if ($ishtml) {
-        $mail->IsHTML(true);
-    	$mail->Body = $body;
+		$body = str_replace('src="'.$rooturl, 'src="'.$rootdir, $body);
+		$mail->MsgHTML($body);
         $mail->AltBody = trim(strip_tags(html_entity_decode($body,ENT_QUOTES,'UTF-8')));
     } else
        {

Issue History

Date Modified Username Field Change
2011-08-23 22:42 peterhol New Issue
2011-08-23 22:42 peterhol File Added: diff.txt
2011-08-24 15:12 peterhol File Added: diff-new.txt
2011-08-24 15:14 mot Note Added: 16130
2011-08-24 15:14 peterhol Note Added: 16131
2011-08-24 15:54 mot Note Added: 16136
2011-08-24 15:55 mot Note Edited: 16136 View Revisions
2011-08-24 15:55 mot Note Edited: 16136 View Revisions
2011-08-24 15:56 mot Note Edited: 16136 View Revisions
2011-08-24 16:00 mot Note Edited: 16136 View Revisions
2011-08-24 16:01 mot Note Edited: 16136 View Revisions
2011-08-24 16:44 peterhol Note Added: 16138
2011-08-24 16:47 peterhol File Added: diff-v3.txt
2011-08-25 17:59 mot Note Added: 16147
2011-08-25 18:00 mot Note Edited: 16138 View Revisions
2011-08-25 18:01 mot Note Edited: 16136 View Revisions
2011-08-25 18:01 mot Note Edited: 16131 View Revisions
2011-08-25 18:02 mot Description Updated View Revisions
2011-08-25 20:38 mot Assigned To => mot
2011-08-25 20:38 mot Status new => feedback
2011-08-25 21:42 peterhol Note Added: 16150
2011-08-25 21:42 peterhol Status feedback => assigned
2011-08-25 23:20 mot Note Added: 16153
2011-08-25 23:36 peterhol Note Added: 16154
2011-08-26 12:47 peterhol Note Added: 16157
2011-08-26 12:50 peterhol Note Edited: 16157 View Revisions
2011-08-26 12:53 peterhol Note Edited: 16157 View Revisions
2011-08-26 15:23 peterhol File Added: diff-common_functions.php.txt
2012-06-21 14:22 c_schmitz Assigned To mot =>
2012-06-21 14:22 c_schmitz Status assigned => confirmed