-
Notifications
You must be signed in to change notification settings - Fork 356
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[1.0] Remove meaningless return values & throw exceptions when needed #521
[1.0] Remove meaningless return values & throw exceptions when needed #521
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So the FileNotFound
exception will be removed in favor of the StorageFailure
exception only ?
src/Gaufrette/Adapter/AwsS3.php
Outdated
@@ -7,7 +7,6 @@ | |||
use Aws\S3\S3Client; | |||
use Gaufrette\Exception\FileNotFound; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This line can be removed too.
2b8732e
to
7d75843
Compare
ca9af5f
to
b21be48
Compare
69878cd
to
c82cc74
Compare
Nope, in the end I think it's better to keep |
So far, write, delete and rename methods returned values: number of written bytes for write, and boolean value for the two other methods. This is no longer the case. The Local adapter was the first adapter to be introduced (see eed10e3), and presumably the Adapter interface has been based on it. Hence others adapters try to mimic low-level file functions. In the end it looks like a bad decision: most adapters are for object storages, not filesystems. Moreover it forces most adapters to recompute the size of the written content locally. Considering CQS principle, it's also better to not return any value from writting methods.
c82cc74
to
c44ea77
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Well done !!
src/Gaufrette/Adapter/GridFS.php
Outdated
$this->bucket->delete($file['_id']); | ||
} catch (\Exception $e) { | ||
throw StorageFailure::unexpectedFailure('delete', ['key' => $key], $e); | ||
} | ||
|
||
return true; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you remove this line please as the delete
function should not return any value ?
src/Gaufrette/Adapter/Local.php
Outdated
$fileInfo = new \finfo(FILEINFO_MIME_TYPE); | ||
|
||
return $fileInfo->file($this->computePath($key)); | ||
if (false === $mimeType = $fileInfo->file($this->computePath($key))) { | ||
throw StorageFailure::unexpectedFailure('size', ['key' => $key]); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
size
=> mimeType
@@ -12,6 +12,7 @@ | |||
* @return bool TRUE if the file exists, FALSE otherwise | |||
* | |||
* @throws \InvalidArgumentException If $key is invalid | |||
* @throws Exception\StorageFailure When the underlying storage fails asserting file exists |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fails to assert
@@ -27,7 +28,7 @@ public function has($key); | |||
* | |||
* @throws Exception\FileNotFound when sourceKey does not exist | |||
* @throws Exception\UnexpectedFile when targetKey exists | |||
* @throws \RuntimeException when cannot rename | |||
* @throws Exception\StorageFailure When the underlying storage fails renaming the file |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fails to rename
and so on
So far, write, delete and rename methods returned values: number of written bytes
for write, and boolean value for the two other methods. This is no longer the case.
The Local adapter was the first adapter to be introduced (see eed10e3),
and presumably the Adapter interface has been based on it. Hence others adapters
try to mimic low-level file functions. In the end it looks like a bad decision:
most adapters are for object storages, not filesystems. Moreover it forces most
adapters to recompute the size of the written content locally.
Considering CQS principle, it's also better to not return any value from
writting methods.
Related to #497.