search

brick / money

A money and currency library for PHP
MIT License
1.65k stars 102 forks source link

Reorganize exceptions structure #32

Open solodkiy opened 3 years ago

solodkiy commented 3 years ago

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: image What should I do? Write useless try-catch, or add @throws annotation (which will be a lie)

After: image Phpstorm just skips this exception check.

Be aware, this changes is not really backward compatible!

BenMorel commented 3 years ago

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 commented 3 years ago

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 commented 3 years ago

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. image

BenMorel commented 3 years ago

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

antonkomarev commented 3 years ago

I like the idea to replace MoneyException base class with MoneyException interface :+1:

jvanschie commented 3 years ago

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 commented 3 years ago

Does anyone know when this is going to be released?

romkatsu commented 3 years ago

@BenMorel When will it be released?

BenMorel commented 3 years ago

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.