Adds support for Provider HTTP Handling #47
Conversation
There was a problem hiding this comment.
@JasonTheAdams Overall this looks like a great start - it gives me really clean software vibes which I love 😆
A few points of feedback on the interfaces and DTOs mostly. The high level approach looks awesome.
|
|
||
| declare(strict_types=1); | ||
|
|
||
| namespace WordPress\AiClient\Providers\Contracts; |
There was a problem hiding this comment.
I think this should live under Providers\Http\Contracts as well as it's directly related to the HTTP requests infrastructure.
| * | ||
| * @since n.e.x.t | ||
| */ | ||
| interface AuthenticationInterface extends WithJsonSchemaInterface |
There was a problem hiding this comment.
As proposed in #46, I think this should be renamed to RequestAuthenticationInterface.
Authentication can be for many things. But what this interface is for is specifically to authenticate a HTTP request (different from e.g. validating incoming authentication credentials in a platform).
This also further underlines the direct connection to the HTTP request layer.
| * | ||
| * @throws InvalidArgumentException If the method is empty. | ||
| */ | ||
| public function __construct(string $method, string $uri, array $headers = [], ?string $body = null) |
There was a problem hiding this comment.
See my notes in #46, alternative to ?string $body we need to allow providing ?array $data. A few reasons:
- For
GETrequests setting a$bodydoes not make sense. We need to support setting key value pairs that would be used in query parameters.- In other words, this would technically alter the
$uriunder the hood.
- In other words, this would technically alter the
- For
POSTrequests setting a$bodyworks, but passing key value pairs or raw PHP array data would still be more intuitive. The correct formatting for the request could be handled at the transport layer.- In other words, this would technically transform the array data into a
$bodystring, encoding it accordingly.
- In other words, this would technically transform the array data into a
Having a $body class property does make sense to me. But how you provide data is a different consideration. It definitely should be possible to provide a raw body as string, but other options are relevant too.
| * | ||
| * @throws InvalidArgumentException If the method is empty. | ||
| */ | ||
| public function __construct(string $method, string $uri, array $headers = [], ?string $body = null) |
There was a problem hiding this comment.
I think we should support something like an ?array $options parameter here.
There is far more to a request than these 4 (or 5) fields. For example, where would you be able to set a timeout (quite critical for some use-cases), or how to handle redirects, or what the user agent should be?
Related to my comment regarding $body and $data, I think another useful option to include would be an enum in what shape $data is provided. Here's an idea:
- We combine
$bodyand$datainto one variable$data, which can be either astringorarray. That already makes sense since you should only ever provide one or the other. - We have a
data_formatoption and a relatedRequestDataFormatEnumwhich can be one of:BODY(this would automatically be implied if$datais a string)QUERY_PARAMS(this would automatically be implied if$datais an array and the method isGET)BODY_PARAMS(if$datais an array, this would mean it would be sent as URL-encoded params in the body)JSON(if$datais an array, this would mean it would be sent as JSON-encoded data in the body)
WDYT?
| throw new InvalidArgumentException('URI cannot be empty.'); | ||
| } | ||
|
|
||
| $this->method = strtoupper($method); |
There was a problem hiding this comment.
I think we should introduce a simple RequestMethodEnum for this :)
| /** | ||
| * Gets the reason phrase. | ||
| * | ||
| * @since n.e.x.t | ||
| * | ||
| * @return string The reason phrase. | ||
| */ | ||
| public function getReasonPhrase(): string | ||
| { | ||
| return $this->reasonPhrase; | ||
| } | ||
|
|
||
| /** | ||
| * Checks if the response indicates success. | ||
| * | ||
| * @since n.e.x.t | ||
| * | ||
| * @return bool True if status code is 2xx, false otherwise. | ||
| */ | ||
| public function isSuccessful(): bool | ||
| { | ||
| return $this->statusCode >= 200 && $this->statusCode < 300; | ||
| } |
There was a problem hiding this comment.
See my notes on #46, maybe these are a bit too opinionated to include here?
- What's
reasonPhrasesupposed to be? - Whether a request is considered successful if between 200-299 or only if 200 exactly doesn't always have a clear answer, it depends on the specific environment and use-case.
Waiting on: #46
This introduces an HTTP system to the Providers framework so models can send requests to the Provider API.
This is built to not depend on a specific HTTP Client (e.g. Guzzle), but instead is happy to work with any PSR-18 compatible Client. It also uses the HTTPlug set of packages to improve auto-discovery of said client.
When we go to implement this into the WordPress system, this will allow us to make our own client that uses (and navigate the quirks of) the WP request functions.