feature/support-request-options #105
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 Unlinked AccountsThe following contributors have not linked their GitHub and WordPress.org accounts: @mohamed.khaled@9hdigital.com. Contributors, please read how to link your accounts to ensure your work is properly credited in WordPress releases. 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. |
Ref34t
force-pushed
the
feature/include-request-options
branch
4 times, most recently
from
f31c78e to
deddb7d
Compare
Ref34t
force-pushed
the
feature/include-request-options
branch
from
deddb7d to
71306f1
Compare
There was a problem hiding this comment.
@Ref34t Thank you for working on this! It's a bit premature given the ongoing discussion, but a large portion of your PR is on the right track, which is great.
| ### Configuring request options | ||
|
|
||
| You can configure HTTP transport options like timeout and maximum redirects using the `RequestOptions` DTO: | ||
|
|
||
| ```php | ||
| use WordPress\AiClient\Providers\Http\DTO\RequestOptions; | ||
|
|
||
| // Set custom timeout for long-running requests | ||
| $options = new RequestOptions(120, 10); | ||
|
|
||
| // Or use defaults and modify | ||
| $options = RequestOptions::defaults()->withTimeout(60); | ||
| ``` | ||
|
|
||
| For implementation ideas in different environments (WordPress, Guzzle, cURL), check the transporter-specific examples in the SDK source and tests. | ||
|
|
There was a problem hiding this comment.
This seems far too specific to document in the overall README.md. Let's not add any documentation for this yet, because we're still missing an overall structure for documentation.
| /** | ||
| * Maximum allowed timeout in seconds (1 hour). | ||
| */ | ||
| public const MAX_TIMEOUT = 3600; | ||
|
|
||
| /** | ||
| * Maximum allowed redirects. | ||
| */ | ||
| public const MAX_REDIRECTS = 100; |
There was a problem hiding this comment.
What is the rationale for these values? They seem a bit arbitrary to me. I'd say let's not validate this, it's on the developer to choose reasonable values. Validating that every value is a positive integer should be sufficient.
| /** | ||
| * Default timeout in seconds. | ||
| */ | ||
| public const DEFAULT_TIMEOUT = 30; | ||
|
|
||
| /** | ||
| * Default maximum number of redirects. | ||
| */ | ||
| public const DEFAULT_MAX_REDIRECTS = 5; |
There was a problem hiding this comment.
I don't think we should set any defaults. Every request options should either have a concrete value, or be null (the default), in which case it should be ignored. All of these options are optional by definition, and if not specified, whatever the default HTTP client uses will be applied.
| /** | ||
| * Constructor. | ||
| * | ||
| * @since n.e.x.t | ||
| * | ||
| * @param int|null $timeout The request timeout in seconds. | ||
| * @param int|null $max_redirects The maximum number of redirects to follow. | ||
| * | ||
| * @throws InvalidArgumentException If timeout or max_redirects is invalid. | ||
| */ | ||
| public function __construct(?int $timeout = null, ?int $max_redirects = null) |
There was a problem hiding this comment.
Instead of a constructor with named arguments, can you adjust this class to be more in line with how https://github.com/WordPress/php-ai-client/blob/trunk/src/Providers/Models/DTO/ModelConfig.php works? We have to anticipate more and more request options in the future (think e.g. user_agent, streaming, ...), and an ordered list of parameters isn't suitable for that.
Let's for now keep only supporting timeout and max_redirects like you have it, but we'll need to architect this in a way that it could easily support 20 different options too, e.g. with setter and getter methods.
| /** | ||
| * Returns a new instance with the specified timeout. | ||
| * | ||
| * @since n.e.x.t | ||
| * | ||
| * @param int|null $timeout The timeout in seconds. | ||
| * @return self A new instance with the timeout. | ||
| * | ||
| * @throws InvalidArgumentException If timeout is invalid. | ||
| */ | ||
| public function withTimeout(?int $timeout): self | ||
| { | ||
| if ($timeout !== null && ($timeout < 0 || $timeout > self::MAX_TIMEOUT)) { | ||
| throw new InvalidArgumentException( | ||
| sprintf( | ||
| 'Timeout must be between 0 and %d seconds.', | ||
| self::MAX_TIMEOUT | ||
| ) | ||
| ); | ||
| } | ||
|
|
||
| $new = clone $this; | ||
| $new->timeout = $timeout; | ||
| return $new; | ||
| } | ||
|
|
||
| /** | ||
| * Returns a new instance with the specified max redirects. | ||
| * | ||
| * @since n.e.x.t | ||
| * | ||
| * @param int|null $maxRedirects The maximum number of redirects. | ||
| * @return self A new instance with the max redirects. | ||
| * | ||
| * @throws InvalidArgumentException If maxRedirects is invalid. | ||
| */ | ||
| public function withMaxRedirects(?int $maxRedirects): self | ||
| { | ||
| if ($maxRedirects !== null && ($maxRedirects < 0 || $maxRedirects > self::MAX_REDIRECTS)) { | ||
| throw new InvalidArgumentException( | ||
| sprintf( | ||
| 'Max redirects must be between 0 and %d.', | ||
| self::MAX_REDIRECTS | ||
| ) | ||
| ); | ||
| } | ||
|
|
||
| $new = clone $this; | ||
| $new->max_redirects = $maxRedirects; | ||
| return $new; | ||
| } |
There was a problem hiding this comment.
See above, let's use setter and getter methods, in line with ModelConfig.
| { | ||
| $psr7Request = $this->convertToPsr7Request($request); | ||
|
|
||
| // PSR-18 clients ignore per-request RequestOptions; custom transporters must apply them. |
There was a problem hiding this comment.
See #99 (comment) for more context on how we should support this.
| * @throws RuntimeException If the response has an invalid status code. | ||
| */ | ||
| public static function throwIfNotSuccessful(Response $response): void | ||
| public static function throwIfNotSuccessful(Response $response, ?Request $request = null): void |
There was a problem hiding this comment.
See my above comment, this seems unrelated? Let's try to focus individual PRs on individual problems, otherwise code reviewing becomes difficult and it can hold up work that otherwise would already be mergeable :)
| } | ||
|
|
||
| throw new \RuntimeException( | ||
| throw new RuntimeException( |
There was a problem hiding this comment.
Great catch! This one's definitely worth keeping, a simple enough fix :)
| $this->expectException(ClientException::class); | ||
| $this->expectExceptionCode(400); | ||
| $this->expectExceptionMessage('Bad Request (400)'); | ||
| $this->expectExceptionMessageMatches( |
There was a problem hiding this comment.
See above, the changes in this test file should be reverted, the ResponseUtil class doesn't need any updates in regards to supporting request options.
| string $uri, | ||
| array $headers = [], | ||
| $data = null, | ||
| ?RequestOptions $options = null |
There was a problem hiding this comment.
There's some open discussion in #99 (comment) on whether this is the preferred approach, so please feel free to share your thoughts there. I'm only flagging this here - maybe what you have here is what we should be doing, but it's still undecided.
2 participants
Expand HTTP Request Controls
Closes #99 – Introduces transport-level request option support and richer HTTP error reporting across the PHP AI Client SDK.
Overview
Adds a first-class RequestOptions DTO that flows from fluent builder usage down to PSR-7 conversions, so callers can specify timeouts and redirect policies per request. While wiring that through the stack, the transporter now wraps PSR-18 network failures in our exception hierarchy, and ResponseUtil translates non-2xx status codes into semantic exceptions with contextual messages. Documentation highlights the new configuration entry point to keep implementers aligned.
What's Included
Request Configuration
Tests
error details.