-
-
Notifications
You must be signed in to change notification settings - Fork 78
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Totally Broken With Guzzle 5 #57
Comments
All the tests pass though: https://travis-ci.org/guzzle/guzzle-services Is there something missing that needs to be covered? |
I'll look into it shortly. |
Ok, my first step is to be 1000% sure that this isn't caused by an issue in my code. This was confirmed by me checking it worked with guzzle 4: GrahamDeprecated/Laravel-CloudFlare-API@468e321. The results from that manual test were that, yes, everything definitely works with guzzle 4. I then reinstalled guzzle 5, then tried disabling my 2 subscribers to make sure they weren't interfering, and they weren't - disabling them made no difference. Third step - look for changes to the guzzle classes I'm interacting with - there are no real changes, only changes to type hints. I'm now going through the guzzle code trying to work out at which point the response variable isn't set correctly... |
At this point: https://github.com/guzzle/guzzle-services/blob/master/src/Subscriber/ProcessResponse.php#L98, the response variable is null. What's going on there? |
Ahh. Your code comment says that this is allowed to be null if there was an http error. https://github.com/guzzle/command/blob/master/src/Event/ProcessEvent.php#L45 |
But then you assumed that it was always a response object in that other subscriber. Also, I'm struggling to see how there could have been an http error? |
I'll try attaching the log subscriber later tonight. |
OH, JUST WOW!
|
Although this is due to a misconfiguration of php on my part, guzzle should still handle this error correctly, instead of just a cryptic error that took a fair bit of digging to work out what was going on. |
Seems like there just needs to be a conditional check in https://github.com/guzzle/guzzle-services/blob/master/src/Subscriber/ProcessResponse.php#L98 that ensures a response is present. Once added, the underlying HTTP exception will eventually be thrown. |
This is now fixed and I added a test case. |
Great. Thanks. :) |
The interesting guzzle stuff happens in here: https://github.com/GrahamCampbell/Laravel-CloudFlare-API/blob/a7ae6e2cfe30af64f967d8235548b62327b2f850/src/Factories/ClientFactory.php.
This used to work with guzzle 4.2 and guzzle services 0.3.0 with guzzle command 0.6.0.
I am currently using guzzle 5.0.1 with guzzle services @0ad83bfcfdf53ee48de506ce8256a85d5d7aff3a and guzzle command 0.7.0.
The text was updated successfully, but these errors were encountered: