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
Comment | File | Size | Author |
---|---|---|---|
#112 | issue-2507495-reroll_diff-109_112.txt | 1.09 KB | BrankoC |
#112 | backup_migrate-n2507495-112.patch | 2.15 KB | BrankoC |
| |||
#109 | backup_migrate-n2507495-interdiff-103_109.txt | 2.08 KB | BrankoC |
#109 | backup_migrate-n2507495-109.patch | 2.16 KB | BrankoC |
| |||
#103 | backup_migrate-n2507495-103.patch | 2.14 KB | BrankoC |
|
Comments
Comment #1
Web.Pergamonteam CreditAttribution: Web.Pergamonteam commentedComment #2
Web.Pergamonteam CreditAttribution: Web.Pergamonteam commentedComment #3
DavidoffC CreditAttribution: DavidoffC commentedI 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.
Comment #4
szczesuil CreditAttribution: szczesuil commentedHi, 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?
Comment #5
mesnitu CreditAttribution: mesnitu commentedHi, same here, any work around on this ?
Comment #6
lesliewagner1165 CreditAttribution: lesliewagner1165 commentedI'm having the same problem. May be related to PHP, as I just updated my PHP to 5.5.
Comment #7
boho999 CreditAttribution: boho999 commentedI have tried replacing the possible double lines with the following code:
but this seems to mess up the attachment. Any ideas on how this can be done?
Comment #8
crantok CreditAttribution: crantok commentedThe attached patch works for me. It just stops consecutive newlines being added.
Comment #9
Tranko CreditAttribution: Tranko as a volunteer commentedIt works for me too!
Comment #10
Pascal.s CreditAttribution: Pascal.s commentedPatch also works for me!
Comment #11
crantok CreditAttribution: crantok commentedThat's two successful patch uses (three if you include mine) and no reported problems in 5 weeks. Is that enough to tag this RTBC?
Comment #12
szczesuil CreditAttribution: szczesuil commented#8 worked for me, as well. thank you very much, crantok.
Comment #13
szczesuil CreditAttribution: szczesuil commentedChanging status to RTBC :))
Comment #14
Lemontonix CreditAttribution: Lemontonix commentedNOT working with me.
My configuration:
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:
Comment #15
drugan CreditAttribution: drugan commentedThe #8 patch worked for me on a shared hosting with the following:
Comment #16
DD 85 CreditAttribution: DD 85 commentedDrupal 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.
Comment #17
sdowney2002 CreditAttribution: sdowney2002 commentedPatch #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
Comment #18
aotus CreditAttribution: aotus commentedPatch #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
Comment #19
aotus CreditAttribution: aotus commentedComment #20
MrPaulDriver CreditAttribution: MrPaulDriver commentedI've just encountered this for the first time. New server running php 5.5
Comment #21
ckngPatch #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
Comment #22
rothlive CreditAttribution: rothlive commented@ckng
I have delete some code and it works nearly.
It comes only alwas 2 mails.
My canged code :
Comment #23
DamienMcKennaThanks 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.
Comment #24
HansKuiters CreditAttribution: HansKuiters commentedTested patch from #8. E-mails are sent fine now, no errors in the log. Didn't use the code from #22.
Comment #25
Lemontonix CreditAttribution: Lemontonix commentedNeither #8 nor #22 working for me :-(
Comment #26
havanablackcu1 CreditAttribution: havanablackcu1 commentedNeither #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 ..."
Comment #27
myDrupal2014_846824658246 CreditAttribution: myDrupal2014_846824658246 commentedTested 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
Comment #28
felixmf CreditAttribution: felixmf commentedTested 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)
Comment #29
crantok CreditAttribution: crantok commentedMy 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?Comment #30
mgstablesTested 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
Comment #31
crantok CreditAttribution: crantok commented@mgstables thanks for trying. Sorry it didn't work.
Comment #32
toomanypets CreditAttribution: toomanypets commentedApplied #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!
Comment #33
NickDickinsonWildeI 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.
Comment #34
Oleg Ko CreditAttribution: Oleg Ko commentedI 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 tobody section
.He uses
\r\n
. So, if you need just\n
, you should change it.PS. Sorry, for my English.
Comment #35
nicorac CreditAttribution: nicorac as a volunteer commentedHere'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 firstContent-Type: multipart/mixed; boundary="xxxxx"
header, while other MIME headers (and contents) must go in$message
.CHANGES:
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.
mail()
parameters and avoid mixing headers."
with'
where string interpolation is not needed.I'm attaching both the patch and the patched file.
Comment #36
nicorac CreditAttribution: nicorac as a volunteer commentedComment #37
nicorac CreditAttribution: nicorac as a volunteer commentedSorry, wrong files added.
Please consider the new ones here...
Comment #38
mgstablesTested 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
Comment #39
dhalbert CreditAttribution: dhalbert as a volunteer commentedWorked for me too: PHP 5.6.28, Drupal 7.53, Backup and Migrate 7.x-3.1.
Comment #40
DD 85 CreditAttribution: DD 85 commented#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.
Comment #41
DD 85 CreditAttribution: DD 85 commentedComment #42
knalstaaf CreditAttribution: knalstaaf commentedIt seems to work, but we're still getting these errors:
Comment #43
knalstaaf CreditAttribution: knalstaaf commentedWhile saving a schedule using this, we're getting this error (unless we're picking a shorter machine name):
Comment #44
couturier CreditAttribution: couturier as a volunteer commentedPlease try upgrading to the new 7.x-3.2 version and see if this affects your issue.
Comment #45
couturier CreditAttribution: couturier as a volunteer commentedClosing after more than two weeks with no activity.
Comment #46
DD 85 CreditAttribution: DD 85 commentedComment #47
DD 85 CreditAttribution: DD 85 commentedUpdated 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
Comment #48
DamienMcKennaGiven we already have some patches..
Comment #49
mtoscano CreditAttribution: mtoscano commented#37 worked for me
Drupal 7.52
Backup and Migrate 7.x-3.1
PHP 5.4.45
Comment #50
BrankoC CreditAttribution: BrankoC as a volunteer commentedThe 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.
Comment #51
couturier CreditAttribution: couturier as a volunteer commented@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.
Comment #52
lorisbel CreditAttribution: lorisbel as a volunteer commentedTested 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.
Comment #53
lorisbel CreditAttribution: lorisbel as a volunteer commentedSorry... Here the fixed patch
Comment #54
lorisbel CreditAttribution: lorisbel as a volunteer commentedComment #55
couturier CreditAttribution: couturier as a volunteer commented@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.
Comment #56
BrankoC CreditAttribution: BrankoC as a volunteer commentedPatch #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.)
Comment #57
couturier CreditAttribution: couturier as a volunteer commentedI 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."
Comment #58
DamienMcKennaAh! 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.
Comment #59
BrankoC CreditAttribution: BrankoC as a volunteer commentedWhat 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"?
Comment #60
DamienMcKennaYes - we don't need to break compatibility with PHP 5.3 just for a minor syntax change.
Comment #61
DamienMcKennaEmail handling can be tested relatively easily, see the Maillog and SMTP modules for examples.
Comment #62
BrankoC CreditAttribution: BrankoC as a volunteer commentedThe 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.
Comment #63
DamienMcKennaDrupal 7 doesn't work with PHP 4, so that point is moot.
Here's a fixed patch.
Comment #64
BrankoC CreditAttribution: BrankoC as a volunteer commentedI 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
Comment #65
mgstablesAfter 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
Comment #66
BrankoC CreditAttribution: BrankoC as a volunteer commentedIs there anything else we can do to get this patch applied?
Comment #67
lgough CreditAttribution: lgough commentedPatch #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
Comment #68
DamienMcKennaThe patch needs to be rerolled.
Comment #69
Tranko CreditAttribution: Tranko as a volunteer commentedHi 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.
Comment #70
BrankoC CreditAttribution: BrankoC as a volunteer commentedReroll of #64.
Comment #71
BrankoC CreditAttribution: BrankoC as a volunteer commentedComment #72
DamienMcKennaThere'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.
Comment #73
BrankoC CreditAttribution: BrankoC as a volunteer commentedWriting tests is currently not one of my skills, so someone else will have to pick that up.
Comment #74
DamienMcKennaNo worries, thanks for helping to push this along.
Comment #75
jakeru CreditAttribution: jakeru as a volunteer commentedI 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 =(
Comment #76
DamienMcKenna@jakeru: now you can :)
Comment #77
jakeru CreditAttribution: jakeru as a volunteer commentedThank 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.
Comment #78
DamienMcKennaThanks @jakeru, that's a much better patch.
Would someone please be able to manually test patch #77? Thanks.
Comment #79
Frosty29 CreditAttribution: Frosty29 commentedHi @DamienMcKenna, i have manually applied #77 and it works for me. (I was using #70 until now.) Thanks @brankoc and @jakeru.
Comment #80
mgstablesI have also manually applied #77 and it works for me to. Thanks.
Comment #81
BrankoC CreditAttribution: BrankoC as a volunteer commentedI 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:
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()?
Comment #82
jakeru CreditAttribution: jakeru as a volunteer commentedHi @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.
Comment #83
BrankoC CreditAttribution: BrankoC as a volunteer commentedThanks 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.
Comment #84
BrankoC CreditAttribution: BrankoC as a volunteer commentedHm, the patch did not upload. Let us try again. Also the numbering is wrong, should be #83.
Comment #85
DamienMcKennaI'll review this soon, but the test coverage puts this into contention to be added to the next release. Thanks!
Comment #86
BrankoC CreditAttribution: BrankoC as a volunteer commentedI 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).
Comment #87
DamienMcKennaChanging 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.
Comment #88
BrankoC CreditAttribution: BrankoC as a volunteer commentedThis 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.
Comment #90
BrankoC CreditAttribution: BrankoC as a volunteer commentedThis changes line 143 of BmTestDestinations.test to test for \r\n instead of \n, for the rest you can use the interdiff of #88.
Comment #92
BrankoC CreditAttribution: BrankoC as a volunteer commentedMaybe 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.
Comment #93
DamienMcKennaWorking on this.
Comment #94
DamienMcKennaDid 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.
Comment #95
DamienMcKennaSome tidying up, and I renamed the file because I think this could focus just on the email handling instead of all destinations.
That said, the test itself will still fail.
Comment #97
BrankoC CreditAttribution: BrankoC as a volunteer commentedI 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.
Comment #98
BrankoC CreditAttribution: BrankoC as a volunteer commentedComment #99
DamienMcKennaThis 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!
Comment #101
DamienMcKennaCommitted. Thanks everyone!
Let's work on any follow-up tasks in new issues.
Comment #102
DamienMcKenna@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.
Comment #103
BrankoC CreditAttribution: BrankoC as a volunteer commentedThis 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.
Comment #104
BrankoC CreditAttribution: BrankoC as a volunteer commentedComment #105
Frosty29 CreditAttribution: Frosty29 commentedFurther 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.
Comment #106
BrankoC CreditAttribution: BrankoC as a volunteer commented@Frosty29, could it be that you ran into this problem: Google Chrome decompresses database backup while keeping the .gz suffix?
Comment #107
Frosty29 CreditAttribution: Frosty29 commented@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.
Comment #108
Frosty29 CreditAttribution: Frosty29 commentedWhat 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!
Comment #109
BrankoC CreditAttribution: BrankoC as a volunteer commentedI 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.
Comment #110
wylbur CreditAttribution: wylbur at Electric Citizen commented@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.
Comment #111
wylbur CreditAttribution: wylbur at Electric Citizen commentedComment #112
BrankoC CreditAttribution: BrankoC as a volunteer commentedGood catch, thank you!
Please find attached a reroll.
Comment #113
BrankoC CreditAttribution: BrankoC as a volunteer commentedComment #115
DamienMcKennaCommitted. Thanks again.