php-http
[POC] HTTP client options
| Q | A |
|---|---|
| Bug fix? | no |
| New feature? | yes |
| BC breaks? | no |
| Deprecations? | no |
| Related tickets | - |
| Documentation | TBC |
| License | MIT |
What's in this PR?
POC for adding support for passing options through to HTTP clients. It is important that these options are standardized, largely supported by every implementation, and that arbitrary implementation specifics are not forwarded, breaking the abstraction. I am initially wanting to support "connect_timeout" and "timeout" only.
Why?
These are important because these mostly default to no timeout, leading in blocking for ever (this was recently a problem for me when the github.com API was partially down for multiple hours last month).
Example Usage
Psr18ClientDiscovery::find(['timeout' => 30]);
Checklist
- [ ] Updated CHANGELOG.md to describe BC breaks / deprecations | new feature | bugfix
- [ ] Documentation pull request created (if not simply a bugfix)
To Do
- [ ] Determine if this feature is wanted
- [ ] Determine what options we want to support
- [ ] Determine if this approach is reasonable, while maintaining BC
- [ ] Determine if we should crash if the resolved client does not support some of the passed options or not
- [ ] Finish implementation
- [ ] Add tests
seems like a good idea to me. i would not want generic options on the psr client itself, but for discovery it seems fine.
ideally, the discovery would also know what options a client supports and only consider those that support the given options. so that we can clearly report if no client was found or if the clients found all do not support the requested option. silentlly ignoring options would be bad.
To provide a bit of historical context: we discarded the idea back then because discovery is only supposed to provide default clients (ie. fallback values in case the user didn't care providing one).
Clients that don't support any of the options we deem necessary will result in inconsistent behavior which is arguably worse.
As far as I can tell these two arguments still hold true.
This is mentioned in the meta docs: https://www.php-fig.org/psr/psr-18/meta/
fyi: I think this was one of my first issues to Httplug: https://github.com/php-http/plugins/issues/13
Thanks for the great feedback here. As far as I'm aware, every single client supports "timeout", but not all (currently) support "connect_timeout" (possibly only the curl-backed ones are able to support this). Timeout is such a critical thing to support, that I think it's worth it. I totally agree not supporting sending options when sending requests, on the http client interface makes sense, but I think supporting it on discovery is an important step in making this useful in practice. Otherwise, I think I'd have to rip out discovery from my libraries and tell people they need to make a client by hand, so they can configure a timeout, or implement discovery myself, which is not great.
If we go ahead with this (or something like it), is timeout the only option you'd be happy with, or would you be happy with connect_timeout too, and if it's not supported, we silently drop the option on the floor (which is how it works in Guzzle at the moment - if ext-curl is not loaded, we use file_get_contents behind the scenes). I think throwing an exception if connect_timeout is passed but is not supported may be problematic, because of implementations like Guzzle that superficially seem to support it, and we cannot know that it is throwing it out without peaking inside.
Otherwise, I think I'd have to rip out discovery from my libraries and tell people they need to make a client by hand, so they can configure a timeout, or implement discovery myself, which is not great.
I think telling them to configure a client themselves is the sensible thing to do. As I said, the purpose of discovery is to provide fallbacks. People should still configure their own clients.
I'm not aware of any HTTP clients in any languages out there where the default HTTP client has any timeout configured. It's always up to the user. Libraries utilizing HTTP clients should be no different IMO.
The alternative is that you come up with an arbitrary timeout that might not be acceptable for the consumers of your library and they will have to configure a client anyway.
One could argue that no timeout is the sensible default: if your code is not prepared to retry timed out calls, it's better to wait for a reply even if it takes a long time. Also, timeout often depends on the network conditions, so how do you come up with one that works in the majority of the cases?
What I'm getting to: even if we add support for timeout, is this something that we want to encourage? Maybe we don't see eye to eye on what discovery should be used for, but the core idea behind it (so far) is to provide defaults.
I am totally agreed that no default is best, however setting a timeout in a generic way is currently impossible. Laravel users expect to not write code to configure a timeout - they would expect to change a line in a config file. I can't tell they they have to bind a client to the container to configure it. I'd have to hard code Guzzle into the package. :(
https://github.com/GrahamCampbell/Laravel-GitHub/blob/10.3/config/github.php#L43-L50 - users would want a timeout option, there.
Hm, I see. So in this case you use discovery to actually create and configure clients of production usage.
In symfony I used a Guzzle bundle to configure a Guzzle client then just linked it in the service/component I needed the client in. From a configuration perspective I found that to be better and less redundant. Isn't something like that possible in laravel?
It's only your library that needs to configure timeout in a generic way, users should already be aware of the clients they installed, so they should be able to do so.