Prevent exception when constructing WP_AI_Client_Prompt_Builder when invalid timeout is provided by filter#11596
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 Core Committers: Use this line as a base for the props when committing in SVN: To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook. |
Test using WordPress PlaygroundThe changes in this pull request can previewed and tested using a WordPress Playground instance. WordPress Playground is an experimental project that creates a full WordPress instance entirely within the browser. Some things to be aware of
For more details about these limitations and more, check out the Limitations page in the WordPress Playground documentation. |
justlevine
left a comment
There was a problem hiding this comment.
@westonruter what benefit does changing wp_ai_client_default_request_timeout to support ?float instead of just float?
0 (or 0.0) can still be the disabled value without the elseif and typecheck. Also gets rid of the separate $timeout<->$default_timeout assignment juggling which IMO is confusing.
The other changes (type inlining) LGTM 🚀
| $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' | ||
| ); | ||
| } |
There was a problem hiding this comment.
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.
| $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; |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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 🤷)
I suppose because this matches the types supported by So I don't think it is completely reliable to use |
@westonruter Not sure I follow your reliability concerns: https://3v4l.org/vk3Pc . But worst case we can coerce to whatever "stable" check you feel comfortable with: As far as matching However, if folks feel a need to align the two, then I'd suggest making the type on the |
It's not reliable to check if a float value is equal to Casting the value to |
what arithmetic though? we're casting it on our side after the filter, and we in full control of the value and what happens to it afterwards. And yeah casting locally to int just for a falsely check wouldn't prevent 0.5 at all... #11596 (comment) |
There was a problem hiding this comment.
Pull request overview
This PR hardens the AI client prompt builder construction so a misbehaving wp_ai_client_default_request_timeout filter does not trigger exceptions/fatal errors, and it also addresses several PHPStan/static-analysis issues across the AI client integration.
Changes:
- Add validation/guardrails for
wp_ai_client_default_request_timeout(accept non-negative numeric values ornull, otherwise fall back +_doing_it_wrong()). - Extend PHPUnit coverage for valid/invalid timeout overrides, including
null. - Static-analysis improvements: add missing imports for
Message/MessagePart, add return types, and add native property types in the HTTP client adapter.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
src/wp-includes/ai-client/class-wp-ai-client-prompt-builder.php |
Validates filtered default timeout and avoids exceptions from invalid values. |
tests/phpunit/tests/ai-client/wpAiClientPromptBuilder.php |
Adds/updates tests for timeout override behavior (float, null, invalid). |
src/wp-includes/ai-client/adapters/class-wp-ai-client-http-client.php |
Adds native property type hints for factories. |
src/wp-includes/ai-client/adapters/class-wp-ai-client-cache.php |
Adds an explicit array return type for getMultiple(). |
src/wp-includes/ai-client.php |
Adds missing imports for PHPDoc types and adds a return type to wp_ai_client_prompt(). |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
For example: add_filter( 'wp_ai_client_default_request_timeout', function ( $value ) {
return $value - (float) get_option( 'my_timeout_delta' );
} );The value being passed in is a float to begin with here. So the casting being added here is just in case there is a string value in one of the filter callbacks. |
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
|
Something else to consider is the fact that --- a/src/wp-includes/ai-client/adapters/class-wp-ai-client-http-client.php
+++ b/src/wp-includes/ai-client/adapters/class-wp-ai-client-http-client.php
@@ -138,7 +138,7 @@ class WP_AI_Client_HTTP_Client implements ClientInterface, ClientWithOptionsInte
);
if ( null !== $options ) {
- if ( null !== $options->getTimeout() ) {
+ if ( null !== $options->getTimeout() && is_finite( $options->getTimeout() ) ) {
$args['timeout'] = $options->getTimeout();
}
While the Oh, something more important I'm seeing as well: currently when |
I feel like that example bolsters my point? Adding a line: + add_filter( 'wp_ai_client_default_request_timeout', '__return_null' ); // somewhere else
add_filter( 'wp_ai_client_default_request_timeout', function ( $value ) {
return $value - (float) get_option( 'my_timeout_delta' );
} );since Perhaps you can add a test that would red if someone refactors this in the future to cast the results of
(Putting aside that the |
I'm just trying to say that the return value if an applied filter will be a float and we can't reliably compare it with Because |
If I'm not mistaken and as you also noted, I still don't understand the point youre trying to make about float-math. There's never a time wed need to do a float === float check, literally any falsey value can be coerced to the |
|
( fwiw wordpress-develop/src/wp-includes/class-wp-http.php Lines 178 to 181 in 074792e |
This is what you suggested in #11596 (comment). |
yes and as the comment there and the rest of our conversation indicate, that was me attempting to understanding your concern, but |
I don't think I suggested this. A timeout of zero actually gets increased to 1 second: wordpress-develop/src/wp-includes/Requests/src/Transport/Curl.php Lines 425 to 431 in 047ef80 I tested this by creating a file called <?php
sleep( 2 );
echo "Awake!";Then, from that directory I started a server: php -S 0.0.0.0:8888Then in the wordpress-develop environment, I put this file: <?php
$response = wp_remote_get(
'http://host.docker.internal:8888/sleep-2.php ',
array( 'timeout' => 0.0 )
);
if ( is_wp_error( $response ) ) {
WP_CLI::error( $response->get_error_message() );
}
WP_CLI::log( 'Code: ' . wp_remote_retrieve_response_code( $response ) );
WP_CLI::log( 'Body: ' . wp_remote_retrieve_body( $response ) );And when I do this: npm run env:cli eval-file -- try-timeout.phpI get this:
So a timeout of If I change the
Note that if I use $response = wp_remote_get(
'http://host.docker.internal:8888/sleep-2.php ',
array( 'timeout' => INF )
);Then this also does not time-out, so it seems to be valid at least for the cURL transport. So we cannot use a value of |
* trunk: Build/Test Tools: Use the exact tag name in version number comments that trail pinned actions. Build/Test Tools: Remove unnecessary use of GitHub Actions expressions for values that resolve to "true" or "false" strings. Build/Test Tools: Update Actionlint to the latest version. Build/Test Tools: Address some issues in GitHub Actions workflow files as reported by Zizmor. Build/Test Tools: Add more workflow file linting with Zizmor. Tests: Print invalid UTF-8 as ASCII to fix hosts test reporting failures. Tests: Use `assertSame()` in `WP_AI_Client_Prompt_Builder` tests. Tests: Use `assertSame()` in `get_adjacent_post()` tests. Tests: Add missing `@covers` tags for some rewrite tests.
There was a problem hiding this comment.
Pull request overview
This PR hardens the AI Client prompt builder’s constructor against invalid values returned by the wp_ai_client_default_request_timeout filter to avoid exceptions during RequestOptions construction, and applies several typing/static-analysis improvements across the AI client adapter layer.
Changes:
- Validate the filtered default request timeout before applying it, falling back to 30 seconds and issuing
_doing_it_wrong()for invalid values. - Update tests to cover valid/invalid timeout override scenarios and switch the timeout override to a float.
- Address PHPStan/static-analysis issues via added imports, return type hints, and native property types.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/phpunit/tests/ai-client/wpAiClientPromptBuilder.php | Updates/expands PHPUnit coverage for default timeout override behavior. |
| src/wp-includes/ai-client/class-wp-ai-client-prompt-builder.php | Adds validation around the default timeout filter to prevent exceptions. |
| src/wp-includes/ai-client/adapters/class-wp-ai-client-http-client.php | Adds native property types for PSR-17 factories. |
| src/wp-includes/ai-client/adapters/class-wp-ai-client-cache.php | Adds an explicit array return type to getMultiple(). |
| src/wp-includes/ai-client.php | Adds missing imports for PHPDoc types and adds a return type for wp_ai_client_prompt(). |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| */ | ||
| public function data_invalid_request_timeouts(): array { | ||
| return array( | ||
| 'negative number' => array( -1 ), |
There was a problem hiding this comment.
non-blocking, but do we want a case with a positive int value too, or the 0.0 or INF edge cases you called out?
justlevine
left a comment
There was a problem hiding this comment.
If I'm not mistaken and as you also noted, wp_safe_remote_request() / WordPress/Requests treats 0 (or 0.0 ) as no timeout
I don't think I suggested this. A timeout of zero actually gets increased to 1 second:
I... don't even know what I misread many apologies for misattributing that to you, and appreciate the time you put in to tracking down the source and confirming the behavior 🙇
Only remaining feedback here from me is nonblocking and basically amounts to how much of these edge cases do we want to codify into tests, so somebody like me doesn't come around and refactor it. Go with your gut or even just feel free to ignore as you see it.
Trac ticket: https://core.trac.wordpress.org/ticket/65094
The primary change here is in
WP_AI_Client_Prompt_Builder::__construct()to check that the return value of thewp_ai_client_default_request_timeoutfilter is in fact a non-negativefloat.It also newly allows the valueOtherwise, the filtered value is discarded in favor of using the original default of 30, and anull._doing_it_wrong()is issued. Without this check, a fatal error would ensue due to an exception being thrown by\WordPress\AiClient\Providers\Http\DTO\RequestOptions::validateTimeout().In addition to this change, static analysis issues were resolved:
floatinstead ofintfor thewp_ai_client_default_request_timeout.MessageandMessagePartfor the PHPDoc forwp_ai_client_prompt().wp_ai_client_prompt()and\WP_AI_Client_Cache::getMultiple().WP_AI_Client_HTTP_Client.The following PHPStan level 10 issues are resolved:
src/wp-includes/ai-client.php: Parameter $prompt of function wp_ai_client_prompt() has invalid type Message.src/wp-includes/ai-client.php: Parameter $prompt of function wp_ai_client_prompt() has invalid type MessagePart.src/wp-includes/ai-client/class-wp-ai-client-prompt-builder.php: Cannot cast mixed to int.Use of AI Tools
AI assistance: Yes
Tool(s): Claude Code
Model(s): Claude Opus 4.7
Used for: Research
This Pull Request is for code review only. Please keep all other discussion in the Trac ticket. Do not merge this Pull Request. See GitHub Pull Requests for Code Review in the Core Handbook for more details.