Skip to content
This repository has been archived by the owner on Mar 15, 2020. It is now read-only.

Move exception handling for downloads - closes #40 #42

Open
wants to merge 5 commits into
base: master
Choose a base branch
from
Open

Move exception handling for downloads - closes #40 #42

Changes from 1 commit
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
4820ddf
Move exception handling for downloads - closes #40
padraic May 17, 2017
e8f293a
Remove unnecessary cleanupAfterError() call
padraic May 18, 2017
792ff81
Remove unnecessary restore_error_handler()
padraic May 18, 2017
0f353f1
Trailing space
padraic May 18, 2017
6bdbeef
Relocate restore_error_handler calls
padraic May 19, 2017
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
22 changes: 13 additions & 9 deletions src/Updater.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -137,9 +137,19 @@ public function update()
|| (!is_bool($this->newVersionAvailable) && !$this->hasUpdate())) {
return false;
}

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

trailing space

$this->backupPhar();
$this->downloadPhar();

try {
$this->downloadPhar();
} catch (\Exception $e) {
restore_error_handler();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

hm why is the restore error handler here?

$this->cleanupAfterError();
throw $e;
}

$this->replacePhar();

return true;
}

Expand Down Expand Up @@ -375,13 +385,7 @@ protected function downloadPhar()
}
}

try {
$this->validatePhar($this->getTempPharFile());
} catch (\Exception $e) {
restore_error_handler();
$this->cleanupAfterError();
throw $e;
}
$this->validatePhar($this->getTempPharFile());
}

protected function replacePhar()
Expand Down Expand Up @@ -498,7 +502,7 @@ protected function validatePhar($phar)

protected function cleanupAfterError()
{
//@unlink($this->getBackupPharFile());
@unlink($this->getBackupPharFile());
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

all the files don't necessarily exists so we need to unlink them only if they exists no?

Copy link
Collaborator Author

@padraic padraic May 19, 2017 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)).

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Moved the restoration to the cleanupAfterError function to make it a bit easier to understand why/when called.

@unlink($this->getTempPharFile());
@unlink($this->getTempPubKeyFile());
}
Expand Down