Closed ruudk closed 6 years ago
Makes me wonder, why is it possible to have a nullable response?
Looking at this code: https://github.com/8p/EightPointsGuzzleBundle/blob/688da531b4b159ddbfe1128a056a45974d9a8cb5/src/Middleware/EventDispatchMiddleware.php#L59-L80
It seems that you are expecting null responses so this PR is the right fix.
But it's kinda sad that the Exception $reason
on line 69 is not passed to the subscribers.
The subscribers listening to PostTransactionEvent
will now get an empty event, that doesn't give any context about the issue.
It would be better to just construct a new Response whenever an exception occurs, like this:
// Get the response. The response in a RequestException can be null too.
$response = $reason instanceof RequestException ? $reason->getResponse() : null;
if (null === $response) {
$response = new Response(200, [], $reason->getMessage());
}
Hmm, stange. But if you want to remove return type hinting then you should remove it from PostTransactionEvent::__construct
too. But let me investigate this problem.
It doesn't have to be removed from the constructor. It's perfectly valid to have a null
response (according to the code that creates this event).
Since this bundle requires PHP >= 7.0
we cannot use the nullable return types that PHP 7.1 gives us. Either we upgrade to PHP 7.1 and fix this with a nice : ?ResponseInterface
or we just remove the return type hint like the old days :)
@ruudk yep, I didn't noticed there = null
, my bad :slightly_smiling_face:
Probably I found place where this event can be triggered without response: https://github.com/guzzle/guzzle/blob/master/src/Handler/StreamHandler.php#L67 probably it's only one place where it can happen.
Also I found place where native guzzle miggleware verifies too that response is not null: https://github.com/guzzle/guzzle/blob/master/src/Middleware.php#L196
I'm not sure that it's good idea to generate response by ourselves. Let's keep it as guzzle did.
Fine by me. So this PR is the right way to fix it, right? :)
I think it would be nice to have the Exception $reason
on the subscriber as well... But that's another issue
@ruudk yep, for me it's right fix. Thank you :+1: I plan to write tests for this part as soon as possible and probably then things will go better and more stable...
Thanks! Could you be so kind to tag v7.2.1 🙏
@ruudk done :slightly_smiling_face: https://github.com/8p/EightPointsGuzzleBundle/releases/tag/v7.2.1
Found this exception:
Checked the code and the issue is the strict return type. Cannot use
: ?ResponseInterface
because this project also supports PHP 7.0