`AssertionFailedException` should be [declared as] unchecked exception

[unkind]

As you know, there is a conception of checked/unchecked exceptions in Java. PHP has this separation de-facto as well, but on conventions level: for example, PhpStorm by default doesn't warning lack of @throws for \RuntimeException and \LogicException.

I consider exception from Assertion:: as an error: if it triggers, then we have bug or we missed validation.

PhpStorm warns about missed @throws \Assert\AssertionFailedException. I can manually add this to unchecked exceptions list (https://blog.jetbrains.com/phpstorm/2018/04/configurable-unchecked-exceptions/), but I think we can resolve it in the library:

1. Remove @throws AssertionFailedException.
2. ... or make abstract class AssertionFailedException extends LogicException (breaks BC, next major version?).

[rquadling]

AssertionFailedException is just an interface for a Throwable.

The actual exception is an \Assert\InvalidArgumentException which extends \InvalidArgumentException.

I can certainly see that a failing assertion shouldn't be JUST about invalid arguments.

This looks to be valid to change from class InvalidArgumentException extends \InvalidArgumentException implements AssertionFailedException to class LogicException extends \LogicException implements AssertionFailedException.

This would be a breaking change, so certainly would be a new major change.

If you'd like to make a PR, I'll certainly look at it.

And maybe change AssertionFailedException to AssertionExceptionInterface?

[unkind]

The actual exception is an \Assert\InvalidArgumentException which extends \InvalidArgumentException.

Actual exception is irrelevant in this case: IDE statically checks whether caller handle exception from the @throws contract correctly or not (he has to either catch exception or declare @throws as well). IDE doesn't warn in case of unchecked exception, but AssertionFailedException cannot be recognized as one by default. And IDE asks to catch exception by Assertion or declare @throws in all methods in the call stack, but I don't agree with both suggestions and I include AssertionFailedException into the list of unchecked exceptions.

To be more precise: from my point of view, Assertion is so low-level detail, so it acts like built-in language feature, e.g. Assertion::string(...) is an equivalent of scalar type hints. Built-in assertion tool in PHP throws \TypeError which is child class of the \Error. And probably you can consider to extend \Error (at least, PHP 7.2 allows it) in the next major version:

final class AssertionFailed extends \Error
{
    // ...
}

and drop @throws declarations. And as you might noticed, it means I also suggest to drop custom exceptions (Assertion::$exceptionClass).

So far intended purpose of the library is not perfectly clear: sometimes it looks like hard assertions for errors and sometimes people use it for recoverable exceptions (they catch exceptions and do some logic after that). Sometimes it looks like you need to declare @throws, sometimes not. Sometimes it looks like checked exception, sometimes like unchecked.

[rquadling]

The code does seemed to have evolved into a many faceted library capable of probably doing more than was originally intended.

If code changes are required to improve the library, then that makes sense for a PR and I'd certainly be happy to review it.

[unkind]

If code changes are required to improve the library, then that makes sense for a PR and I'd certainly be happy to review it.

I can submit PR, but I need to know which of solutions are acceptable for you:

1. Remove @throws AssertionFailedException.
2. ... or make abstract class AssertionFailedException extends LogicException (breaks BC, next major version?).

[rquadling]

What do other assertion libraries do? But I think the LogicException option makes sense. Failing assertions are not really about a bad value, but about the logic that allowed the bad value.

[unkind]

It turns out that all @throws declarations were removed by this commit: https://github.com/beberlei/assert/commit/d5ac904edda7d4e668943cbdbf482eb072cacb29#diff-4066b84b483f6098378f612ecc64f381L381 (which is technically BC break I suppose and probably should be mentioned in the changelog).

So in fact, problem doesn't exist anymore, IDE inspection won't generate warning message, so I close this one.

However, I feel that names of exceptions are kinda weird. What does mean InvalidArgumentException? Assertion failed with testable data (that came from external source) or Assertion failed by itself due to, let's say, typo in regex in Assertion::regex()? I consider these as two semantically different errors. But yes, both are generated by "invalid argument".

My solution in side-project looks like the following:

public function matchesRegEx(string $pattern, string $message = '"{subject}" does not match PCRE pattern "{pattern}".'): self
{
    $result = ErrorSandbox::invoke(function () use ($pattern) {
        return preg_match($pattern, $this->subject);
    });

    if ($result->getReturnValue() === 1) {
        return $this;
    }

    if ($result->hasErrors()) {
        throw new AssertionContainsError(
            sprintf('Pattern "%s" is invalid (code: %d): "%s".', $pattern, preg_last_error(), $result->getLastError())
        );
    }

    throw new AssertionFailed($message, ['subject' => $this->subject, 'pattern' => $pattern]);
}

Invalid data produces AssertionFailed, invalid regex — AssertionContainsError.