Move exception handling for downloads - closes #40 #42
Conversation
Ensure backup phar (made pre-download) is deleted if download fails
src/Updater.php
Outdated
| @@ -137,9 +137,19 @@ public function update() | |||
| || (!is_bool($this->newVersionAvailable) && !$this->hasUpdate())) { | |||
| return false; | |||
| } | |||
|
|
|||
src/Updater.php
Outdated
| try { | ||
| $this->downloadPhar(); | ||
| } catch (\Exception $e) { | ||
| restore_error_handler(); |
There was a problem hiding this comment.
hm why is the restore error handler here?
Should be called within Strategy classes as part of download() methods (if used).
|
@theofidry Will need to check those Travis failures...looks like removing one of those calls breaks something. |
src/Updater.php
Outdated
| @@ -498,7 +500,7 @@ protected function validatePhar($phar) | |||
|
|
|||
| protected function cleanupAfterError() | |||
| { | |||
| //@unlink($this->getBackupPharFile()); | |||
| @unlink($this->getBackupPharFile()); | |||
There was a problem hiding this comment.
all the files don't necessarily exists so we need to unlink them only if they exists no?
There was a problem hiding this comment.
That's one solution. The problem is that the errors were being thrown as exceptions as we use an error handler when checking PHAR signatures. If the handler is not restored, it would leak into subsequent code (in this case the unlink() calls with @ suppression ignored in our unit tests, but could also be the user's own code which is probably isn't desired behaviour)).
There was a problem hiding this comment.
Moved the restoration to the cleanupAfterError function to make it a bit easier to understand why/when called.
Ensure backup phar (made pre-download) is deleted if download fails.