Hi, Could anyone help me ?

A few days ago I noticed that I did not recieve database on my mail. When I tried to back up manually to email and I sow an error

Warning: mail(): Multiple or malformed newlines found in additional_header in mime_mail->send() (line 138 of .../sites/all/modules/backup_migrate/includes/destinations.email.inc).

in line 138 I have this code mail(trim($this->to), $this->subject, "", $mime);

I tried to change mail destination to different mail(google,yahoo,mail.ru,yandex), but the same error

Shared hosting 1and1
Drupal 7.37
Backup and Migrate Version: 7.x-3.1
I tried with PHP 5.6, PHP 5.5, PHP 5.4

Thanks for your help

Update:

PHP was upgraded 5.5.25 to 5.5.26.
A security risk in PHP has been fixed and additional_headers parameter is now more strictly validated.
What previously worked fine, now was causing mail() to fail, though no header injection was happening. Just had some extra newlines in additional_headers.
Solution: validate additional_headers self against header injection. These count as "multiple or malformed newlines": \r\r, \r\0, \r\n\r\n, \n\n, \n\0

From

http://stackoverflow.com/questions/30887610/error-with-php-mail-multiple-or-malformed-newlines-found-in-additional-header

CommentFileSizeAuthor
#112 issue-2507495-reroll_diff-109_112.txt1.09 KBBrankoC
#112 backup_migrate-n2507495-112.patch2.15 KBBrankoC
#109 backup_migrate-n2507495-interdiff-103_109.txt2.08 KBBrankoC
#109 backup_migrate-n2507495-109.patch2.16 KBBrankoC
#103 backup_migrate-n2507495-103.patch2.14 KBBrankoC
#97 backup_migrate-n2507495-interdiff_95_97.txt6.84 KBBrankoC
#97 backup_migrate-n2507495-97.patch18.4 KBBrankoC
#95 backup_migrate-n2507495-95.patch17.7 KBDamienMcKenna
#95 backup_migrate-n2507495-95.interdiff.txt19.54 KBDamienMcKenna
#90 backup_migrate-n2507495-89.patch15.18 KBBrankoC
#88 backup_migrate-n2507495-interdiff_86_88.txt11.17 KBBrankoC
#88 backup_migrate-n2507495-88.patch15.18 KBBrankoC
#86 backup_migrate-n2507495-interdiff_82_86.txt4.36 KBBrankoC
#86 backup_migrate-n2507495-86.patch9.54 KBBrankoC
#84 backup_migrate-n2507495-82.patch7.5 KBBrankoC
#83 backup_migrate-n2507495-interdiff-77_82.txt6.13 KBBrankoC
#77 backup_migrate-n2507495-77.patch3.39 KBjakeru
#70 backup_migrate-n2507495-interdiff_64_70.txt4.57 KBBrankoC
#70 backup_migrate-n2507495-70.patch7.83 KBBrankoC
#64 backup_migrate-n2507495-64.interdiff.txt1.64 KBBrankoC
#64 backup_migrate-n2507495-64.patch7.81 KBBrankoC
#63 backup_migrate-n2507495-63.interdiff.txt3.42 KBDamienMcKenna
#63 backup_migrate-n2507495-63.patch7.62 KBDamienMcKenna
#53 fix_issue-2507495-52.patch8.37 KBlorisbel
#52 fix_issue-2507495-52.patch8.21 KBlorisbel
#8 fix_malformed_newlines-2507495-8.patch1.47 KBcrantok
destinations.email_.inc .txt4.97 KBrothlive
#33 fix_malformed_newlines-2507495-33.patch3.67 KBNickDickinsonWilde
#34 destinations.email_.zip1.83 KBOleg Ko
#35 destinations.email_.inc_.zip2.22 KBnicorac
#35 0001-Fixed-MIME-headers-error-in-newer-PHP-versions.patch5.91 KBnicorac
#37 0002-Fixed-MIME-headers-error-in-newer-PHP-versions.patch5.89 KBnicorac
#37 destinations.email_.inc_.zip2.21 KBnicorac
#41 bm2.jpg139.6 KBDD 85
#41 bm1.jpg137.22 KBDD 85
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Web.Pergamonteam’s picture

Issue summary: View changes
Web.Pergamonteam’s picture

Issue summary: View changes
DavidoffC’s picture

I also see these errors appearing for some time now, Backup and Migrate doesn't e-mail backups any more but saves them normally to a server's folder, PHP version is 5.4.42 and this issue happens with different servers/hosting providers, all of them shared.

Error message is:
'Warning: mail(): Multiple or malformed newlines found in additional_header in mime_mail->send() (line 138 of /var/www/vhosts/x/httpdocs/sites/all/modules/backup_migrate/includes/destinations.email.inc).'

Backup files are up to 10 megabytes in size.

Also looking for help.

szczesuil’s picture

Hi, email backups stopped working a couple weeks ago and I'm also getting this warning:

mail(): Multiple or malformed newlines found in additional_header in mime_mail->send() (line 138 of .../sites/all/modules/backup_migrate/includes/destinations.email.inc).

So: I upgraded to Version: 7.x-3.1 and am running Drupal 7.38.

When I manual try to email a backup, I get a success message:

Default Database backed up successfully to MyDomain-2015-07-21T09-32-06 (1.43 MB) in destination email attachment in 4 sec.

Although, no email is sent.

The scheduled backup emails had been working fine for months. Just up and stopped. Any ideas?

mesnitu’s picture

Hi, same here, any work around on this ?

lesliewagner1165’s picture

I'm having the same problem. May be related to PHP, as I just updated my PHP to 5.5.

boho999’s picture

I have tried replacing the possible double lines with the following code:


...
$mime = str_replace(array("\r\r", "\r\0", "\r\n\r\n", "\n\n", "\n\0"),array("\r", "\r", "\r\n", "\n", "\n"),$mime);
...

but this seems to mess up the attachment. Any ideas on how this can be done?

crantok’s picture

Category: Support request » Bug report
Status: Active » Needs review
FileSize
1.47 KB

The attached patch works for me. It just stops consecutive newlines being added.

Tranko’s picture

It works for me too!

Pascal.s’s picture

Patch also works for me!

crantok’s picture

That's two successful patch uses (three if you include mine) and no reported problems in 5 weeks. Is that enough to tag this RTBC?

szczesuil’s picture

#8 worked for me, as well. thank you very much, crantok.

szczesuil’s picture

Status: Needs review » Reviewed & tested by the community

Changing status to RTBC :))

Lemontonix’s picture

NOT working with me.

My configuration:

  • Drupal 7.41
  • PHP 5.6.14
  • Backup Migrate 3.1 & 3.1+6-dev

Before appliying the match I was not able to send an eMail at all with warning described above.
After applying the patch there is no warning any more and an eMail was sent, but the EMail did not contain the attachment.

It looks like the EMail is not composed correctly:

  • Thunderbird indicates that eMail has no attachment
  • GMail indicates that in the Inbox that there is an eMail with attachment, but when you open the eMail the attachment is lost
drugan’s picture

The #8 patch worked for me on a shared hosting with the following:

  • PHP 5.4.45
  • Drupal 7.41
  • Backup Migrate 7.x-3.1
DD 85’s picture

Drupal 7.41
Backup and Migrate 7.x-3.1
Tried #8 patch with the following versions of PHP
PHP 5.4.45
PHP 5.5.30
PHP 5.6.14
The letter comes with a damaged empty archive.

sdowney2002’s picture

Patch #8 not working for me. .gz archive is attached to email, but the archive is empty.

Drupal 7.41
Backup & Migrate 7.x-3.1
PHP 5.4.43

aotus’s picture

Patch #8 doesn't work for me either on multiple sites, all emailed archives are zero bytes
Drupal 7.41
Backup & Migrate 7.x-3.1
PHP 5.5.30
The failures started last June after upgrading Drupal 7.37 to 7.38

aotus’s picture

Status: Reviewed & tested by the community » Active
MrPaulDriver’s picture

I've just encountered this for the first time. New server running php 5.5

ckng’s picture

Version: 7.x-3.1 » 7.x-3.x-dev

Patch #8 not working for me as well, mail sending failed.

backup_migrate-7.x-3.1+6-dev
PHP 7.0.3

I believe this is related to https://bugs.php.net/bug.php?id=68776

rothlive’s picture

@ckng
I have delete some code and it works nearly.
It comes only alwas 2 mails.
My canged code :

<?php


/**
 * @file
 * Functions to handle the email backup destination.
 */

/**
 * A destination for emailing database backups.
 *
 * @ingroup backup_migrate_destinations
 */
class backup_migrate_destination_email extends backup_migrate_destination {
  var $supported_ops = array('scheduled backup', 'manual backup', 'remote backup', 'configure');

  /**
   * Save to (ie. email the file) to the email destination.
   */
  function save_file($file, $settings) {
    $size = filesize($file->filepath());
    $max = variable_get('backup_migrate_max_email_size', 20971520);
    if ($size > $max) {
      _backup_migrate_message('Could not email the file @file because it is @size and Backup and Migrate only supports emailing files smaller than @max.', array('@file' => $file->filename(), '@size' => format_size($size), '@max' => format_size($max)), 'error');
      return FALSE;
    }
    $attachment = new stdClass();
    $attachment->filename = $file->filename();
    $attachment->path = $file->filepath();
    _backup_migrate_destination_email_mail_backup($attachment, $this->get_location());
    return $file;
  }

  /**
   * Get the form for the settings for this filter.
   */
  function edit_form() {
    $form = parent::edit_form();
    $form['location'] = array(
      "#type" => "textfield",
      "#title" => t("Email Address"),
      "#default_value" => $this->get_location(),
      "#required" => TRUE,
      "#description" => t('Enter the email address to send the backup files to. Make sure the email sever can handle large file attachments'),
    );
    return $form;
  }

  /**
   * Validate the configuration form. Make sure the email address is valid.
   */
  function settings_form_validate($values) {
    if (!valid_email_address($values['location'])) {
      form_set_error('[location]', t('The e-mail address %mail is not valid.', array('%mail' => $form_state['values']['location'])));
    }
  }
}

/**
 * @function
 * Temporary mail handler class.
 *
 * Defines a mail class to send a message with an attachment. Eventually Drupal
 * core should provide this functionality, at which time this code will be
 * removed.
 *
 * More info on sending email at <http://php.net/function.mail>.
 * This function taken from dba.module.
 *
 * @param $attachment
 *   An object which contains two variables "path" the path to the file and
 *   filename and "filename" which is just the filename.
 */
function _backup_migrate_destination_email_mail_backup($attachment, $to) {
  // Send mail
  $attach        = fread(fopen($attachment->path, "r"), filesize($attachment->path));
  $mail          = new mime_mail();
  $mail->from    = variable_get('site_mail', ini_get('sendmail_from'));
  $mail->headers = 'Errors-To: [EMAIL='. $mail->from .']'. $mail->from .'[/EMAIL]';
  $mail->to      = $to;
  $mail->subject = t('Database backup from !site: !file', array('!site' => variable_get('site_name', 'drupal'), '!file' => $attachment->filename));
  $mail->body    = t('Database backup attached.') ."\n\n";

  $mail->add_attachment("$attach", $attachment->filename, "Content-Transfer-Encoding: base64 /9j/4AAQSkZJRgABAgEASABIAAD/7QT+UGhvdG9zaG", NULL, TRUE);
  $mail->send();
}

class mime_mail {
  var $parts;
  var $to;
  var $from;
  var $headers;
  var $subject;
  var $body;

  function mime_mail() {
    $this->parts   = array();
    $this->to      = "";
    $this->from    = "";
    $this->headers = "";
    $this->subject = "";
    $this->body    = "";
  }

  function add_attachment($message, $name = "", $ctype = "application/octet-stream", $encode = NULL, $attach = FALSE) {
    $this->parts[] = array(
      "ctype" => $ctype,
      "message" => $message,
      "encode" => $encode,
      "name" => $name,
      "attach" => $attach,
    );
  }

  function build_message($part) {
    $content_type = "Content-Type: ". $part["ctype"] . ($part["name"] ? "; name = \"". $part["name"] ."\"" : "");
    $encoding = "Content-Transfer-Encoding: base64";
     $disposition = $part['attach'] ? "Content-Disposition: attachment; filename=$part[name]\n" : '';
    $message  = chunk_split(base64_encode($part["message"]));
   return "$content_type\n$encoding\n$disposition$message";
   
   
  }

  function build_multipart() {
   
    $boundary = "b". md5(uniqid(time()));
    $multipart = "Content-Type: multipart/mixed; boundary = $boundary\nThis is a MIME encoded message.\n--$boundary";
   for ($i = sizeof($this->parts) - 1; $i >= 0; $i--) {
       $multipart .= "\n". $this->build_message($this->parts[$i]) ."--$boundary";
     }
    return $multipart .= "--\n";
  }

  function send() {
    $mime = "";
    if (!empty($this->from)) $mime .= "From: ". $this->from ."\n";
    if (!empty($this->headers)) $mime .= $this->headers ."\n";
    if (!empty($this->body)) $this->add_attachment($this->body, "", "text/plain");
    $mime .= "MIME-Version: 1.0\n". $this->build_multipart();
    mail(trim($this->to), $this->subject, "", $mime);
  }
}
DamienMcKenna’s picture

Status: Active » Needs review

Thanks for the patch and follow-up, hopefully one of the maintainers will get to it soon.

BTW, please remember to set the issue status to "needs review" when you upload a patch, that helps everyone else know there's a possible fix for the bug.

HansKuiters’s picture

Tested patch from #8. E-mails are sent fine now, no errors in the log. Didn't use the code from #22.

Lemontonix’s picture

Neither #8 nor #22 working for me :-(

havanablackcu1’s picture

Neither #8 nor #22 working for me too!
But with the code on this page rothlive.de I've eliminated the "Warning: mail(): Multiple or malformed newlines found in additional_header in mime_mail->send() (line 138 ..."

myDrupal2014_846824658246’s picture

Tested patch from #8. Not working.
Php 5.5.31
Drupal 7.43
Backup and Migrate (backup_migrate) Version: 7.x-3.1

Tested patch from #22. Not working. Message is gone, log message is oke but not file in email
Php 5.5.31
Drupal 7.43
Backup and Migrate (backup_migrate) Version: 7.x-3.1

felixmf’s picture

Tested patch from #8. E-mails are sent fine now, no errors in the log. Didn't use the code from #22.

Thanks again.

(El patch de #8 ha solucionado el error en mi caso)

crantok’s picture

My patch in #8 has only worked for some of the people who tried it.

I've been doing some work on email in another system written in Go and I notice that \r\n has been used in all line breaks. My patch (and the code it replaced) used \n for line breaks. Does anyone know whether this might be why some people have not found the patch effective and other have? e.g. different mail servers being differently strict about line breaks?

Would someone who has not found the patch useful like to try changing the line breaks to \r\n and see whether that fixes things for them and then report back?

mgstables’s picture

Tested patch from #8 with changing all line breaks to \r\n
E-Mail is send, log message is oke, but no Backup file in the email.
PHP 5.6.24
Drupal 7.50
Backup and Migrate, Version: 7.x-3.1

crantok’s picture

@mgstables thanks for trying. Sorry it didn't work.

toomanypets’s picture

Applied #8 to 7.x-3.1 -- no problems.
CentOS 6.8 + Apache 2.2.26 + PHP 7.0.9 (CGI/FastCG).
Thanks for the patch!

NickDickinsonWilde’s picture

I like a few other people, experienced this bug and the patch from #8: Let users cancel their accounts didn't work. So I did some deep-ish readings of RFCs and PHP docs and found:
1. The standard is \r\n
2. BUT some rare MTA (Mail Transfer Agents) especially on Windows may fail unless it is just \n
3. Despite the standard being \r\n some MTAs will process \n okay.

Conclusion: Backup & Migrate should use \r\n by default *but* easily swap to \n only.

Patch attached. All that is needed to switch to \n is to add:
`$conf['backup_migrate_eol'] = "\n";`
to your site's settings.php.

Anyways, anyone who has or hasn't had the patch from #8 work give this a try. Did do some extra changes to ensure that all possibilities are covered for this bug (hopefully) and a bit of standards only fixes within the already touched functions.

Oleg Ko’s picture

I tried patch from #8 and patch from #33. In my case I was able to send email. But my Thunderbird couldn't read message and attachment (though they are both in the letter).

So, my associate (he is good in PHP) changed destinations.email.inc for me. Now it works fine for me. And it is attached.

He told that this issue happened because the script put message and attachment into main section of the letter. So, he moved it to body section.

He uses \r\n. So, if you need just \n , you should change it.

PS. Sorry, for my English.

nicorac’s picture

Here's my attempt to fix the issue.
New PHP versions emit a warning because both $message and $additional_headers contain headers.
In MIME messages the $additional_header must contain only the first Content-Type: multipart/mixed; boundary="xxxxx" header, while other MIME headers (and contents) must go in $message.

CHANGES:

  • Added a define at the beginning of file to let users with really old mail servers switch from \r\n to \n.
    This should be a configuration parameter in the future.
  • Rewritten MIME build functions to correctly assign mail() parameters and avoid mixing headers.
  • Added Drupal styled comments to new functions.
  • Replaced " with ' where string interpolation is not needed.

I'm attaching both the patch and the patched file.

nicorac’s picture

nicorac’s picture

Sorry, wrong files added.
Please consider the new ones here...

mgstables’s picture

Tested patch from #37, works great!

Mail is send
Log message is oke
Backup file attached to the email

PHP 5.6.24
Drupal 7.52
Backup and Migrate, Version: 7.x-3.1

I am happy! Thank you @nicorac

dhalbert’s picture

Worked for me too: PHP 5.6.28, Drupal 7.53, Backup and Migrate 7.x-3.1.

DD 85’s picture

#37 working.
Drupal 7.53
Backup and Migrate 7.x-3.1
PHP 5.3, 5.4, 5.5, 5.6

But on the page admin/config/system/backup_migrate changed settings.
Paragraph «E-mail» moved from the Offsite in Locally. It is not comfortable. It is impossible to send to E-mail and Download checkbox.
It is necessary to return the item «E-mail» in Offsite. Or insert «E-mail» and Offsite and Locally.

DD 85’s picture

knalstaaf’s picture

It seems to work, but we're still getting these errors:

Notice: Use of undefined constant MAIL_EOL - assumed 'MAIL_EOL' in require_once() (line 3 of sites/all/modules/contrib/backup_migrate/includes/destinations.email.inc).
Notice: Undefined variable: encode in mime_mail->add_attachment() (line 126 of sites/all/modules/contrib/backup_migrate/includes/destinations.email.inc).
Notice: Undefined variable: encode in mime_mail->add_attachment() (line 126 of sites/all/modules/contrib/backup_migrate/includes/destinations.email.inc).
Notice: Use of undefined constant MAIL_EOL - assumed 'MAIL_EOL' in require_once() (line 3 of sites/all/modules/contrib/backup_migrate/includes/destinations.email.inc).

knalstaaf’s picture

Status: Needs review » Needs work

While saving a schedule using this, we're getting this error (unless we're picking a shorter machine name):

Notice: Use of undefined constant MAIL_EOL - assumed 'MAIL_EOL' in require_once() ( 3 of /sites/all/modules/contrib/backup_migrate/includes/destinations.email.inc).
PDOException: SQLSTATE[22001]: String data, right truncated: 1406 Data too long for column 'machine_name' at row 1: INSERT INTO {backup_migrate_schedules} (machine_name, name, source_id, destination_id, copy_destination_id, profile_id, keep, period, enabled, cron, cron_schedule) VALUES (:db_insert_placeholder_0, :db_insert_placeholder_1, :db_insert_placeholder_2, :db_insert_placeholder_3, :db_insert_placeholder_4, :db_insert_placeholder_5, :db_insert_placeholder_6, :db_insert_placeholder_7, :db_insert_placeholder_8, :db_insert_placeholder_9, :db_insert_placeholder_10); Array ( [:db_insert_placeholder_0] => wekelijks_backup_mailen_naar_it_partner [:db_insert_placeholder_1] => Mail weekly backup [:db_insert_placeholder_2] => db [:db_insert_placeholder_3] => ict_denp [:db_insert_placeholder_4] => [:db_insert_placeholder_5] => default [:db_insert_placeholder_6] => 0 [:db_insert_placeholder_7] => 604800 [:db_insert_placeholder_8] => 1 [:db_insert_placeholder_9] => builtin [:db_insert_placeholder_10] => 0 4 * * * ) in drupal_write_record() ( 7383 of /includes/common.inc).

couturier’s picture

Status: Needs work » Postponed (maintainer needs more info)

Please try upgrading to the new 7.x-3.2 version and see if this affects your issue.

couturier’s picture

Status: Postponed (maintainer needs more info) » Closed (outdated)

Closing after more than two weeks with no activity.

DD 85’s picture

Status: Closed (outdated) » Active
DD 85’s picture

Updated the module to the version Backup and Migrate 7.x-3.3
I tried sending to e-mail with PHP 5.4, 5.5, 5.6, 7.0, 7.1 mail does not come.
With PHP 5.3 works, but this version is not suitable for other modules requiring a minimum version of PHP 5.4

DamienMcKenna’s picture

Status: Active » Needs work

Given we already have some patches..

mtoscano’s picture

#37 worked for me
Drupal 7.52
Backup and Migrate 7.x-3.1
PHP 5.4.45

BrankoC’s picture

The ZIP file of #37 seems to contain several changes that are not included in the patch of #37.

The complaints in #40 that #37 doesn't work, seem to be based on changes in the ZIP file that are not present in the patch.

This is confusing.

Some of the changes in the patch of #37 seem to have to do with coding style or aesthetics rather than the problem at hand. For instance, the author changes string variable assignments that use double quotes to the same assignments, but with single quotes.

I am not familiar enough with Drupal patching to know whether this is encouraged or discouraged, but it doesn't enhance the readability of the patch.

couturier’s picture

@brankoc Development resources on this module are severely limited right now, and most of the work is going toward getting a stable D8 module released. If you have the time or interest to do some work on this patch with the fixes you suggested, that would be fantastic.

lorisbel’s picture

Tested patch from #37 works great also for me!

Mail is sent
Log message is reported
Backup file correctly attached to the email

PHP 7.1.13
Drupal 7.56
Backup and Migrate, Version: 7.x-3.5

I just changed the constructor of the mime_mail class to avoid the warning: "Deprecated function: Methods with the same name as their class will not be constructors in a future version of PHP".

I suggest to consider the fixed patch attached.

lorisbel’s picture

Sorry... Here the fixed patch

lorisbel’s picture

Priority: Normal » Major
Status: Needs work » Needs review
couturier’s picture

@brankoc This patch has been fixed by @lorisbel and you are the next most recent user to comment on this issue. Would you give us a review of the new patch? This module doesn't get a lot of attention from reviewers, and patches for other issues have been sitting for months unreviewed, so if we want to get this committed we need help, and I don't utilize mail functions with the one website I still have in D7. Thanks.

BrankoC’s picture

Patch #53 works for me.

Tested on:

Locally, on Windows 10 Home: Drupal 7.58-dev, PHP 5.6.15, MariaDB 5.5.5, Apache 2.4.17.

Remote (Debian it seems): Drupal 7.56, MySQL/MariaDB/equivalent 5.5.58, PHP 5.6.27, Apache.

I performed the following tests successfully both locally and remotely: 1) Ran the Quick Backup. 2) Opened the mail in my mail client (Thunderbird 52.6.0). 3) Saved the attachment, and, rather superfluously 4) imported the database in MariaDB.

I have also successfully tested the scheduler with an e-mail destination.

If you have any specific tests you would like me to perform, please let me know.

As an aside, should the class mime_mail not be namespaced? (But perhaps not here and now.)

couturier’s picture

Status: Needs review » Reviewed & tested by the community

As an aside, should the class mime_mail not be namespaced? (But perhaps not here and now.)

I don't know the answer to this. Maybe someone else can advise.

Thanks for your help! I can't think of any further tests so I'm moving this to "reviewed and tested by the community."

DamienMcKenna’s picture

Status: Reviewed & tested by the community » Needs work

Ah! The test coverage isn't very good yet so it didn't fall over on the short array syntax.

This needs a little tidying up before it can be considered.

BrankoC’s picture

What is the problem with the short array syntax?

Is it this point from the Drupal Coding Standards: "Drupal 7 core and Drupal 7 contributed projects without an explicit PHP 5.4+ requirement must use long array syntax"?

DamienMcKenna’s picture

Yes - we don't need to break compatibility with PHP 5.3 just for a minor syntax change.

DamienMcKenna’s picture

Issue tags: +Needs tests

Email handling can be tested relatively easily, see the Maillog and SMTP modules for examples.

BrankoC’s picture

The recent name change of the constructor method of the mime_mail class in this patch also breaks compatibility with older PHP versions (PHP 4 and earlier). How compatible do we need to be?

I tested mail with mailtodisk, which may or may not be shipped standard with XAMPP. Something like mailcatcher should also work.

DamienMcKenna’s picture

Status: Needs work » Needs review
FileSize
7.62 KB
3.42 KB

Drupal 7 doesn't work with PHP 4, so that point is moot.

Here's a fixed patch.

BrankoC’s picture

I found the following 2 issues with patch #63:

Newline should be added to the end of the patched file, cf. https://www.drupal.org/docs/develop/standards/coding-standards#indenting

Recommended action: add a newline to the end of includes/destinations.email.inc.

Module defined constants should be prefixed by a uppercase version of the module name, cf. https://www.drupal.org/docs/develop/standards/coding-standards#naming

Recommended action: rename all instances of 'MAIL_EOL' to 'BACKUP_MIGRATE_MAIL_EOL'.

Tested locally, on Windows 10 Home: Drupal 7.58-dev, PHP 5.6.15, MariaDB 5.5.5, Apache 2.4.17, using XAMPP and mailtodisk.

After creating a new destination, I performed the following test successfully locally: 1) Ran the Quick Backup on the database. 2) Opened the resulting mail in my mail client (Thunderbird 52.6.0). 3) Saved the attachment (a ZIP file), and, 4) opened the ZIP file.

I wasn't sure if the reviewer is supposed to create the new patch, so I did anyway, please find attached. Above test was performed on patch #64, not #63.

https://www.drupal.org/files/issues/backup_migrate-n2507495-64.interdiff...
https://www.drupal.org/files/issues/backup_migrate-n2507495-64.patch

mgstables’s picture

After update Backup and Migrate, Problem returned.
Used patch from #64

Mail is send
Log message is oke
Backup file attached to the email

PHP 7.0.28
Drupal 7.58
Backup and Migrate, Version: 7.x-3.5

BrankoC’s picture

Is there anything else we can do to get this patch applied?

lgough’s picture

Patch #64 worked for me

Mail sent, log message present, backup attached to email

PHP 5.6.32
Drupal 7.59
Backup and Migrate version 7.x-3.5

DamienMcKenna’s picture

Status: Needs review » Needs work

The patch needs to be rerolled.

Tranko’s picture

Hi everybody,

Same issue with the last version of the 16th Dec 2018.
Patch #64 works for me (again).

PHP 7.1.25
Drupal 7.61
Backup and Migrate version 7.x-3.6 released 16 December 2018

Regards,
Tranko.

BrankoC’s picture

BrankoC’s picture

Status: Needs work » Needs review
DamienMcKenna’s picture

Status: Needs review » Needs work

There's a bunch more going into this patch that isn't related to the requested change. I think it'd be best to split off the unrelated changes into a separate issue (which we can commit first) and focus on the changes necessary for fixing this bug.

I forget what module had a good example of test coverage for mail handling, but we need it before we can commit more changes to this functionality.

BrankoC’s picture

Writing tests is currently not one of my skills, so someone else will have to pick that up.

DamienMcKenna’s picture

No worries, thanks for helping to push this along.

jakeru’s picture

I ran into the same issue. I do find patch #70 a good and working patch but in order to be able to close this issue I wrote a smaller patch, which hopefully can be merged. Unfortunately I cannot post the patch because I am not a 'confirmed' user =(

DamienMcKenna’s picture

@jakeru: now you can :)

jakeru’s picture

Thank you DamienMcKenna! This is what the attached patch is about:

According to the PHP documentation for the function mail() CRLF
should be used as line endings instead of LF, so lets do that.

Also, put the actual message in the message parameter instead
of the additional_headers parameter. Since PHP 5.4.42 the mail()
function does not like consecutive newlines.

With this patch, we should no longer get this:

Warning: mail(): Multiple or malformed newlines found in
additional_header in mime_mail->send().

Also wrap at 70 characters instead of 76 when building the MIME parts
of the message. This is also according to the documentation.

DamienMcKenna’s picture

Thanks @jakeru, that's a much better patch.

Would someone please be able to manually test patch #77? Thanks.

Frosty29’s picture

Hi @DamienMcKenna, i have manually applied #77 and it works for me. (I was using #70 until now.) Thanks @brankoc and @jakeru.

mgstables’s picture

I have also manually applied #77 and it works for me to. Thanks.

BrankoC’s picture

I have been toying a little with the Testing module to see if I could come up with tests for this change. Since I have never written these types of test before, I expect 95% of what I have done so far to be wrong.

I came up with the following test:

    // Send an attachment per e-mail.

    // Load the profile. This will be interacted with directly because otherwise
    // the number of form fields will likely make it impossible to execute
    // properly due to the max_input_vars setting defaulting to 1000.
    $profile = $this->getProfile();

    // Run a backup.
    $this->runBackup();

    // Extract path and filename.
    $files = $this->listBackupFiles();
    $key = key($files);

    if ($key === NULL || empty($files[$key]->path)) {
      // This is bad testing I bet.
      $this->assertText('No backup found.');
    }
    else {
      $file = $files[$key];

      // Copied from destinations.email.inc to build the right parameter.
      $attachment = new stdClass();
      $attachment->filename = $file->name;
      $attachment->path = $file->path;

      $this->assertTrue(_backup_migrate_destination_email_mail_backup($attachment, 'example@example.com'), 'A MIME mail with attachment was accepted.');
    }

This however requires the function _backup_migrate_destination_email_mail_backup() (defined in includes/destinations.email.inc) to return a value, which in turn requires the send() method of the mime_mail class to return a value.

Would it be bad to change these functions so they return values (i.e. TRUE or FALSE)? Currently they do not, and PHP functions that do not return a value, return NULL. I can imagine that this would break software that expects a NULL.

What would other methods be of testing _backup_migrate_destination_email_mail_backup()?

jakeru’s picture

Hi @brankoc, your test in #81 is a good start!

From what I can see it looks like a good idea to modify the send() method in the mime_mail class to capture and return the value (TRUE or FALSE) it gets from the call to mail().

You may then modify _backup_migrate_destination_email_mail_backup() so that it in turn returns the value from the call to $this->send().

However. The unit test would rely on the mail() function and that the computer that runs the test is configured to send mails. I have no idea if the Drupal unit tests machines are happy to send mails. And I do not want to be the recipient of those ;)

So. For a unit test, what we probably would want to do is to never actually call the function mail(). Instead, we could just verify that $message and $headers are correct according to the documentation for the function mail().

That would require some minor refactoring of the current code and then new code to perform the actual verification. I like the idea of a unit test and I encourage you to continue working on it. That said, I do hope that @DamienMcKenna is happy to resolve this issue without a unit test ready.

BrankoC’s picture

Thanks for the kind words.

In #72, DamienMcKenna says: "I forget what module had a good example of test coverage for mail handling, but we need it before we can commit more changes to this functionality."

I don't know what counts as a "good example of test coverage for mail handling", but I did some digging and found out several core modules use a Drupal-internal mail catcher.

Contact, uses the drupalGetMails() method from class DrupalWebTestCase to test that an e-mail has been sent. The way it seems to test sending of a mail is by submitting a contact form. The contact form used drupal_mail() to send e-mail.

OpenID, uses the drupalGetMails() method from class DrupalWebTestCase to test that an e-mail has been sent.

Simple Test tests mail capture, assertMail and drupalGetMails in SimpleTestMailCaptureTestCase.

Simple Test provides several assertions for testing mail contents: assertMail(), assertMailString(), assertMailPattern(), verboseEmail(). It seems to set up a TestingMailSystem defined in System module (system.mail.inc).

Mail capture for testing was implemented here: https://www.drupal.org/project/drupal/issues/296001

The caveat is that using this functionality requires using drupal_mail() instead of mail().

I have attached a patch that does exactly that, but that leaves the question: why are we using mail() in the first place?

One thing I had to do to make our current functionality fit drupal_mail() was shorten the name of the attachment so that it fits on a single line.

BrankoC’s picture

Hm, the patch did not upload. Let us try again. Also the numbering is wrong, should be #83.

DamienMcKenna’s picture

I'll review this soon, but the test coverage puts this into contention to be added to the next release. Thanks!

BrankoC’s picture

I have added a bunch more tests, including tests for the Add Destination form and a test that tries to e-mail a back-up through the BM main page (using the runBackup() method together with the recently created destination).

I fear most of this has little to do with the issue at hand, but it seemed a little weird testing the e-mail back without first testing the creation of destinations.

If we are to switch to drupal_mail() instead of mail(), I am not even sure what all this testing is to achieve, considering we are testing against the mail interface for testing (TestingMailSystem as defined in the System module).

DamienMcKenna’s picture

Changing from mail() to drupal_mail() is the better option in the long run. The test should confirm that the email is sent, the attachments are included, and maybe then confirm the attachments are valid backups but that could IMHO be left as a @todo item for later.

BrankoC’s picture

This latest patch contains the following:

- A new source to test against (a .txt file containing a snippet of Wordsworth), because testing against a large database creeps me out in several ways.
- Tests to see that the right boundaries and the right amount of boundaries are included in the e-mail.
- A test to see the attachment is sent to drupal_mail().
- An assertion to check something occurs the right amount of times in an e-mail.
- A function that creates a new file source.
- Small fixes of my previous patches.

The inclusion of a source means I had to adapt runBackup() so that I could set the source.

Status: Needs review » Needs work

The last submitted patch, 88: backup_migrate-n2507495-88.patch, failed testing. View results
- codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

BrankoC’s picture

Status: Needs work » Needs review
FileSize
15.18 KB

This changes line 143 of BmTestDestinations.test to test for \r\n instead of \n, for the rest you can use the interdiff of #88.

Status: Needs review » Needs work

The last submitted patch, 90: backup_migrate-n2507495-89.patch, failed testing. View results
- codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

BrankoC’s picture

Maybe somebody else could look at this? The tests work on my Windows 10 Home PC. I suspect that something is either going wrong with line endings or whitespace, but not having any ready access to a Unix-like computer, the problem is hard to track down if I cannot reproduce it.

DamienMcKenna’s picture

Assigned: Unassigned » DamienMcKenna

Working on this.

DamienMcKenna’s picture

Assigned: DamienMcKenna » Unassigned

Did some testing during the Contrib Half Hour (hello everyone!) and the regex test at the end of testAddEmailDestination() is looking for two equals symbols at the end of a line, but they don't exist.

DamienMcKenna’s picture

Status: Needs review » Needs work

The last submitted patch, 95: backup_migrate-n2507495-95.patch, failed testing. View results
- codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

BrankoC’s picture

I took a different approach to detecting the presence of an attachment in the e-mail, so this should test fine. (The approach is also a better test, IMO.)

I also fixed some bugs in the e-mail string count assertion I introduced and improved the documentation of that function.

BrankoC’s picture

Status: Needs work » Needs review
DamienMcKenna’s picture

Status: Needs review » Reviewed & tested by the community
Related issues: +#3036855: Add test coverage to ensure the backup contains what it's supposed to

This is fantastic, thanks @brankoc!

I've created a new issue for extending the test coverage to make sure that the backups contain the data we expect: #3036855: Add test coverage to ensure the backup contains what it's supposed to

So, I think this one is good to go!

  • DamienMcKenna committed f779300 on 7.x-3.x
    Issue #2507495 by brankoc, DamienMcKenna, nicorac, lorisbel, crantok,...
DamienMcKenna’s picture

Status: Reviewed & tested by the community » Fixed

Committed. Thanks everyone!

Let's work on any follow-up tasks in new issues.

DamienMcKenna’s picture

Status: Fixed » Needs work

@brankoc brought up a point - given how long people have had their sites with email delivery not working, it'd be best for administrators to have to review their configurations before it starts sending again. As such, am reopening this so we can add an update script to disable backups email destinations and leave a message for admins to review the configurations.

BrankoC’s picture

This is a patch against the most recent version of 7.3.x, which is why it only contains the update hook.

I did not know how to disable destinations, so instead this disables all schedules that contain an e-mail destination.

I am not sure how to test this. I wrote a bunch of functional tests using Simpletest (not included) and they seem to work, but I am not sure if that is the proper way to go about it.

Allternate solutions:

s1. disable the cron for the module, but that will disable all scheduled back-ups.

s2. disable the cron for enabled schedules with an e-mail destination.

The message output by the update hook needs improvement, I am sure.

I used direct database calls, because when I wrote a Simpletest test using backup_migrate_get_destinations() and backup_migrate_get_schedules(), I did not get the desired results. It has been a few days, but if I remember correctly it was the latter function that failed.

In Simpletest, db_query() queried the original database by the way, which is why I used db_select().

This will also disable scheduled back-ups in the following cases, which I assume is undesirable:

d1. When a site is hosted on 5.5.25 or earlier, which never had the problem of this issue.

d2. When a schedule has two destinations, one of which is not an e-mail destination.

BrankoC’s picture

Status: Needs work » Needs review
Frosty29’s picture

Further to #79 I had an issue with a backup I needed to use from an email. I was unable to open or extract the bz2 file within Mime email, some issue caused by manual patching I guess, which caused quite a problem. I have now switched to latest dev version (7.x-3.6+7-dev) and email backups seem to be behaving properly again. Great module, thanks, a shame this php/mail problem has wasted a lot of time it seems.

BrankoC’s picture

@Frosty29, could it be that you ran into this problem: Google Chrome decompresses database backup while keeping the .gz suffix?

Frosty29’s picture

@BrankoC i had that issue previously. (I switched to use bz now.) Regarding #105 I ended up with an email with some corrupted compressed file embedded in it (not a proper attachment) and was unable to extract it. The backup emails were the right size so i had not noticed they had become unusable! Since switching to dev version, I have no manual patching work to do and it all works well for me, for download and by email.

Frosty29’s picture

What a shame the Dev version has been updated but not released to main branch - manual update of my sites required to prevent update nags, and so I can still use email backups, as that only works in Dev branch for me!

BrankoC’s picture

I recently learned that update hooks should only contain a single update script. This patch splits backup_migrate_update_7307() into two functions, backup_migrate_update_7307() and backup_migrate_update_7308() in order to comply with that requirement.

wylbur’s picture

@BrankoC this patch has been committed, so the patch you provided will need to be rerolled with a new update number:
https://www.drupal.org/project/backup_migrate/issues/2663066

I can try that of you are not able to. Feel free to assign to me if need be.

wylbur’s picture

Status: Needs review » Needs work
BrankoC’s picture

Good catch, thank you!

Please find attached a reroll.

BrankoC’s picture

Status: Needs work » Needs review

DamienMcKenna’s picture

Status: Needs review » Fixed

Committed. Thanks again.

Status: Fixed » Closed (fixed)

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