Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Switch to a custom exception instead of InvalidArgumentException #202

Open
githoober opened this issue Jun 9, 2020 · 9 comments
Open

Switch to a custom exception instead of InvalidArgumentException #202

githoober opened this issue Jun 9, 2020 · 9 comments

Comments

@githoober
Copy link

githoober commented Jun 9, 2020

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.

@zerkms
Copy link
Contributor

zerkms commented Jun 9, 2020

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.

@BackEndTea
Copy link
Collaborator

This library could provide a custom exception without it being a BC break. The custom exception would just have to extend InvalidArgumentException.

@githoober
Copy link
Author

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

@theofidry
Copy link

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

@theofidry
Copy link

/cc @andrew-demb @Ocramius as I think this concerns #222 too

@theofidry
Copy link

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

@Ocramius
Copy link
Contributor

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.

@githoober
Copy link
Author

githoober commented Oct 28, 2021

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

@Ocramius
Copy link
Contributor

The BC break ia still there though.

Additional inheritance level may not be the best scenario, but it is better than breaking compatibility.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

5 participants