Use assertion library

[sagikazarmark]

There are some assertions used as I can see, would be good to replace them with an existing assertion library, like beberlei/assert.

[mnapoli]

Why not if you want to do it.

[sagikazarmark]

Also, it would be better to throw InvalidArgumentException for invalid input values instead of the generic StorageException

[sagikazarmark]

@mnapoli What do you think about testing? I usually don't test failing values when using an assertion library. The assertion lib itself is tested. Any objections if I remove the tests with bad value?

[mnapoli]

The exception stuff is unrelated. But InvalidArgumentException would need to extend StorageException then (so that user catch it).

[mnapoli]

Do you have an example?

[sagikazarmark]

Why should it extend StorageException? If it extends StorageException, it will not be an InvalidArgumentException any more. Also, why should it be a StorageException. This is something that I never understood, but I see everywhere. I don't feel the need for having a common class representing all possible exception thrown by the library. Can you explain me why this is necessary?

For example this wouldn't be necessary anymore:

https://github.com/mnapoli/BlackBox/blob/master/tests/Transformer/AesEncrypterTest.php#L76

[mnapoli]

The idea is that me, as a user, can catch all exceptions of the storage stuff easily:

try {
    $storage->set('foo', 'bar');
} catch (StorageException $e) {
    // warn user
}

Of course you don't always want to catch exceptions, but when you do you never want to let a use case slip by (e.g. imagine it's a deamon script, you don't want to let uncatched exception kill it). Without a base class or interface, you would have so many catch blocks…

So by using a base class, I'm covered. If I want more specific stuff, I still could (if the exception exist):

try {
    $storage->set('foo', 'bar');
} catch (FileNotWritable $e) {
    // warn user
}

I usually don't test failing values when using an assertion library. The assertion lib itself is tested.

In the example of the AesEncrypter the test is valid as long as we agree that the exception should be thrown by design (i.e. not a bug, it's a feature to prevent users storing int where they probably meant to store numbers). Of course we could go the other way and cast to string, but anyway I don't see how it's testing the assertion lib, I'm testing the behavior of the class when provided with a int instead of a string.

[sagikazarmark]

Well, if you want to catch all exceptions, catch Exception. I also don't see why it could help to catch all exceptions of the library. ( At least not in case of a good design.:P) If the point is to silence all exceptions, why only that library? Otherwise, catching all exceptions means you have no idea what happened, you just report to the user that everything is broken forever.

The main reason I usually against such a solution of exceptions that it breaks the provided SPL Exceptions.

So I have to say, I am not convinced. However I can live with an interface, which lets us use the SPL (or any other) exceptions without messing with such inheritance.

What I am saying, is that we can agree that the assertion class will report, if something is wrong. Since we know, how the assertion lib behaves (except in case of a bug), we don't have to test it. So I think at this point the only thing we have to test is that the data is validated at all. However this is something, that can be interpreted this or that way, so I have no problem with saying we care about the behviour under several conditions (but that behaviour is already handled by the assertion library).