openai-php
fix: add throwIfNotSuccessfulStatusCode method to validate HTTP statu…
Summary
This PR introduces stricter HTTP status code validation to the HttpTransporter by adding a new method: throwIfNotSuccessfulStatusCode. The method ensures that only successful HTTP responses (status codes 200–299) are allowed to proceed. Any other status code results in a thrown UnexpectedStatusCodeException.
Changes
- ✅ Added
throwIfNotSuccessfulStatusCodemethod to validate response status codes. - 🔄 Updated
requestObject,requestContent, andrequestStreammethods to call this validation method. - 🧪 Added comprehensive tests for various non-successful HTTP status codes.
- 🔧 Fixes issues: #341 and #339.
Expected Outcome
With this improvement, the client will now provide immediate and clearer feedback when encountering invalid or unexpected HTTP responses, preventing potentially misleading or silent failures.
Looking forward to your feedback!
cc @bytestream who has some ideas on Exception improvement.
As I look at this - I'm wondering if we ever get a 200 back from OpenAI that is not valid JSON. There is a common complaint that we don't bubble (in the exceptions) anything about the raw response so folks can't triage the exact reasoning why. If we were to introduce a new Exception I think it should be flexible enough to capture the mixed raw response (could be null or string I'm guessing) so userland can use it - if needed.
However, thats where I was wondering if a HTTP 200 with invalid JSON would arrive, which would throw UnserializableResponse that would be missing the raw response content.
Minor @sinashakouri - you just have some CI failures.
@iBotPeaches based on what you said, it sounds like this fits the bill a little better. It just needs to include the request and response objects in the exception so that userland can act accordingly.
if a HTTP 200 with invalid JSON would arrive
Anything is possible when interacting with third party services. In my mind, it's better to be type certain - PHPStan would not be happy at the moment ;) I'm just not sure whether it's deliberately very loosely typed to allow for:
this library is used for so many more services than just OpenAI
I personally thought the clue was in the name openai-php/client and it was exclusive to OpenAI 😅
I personally thought the clue was in the name
openai-php/clientand it was exclusive to OpenAI 😅
Like would be easier if that was the case for sure lol. Half the merges in past week are just fixes for non-openai models.
Thank you so much for your insightful feedback and suggestions. I really appreciate the time you’ve taken to review the code and provide your valuable insights, which come from your extensive experience. I’ll definitely take the points you mentioned into consideration and will work on addressing and refining them as suggested.
Hey @iBotPeaches, thanks again for the earlier feedback! I’ve made the suggested changes:
- the exception now includes the request and response, and tests have been updated accordingly.
CI is green now as well 🎉
Would appreciate it if you could take another look when you get a chance 🙏
Thanks! I've got my head down trying to finish this Responses API, but will visit this for sure soon
Won't this require upstream refactoring, since VectorStoresFiles::create() still doesn't catch or throw anything?
The HTTP transporter is what is throwing exceptions based on the response.
