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

Introduce StorageFailure and FileNotFound exceptions #517

Merged
merged 4 commits into from
Jul 7, 2017

Conversation

akerouanton
Copy link
Contributor

@akerouanton akerouanton commented Jul 7, 2017

The main reason: it's currently impossible to know what's going wrong when one of the method fails. So rather than bad return values, I updated the Adapter API to throw exceptions.

So, this PR introduces two exceptions to be used by adapters: Gaufrette\Exception\StorageFailure and Gaufrette\Exception\FileNotFound. I implemented only AwsS3 for now and Filesystem methods would benefit from such overhauling (something like FilesystemException). But this patch has been done mostly to demonstrates what we could do to enhance the current API, more will come if this one is accepted.

See #497.

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.

Much cleaner like this 👍

*/
class StorageFailure extends \RuntimeException implements Exception
{
public static function unexpectedFailure($action, $args, \Exception $previous = null)
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 type hint the $args parameter (and also add a doc block) please ?

@akerouanton
Copy link
Contributor Author

I added the missing phpdoc and typehint ;)

@nicolasmure
Copy link
Contributor

Thank you ! 👍

@nicolasmure nicolasmure merged commit 8d47c88 into KnpLabs:1.x Jul 7, 2017
@akerouanton akerouanton deleted the 1.x-fs-exceptions branch July 7, 2017 14:31
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