PSR-18: Network / Request exception inheritance

[davidgrayston]

Q	A
Bug?	possibly
New Feature?	no
Version	2.0.0

This could be the intended behaviour, so please close if so.

Actual Behavior

NetworkException extends RequestException

Expected Behavior

According to https://www.php-fig.org/psr/psr-18/meta/ I wouldn't expect NetworkException to extend RequestException

The domain exceptions NetworkExceptionInterface and RequestExceptionInterface define a contract very similar to each other. The chosen approach is to not let them extend each other because inheritance does not make sense in the domain model. A RequestExceptionInterface is simply not a NetworkExceptionInterface

Steps to Reproduce

try {
    throw new \Http\Client\Exception\NetworkException(
        'some network problem',
        $someRequest
    );
} catch (\Psr\Http\Client\RequestExceptionInterface $e) {
    // Exception is caught here. 
    echo 'caught request exception';
} catch (\Psr\Http\Client\NetworkExceptionInterface $e) {
    // Expect to catch here.
    echo 'caught network exception';
}

Possible Solutions

• Change NetworkException so that it doesn't extend RequestException, then do one of the following:
  • Implement ::getRequest() in each (or use a trait)
  • Extend a class such as "RequestAwareException" that provides ::getRequest() method
• Document the current structure (instruct to catch \Psr\Http\Client\NetworkExceptionInterface before \Psr\Http\Client\RequestExceptionInterface)

[dbu]

thanks for reporting this issue (and in such a well documented way)

if i remember correctly, we had quite some discussion about that when we defined the PSR, but the end result was that the two interfaces should not extend each other. with your illustration, i agree that the implementation is not ideal and agree that we should change the code.

i am somewhat worried if this could trip people who rely on the issue and only catch RequestExceptionInterface. those would then start to fail when there is a NetworkException being thrown.

however, i think the risk is small enough and it was not intended the way it now is, i'd go to have 2.1.0 fix the exception classes as you propose, and clearly mention it in the CHANGELOG.md file.

@php-http/httplug any objections if we make sure that the 2 exceptions become distinct, as the interfaces are?