Handle client errors in the requestStream method using ResponseClientException

[IshtarStar]

This pull request introduces changes to the requestStream method to handle client errors in the 400 range by using a response exception class ResponseClientException that implements the ClientExceptionInterface. This ensures proper error handling while maintaining compatibility with the required interface.

Changes in this PR include:

1. Addition of the ResponseClientException class that implements ClientExceptionInterface.
2. Modification of the requestStream method in the HttpTransporter class to handle client errors in the 400 range by throwing the ResponseClientException.
3. Two new tests to cover scenarios where the requestStream method receives a 401 and a 404 status code.

With these changes, client errors in the 400 range are properly handled, making the requestStream method more robust and reliable.

[gehrisandro]

Hi @IshtarStar

I think now I may get it, what the cause of our confusion is.

Is it possible that you are not using Guzzle as the HTTP client? Because Guzzle does already throw an exception if the status code is 4xx.

[IshtarStar]

Hi @gehrisandro.

I use the symfony/http-client with nyholm/psr7 via curl all the time.

I added the guzzle RequestException to the catch and a third test.

This way the requestStream response should work as expected independently from the client.

[gehrisandro]

Thank you for clarifying which client you are using.

It looks like Guzzle does not respect the PSR-18 standard properly as it should not throw an exception in case of a 4xx status code: https://www.php-fig.org/psr/psr-18/#error-handling
But anyway we can not change the way Guzzle works.

In my opinion your approach is not too bad but we should not throw different exceptions depending on the HTTP client used. Therefore I would prefer to throw the existing TransporterException in both cases.

What do you think?

[IshtarStar]

I have adjusted it so that the TransporterException is thrown, independent of the client.

[gehrisandro]

Thank you for the update. Now it is a breaking change on the TransporterException and I wonder if this is necessary or if we could avoid this.

If you want me to have a closer look I can do this later this week.

[IshtarStar]

@gehrisandro Yes, let's find a solution that you think is appropriate. I know how many users already rely on this exception that it really becomes a breaching change.

If you find time, feel free to look over it for your own way.

[gehrisandro]

I did a deeper research on the topic and I think we should unify the exception handling instead of just "fixing" the stream requests: #113

@IshtarStar This would solve your issue too? Right?