Client instantiation loses (default) config options

[otsch]

PHP version: 8.0.14

Description
When using the Client it happened that I got a 301 redirect response. So not the response from the uri where it redirected to but an actual response object with status code 301. The guzzle default should be to allow and automatically follow redirects and it also did then when I tried with guzzle directly.

I then also tried the createWithConfig() static method of the Client and explicitly gave it the config (allow_redirects as in the docs), but with the same outcome.

How to reproduce
Make a Client and request a uri that redirects to somewhere else.

Possible Solution
The private static buildClient() method creates a handler stack and passes it along with the config over to the new guzzle client. When the guzzle client doesn't get a handler stack it creates one by calling HandlerStack::create(). I think the buildClient() method should either use this method or even better, just don't create a handler stack itself but just leave this to guzzle itself?

I'll prepare a PR for this.

[otsch]

🤔 OK, the tests from https://github.com/php-http/client-integration-tests expect a request that gets a 30x response to actually get that response, which makes sense when you don't assume any default configuration of the underlying client.

But as this is a guzzle adapter, I'd expect it to have the guzzle defaults.

If it shouldn't use the guzzle defaults, there's still the problem that also explicitly passing a config doesn't work.

[otsch]

Also just found that the implementation of PSR-18 sendRequest method in the guzzle client itself (https://github.com/guzzle/guzzle/blob/master/src/Client.php#L134) sets allow_redirects to false in any case, so is it actually not possible/allowed to automatically follow redirects with a PSR-18 client?

Somehow it makes sense to me, as the PSR-7 response object doesn't have any option to get what uri was actually delivered (or also redirect steps in between). I just wondered as I previously didn't really find anything about redirects in the PSR-18 document. Just found something in the Meta document. I guess it just presumes that not automatically following redirects is standard.

So does that mean, that every library supporting a PSR-18 http client has to implement redirects themselves?

[dbu]

i just re-read our PSR-18, and notice that redirects are not explicitly mentioned. when we wrote the PSR, the intention was that the PSR-18 compliant client does not follow redirects (*). The important thing is that all clients behave the same way, otherwise you can't replace one client with another. Therefore it is by design that the guzzle adapter aims to have that behaviour.

If you want to use PSR-18 and decided that you don't need to know about redirects: wrap the client in a plugin client and use the redirect plugin - that will then work with all clients, should you chose to switch from guzzle to something else.

If you want to only use guzzle and have your application rely on guzzle behaviour, i would recommend to use it directly and not use the PSR-18 abstraction.

I was not aware that guzzle client overwrites the options. I find that a bit unfortunate, because as you say, it makes createWithConfig do unexpected things. To me, the PSR-18 meta implies that it should be possible to configure a client to "misbehave" according to the spec:

It is temping to allow configuration or add middleware to an HTTP client so it could i.e. follow redirects or throw exceptions. If that is a decision from an application developer, they have specifically said they want to break the specification. That is an issue (or feature) the application developer should handle. Third party libraries MUST NOT assume that a HTTP client breaks the specification.

But this is in the guzzle repository, you would have to take that up with the maintainers there. (and changing it right away would not be possible as it would be quite the BC break)

(*) Why not follow redirects: If you write an API client, you at least want to log a warning, or in test systems probably throw an error when getting redirected, because the overhead of doing every API call twice would be really bad.

[otsch]

Thank you for the detailed answer!
I understand that it totally makes sense to not automatically follow redirects 👍🏻 and actually I need to know about the redirects in my use case, so I just implemented handling redirects manually because I still want to support using different PSR-18 client implementations.

I was not aware that guzzle client overwrites the options. I find that a bit unfortunate, because as you say, it makes createWithConfig do unexpected things.

I think only guzzle's own PSR-18 implementation, the sendRequest method in the guzzle client itself resets some configs. The sendAsync method this package uses, doesn't I think. I think the reason passing configs to createWithConfig doesn't work is, that the private buildClient method passes a handler stack with only the prepare_body middleware to the guzzle client.
Maybe building the client should work differently when called via the createWithConfig method.

[dbu]

async

async is not part of PSR-18, so that makes sense. there is also the send method in the guzzle client that does not overwrite the options. sendRequest is the only PSR-18 method.

@Nyholm wdyt about adapter createWithConfig not working as would be expected because guzzles sendRequest overwrites the follow redirect option on the request?

[otsch]

The sendAsync method this package uses

With that i meant: both methods sendRequest and sendAsyncRequest from the Client implementation of this package in the end call guzzle's sendAsync method, which doesn't overrule client config as far as I can see. So no problem there.

As mentioned I think the problem is how the guzzle client is built when using createWithConfig. Not guzzle's sendRequest method, as this project doesn't use it.

[dbu]

ah, so createWithConfig works as expected, when you use this adapter?

[otsch]

No, if it should allow to make the client automatically redirect when providing the guzzle allow_redirect config.

This still returns a 301 response:

And as mentioned I think it's because of how the underlying guzzle client is created in the private buildClient method:

Because in guzzle it then looks like this:

and the HandlerStack create method:

So, as the adapter buildClient method provides the constructor with a handler only containing the prepare_body handler, it'll never add the redirect handler even if you provide the allow_redirect config.

[dbu]

we create the custom handler to not have the middlewares. would the correct overwriteable behaviour be to create the client without handler (so that the handler is autocreated as normal) but have default options for each default middleware to disable that middleware? is that how guzzle itself works when you would do new Client(['allow_redirects' => false])?

but just as default options, overwriteable in createWithConfig?

[otsch]

I'd say yes. At least I think that should work 👍🏻

[dbu]

@Nyholm do you have an opinion on this?

my thinking is that we should copy the HandlerStack::create method (to be sure changes in guzzle do not throw us off) and set all four middleware options to false by default, so that they can be overwritten in createWithConfig.