PSR-18: Network / Request exception inheritance

[davidgrayston]

Q	A
Bug fix?	yes
New feature?	no
BC breaks?	changed exception interface implementation
Deprecations?	no
Related tickets	fixes #155

What's in this PR?

• Http\Client\Exception\NetworkException no longer extends Http\Client\Exception\RequestException
• Http\Client\Exception\NetworkException now extends Http\Client\Exception\TransferException
• Http\Client\Exception\NetworkException and Http\Client\Exception\RequestException both use Http\Client\Exception\RequestAwareTrait (not sure if this is the best approach?)

Example Usage

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

Checklist

• Updated CHANGELOG.md to describe bugfix

[dbu]

thanks! can you do the 2 phpdoc tweaks please?

[Nyholm]

Could we really merge this? Isnt this considered a BC break.

Also, the current implementation is not a violation on PSR-18 if I recall correctly.

[dbu]

@Nyholm did you see the motivation in #155 for this? i think it is indeed quite unexpected that catching the RequestExceptionInterface will hide NetworkExceptionInterface and you would need to order the catches inverse if you want to separate them. both exceptions still extend ClientExceptionInterface, which would be the thing to catch/typehint if you mean both exceptions.

i agree that this might trip up somebody in some way, but consider it a design bug with potential for BC breaks if users have relied on the bug. there is however a real risk of psr-18 consuming applications not working as expected when using httplug as it is now, with the mixed-up exceptions. my suggestion is to do a new minor version rather than a patch release and document it in the changelog. do you think that is not strict enough?

[GrahamCampbell]

Well, all bug fixes are BC breaks by their very nature. Usually a breaking change is defined as a BC break that is not bug fix. :)

[sagikazarmark]

Well, this is kind of an interesting "incompatibility" between the HTTPlug and PSR design ideas.

Ideally, there should always be at least a catch for Http\Client\Exception as well, so I would assume exceptions will be caught one way or another.

Those who specifically want to handle NetworkExceptions will not be affected. Those who just want to catch all exceptions should use the interface instead.

So I think we can classify this as a bug and fix it accordingly.

[dbu]

Ideally, there should always be at least a catch for Http\Client\Exception as well, so I would assume exceptions will be caught one way or another.

imho the correct thing for PSR users to catch all client exceptions is to catch Psr\Http\Client\RequestExceptionInterface and not the implementation exception. but yeah, there is a common base thing to catch all kind of exceptions.

[dbu]

@Nyholm okay if i merge this for a 2.1 release, or do you still object?

[dbu]

haha, of course this broke the httplug library that i maintain: FriendsOfSymfony/FOSHttpCache#474 :-/ (because i misunderstood the exceptions of httplug when writing that code)