Support defining request options

[felixarntz]

There is currently no way to specify request options in this SDK, which is a severe limitation when it comes to certain use-cases. To clarify with what I mean with options: It's request transport behaviors that cannot be expressed by the request URL, method, headers, or body, such as timeout or how many redirections to follow (if any).

We need to allow passing such options (the two above are crucial examples, though there may be more). In any case, we can start simple, with those two, and add more as a need comes up. The key part is figuring out how to architecturally enable this.

I believe we have two reasonable options:

• Either allow specifying options in our Request DTO: This would nicely centralize responsibility in the DTO we already use for everything.
• Or we allow passing options to the HttpTransporter::send method, via an optional second parameter: This would mean there are two touch points for how to make a request (the Request DTO itself and this parameter), which is not as neat. But on the other hand, one may argue that these request options should be centrally set, or we could even allow providing simple callbacks that can dynamically determine the value to use based on the current Request that will be made.

In any case, I believe we should introduce a new DTO RequestOptions (which would either be used as a Request property or a HttpTransporter::send parameter) - let's avoid the WordPress array $args pattern and keep things type-safe :)

For reference, this is the shape of our current Request DTO:

RequestArrayShape array{
    method: string,
    uri: string,
    headers: array<string, list<string>>,
    body?: string|null
}

The last thing to check that is crucial and may influence our decision is how this is handled by the relevant PSR implementations, including Guzzle. We'll want to make sure our implementation is interoperable with that.

[JasonTheAdams]

I agree! It's just a touch complicated. 😆

The reason we didn't do this when first building this is because the PSR-7 RequestInterface does not have parameters for things like this, and we convert our own Request option to this when sending via the PSR-18 ClientInterface.

I think this means we'd need to do one of two things:

1. Have our Request object implement RequestInterface so we can pass additional parameters which the concrete client can choose to support.
2. Make a separate SuperPsr7Request (firm on the name 😁) object that implements RequestInterface for the same reason.

I'm not sure how we'd pass along the additional request options apart from this. I believe I saw that HTTPlug has their own, funky way of doing this, but it's just as odd.

What do you think?

[felixarntz]

@JasonTheAdams Would be good to check how Guzzle & Co. handle this. They surely must have this as it's a critical aspect of making requests.

Maybe we could learn from their implementation and apply a similar approach.

[JasonTheAdams]

Guzzle has it's own RequestInterface::send(RequestInterface $request, array $options = []): ResponseInterface. I'm not even sure if it has a PSR-18 client anywhere.

In HttPlug, it has a special GuzzleAdapter::createWithConfig() method that has a second config parameter which is passed to the $options parameter in the Guzzle interface.

---

Ok, after a long conversation with ChatGPT, here's what I'm thinking:

1. Create a RequestOptions DTO
2. Create a ClientWithOptions Interface with a send($request, $options) method
3. Implement the ClientWithOptions interface in the WP AI Client Transporter client
4. In this repo, update the HttpTransporter::send() method to accept options
5. Before sending to the client, check if the client implements ClientWithOptions. If it does, use that send; if it doesn't, fallback on the PSR-18 sendRequest method.

[felixarntz]

Love this idea - let's go with that approach.

Follow-up question: Should the RequestOptions instance be passed to HttpTransporter::send, or should it be stored in the Request instance (via an additional constructor parameter)? Or maybe even both, allow expressing options within the request and/or the HttpTransporter::send call - that would probably be the most flexible? Some options may inherently be request specific (e.g. "I want to set a 30 seconds timeout because I'm generating an image") while others may be more generic ("I do not want to follow any redirects because I consider them a problem"). WDYT?

@Ref34t I see you already opened #105 for this, but we hadn't fully determined the technical direction yet. At a first glance, I think most of your PR is looking in line with the above (especially around the RequestOptions DTO), but it would be great if you could iterate on the PR to encompass the additional considerations. :)

[felixarntz]

@JasonTheAdams Another thought: While in theory I think your approach in point 5 is reasonable, I think we should add another condition specifically for Guzzle, since it's sooo popular. It wouldn't be right to not support its ability to accept request options just because it technically couldn't reasonably implement our custom interface.

So I think we should also check if the given client implements GuzzleHttp\ClientInterface, and if so we should call its send($request, array $options) method (which would be trivial via our RequestOptions::toArray()). It's a little somewhat opinionated tweak that'll go a long way in making this practically usable for the masses.

[JasonTheAdams]

Glad you like the idea!

Should the RequestOptions instance be passed to HttpTransporter::send, or should it be stored in the Request instance (via an additional constructor parameter)? Or maybe even both, allow expressing options within the request and/or the HttpTransporter::send call - that would probably be the most flexible? Some options may inherently be request specific (e.g. "I want to set a 30 seconds timeout because I'm generating an image") while others may be more generic ("I do not want to follow any redirects because I consider them a problem"). WDYT?

Hmm. That would just mean we'd need we'd need our own Request object or some other one to be PSR-7 compliant, as the the message sent needs to implement this. I think I remember you specifically not wanting to implement that on our internal Request DTO, rigth? I'm guessing this is why the Guzzle example uses a second parameter to send/receive the options.

So I think we should also check if the given client implements GuzzleHttp\ClientInterface, and if so we should call its send($request, array $options) method (which would be trivial via our RequestOptions::toArray()). It's a little somewhat opinionated tweak that'll go a long way in making this practically usable for the masses.

Yeah, I'm good with this idea. I went down a whole rabbit hole to see if the Guzzle Adapter could be useful here, but it turns out it's for the opposite solution (turning Guzzle into a PSR-18 client), so yeah, just checking for the Guzzle interface directly is nice.

The only thing I don't like is that, unless I'm wrong, this would mean installing Guzzle via composer? Wouldn't that mean it gets installed every time someone uses this library?

[felixarntz]

Hmm. That would just mean we'd need we'd need our own Request object or some other one to be PSR-7 compliant, as the the message sent needs to implement this. I think I remember you specifically not wanting to implement that on our internal Request DTO, rigth? I'm guessing this is why the Guzzle example uses a second parameter to send/receive the options.

I'm not sure I follow. Our own Request object could just receive a new property for the RequestOptions, and our own request is passed to our HttpTransporter. The logic in that transporter could then get the request options $request->getOptions() and pass it to the underlying PSR-18 client if supported, or our custom interface. I don't think there needs to be any PSR involvement in that consideration, unless I'm missing something.

The only thing I don't like is that, unless I'm wrong, this would mean installing Guzzle via composer? Wouldn't that mean it gets installed every time someone uses this library?

That's a great point, I didn't consider that. I would even say that I object to this strongly 😆 That being said, I still think this Guzzle feature would be a crucial thing to support. Maybe we can find another workaround. We could check whether the given instance has some other "thing" on it that makes it clear it's the Guzzle client, without explicitly depending on some code from Guzzle (thinking around things like method_exists, or a check for a constant, something along these lines). It may involve some phpstan-ignore, it may be ugly - it certainly would not be clean from a code perspective, but I think that's a compromise I'd be happy to make to support this extremely popular HTTP client library without installing it for everyone.

[JasonTheAdams]

I'm not sure I follow. Our own Request object could just receive a new property for the RequestOptions, and our own request is passed to our HttpTransporter. The logic in that transporter could then get the request options $request->getOptions() and pass it to the underlying PSR-18 client if supported, or our custom interface. I don't think there needs to be any PSR involvement in that consideration, unless I'm missing something.

Ohhhhh. 🤦‍♂️ Hahah! I completely misunderstood you. I thought you were saying that you wanted to pass the options as part of the PSR-7 message in the transporter. No, you're saying that when someone is building the Request object within our system, the options are stored as part of that, and then all pulled out within the transporter.

Yes, I 100% agree. Hahah!

That's a great point, I didn't consider that. I would even say that I object to this strongly 😆 That being said, I still think this Guzzle feature would be a crucial thing to support. Maybe we can find another workaround. We could check whether the given instance has some other "thing" on it that makes it clear it's the Guzzle client, without explicitly depending on some code from Guzzle (thinking around things like method_exists, or a check for a constant, something along these lines). It may involve some phpstan-ignore, it may be ugly - it certainly would not be clean from a code perspective, but I think that's a compromise I'd be happy to make to support this extremely popular HTTP client library without installing it for everyone.

😂 😂

We could use reflection on the client to see if it has the method and signature we're looking for (as far as we can go, anyways, without importing Guzzle). Put that in a private isGuzzleClient() method and it shouldn't be too bad.

[felixarntz]

Sounds good. I'd say let's do both then:

• Allow storing a RequestOptions instance within a Request DTO instance.
• Allow passing a RequestOptions instance to the HttpTransporter::send method.
• In HttpTransporter::send, if there's a RequestOptions instance from both the DTO and from the parameter, they should be merged. I think with the one from the parameter taking precedence, as that would be more explicit at that point?