List of possible options

[sagikazarmark]

We need to have a list of possible values for the options parameter. These are general options which should work with ALL the clients.

Possible options:

• timeout [0]
• exceptions [false]: whether the user wants 4xx and 5xx responses to be converted to exceptions
• base_uri [null]: from #36

[dbu]

by the way a neat thing for such options is the symfony/options-resolver component: http://symfony.com/doc/current/components/options_resolver.html . the main thing is it also allows for validating options and for handling defaults. but this can be an adapter implementation detail, the interface can work with an array.

which leads to the question: should adapters complain about unknown options? i would say it SHOULD. if it does not, ok, but its better to know. otherwise things like timeuot and exeptions and similar spelling mistakes will be very hard to spot.

should we call exceptions errors_as_exceptions or something? we would always throw an exception when there is no response, as in DNS not resolved, no network, ...

i think we have to be very careful with mandating options for the client, can't think of something obvious that we should add here.

and actually, it seems like bad architecture to have to pass this (particularly the exceptions option) in each call, so carry configuration around in the application. rather, it should be a constructor argument for my client. but we might want to keep it to allow overriding options for particular situations. like in one place supress exceptions or use a particular long timeout (though you could just use a different client instance then)

[dbu]

on the question whether we should have it: some clients may come up with clever options that do make a lot of sense to be request specific. just for allowing that i think having the options in the interface is fine. and the two things you list in the issue do make sense to have once we have options.

[sagikazarmark]

but this can be an adapter implementation detail

I think it should be an implementation detail.

should adapters complain about unknown options?

That's interesting indeed, great idea. I think they should. But we should have our own exception here as well.

should we call exceptions errors_as_exceptions or something?

Not sure. Better explanation, but longer.

i think we have to be very careful with mandating options for the client

Agree

rather, it should be a constructor argument for my client

You are also right here. However it is an implementation detail as well. The reason why options are in method calls is overridability. (Does this word exist at all? :D)

Options should be a few configuration things which can be independent from the actual client.

[sagikazarmark]

Added base URI to the list as @ddeboer mentioned it in #36

[dbu]

cool. i just feel that a bit more verbosity would be good. as a dev, when i read expections => false in my code, i might think i supress all exceptions, which is not the case. i think errors_as_exceptions would be more clear.

[sagikazarmark]

I understand, and I have similar feelings. The most appropriate would be convert_http_errors_to_exceptions, but that's too long. We need to decide a short name and then document it.

[dbu]

actually i think that name is not that bad. i prefer verbosity over confusion any day :-) and the documentation becomes really trivial with that name as its all said.

[sagikazarmark]

Options must be listed in documentation anyway, as they are not part of the interface, but part of the spec.

[joelwurtz]

We should keep the number of options as minimal as possible as otherwise it will induce a huge maintenance for each adapter.

For me only the timeout options make sense for the moment. Transform of http errors to exception should be done in a Middleware Base Uri is also very specific of the adapter, (like in the Socket Adapter i want to support Unix socket domain, unix://var/run/socket.sock, and this will make no sense for other adapter like Curl / Guzzle / etc ...)

Also we can imagine there will be ssl options but some adapters will not support this, how do we handle this use case ?

IMO, each adapter have its own options. Then library using php-http client should always expose the $options parameter of the request, and in the end this will be a choice for the user which will have the knowledge of the adapter used and so, will know the available options.

[sagikazarmark]

We should keep the number of options as minimal as possible as otherwise it will induce a huge maintenance for each adapter.

Agreed.

Transform of http errors to exception should be done in a Middleware

Well, I think it is a simple thing which is actually independent from the underlying client (in case of adapters).

IMO, each adapter have its own options.

The most important thing here is interoperability. The user should not be aware of the actual client used in the code. It should be a configuration detail. Any further options, like SSL should be configured prior to making a request. At least this is how I imagined options so far.

end this will be a choice for the user which will have the knowledge of the adapter used and so, will know the available options.

As explained above, this is only the case when the user configures which adapter should be used. The actual code using the client should not be aware of client dependent options.

[dbu]

Transform of http errors to exception should be done in a Middleware

Well, I think it is a simple thing which is actually independent from the underlying client (in case of adapters).

we could also provide a trait for that, like the trait for sendRequests.

The most important thing here is interoperability. The user should not be aware of the actual client used in the code. It should be a configuration detail. Any further options, like SSL should be configured prior to making a request. At least this is how I imagined options so far.

i totally agree with this. so in my opinion the best usage is: i configure the client and inject into my class (e.g. symfony DI system) and the class simply does calls on it. to avoid needing too many different services, we can have general options, for example for the base path or the timeout.

although, with this model in mind, even the base path could be a bit weird because its just another piece of configuration to pass. we could also just pass two properly pre-configured clients.

the errors-to-exception thing is actually worrying in that perspective, as it could mean that a generic consumer has to be explicit about the exception throwing in each call, as the code will look differently depending on whether you check return status or handle http exceptions. why exactly do we want such an option? could we also decide for just one of those approaches and stick to it? i would prefer the exceptions, as there are exceptions anyways for when there is no response at all, so the application needs exception handling anyways.

[sagikazarmark]

we could also provide a trait for that, like the trait for sendRequests.

Provide a trait for what? For the option or the code that does the actual converting?

although, with this model in mind, even the base path could be a bit weird because its just another piece of configuration to pass. we could also just pass two properly pre-configured clients.

Yeah, base path might be better in a plugin. However we should keep in mind that the configuration of clients is NOT interoperable. Also, in case of an API client you might want to set the endpoint in the API client. For example in one of the APIs I develop the user API has a different endpoint, but the API client is the same. In such case, the base path option would be useful, because I don't have to prepend the url to the path every time. The same is true for simpler cases: different basepath can be an url+first segment to the actually accessed resource in the API. In this case the "url" passed to the client would be the actual action (for example http://api.my.com/users and /create).

the errors-to-exception thing is actually worrying in that perspective, as it could mean that a generic consumer has to be explicit about the exception throwing in each call, as the code will look differently

I think it is a matter of taste. Someone maybe like to handle status code checking. Also, in some cases the status code needs to be checked anyway: if not 200, then it is a bad response from the user's perspective. It's a double check if exceptions are always thrown.

[dbu]

we could also provide a trait for that, like the trait for sendRequests.

Provide a trait for what? For the option or the code that does the actual converting?

yes, for the code that converts error responses to exceptions. as its not actually client specific logic, thanks to PSR-7.

although, with this model in mind, even the base path could be a bit
weird because its just another piece of configuration to pass. we
could also just pass two properly pre-configured clients.

Yeah, base path might be better in a plugin. However we should keep in mind that the configuration of clients is NOT interoperable. Also, in case of an API client you might want to set the endpoint in the API client. For example in one of the APIs I develop the user API has a different endpoint, but the API client is the same. In such case, the base path option would be useful, because I don't have to prepend the url to the path every time. The same is true for simpler cases: different basepath can be an url+first segment to the actually accessed resource in the API. In this case the "url" passed to the client would be the actual action (for example http://api.my.com/users and /create).

i am not arguing against the adapter having that option, only against having it as a mandatory supported option on each call. it should rather be configured on creation. you should not reuse the same client instance to talk to different servers.

the errors-to-exception thing is actually worrying in that
perspective, as it could mean that a generic consumer has to be
explicit about the exception throwing in each call, as the code will
look differently

I think it is a matter of taste. Someone maybe like to handle status code checking. Also, in some cases the status code needs to be checked anyway: if not 200, then it is a bad response from the user's perspective. It's a double check if exceptions are always thrown.

the problem is when you write a reusable library. you have no control what kind of client people are passing you, but your code will be written with one or the other assumption. so not having an option for the exceptions but a hard decision in the specification would make things a lot simpler here.

regarding status codes, there is also the topic whether to follow redirections or not, for example. i wonder what the generic library should do: just document how it wants its client configured and hope that is respected? or always pass options in every call? both are not great solutions.

[sagikazarmark]

what kind of client people are passing you, but your code will be written with one or the other assumption

You have a point here. I agree, the more configurable options we have which actually have effect on the outcome, the less stable the whole thing is.

My concern is that (as expressed earlier) HTTP errors are not always exceptional. So always throwing an exception might not be a good idea. Maybe we could move this functionality to a plugin? It makes the spec cleaner and we still preserve the functionality. This is not something after all which has anything to do with underlying clients in adapter, but with the response itself.

If we move this functionality to a plugin, whould we leave the exceptions in the main repo? I think not. Also, I am thining about creating an HttpErrorException (possibly in the future plugin) which can be used to catch HTTP Errors.

regarding status codes, there is also the topic whether to follow redirections or not

In the original package it is implemented as a subscriber. Since PSR 7 interfaces are immutable, event-driven logic is not really an option anymore. I am thinking about something similar to guzzle plugin architecture.

[sagikazarmark]

Following the previous logic base url should also go into a plugin. Everything that modifies the input or the output should be in a plugin. Only those things should go into options which affects the way requests are processed (like timeout).

[dbu]

is there a mechanism for the client library to check which plugins exist? and what do plugins do, is there some sort of event listening with the option to return a different response? or do you create multiple layers of clients wrapping each other to add behaviour?

[sagikazarmark]

See #35 for details about the plugin system.

IMO Event-driven logic is not an option, since PSR7 is immutable and the Request/Response should be reset in the event object.

I think a middleware-like system would be better. Actually there are quite some PSR7 middlewares aout there, but they are server side and not client side.

The main plugin stack would implement HttpPsrClient and plugins would be called in a middleware chain. I am not sure though how it would work with the parallel request sending.

[joelwurtz]

The main plugin stack would implement HttpPsrClient and plugins would be called in a middleware chain. I am not sure though how it would work with the parallel request sending.

No need for a middleware chain, we can do the same thing as the stack php convention :

• A plugin must implement HttpPsrClient
• A plugin must take the decorated client as the first constructor argument
• A plugin decorate the handle request, and delegate the request to the decorated client

The problem is for the HttpMethods interface which complexify this architecture

[sagikazarmark]

StackPHP also uses a middleware chain, but instead of passing the next middleware in the handle method, it injects it into the constructor. The main reason for this (I guess) is that it uses the Symfony HttpKernelInterface (interoperability), so it cannot have it's own interface.

A custom interface is cleaner, because we don't have to make assumptions you mentioned.

The problem is for the HttpMethods interface which complexify this architecture

The plan is to have a client utility which accepts an HttpPsrClient, implements HttpMethodsClient and uses a MessageFactory to create requests. This way any HttpPsrClient can be "transformed" into an HttpMethodsClient.

[dbu]

IMO Event-driven logic is not an option, since PSR7 is immutable and the Request/Response should be reset in the event object.

while its not exactly elegant, you could have an event that is mutable and allow plugins/listeners to update the request resp. response in the event and switch to that over the original one.

not sure how good/bad that is, but symfony does that. might be a bit more lightweight than wrapping the client over and over again.

[sagikazarmark]

Personally I don't like it. I think it is more complicated and harder to test.

wrapping the client over and over again.

From the outside work only one wrapping is required: when the client is injected into the plugin stack. Everything else is is done internally.

The other reason I don't like it is the dependency: we need to rely on one implementation. This is especially bad in case of event listeners: a symfony user would probably hate to install league/event, a Proton user would probably hate to instal symfony event dispatcher.

Guzzle also changed to a middleware system from event-driven one because of the same reason: immutability.

[dbu]

lets move this discussion to #37 for the options: the only thing left would be timeout, right? the other two would be "plugins".

[sagikazarmark]

For now, yes.

[dbu]

cool, then lets update the phpdoc and solve this issue.

[sagikazarmark]

Will you?

[dbu]

i am very busy atm. i can try to but might take a moment.