Send one notification on backup failure #1707
Merged
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Fixes #1662
Summary
This PR addresses an issue with exception handling in BackupJob.
As both the
BackupCommand
command andBackupJob
are handling exceptions the result is both send the failure notification when they deem the process to have failed.With these changes
BackupCommand
will exclusively handle notifying the user if a failure scenario occurs. Due to the command being able to retry itself it makes the most sense to allow it to determine when it considers the process to have failed.Before these changes the
copyToBackupDestinations
method ofBackupJob
would fire the backup failed event and pass the destination as a parameter.laravel-backup/src/Tasks/Backup/BackupJob.php
Line 300 in f4a2abc
To ensure behavior remains the same, I have added the
BackupFailed
exception which acts as a decorator with a public$backupDestination
property which is used to set the same property in the backup failed event.The backup destination property is set by chaining on the exception.
laravel-backup/src/Tasks/Backup/BackupJob.php
Line 304 in c394924
The notification receives the original exception for the user to debug.
laravel-backup/src/Commands/BackupCommand.php
Line 84 in dc70da6
Possible Concerns
BackupJob
still handles success notifications and informative (including error) output to the console, we would need to completely decouple the job from the command to change this behavior, which I do not believe has any benefit to the package.getPrevious()
returningThrowable|null
asBackupHasFailed
event acceptsException
- to fix this an@method
has been added toBackupFailed
. This can be replaced with aprevious(): Exception
method if you prefer.