Reorganize exceptions structure

[solodkiy]

Usually exceptions divide into checked and unchecked. It is more java concept, but also suitable for php world.

When \LogicException (unchecked) arises you don't usually want to catch and continue to work, better way is just show some error and fail fast. Because this exception means some bug in source code and only good way to deal with it - fix this bug.
This is the reason why phpstorm skips \LogicException by default.

My proposal is reorganisation money exceptions structure to make MoneyMismatchException and UnknownCurrencyException unchecked by extending \LogicException.

Before:

What should I do? Write useless try-catch, or add @throws annotation (which will be a lie)

After:

Phpstorm just skips this exception check.

Be aware, this changes is not really backward compatible!

[solodkiy]

Something was wrong with the line endings here.

[BenMorel]

Hi, I know it's annoying to have PHPStorm complain about checked exceptions, however I'm not sure if I agree that UnknownCurrencyException should extend LogicException; if can happen in legitimate cases, like getting a currency code from user input and attempting to create a Money from there; you might want to catch the exception to warn the user that they provided an unknown currency.

[solodkiy]

In my PR UnknownCurrencyException doesn't extend LogicException directly, but through DomainException with is completely pass the case.

getting a currency code from user input and attempting to create a Money from there

You just should validate input first. You also can got something like array or object from some input and raise \TypeError. But should you catch it? I don't think so.

Also unchecked exceptions don't mean that you can catch it. You still can do it if you really want.

[solodkiy]

Also, there is already unchecked \InvalidArgumentException (which extend LogicException) in Currency constructor.
And still, we can create this object from user, or db data, we just should validate it first.

[BenMorel]

Good point, I agree that there may be inconsistencies, but I'm still not sure which way is the right way to go.

[antonkomarev]

I like the idea to replace MoneyException base class with MoneyException interface 👍

[jvanschie]

I like the idea behind the logic exception, but maybe a runtime exception is better.

I also find it quite annoying that I have to add a throws annotation to all my doc blocks.

[jvanschie]

Does anyone know when this is going to be released?

[romkatsu]

@BenMorel When will it be released?

[BenMorel]

This will be released as soon as I know for sure which direction is the way to go :)
I need to sit down and take the time to review each use case / exception and see if I agree with which one should be "checked" and which one shouldn't.