Skip to content
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

Allow more status codes #334

Closed
wants to merge 2 commits into from

Conversation

gmponos
Copy link
Member

@gmponos gmponos commented May 17, 2020

According to this: symfony/symfony#36823

@Tobion WDYT about it?

@gmponos
Copy link
Member Author

gmponos commented May 17, 2020

https://tools.ietf.org/html/rfc7231#section-6

so if someone does not respect a standard... then what's next?

@Tobion
Copy link
Member

Tobion commented May 17, 2020

This would make this test fail: https://github.com/php-http/psr7-integration-tests/blob/7c963b64105b259da8f0e08226ce4bc71e5a3b8c/src/ResponseIntegrationTest.php#L71

So I would not do this unless there is some real good reason. And just one site misbehaving is not enough reason to me.

@gmponos gmponos closed this May 17, 2020
@BenWalters
Copy link

@Tobion @gmponos
I appreciate that the RFC is to have status codes between 1xx and 5xx but when consuming a service from a 3rd party you are not in control of the status codes they choose to send back to you.
LinkedIn is owned by Microsoft! If they do it can you expect that many other might do the same.

In fact, the very reason I have landed on this PR is because I too have a 3rd party integration to complete that returns 7xx statuses. I don't like it but the fact is that Guzzle is preventing me from properly handling errors from the API.

Also see guzzle/guzzle#2751 and #349

@strarsis
Copy link

strarsis commented Oct 13, 2020

+1, I also have to use a 3rd party API that uses status codes in the 9x range.
Guzzle will fail and currently I have no means to get at least the response body for the failing message.
At least an option should be offered to make those exceptions optional, or give the exceptions for invalid status code range more context, like the reponse object.

@Tobion: Any way monkeypatching this or getting around the exception - or at least to get the original response object for the response body and headers?

@GrahamCampbell
Copy link
Member

Please contact the API provider and tell them to fix their API. It is not an HTTP API if they are returning status codes larger than 599.

@GrahamCampbell
Copy link
Member

The definition of being an HTTP 1.1 API is the official spec, which does not allow such codes.

@strarsis
Copy link

Sadly it won't be fixed anytime soon. :(
Can I use some middleware or something low level to manipulate the status code to something that is allowed by guzzle?

@BenWalters
Copy link

@GrahamCampbell Whilst I agree that it would be great for every API I ever had to work with to follow every standard out there... Bottom line is that this is the Internet and there are developers/companies that do not follow them. My asking of them to 'fix' their API is basically going to go unheard. And even if it didn't I could be waiting months or longer for them to change it.

In it's current state, guzzle is overly restrictive and causing problems that could otherwise be worked around.

@strarsis
Copy link

strarsis commented Oct 14, 2020

@GrahamCampbell: So I tried to extend the existing Response class to use a different status code assertion,
but due to all the final classes and private methods, this proved basically impossible.
Could the InvalidArgumentException be extended to include the raw response data/body, so I can see what the API actually send as text? Currently I have to use a HTTP proxy to intercept the HTTP traffic to find the response body contents out.
The only way to get around this would be using a patch/fork branch, otherwise the complete cURL handler (including EasyHandler) code would have to be duplicated, just to have a different/no HTTP status code assertion.

joejoseph00 pushed a commit to joejoseph00/psr7 that referenced this pull request May 11, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants