Adds support for HTTP Transporter Options #109
Conversation
|
The following accounts have interacted with this PR and/or linked issues. I will continue to update these lists as activity occurs. You can also manually ask me to refresh this list by adding the If you're merging code through a pull request on GitHub, copy and paste the following into the bottom of the merge commit message. To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook. |
|
This is ready for you, @felixarntz! One thing I went back and forth on: Are DTOs supposed to be immutable? At first I thought yes, but only because I thought I remembered it, but then I realized that |
Yes, I think they generally should be. The |
There was a problem hiding this comment.
@JasonTheAdams Overall this looks great!
A few points of feedback and a few questions that I think need to be addressed, but it's near-perfect, pending a few simplifications.
Note that there was already a PR for this in #105, so we'll need to see how we want to proceed.
| * @param ClientInterface $client The HTTP client instance. | ||
| * @return bool True when the client exposes Guzzle's send signature. | ||
| */ | ||
| private function isGuzzleClient(ClientInterface $client): bool |
There was a problem hiding this comment.
Love this! It's a really creative and pragmatic solution which, while hacky, is exactly what we need to make for a good DX for most, without forcing Guzzle on everyone. 🙌
And with these methods being private, there's no risk. If we think this is bad later, we could simply remove it.
There was a problem hiding this comment.
@JasonTheAdams Looks great - only minor notes now, nothing blocking.
| /** | ||
| * @var float|null Maximum duration in seconds to wait for the full response. | ||
| */ | ||
| protected ?float $timeout = null; | ||
|
|
||
| /** | ||
| * @var float|null Maximum duration in seconds to wait for the initial connection. | ||
| */ | ||
| protected ?float $connectTimeout = null; |
There was a problem hiding this comment.
Are these values usually handled as float? I totally see why that could be reasonable, but at the same I don't recall ever seeing a non-integer timeout value anywhere. Given we're not talking about very small durations for either of these, maybe it seems a bit excessive to support "I want to wait 5.5 seconds".
Curious what you think.
There was a problem hiding this comment.
It's a float in WP_Http::request(), so I see no reason to limit it. If someone needs that level of precision, then power to them. Hahah! If the subsequent system only supports ints, then it can truncate it without much concern.
Resolves #99
This updates the HTTP stack so we can pass request-specific transport options end to end. We now have a nullable RequestOptions DTO that threads through
Request,ModelConfig, and the transporter contracts, so theHttpTransporterunderstands both our option-aware clients and Guzzle-style clients that expose a send($request, $options) signature. The change also touches supporting enums/mocks and adds unit coverage to lock in the new behaviour.Alongside that, the new
ClientWithOptionsInterfaceoffers a lightweight extension point for PSR-18 clients that already know how to accept option bags. Libraries that wrap Guzzle or provide their own richer transport (like our WP AI Client repo) can implement this interface to let the transporter hand them aRequestOptionsinstance directly, avoiding reflection while keeping the core client agnostic.