Skip to content
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

Merged
merged 11 commits into from
Feb 8, 2018

Conversation

akerouanton
Copy link
Contributor

@akerouanton akerouanton commented Aug 8, 2017

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.

@akerouanton akerouanton requested a review from nicolasmure August 8, 2017 02:05
Copy link
Contributor

@nicolasmure nicolasmure left a 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 ?

@@ -7,7 +7,6 @@
use Aws\S3\S3Client;
use Gaufrette\Exception\FileNotFound;
Copy link
Contributor

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.

@akerouanton akerouanton force-pushed the 1.x-remove-return-values branch 2 times, most recently from 2b8732e to 7d75843 Compare January 5, 2018 18:02
@akerouanton akerouanton force-pushed the 1.x-remove-return-values branch 3 times, most recently from ca9af5f to b21be48 Compare January 8, 2018 03:19
@akerouanton akerouanton added this to the v1.0 milestone Jan 8, 2018
@akerouanton akerouanton force-pushed the 1.x-remove-return-values branch 2 times, most recently from 69878cd to c82cc74 Compare January 10, 2018 11:36
@akerouanton
Copy link
Contributor Author

Nope, in the end I think it's better to keep FileNotFound exception. I re-added it to adapters, excludng: OpenCloud, Ftp and PhpspeclibSftp.

Albin Kerouanton added 10 commits January 11, 2018 15:42
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.
@akerouanton akerouanton force-pushed the 1.x-remove-return-values branch from c82cc74 to c44ea77 Compare January 11, 2018 15:06
Copy link
Contributor

@nicolasmure nicolasmure left a comment

Choose a reason for hiding this comment

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

Well done !!

$this->bucket->delete($file['_id']);
} catch (\Exception $e) {
throw StorageFailure::unexpectedFailure('delete', ['key' => $key], $e);
}

return true;
Copy link
Contributor

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 ?

$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]);
Copy link
Contributor

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
Copy link
Contributor

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
Copy link
Contributor

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

@akerouanton akerouanton merged commit f405784 into KnpLabs:1.x Feb 8, 2018
@akerouanton akerouanton deleted the 1.x-remove-return-values branch February 8, 2018 10:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants