[downstream] Report failures for SignApp and SignCore #2724
Conversation
…mands Signed-off-by: Lukas Reschke <[email protected]>
Signed-off-by: Lukas Reschke <[email protected]>
|
@LukasReschke, thanks for your PR! By analyzing the history of the files in this pull request, we identified @rullzer and @nickvergessen to be potential reviewers. |
| throw new \Exception('Directory does not exist.'); | ||
| } | ||
| $appInfoDir = $path . '/appinfo'; | ||
| $this->fileAccessHelper->assertDirectoryExists($path); |
There was a problem hiding this comment.
is_dir does not fail, when one of the parents doesn't exist, so this can be removed in favor of the next line
| */ | ||
| public function writeCoreSignature(X509 $certificate, | ||
| RSA $rsa, | ||
| $path) { | ||
| $coreDir = $path . '/core'; | ||
| $this->fileAccessHelper->assertDirectoryExists($path); |
There was a problem hiding this comment.
is_dir does not fail, when one of the parents doesn't exist, so this can be removed in favor of the next line
| ); | ||
| ); | ||
| } catch (\Exception $e){ | ||
| if (!$this->fileAccessHelper->is_writeable($appInfoDir)){ |
There was a problem hiding this comment.
check if signature.json is writeable instead of the folder?
There was a problem hiding this comment.
is_writable will return false is the file doesn't exist yet. So I think checking for the folder is right here.
| ); | ||
| ); | ||
| } catch (\Exception $e){ | ||
| if (!$this->fileAccessHelper->is_writeable($coreDir)){ |
There was a problem hiding this comment.
Same here, check that signature.json is writable
| * @return bool | ||
| */ | ||
| public function is_writeable($path){ | ||
| return is_writeable($path); |
There was a problem hiding this comment.
I suspect this has been done to allow mocking here. Let me actually write tests for this and then that makes some more sense.
There was a problem hiding this comment.
Well no problem with the wrapper name, but the actual function in php is:
function is_writeable($path) {
return is_writable($path);
}so killing the e will reduce nesting by 1
Signed-off-by: Lukas Reschke <[email protected]>
|
@nickvergessen Adjusted as requested and now all paths should also be covered by tests. |
From owncloud/core#26822