laravel-backup

laravel-backup copied to clipboard

In case of a backup failure, the BackupHasFailed event is fired twice, resulting in receiving twice the notification each time (e.g. Slack or mail).

This is due to the fact that the BackupHasFailed event is fired:

1. At BackupJob.php:172
2. Because of the throw $exception at BackupJob.php:176, it is fired again at BackupCommand.php:67

Feel free to make a PR for fix that Also you can link lines instead of BackupCommand.php:67 https://github.com/spatie/laravel-backup/blob/994d10018aad578258ed6bf341361c3a532eb620/src/Commands/BackupCommand.php#L67

But maybe you config report() to send notification(Slack or mail) https://github.com/spatie/laravel-backup/blob/994d10018aad578258ed6bf341361c3a532eb620/src/Commands/BackupCommand.php#L64

https://github.com/ffeytons/laravel-backup/commit/e8148bca7013ffafee3165e190c9e6234eff6ac5

@PaolaRuby I'm not sure to understand what you mean by "Also you can link lines"?

For the suggestion that the problem would come from the report($exception) that would cause a double notification, it is not the case. There are multiple firings of the BackupHasFailed event, we can even have 3 of them fired at once if the error happens during the copy of the backup to its destination, in which case it will happen:

1. First at BackupJob.php:321 (copyToBackupDestinations)
2. Second at BackupJob.php:168 (run, because the exception fired in copyToBackupDestinations is caught there)
3. Third at BackupCommand.php:67

Happy to do a PR for this. My opinion would be to remove the BackupHasFailed event from BackupCommand and only fire it from BackupJob. Do you agree with this?

Do you agree with this?

Not the best, because when the exception is outside of the try on BackupJob.php, it never notified

Indeed. However, this would mean having the BackupWasSuccessful managed by BackupJob.php along with all the other notifications while only BackupHasFailed would be handled on the upper layer (BackupCommand) -- which is not an ideal situation neither. Looks like it'd be worth to refactor a bit the exception management of BackupJob/BackupCommand along with the fix of this bug.

Looks like it'd be worth to refactor a bit the exception management

Yes, it could be the best on this case

Dear contributor,

because this issue seems to be inactive for quite some time now, I've automatically closed it. If you feel this issue deserves some attention from my human colleagues feel free to reopen it.