I am getting the following error when the cron job is run to send newsletters.

* warning: Invalid argument supplied for foreach() in /home/cbenoist/public_html/drupal-6.2/includes/mail.inc on line 181.
* warning: Invalid argument supplied for foreach() in /home/cbenoist/public_html/drupal-6.2/includes/mail.inc on line 181.
* warning: pg_query() [function.pg-query]: Query failed: ERROR: syntax error at or near ")" at character 50 in /home/cbenoist/public_html/drupal-6.2/includes/database.pgsql.inc on line 138.
* user warning: query: DELETE FROM simplenews_mail_cache WHERE mcid IN () in /home/cbenoist/public_html/drupal-6.2/sites/all/modules/simplenews/simplenews.module on line 1470.

The code is referencing the $message variable at various locations, but this isn't the root cause of this issue. I tracked this one down to the following line of code in the simplenews_mail_cache_get method.

$messages[$data->mcid] = unserialize($data->message);

It looks like the unserialize method is failing. So I read up on unserialize and discovered that if this method fails it will return FALSE. I am submitting the following patch to allow the error log to show the fact that this method returned FALSE and is in error. Hopefully the information I am writing to the log here will help us debug the issue.

CommentFileSizeAuthor
#7 simplenews.260775.patch19.52 KBsutharsan
#5 simplenews_code_serializebase64.patch1.1 KBcbenoist
#2 simplenews_code_unserialize_is_array.patch808 bytescbenoist
simplenews_code_unserializeFALSE.patch809 bytescbenoist

Comments

cbenoist’s picture

cbenoist commented

I forgot to mention in my first post that the error message may need some work. It really doesn't make sense that way it is worded in my patch, database should read table. I also wasn't sure if there was a standard way of writing errors to the logs.

cbenoist’s picture

cbenoist commented
StatusFileSize
new simplenews_code_unserialize_is_array.patch808 bytes

The way I was checking false was not the best way to do this check in the previous patch. I fixed the check and the wording noted in my previous post.

sutharsan’s picture

sutharsan commented
Status: Active » Postponed (maintainer needs more info)

Thanks for reporting.

Do you know or can you reproduce the cause of this data is the cache table?
Can we prevent it in code? If not, than we need a solution to prevent errors.

A technical message like this in the watchdog is IMO not very usefull. It should point the admin to the problem and if possible suggest a solution.
Drupal does not use camel case for variable names.

cbenoist’s picture

cbenoist commented
Status: Postponed (maintainer needs more info) » Active

Here is the string that I was trying to unserialize that failed.

a:7:{s:2:"id";s:15:"simplenews_node";s:2:"to";s:25:"cbenoist@cliffbenoist.com";s:4:"from";s:53:""Drupal Development Site" ";s:8:"language";O:8:"stdClass":11:{s:8:"language";s:2:"en";s:4:"name";s:7:"English";s:6:"native";s:7:"English";s:9:"direction";i:0;s:7:"enabled";i:1;s:7:"plurals";i:0;s:7:"formula";s:0:"";s:6:"domain";s:0:"";s:6:"prefix";s:0:"";s:6:"weight";i:0;s:10:"javascript";s:0:"";}s:7:"subject";s:44:"[Drupal Development Site newsletter] testing";s:4:"body";s:204:"-------- TESTING \012-------------------------------------------------------------\012\012test\012\012\012\012\012\012--\012Unsubscribe from this newsletter: \012http://cliffbenoist.com/drupal-6.2/newsletter/confirm/remove/3555b868625t";s:7:"headers";a:11:{s:12:"MIME-Version";s:3:"1.0";s:12:"Content-Type";s:51:"text/plain; charset=UTF-8; format=flowed; delsp=yes";s:25:"Content-Transfer-Encoding";s:4:"8Bit";s:8:"X-Mailer";s:6:"Drupal";s:9:"Errors-To";s:25:"cbenoist@cliffbenoist.com";s:11:"Return-Path";s:25:"cbenoist@cliffbenoist.com";s:6:"Sender";s:25:"cbenoist@cliffbenoist.com";s:8:"Reply-To";s:53:""Drupal Development Site" ";s:4:"From";s:53:""Drupal Development Site" ";s:10:"Precedence";s:4:"bulk";s:16:"List-Unsubscribe";s:75:"";}}

I have a feeling it may be the quotes in the "From" string that are causing the problem. I will have to whip up a simple php page to give that theory a try, won't get to the test until later tonight EST. We should be able to fix this problem in code if it has to do with the data we are storing.

Thanks for the info on the variable naming. I will be sure to watch out for that in the future.

cbenoist’s picture

cbenoist commented
StatusFileSize
new simplenews_code_serializebase64.patch1.1 KB

I put together a small tester page to check this out. It looks like the we have a bit of a problem with the body in this message. The number of characters in the body string isn't correct. It has 204, but should have 235. Once I changed this length of string value in the unserialize worked. So the issue isn't the unserialize or serialize. It appears to be the database converting line feed characters into "\012", this could be a result of having an ascii database. I think it is a good idea to fix it regardless of this fact.

So the solution to fix this problem is fairly simple. We just need to convert the string going into the database into base64. This avoids any conversions in the serialized string when inserting into the database. Then on the way out we convert back to a normal string and unserialize. This change is a breaking change for the data stored previously. Not sure how we can handle the old data that is not encoded.

After all this the cron email job is now working as I would expect it to work.

sutharsan’s picture

sutharsan commented

Base64-encoded data takes about 33% more space than the original data (php.net). This brings up an inherent problem with the cashing solution I chose. When sending an email ALL emails are generated and stored in the database. On each cron run a number of record retrieved and are send. To store ALL emails may cause a problem with large mailing lists, several simultaneous mailings and long emails. Increasing this further with a 33% may not be a good idea.

A solution could be to use the cache (or spool, if you want) to store only nid, vid and user data. On each cron run a number of records are retrieved from the cache, emails are rendered and send immediately. This eliminates the use of base64 encoding for the mail body and reduces the database load. The downside is less efficient email processing.

sutharsan’s picture

sutharsan commented
Status: Active » Needs review
StatusFileSize
new simplenews.260775.patch19.52 KB

The attached patch should solve this problem using the above described method.
By using a spool I opened possibilities to do statistics of send mails. But this is for the future.

If you upgrade your site with this patch, make sure all newsletter mails are send before updating. Otherwise all pending emails will be lost!

It is a fundamental change in the code so I like to have your test results before I commit this patch.

cbenoist’s picture

cbenoist commented

Patch tested and verified. Error no longer occurs on cron run.

We will have to keep an eye on performance. It may effect the number of newsletters per cron run.

I haven't looked at the details much, but can't we just format the body one time and send the newsletter to everyone from the same generated newsletter body? If we could group the newsletters being sent on a cron run we could remove the need to regenerate the body for every email sent. That may speed things up. That may even make this option faster than the previous caching option. The spool method of doing it does open this possibility.

sutharsan’s picture

sutharsan commented
Status: Needs review » Fixed

The body of the newsletter is formatted one time (per cron run) as far as possible. Because variables can be placed in a newsletter, email bodies may vary from one mail to another. Please study simplenews_mail() for this purpose.

Patch committed.

Anonymous’s picture

Anonymous (not verified) commented
Status: Fixed » Closed (fixed)

Automatically closed -- issue fixed for two weeks with no activity.