Switch to a custom exception instead of InvalidArgumentException
githoober opened this issue · 9 comments
This library should have its own AssertException to avoid a need to:
- override reportInvalidArgument method - a dubious solution as it breaks userland Psalm checks because each assert method still has a tag "@throws InvalidArgumentException". Plus, not every single assert check is associated with some invalid parameters. Some checks, for example, run against function results.
- to catch InvalidArgumentException and convert it to some other exceptions because it collides with InvalidArgumentException coming from elsewhere. This prevents proper exception translation between application layers.
I realize this a BC break, but this issue is too serious to keep ignoring. At least a few tickets related to this problem were closed without any resolution and people will continue fighting it again in the future.
There are other issues/requests that are breaking BC #197
So I think it would be reasonable to create a milestone to collect all in one place.
This library could provide a custom exception without it being a BC break. The custom exception would just have to extend InvalidArgumentException.
@BackEndTea it's already possible to do that, but it's not ideal. The point is, there is hardly any reason for the library to use InvalidArgumentException.
@BackEndTea it's already possible to do that, but it's not ideal. The point is, there is hardly any reason for the library to use InvalidArgumentException.
There is: it's an assertion library meant to add guards for things the PHP type system is lacking. This is not a validation library, because as soon as you want that you start to dwell on:
- constructing more complex assertions with combinations of or/and and other logical operators
- different validation process: stop at the first error or get all the violations first
- being able to trace back the error, i.e. have a way to know from where the error comes from without relying on the error message
- being able to translate the error messages
None of this is trivial and would make this library a whole new beast. I understand it may be frustrating to see redundancy in some assertions, but the goal and the way it works is very different.
/cc @andrew-demb @Ocramius as I think this concerns #222 too
Worth mentioning: I'm saying this as my & webmozart opinion. We however left the repository in the hands of @BackEndTea so obviously if Gert wishes to change the direction of the library to take this path then so be it
No need for a BC break - can extend InvalidArgumentException and have a specific library exception, getting the best of both worlds.
Can be done in a minor release.
@Ocramius in this case, the logic of your application would need to be able to distinguish between InvalidArgumentException and its descendent used by the Assert library. Hardly the best of two worlds. A proper solution is to have a custom exception that based on the context of an assertion, could be translated to InvalidArgumentException if needed, but in the majority of cases could be left as is.
A process of asserting is not always about checking some arguments that may end up being invalid. Asserting -> AssertException.
The BC break ia still there though.
Additional inheritance level may not be the best scenario, but it is better than breaking compatibility.