flysystem: Introduce a common exception for failed commands

This is somewhat related to #611.

If I try to write a file on S3 using Flysystem, and a connection problem occurs, I get an S3 exception:

Aws\S3\Exception\S3Exception: Error executing “PutObject” on …

For Dropbox, I get a (subclass of) Dropbox\Exception.

First of all, these adapters do not respect the AdapterInterface contract by allowing an exception to be thrown, instead of returning false on failure.

Secondly, I would suggest throwing an exception in all cases when something goes wrong on the underlying storage. And to keep code portable (i.e. allowing to change the storage adapter at the flip of a switch), this exception should belong to the Flysystem package.

At the moment, I have to do:

try {
    $filesystem->write($path, $contents);
} catch (S3Exception $e) {
    // ...
}

If I decide to switch to Dropbox, I have to search through my entire codebase for Flysystem calls, and update them to something like:

try {
    $filesystem->write($path, $contents);
} catch (Dropbox\Exception $e) {
    // ...
}

And if I’m writing an app that can accept any adapter, using information from a config file, I have to take all possible errors into account:

try {
    $filesystem->write($path, $contents);
} catch (S3Exception $e) {
    // ...
} catch (Dropbox\Exception $e) {
    // ...
}

And so on, without forgetting any adapter. As you can see, this is not a very good practice.

Introducing a common interface for these errors would allow us to call:

try {
    $filesystem->write($path, $contents);
} catch (AdapterException $e) {
    // ...
}

This way, there is no need to rewrite any code when switching to another adapter.

As I understand the project in its current status, you decided to return false in some methods, when something goes wrong. The fact is, the few adapters I had a chance to look at do not respect this contract properly (S3 and Dropbox thrown their own exceptions, Ftp and Local raise PHP errors).

There are two solutions to this problem:

Fix all the adapters to catch any exception or error (including PHP warnings for adapters relying on native functions) and return false where appropriate
Change the contract to never return false on methods such as write(), and throw an AdapterException on failure instead

I would strongly suggest the second option, because it breaks the application flow when something goes wrong; otherwise an error could go unnoticed if the developer forgets to check the return value using something like:

if (! $fs->write($path, $contents)) {
    ...
}

Plus, although returning false gives an opportunity to catch failures on operations such as write(), there is no such opportunity on methods that return boolean values, such as has().

Having all of them throw an exception in case of a failure harmonizes the whole thing.

Happily waiting for your feedback on this one. I could help rewrite the code if you agree with the idea!

About this issue

Original URL
State: closed
Created 8 years ago
Reactions: 13
Comments: 19 (6 by maintainers)

Most upvoted comments

With all due respect @frankdejonge, especially for the otherwise quality work you’ve done on this library, you don’t seem to get the point of specific, nested exceptions (no offense, I’ve been there too):

you can just use the base exception class for this, after you catch the ones you expect you can add. This accomplished the same thing without needing to add all of the try catch rethrows in flysystem.

You actually closed #570 for the same reason:

@PatrickRose you might as well catch the global exception class then. The wrapping and re-throwing seems redundant to me.

If that helps, here is an excerpt from the Exception handling article on Wikipedia, which is worth reading:

… defining and declaring exception types that are suitable for the level of abstraction of the called method and mapping lower level exceptions to these types, preferably wrapped using exception chaining in order to preserve the root cause.

You do not want to catch any exception when calling a flysystem method. You want to catch a specific exception, that every adapter will throw by wrapping their own, lower-level exception inside.

To better picture how that would translate into code, your AdapterException could be very simple:

class AdapterException extends \Exception
{
    public static function wrap(\Exception $e)
    {
        return new self($e->getMessage(), $e->getCode(), $e);
    }
}

The FileSystemInterface would look like:

/**
 * Write a new file.
 *
 * @param string $path     The path of the new file.
 * @param string $contents The file contents.
 * @param array  $config   An optional configuration array.
 *
 * @return void
 *
 * @throws FileExistsException If the file already exists.
 * @throws AdapterException    If a low-level, adapter-specific error occurs
 *                             (disk full, permission denied, network error, ...)
 */
public function write($path, $contents, array $config = []);

And for example, the S3 adapter implementation would be something like:

public function write($path, $contents, Config $config)
{
    try {
        return $this->upload($path, $contents, $config);
    } catch (S3Exception $e) {
        throw AdapterException::wrap($e);
    }
}

This way, all adapters would respect a homogeneous API, and one could switch to another adapter without having to change the exception handling code, and without having to blindly catch \Exception, which might incidentally catch some unrelated exception: for example, if the S3 adapter’s upload() method suddenly throws something else than an S3Exception, it should not throw an AdapterException, and I do not want to catch this exception!

Also, may I insist that several adapters don’t respect the original contract, which would be to return false instead of throwing an exception or a PHP error in case of failure. Flysystem’s error handling seems to need some tidy up, and this would be a very nice opportunity.

BenMorel on Jan 27, 2017

I was about to send an issue about that but this post is just perfect. I don’t even understand why we have to talk about it and why after all this time nothing change, it should be obvious.

Adapters wich doesn’t respect the interface are knowing the interface is bad about error handling, we are talking about API here, we have to try catch every API call to handle remote exceptions. An AdapterException is the good way, and the only way. You know it. Trust the force.

johndodev on Jan 27, 2017