Skip to content

Prevent exception when constructing WP_AI_Client_Prompt_Builder when invalid timeout is provided by filter #11596

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
westonruter wants to merge 14 commits into from
Closed

Prevent exception when constructing WP_AI_Client_Prompt_Builder when invalid timeout is provided by filter #11596

Show file tree
Hide file tree
Changes from 8 commits
Commits
Show all changes
14 commits
Select commit Hold shift + click to select a range
438011b
Add missing PHP imports
westonruter Apr 16, 2026
7eb2c19
Add missing return types
westonruter Apr 16, 2026
8006183
Use PHP class member variable type hints
westonruter Apr 16, 2026
63542e4
Ensure default timeout is number casted to float and is at least zero
westonruter Apr 16, 2026
ed8d8d6
Use float value explicitly for timeout
westonruter Apr 18, 2026
9925c25
Allow filtered timeout to be null and warn when invalid value provided
westonruter Apr 18, 2026
f01e50d
Update test to use float value
westonruter Apr 18, 2026
a14916b
Merge branch 'trunk' into fix/request-timeout-exception
westonruter Apr 18, 2026
0068422
Fix grammar typo
westonruter Apr 20, 2026
8e4c3df
Use a data provider for invalid request timeout tests
westonruter Apr 20, 2026
49ea5bb
Merge branch 'trunk' into fix/request-timeout-exception
westonruter Apr 21, 2026
226ac98
Eliminate null value for wp_ai_client_default_request_timeout filter
westonruter Apr 21, 2026
6a03377
Rename variable for clarity
westonruter Apr 22, 2026
3d6cf35
Refactor valid request timeout test to use a data provider and add cases
westonruter Apr 22, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 3 additions & 1 deletion src/wp-includes/ai-client.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,8 @@
*/

use WordPress\AiClient\AiClient;
use WordPress\AiClient\Messages\DTO\Message;
use WordPress\AiClient\Messages\DTO\MessagePart;

/**
* Returns whether AI features are supported in the current environment.
Expand Down Expand Up @@ -55,6 +57,6 @@ function wp_supports_ai(): bool {
* conversations. Default null.
* @return WP_AI_Client_Prompt_Builder The prompt builder instance.
*/
function wp_ai_client_prompt( $prompt = null ) {
function wp_ai_client_prompt( $prompt = null ): WP_AI_Client_Prompt_Builder {
return new WP_AI_Client_Prompt_Builder( AiClient::defaultRegistry(), $prompt );
}
2 changes: 1 addition & 1 deletion src/wp-includes/ai-client/adapters/class-wp-ai-client-cache.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -104,7 +104,7 @@ public function clear(): bool {
* @param mixed $default_value Default value to return for keys that do not exist.
* @return array<string, mixed> A list of key => value pairs.
*/
public function getMultiple( $keys, $default_value = null ) {
public function getMultiple( $keys, $default_value = null ): array {
/**
* Keys array.
*
Expand Down
6 changes: 2 additions & 4 deletions src/wp-includes/ai-client/adapters/class-wp-ai-client-http-client.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,17 +32,15 @@ class WP_AI_Client_HTTP_Client implements ClientInterface, ClientWithOptionsInte
* Response factory instance.
*
* @since 7.0.0
* @var ResponseFactoryInterface
*/
private $response_factory;
private ResponseFactoryInterface $response_factory;

/**
* Stream factory instance.
*
* @since 7.0.0
* @var StreamFactoryInterface
*/
private $stream_factory;
private StreamFactoryInterface $stream_factory;

/**
* Constructor.
Expand Down
22 changes: 20 additions & 2 deletions src/wp-includes/ai-client/class-wp-ai-client-prompt-builder.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -190,14 +190,32 @@ public function __construct( ProviderRegistry $registry, $prompt = null ) {
$this->error = $this->exception_to_wp_error( $e );
}

$default_timeout = 30.0;

/**
* Filters the default request timeout in seconds for AI Client HTTP requests.
*
* @since 7.0.0
*
* @param int $default_timeout The default timeout in seconds.
* @param float|null $default_timeout The default timeout in seconds, or null to disable the timeout.
* If not null, must be greater than or equal to zero.
*/
$default_timeout = (int) apply_filters( 'wp_ai_client_default_request_timeout', 30 );
$timeout = apply_filters( 'wp_ai_client_default_request_timeout', $default_timeout );
if ( is_numeric( $timeout ) && (float) $timeout >= 0.0 ) {
Comment thread
westonruter marked this conversation as resolved.
Outdated
$default_timeout = (float) $timeout;
Comment thread
westonruter marked this conversation as resolved.
Outdated
} elseif ( null === $timeout ) {
$default_timeout = null;
} else {
_doing_it_wrong(
__METHOD__,
sprintf(
/* translators: %s: wp_ai_client_default_request_timeout */
__( 'The %s filter must return a non-negative number or null.' ),
'<code>wp_ai_client_default_request_timeout</code>'
),
'7.0.0'
);
}
Copy link
Copy Markdown

@justlevine justlevine Apr 20, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Even if there is a reason to support a separate null , I'd still coerce to avoid the confusing $default_timeout -> $timeout -> $default_timeout flow. But I don't think it's necessary.

We definitely don't need the extra handholding for < 0 validation, since that's handled by setTimeout directly.

Suggested change
$timeout = apply_filters( 'wp_ai_client_default_request_timeout', $default_timeout );
if ( is_numeric( $timeout ) && (float) $timeout >= 0.0 ) {
$default_timeout = (float) $timeout;
} elseif ( null === $timeout ) {
$default_timeout = null;
} else {
_doing_it_wrong(
__METHOD__,
sprintf(
/* translators: %s: wp_ai_client_default_request_timeout */
__( 'The %s filter must return a non-negative number or null.' ),
'<code>wp_ai_client_default_request_timeout</code>'
),
'7.0.0'
);
}
$default_timeout = (float) apply_filters( 'wp_ai_client_default_request_timeout', 30.0 );
// Coerce falsy values to null. But IMO even this is unnecessary and `0.0` is fine.
$default_timeout = 0.0 === $default_timeout ? null : $default_timeout;

Copy link
Copy Markdown
Member Author

@westonruter westonruter Apr 21, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I removed null.

We definitely don't need the extra handholding for < 0 validation, since that's handled by setTimeout directly.

We do as otherwise an exception is thrown.

Copy link
Copy Markdown

@justlevine justlevine Apr 21, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Quick gut check: are we sure throwing an exception is bad DX? The only way this value can be a bad number is if someone sets it wrong in code, so an early and obvious failure before the code gets committed. 🤔

(This was recently raised via community feedback in the context of the abilities api. Tl;dr it logs instead of erroring if e.g. someone sets a bad category on an ability. Arguably an exception would be better than making them check their debug.log files 🤷)

Copy link
Copy Markdown
Member Author

@westonruter westonruter Apr 22, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since the failure to set a time limit is a low-severity issue, I think logging is good here. Also, the timeout is variable by user role, it seems less ideal for them to experience a fatal error if a developer who set up the timeouts forgot to test their specific configuration.


$this->builder->usingRequestOptions(
RequestOptions::fromArray(
Expand Down
69 changes: 65 additions & 4 deletions tests/phpunit/tests/ai-client/wpAiClientPromptBuilder.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -192,15 +192,16 @@ public function test_constructor_sets_default_request_timeout() {
}

/**
* Test that the constructor allows overriding the default request timeout.
* Test that the constructor allows overriding the default request timeout with a higher value.
*
* @ticket 64591
* @ticket 65094
*/
public function test_constructor_allows_overriding_request_timeout() {
public function test_constructor_allows_overriding_request_timeout_with_higher_value() {
add_filter(
'wp_ai_client_default_request_timeout',
static function () {
return 45;
return 45.5;
}
);

Expand All @@ -210,7 +211,67 @@ static function () {
$request_options = $this->get_wrapped_prompt_builder_property_value( $builder, 'requestOptions' );

$this->assertInstanceOf( RequestOptions::class, $request_options );
$this->assertEquals( 45, $request_options->getTimeout() );
$this->assertEquals( 45.5, $request_options->getTimeout() );
}

/**
* Test that the constructor allows overriding the default request timeout with null.
*
* @ticket 65094
*/
public function test_constructor_allows_overriding_request_timeout_with_null() {
add_filter( 'wp_ai_client_default_request_timeout', '__return_null' );

$builder = new WP_AI_Client_Prompt_Builder( AiClient::defaultRegistry() );

/** @var RequestOptions $request_options */
$request_options = $this->get_wrapped_prompt_builder_property_value( $builder, 'requestOptions' );

$this->assertInstanceOf( RequestOptions::class, $request_options );
$this->assertNull( $request_options->getTimeout() );
}

/**
* Test that the constructor disallows overriding the default request timeout with a invalid value.
*
Comment thread
westonruter marked this conversation as resolved.
Outdated
* @ticket 65094
*
* @expectedIncorrectUsage WP_AI_Client_Prompt_Builder::__construct
*/
public function test_constructor_disallows_overriding_with_negative_request_timeout() {
add_filter(
'wp_ai_client_default_request_timeout',
static function () {
return -1;
}
);

$builder = new WP_AI_Client_Prompt_Builder( AiClient::defaultRegistry() );

/** @var RequestOptions $request_options */
$request_options = $this->get_wrapped_prompt_builder_property_value( $builder, 'requestOptions' );

$this->assertInstanceOf( RequestOptions::class, $request_options );
$this->assertEquals( 30, $request_options->getTimeout() );
}

/**
* Test that the constructor disallows overriding the default request timeout with a invalid value.
*
Comment thread
westonruter marked this conversation as resolved.
Outdated
* @ticket 65094
*
* @expectedIncorrectUsage WP_AI_Client_Prompt_Builder::__construct
*/
public function test_constructor_disallows_overriding_with_bad_request_timeout_type() {
add_filter( 'wp_ai_client_default_request_timeout', '__return_empty_array' );

$builder = new WP_AI_Client_Prompt_Builder( AiClient::defaultRegistry() );

/** @var RequestOptions $request_options */
$request_options = $this->get_wrapped_prompt_builder_property_value( $builder, 'requestOptions' );

$this->assertInstanceOf( RequestOptions::class, $request_options );
$this->assertEquals( 30, $request_options->getTimeout() );
}

/**
Expand Down
Loading