Added new functionality: Email notification if there was error

[dafuer]

Hello,

I had many times a problem: DropBox changes something in the API and the bundle stops to work. I realised the problem after long time. So, I hadn't backup and this could be dangerous. I have added a functionality to send an email if something happen during the backup process. What do you think? It could be useful for everybody?

If you want to use it, you have to define an email account to be notified:

error_notification: 
    from: dizda@backupbundle.com
    to: your@email.com
    subject: Backup error
    body: There was a problem during backup process

Cheers,

[dizda]

Hello @dafuer,

Thanks for your PR, it's a great idea. I reviewed it, and I've made some comments, so if you can fix or tell me what you think, it will be great.

[dafuer]

Hi @dizda ,

I have followed all your comments and pulled another commit fixing everything. Thanks you very much, it was really useful. Comments:

• I have removed subject and body because when I was writing the code I was thinking in filter emails from different applications. It does not have sense because in fact they have to come from different email address (specified in from). So, fix subject + different from is good enough for filter you email inbox.
• I don't understand "Useless, you just have to throw a new Exception after line 146." sorry. Could you explain to me better?

Long live the CloudBackupBundle! :)

[dizda]

Hey @dafuer :)

Thanks for the fixes. Instead of the current code, it's more proper to do something like this:

if($this->getContainer()->getParameter('dizda_cloud_backup.error_notification')['to']!=NULL){
    $mailer = $container = $this->getContainer()->get('mailer');
    $message = $mailer->createMessage()
        ->setFrom($this->getContainer()->getParameter('dizda_cloud_backup.error_notification')['from'])
        ->setTo($this->getContainer()->getParameter('dizda_cloud_backup.error_notification')['to'])
        ->setSubject("DizdaBackupBundle: Backup error")
        ->setBody($e->getMessage()."( code: ".$e->getCode()."; file: ".$e->getFile()."; line: ".$e->getLine().")")
    ;

    $mailer->send($message);
}

throw new \Exception($e->getMessage(), $e->getCode(), $e);

[jongotlin]

Nice pr!

What do you think about having the to-parameter as an array that allows multiple recipients?

Regarding @dizda's last comment you can just re-throw the exception with throw $e.

Also, omit the $container = in $mailer = $container = $this->getContainer()->get('mailer');

[dizda]

Indeed @jongotlin, you're right!

I'm also agree that an array of multiple recipients could be a great idea :+1:

[dafuer]

Yes, ok! I've understood!

This afternoon I'll do that change and I'll add the possibility to set an array of recipients.

[dizda]

And you'll have to rebase to be sync with the master

[dafuer]

Hi,

I think is done, but I had some problems with the rebase and I think the commit history has some incongruences (commits repeated?). Do you prefer that I try to fork the repository again and applying the changes from the beginning?

In addition (I just watched it): Has travis found an error?

[dafuer]

Hi,

I do the foreach as test the emails exists and then send them one by one. It's tested with and without define the settings. In addition, with this test, we are testing that "to" exists, which is completely mandatory.

What's about travis?

Cheers,

[dizda]

I think it's better to test if emails are setted this way:

$emails = $this->getContainer()->getParameter('dizda_cloud_backup.error_notification')['to'];

if (count($emails) > 0) {
    // send emails there
    $message = $this->getContainer()->get('mailer')->createMessage()
        ->setFrom($this->getContainer()->getParameter('dizda_cloud_backup.error_notification')['from'])
        ->setTo($emails)
        ->setSubject("DizdaBackupBundle: Backup error")
        ->setBody($e->getMessage()."( code: ".$e->getCode()."; file: ".$e->getFile()."; line: ".$e->getLine().")")
        ;

    $this->getContainer()->get('mailer')->send($message);
}

throw $e;

Instead of the foreach. What do you think?

[dafuer]

Ok,

as you prefer. I'll do it this afternoon. But... if I'm going to remove this line:

 $container->setParameter('dizda_cloud_backup.error_notification', $config['error_notification']);

from DependencyInjection/DizdaCloudBackupExtension.php and this task will be done by the line 34 when I'll call getParameter('dizda_cloud_backup.error_notification') if it does not exists I think is going to crash...

What do you think? I can check it this evening.

[dizda]

You can use $this->getContainer()->hasParameter('dizda_cloud_backup.error_notification') to test if exist.

Moreover, the setTo() can accept string, but also array. That's the reason why I think you should remove the foreach and put the variable directly, SwiftMailer will take care of the rest.

[dafuer]

Done :)

[dizda]

That's great, thank you @dafuer :-)